-
Notifications
You must be signed in to change notification settings - Fork 868
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
Ignore disconnected status while changing region #16285
Conversation
4647ca6
to
0749bf4
Compare
0749bf4
to
e8330bb
Compare
When user changes region while connected, status should be connecting till region change completes.
e8330bb
to
75920b0
Compare
|
// noti again. So, ignore second one. | ||
// On exception - we allow from connecting to disconnected in canceling | ||
// scenario. | ||
// Ignore disconnected state while connecting is in-progress. |
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.
These updates to comments lose the context for WHEN this can happen- which is when person is changing regions.
Can we add that back in?
// When user connects to different region while connected,
// we disconnect current connection and connect to newly selected
// region.
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.
wait- I missed the actual change to the logic 😅 You're correct to delete this, sorry!
@@ -585,14 +585,6 @@ void BraveVpnService::LoadPurchasedState(const std::string& domain) { | |||
return; | |||
} | |||
|
|||
#if !BUILDFLAG(IS_ANDROID) | |||
if (!IsNetworkAvailable()) { |
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 removing this code is great 👍 LoadPurchasedState
is called when person is clicking on VPN
button... and if they didn't have network connectivity (briefly, for example), this code would be letting the connection go into FAILED (when no connect ever was made by user).
Nice! 😄 👍
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.
As we cache expiration date, browser will enter to purchased state even network is disabled.
Then, user can get connect failure when user try to connect.
Or it's already expired, browser will ask to refresh skus credential and skus service will make use not purchased state.
So we could get proper state w/o this early checking.
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.
++
Ignore disconnected status while changing region
Verified
|
Brave | 1.48.57 Chromium: 108.0.5359.128 (Official Build) nightly (64-bit) |
---|---|
Revision | 1cd27afdb8e5d057070c0961e04c490d2aca1aa0-refs/branch-heads/5359@{#1185} |
OS | Windows 10 Version 22H2 (Build 19045.2364) |
Steps:
- purchased, set up, and connected to
BraveVPN
viaaccount.bravesoftware.com
- confirmed I was connected to the
USA (West)
server/region, by default - clicked on the
VPN
button on the browser toolbar - selected
Australia
and clickedConnect
- confirmed no disconnects or other errors (but the DoH warning dialog*, as expected), while changing connections
- selected
Spain
- confirmed no disconnects or other errors, same as above
BraveVPN connected to USA (West) |
Australia |
Spain |
* DoH warning dialog |
---|---|---|---|
fix brave/brave-browser#27267
fix brave/brave-browser#27196
When user changes region while connected, status should be connecting till region change completes.
Also removed network status checking with
IsNetworkAvailable()
.Instead of using
IsNetworkAvailable()
, vpn service will get actual state by issuing network request.Also do load & save prefs twice when creating vpn entry on macOS to fix connect failure when there is no os vpn entry on macOS
Resolves
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 lint
,npm 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: