-
Notifications
You must be signed in to change notification settings - Fork 940
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add VPN enabled duration metric #28020
Conversation
we need someone owning vpn to review this one |
e1be9fa
to
6370320
Compare
@@ -79,16 +86,14 @@ class BraveVpnService : | |||
#endif // BUILDFLAG(IS_ANDROID) | |||
|
|||
std::string GetCurrentEnvironment() const; | |||
bool is_purchased_user() const { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's better to leave is_purchased_user()
and IsConnected()
as BraveVpnService
's own api because both are part of vpn service's functionality.
IMO, we can simply call it via delegate's interface (ex, Delegate::GetIsPurchasedUser()
or Delegate::GetIsConnected()
) w/o name collision. WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i did something very similar for Leo, where I included existing methods in the delegate:
brave-core/components/ai_chat/core/browser/ai_chat_metrics.h
Lines 123 to 129 in eb542be
class ConversationHandlerForMetrics { | |
public: | |
virtual ~ConversationHandlerForMetrics() = default; | |
virtual size_t GetConversationHistorySize() = 0; | |
virtual bool should_send_page_contents() const = 0; | |
virtual mojom::APIError current_error() const = 0; | |
}; |
imho, it seems odd to have duplicated methods (i.e. having IsConnected
and GetIsConnected
side-by-side), especially for these getters.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would not block merging with this but still it seems strange to me to get vpn service's core state from metrics interface.
6370320
to
5c06237
Compare
Chromium major version is behind target branch (134.0.6998.95 vs 135.0.7049.17). Please rebase. |
5c06237
to
1615e17
Compare
1615e17
to
8a72daa
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Released in v1.78.64 |
Resolves brave/brave-browser#44492
Submitter Checklist:
QA/Yes
orQA/No
;release-notes/include
orrelease-notes/exclude
;OS/...
) to the associated issuenpm run test -- brave_browser_tests
,npm run test -- brave_unit_tests
wikinpm run presubmit
wiki,npm run gn_check
,npm run tslint
git rebase master
(if needed)Reviewer Checklist:
gn
After-merge Checklist:
changes has landed on
Test Plan: