Advanced server selection follow up android #25604

deeppandya · 2024-09-17T17:06:49Z

Resolves brave/brave-browser#41106

Submitter Checklist:

I confirm that no security/privacy review is needed and no other type of reviews are needed, or that I have requested them
There is a ticket for my issue
Used Github auto-closing keywords in the PR description above
Wrote a good PR/commit description
Squashed any review feedback or "fixup" commits before merge, so that history is a record of what happened in the repo, not your PR
Added appropriate labels (QA/Yes or QA/No; release-notes/include or release-notes/exclude; OS/...) to the associated issue
Checked the PR locally:
- npm run test -- brave_browser_tests, npm run test -- brave_unit_tests wiki
- npm run presubmit wiki, npm run gn_check, npm run tslint
Ran git rebase master (if needed)

Reviewer Checklist:

A security review is not needed, or a link to one is included in the PR description
New files have MPL-2.0 license header
Adequate test coverage exists to prevent regressions
Major classes, functions and non-trivial code blocks are well-commented
Changes in component dependencies are properly reflected in gn
Code follows the style guide
Test plan is specified in PR before merging

After-merge Checklist:

The associated issue milestone is set to the smallest version that the
changes has landed on
All relevant documentation has been updated, for instance:

Test Plan:

simonhong · 2024-09-18T01:36:36Z

components/brave_vpn/browser/connection/brave_vpn_region_data_helper.cc

@@ -94,6 +96,9 @@ mojom::RegionPtr GetRegionFromValueWithoutCity(const base::Value::Dict& value) {
  if (auto* name_pretty = value.FindString(brave_vpn::kRegionNamePrettyKey)) {
    region->name_pretty = *name_pretty;
  }
+  if (auto* country = value.FindString(brave_vpn::kRegionCountryKey)) {
+    region->country = *country;


Curious what this data has and destkop also needs changes with this new property?

It was a suggestion from the guardian team to persist the country and continent to use it for region selection to avoid future migration.

country has country name? and then maybe desktop also need to use this property for country name instead of name_pretty?

Ah, ok. I saw it from region-list.ts. It has country name.
I'll f/u to replace it with namePretty on desktop.

brave-builds · 2024-09-18T12:57:00Z

A Storybook has been deployed to preview UI for the latest push

brave-builds · 2024-09-18T13:35:00Z

A Storybook has been deployed to preview UI for the latest push

SergeyZhukovsky · 2024-09-18T14:13:59Z

android/java/org/chromium/chrome/browser/appmenu/BraveTabbedAppMenuPropertiesDelegate.java

@@ -131,12 +131,18 @@ public void prepareMenu(Menu menu, AppMenuHandler handler) {
                        menu.findItem(R.id.request_vpn_location_row_menu_id).getSubMenu();
                MenuItem vpnLocationSubMenuItem =
                        vpnLocationSubMenu.findItem(R.id.request_vpn_location_id);
+                String isoCode = BraveVpnPrefUtils.getRegionIsoCode();
+                String country =
+                        (!BraveVpnPrefUtils.getRegionCountry().equals("")


why are there brackets at the beginning and at the end?

SergeyZhukovsky

++

Resolve PR comment

github-actions · 2024-09-18T19:39:08Z

[puLL-Merge] - brave/brave-core@25604

Description

This PR makes significant changes to the Brave VPN functionality, particularly in how server regions are handled and displayed. It modifies several Java files in the Android app and updates the data model for VPN regions.

Changes

Changes

BraveTabbedAppMenuPropertiesDelegate.java:
- Added import for java.util.Locale
- Modified how the VPN location is displayed, now using the country name instead of a "pretty" name
- Added fallback to use Locale.getDisplayCountry() if the country name is not available
VpnServerActivity.java:
- Updated the BraveVpnServerRegion constructor call with new parameters
- Changed how the selected server region is set, now including more detailed information like country and continent
VpnServerSelectionActivity.java:
- Modified the BraveVpnServerRegion constructor call for automatic server selection
- Removed the call to BraveVpnPrefUtils.setAutomaticServerSelection()
BraveVpnServerSelectionAdapter.java:
- Changed the condition for checking if a server is selected, now comparing ISO codes instead of region names
VpnServerAdapter.java:
- Significantly updated the logic for determining if a server is enabled
- Added checks for country and city precision
BraveVpnServerRegion.java:
- Added new fields: mIsAutoSelected, mCountry, and mContinent
- Removed mRegionCityName and mRegionCityNamePretty
- Updated constructor and getter methods accordingly
BraveVpnPrefUtils.java:
- Added new methods for getting and setting region country and continent
- Removed methods related to city name and city name pretty
- Updated setPrefModel() method to reflect new BraveVpnServerRegion structure
BraveVpnRegionDataHelper.cc:
- Added country field to the region dictionary
brave_vpn_constants.h:
- Added a new constant kRegionCountryKey
brave_vpn.mojom:
- Added country field to the Region struct
region-list.ts:
- Added country field to all region and city objects in the mock data

Possible Issues

The removal of setAutomaticServerSelection() in VpnServerSelectionActivity.java might affect the automatic server selection feature if it's not handled elsewhere.
The changes in how regions are compared and selected might cause unexpected behavior if not all parts of the codebase are updated consistently.

Security Hotspots

None identified. The changes appear to be primarily related to data structure and UI updates, without introducing apparent security risks.

brave-builds · 2024-09-19T00:33:53Z

A Storybook has been deployed to preview UI for the latest push

simonhong

components/brave_vpn/ ++

deeppandya added this to the 1.72.x - Nightly milestone Sep 17, 2024

deeppandya requested review from simoarpe, simonhong and SergeyZhukovsky September 17, 2024 17:06

deeppandya self-assigned this Sep 17, 2024

deeppandya changed the title ~~Advanced server selection follow up android~~ [WIP]Advanced server selection follow up android Sep 17, 2024

simonhong reviewed Sep 18, 2024

View reviewed changes

deeppandya force-pushed the advanced_server_selection_follow_up_android branch from cacbacf to 80aa94e Compare September 18, 2024 11:59

github-actions bot added the CI/storybook-url Deploy storybook and provide a unique URL for each build label Sep 18, 2024

deeppandya requested a review from simonhong September 18, 2024 12:01

deeppandya marked this pull request as ready for review September 18, 2024 12:01

deeppandya changed the title ~~[WIP]Advanced server selection follow up android~~ Advanced server selection follow up android Sep 18, 2024

SergeyZhukovsky reviewed Sep 18, 2024

View reviewed changes

SergeyZhukovsky approved these changes Sep 18, 2024

View reviewed changes

deeppandya added 5 commits September 19, 2024 01:07

CHeck selected country based on country ISO code

167916d

Remove cityname and cityname pretty and rely on region name only

5141e9f

Update upgrade behaviour for server selection

35cc39d

Update conditions for vpn server selection with iso code

08452d4

Add mock data for country

32acb34

Resolve PR comment

deeppandya force-pushed the advanced_server_selection_follow_up_android branch from 80aa94e to 32acb34 Compare September 18, 2024 19:38

github-actions bot added the puLL-Merge label Sep 18, 2024

simonhong approved these changes Sep 19, 2024

View reviewed changes

deeppandya merged commit 88bf0b8 into master Sep 19, 2024
17 checks passed

deeppandya deleted the advanced_server_selection_follow_up_android branch September 19, 2024 12:51

brave-builds added a commit that referenced this pull request Sep 19, 2024

Uplift of #25604 (squashed) to beta

48a4ebd

brave-builds mentioned this pull request Sep 19, 2024

Advanced server selection follow up android (uplift to 1.71.x) #25649

Open

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Advanced server selection follow up android #25604

Advanced server selection follow up android #25604

deeppandya commented Sep 17, 2024 •

edited

Loading

simonhong Sep 18, 2024

deeppandya Sep 18, 2024

simonhong Sep 19, 2024 •

edited

Loading

simonhong Sep 19, 2024

brave-builds commented Sep 18, 2024

brave-builds commented Sep 18, 2024

SergeyZhukovsky Sep 18, 2024

SergeyZhukovsky left a comment

github-actions bot commented Sep 18, 2024

Changes

brave-builds commented Sep 19, 2024

simonhong left a comment

Advanced server selection follow up android #25604

Advanced server selection follow up android #25604

Conversation

deeppandya commented Sep 17, 2024 • edited Loading

Submitter Checklist:

Reviewer Checklist:

After-merge Checklist:

Test Plan:

simonhong Sep 18, 2024

Choose a reason for hiding this comment

deeppandya Sep 18, 2024

Choose a reason for hiding this comment

simonhong Sep 19, 2024 • edited Loading

Choose a reason for hiding this comment

simonhong Sep 19, 2024

Choose a reason for hiding this comment

brave-builds commented Sep 18, 2024

brave-builds commented Sep 18, 2024

SergeyZhukovsky Sep 18, 2024

Choose a reason for hiding this comment

SergeyZhukovsky left a comment

Choose a reason for hiding this comment

github-actions bot commented Sep 18, 2024

Description

Changes

Possible Issues

Security Hotspots

brave-builds commented Sep 19, 2024

simonhong left a comment

Choose a reason for hiding this comment

deeppandya commented Sep 17, 2024 •

edited

Loading

simonhong Sep 19, 2024 •

edited

Loading