Conversation
📋 Code Owners Summary8 file(s) changed, 1 with assigned owners 1 team(s) affected: Owners and Their Files
|
| #include "base/memory/weak_ptr.h" | ||
| #include "base/timer/timer.h" | ||
| #include "base/timer/wall_clock_timer.h" | ||
| #include "brave/components/ai_chat/core/browser/ai_chat_credential_manager.h" |
There was a problem hiding this comment.
AIChatCredentialManager is only used via std::unique_ptr in this header, and ~AIChatMetrics() is defined in the .cc file. A forward declaration would suffice here and avoid pulling in heavy transitive includes from ai_chat_credential_manager.h (skus mojom, mojo remote/remote_set headers). Move the full #include to the .cc file. best practice
There was a problem hiding this comment.
If we were including headers from another non-base target, I would agree, but this sort of include is quite ubiquitous.
| raw_ptr<PrefService> local_state_; | ||
| raw_ptr<PrefService> profile_prefs_; | ||
|
|
||
| std::unique_ptr<AIChatCredentialManager> credential_manager_; |
There was a problem hiding this comment.
nit: credential_manager_ is an owning unique_ptr placed after the non-owning raw_ptr<PrefService> members. Per convention, owning members should come before unowned raw_ptr members to make ownership semantics visually clear. Consider grouping it with the other unique_ptr members (tab_focus_metrics_, skills_metrics_) below. best practice
There was a problem hiding this comment.
Curious how this guideline came about. Is this an upstream code style matter?
There was a problem hiding this comment.
It's from https://chromium.googlesource.com/chromium/src/+/lkgr/docs/dangling_ptr_guide.md
It recommends placing owning members (std::unique_ptr, scoped_refptr) before unowned members (raw_ptr)
The rationale is about destruction safety: C++ destroys members in reverse declaration order, so placing raw_ptr members last means they are destroyed first. Before the owning members they may reference are freed. This prevents dangling pointer issues.
Resolves brave/brave-browser#53154