Update Geolocation permission bubble

[simonhong]

fix brave/brave-browser#16897

Added more information to users for getting precise location.
With passed enableHighAccuracy, permission bubble could give more precise info.

When page requests current position w/o highAccuracy, below bubble should shown.
[Type A]

When page requests current position with highAccuracy, we have two types of bubble.
When os location service is enabled, below bubble is shown
[Type B]

When os location service is disabled, this is shown.
[Type C]

Resolves

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:
  • https://github.com/brave/brave-browser/wiki/Deviations-from-Chromium-(features-we-disable-or-remove)
  • https://github.com/brave/brave-browser/wiki/Proxy-redirected-URLs
  • https://github.com/brave/brave-browser/wiki/Fingerprinting-Protections
  • https://github.com/brave/brave-browser/wiki/Brave%E2%80%99s-Use-of-Referral-Codes
  • https://github.com/brave/brave-browser/wiki/Web-Compatibility-Exceptions-in-Brave
  • https://github.com/brave/brave-browser/wiki/QA-Guide
  • https://github.com/brave/brave-browser/wiki/P3A

Test Plan:

GeolocationPermissionRequestBrowserTest.SetEnabledHighAccuracyTest
LocalizationUtilTest.GetLocalizedResourceUTF16StringWithPlaceholders

NOTE: geolocation permission bubble is not changed on Linux

1. Launch browser and open inspector with any tabs
2. Run navigator.geolocation.getCurrentPosition(() => {}, () => {}, { enableHighAccuracy : false });
3. check Type A bubble is shown
4. Dismiss bubble and reset permission state if needed
5. Disable OS location service
6. Run navigator.geolocation.getCurrentPosition(() => {}, () => {}, { enableHighAccuracy : true });
7. Chek Type B bubble is shown
8. Dismiss bubble and reset permission state if needed
9. Enable OS location service (On macOS, brave should be enabled also)
10. Run navigator.geolocation.getCurrentPosition(() => {}, () => {}, { enableHighAccuracy : true });
11. Check Type C bubble is shown

[simonhong]

If permission bubble knows about whether web site requested geolocation with high accuracy or not,
it would be helpful to give more precise info(warning) to users about geolocation permission request from web site.

Renderer uses Geolocation mojo interface(geolocation.mojom) and it's used from WebContentsImpl.
It means, it's in contents layer internal implementation so it's hard to get about high accuracy info from client layer.
Instead of touching WebContents, Geolocation, GeolocationContext interface and etc, it would be more simple
to have separated mojo interface to simply passing it from renderer to browser process.

[simonhong]

PermissionPromptBubbleOneOriginViewTest.AccessibleTitleDoesNotMentionTooManyPermissions is failed in PR builder but it's ok on local build. checking. => Fixed. Browser could be null in some unit tests.

[goodov]

I was thinking of using placeholders like so, but maybe that's not the best way, I'm not sure.

IDS_GEOLOCATION_PERMISSION_BUBBLE_REQUESTED_LOCATION_TYPE_LABEL:
This site has requested your <ph name="LOCATION_TYPE">$1<ex>general location</ex></ph>. <ph name="LEARN_MORE">$2<ex>Learn more</ex></ph>.

IDS_GEOLOCATION_PERMISSION_BUBBLE_LOCATION_TYPE_GENERAL desc="Used as a placeholder replacement in IDS_GEOLOCATION_PERMISSION_BUBBLE_REQUESTED_LOCATION_TYPE_LABEL":
general location

IDS_GEOLOCATION_PERMISSION_BUBBLE_LOCATION_TYPE_PRECISE desc="Used as a placeholder replacement in IDS_GEOLOCATION_PERMISSION_BUBBLE_REQUESTED_LOCATION_TYPE_LABEL":
precise location

IDS_GEOLOCATION_PERMISSION_BUBBLE_REQUESTED_LOCATION_TYPE_WITHOUT_LOCATION_SERVICE_LABEL:
Brave will only able to provide <ph name="LOCATION_TYPE">$1<ex>general location</ex></ph> data due to <ph name="LOCATION_SERVICES">$2<ex>Location Services</ex></ph> being disabled. <ph name="LEARN_MORE">$2<ex>Learn more</ex></ph>

IDS_GEOLOCATION_PERMISSION_BUBBLE_LOCATION_SERVICES:
Location Services

[goodov]

please remove these export variables and add brave_geolocation_permission_blink directly into blink core via brave_blink_renderer_core_deps.

[simonhong]

Addressed via 17ba984 but it seems above defines are needed.
Got build failure w/o them.

[github-actions]

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

Description

This PR adds more detailed informational text to the geolocation permission request dialog. The text varies depending on if the user has requested high accuracy location and if the system location services are enabled. It also includes a "Learn more" link to platform-specific documentation about location services.

Changes

Changes

• app/brave_generated_resources.grd: Added new string resources for the geolocation permission dialog text with placeholders.
• browser/brave_content_browser_client.cc: Registered the BraveGeolocationPermission interface.
• browser/brave_tab_helpers.cc: Created the BraveGeolocationPermissionTabHelper for each web contents.
• browser/ui/BUILD.gn: Added build targets for the new geolocation permission code.
• browser/ui/geolocation/brave_geolocation_browsertest.cc: Added a browser test for the BraveGeolocationPermissionTabHelper.
• browser/ui/geolocation/brave_geolocation_permission_tab_helper.cc and browser/ui/geolocation/brave_geolocation_permission_tab_helper.h: Implemented the tab helper to track if high accuracy was requested by the page.
• browser/ui/geolocation/geolocation_utils_*.cc: Added platform-specific code to check if the system location services are enabled.
• chromium_src/chrome/browser/ui/views/permissions/permission_prompt_bubble_base_view.cc: Modified the permission dialog to include the new geolocation text.
• components/brave_geolocation_permission/common/: Added a new Mojo interface for the renderer to communicate to the browser if high accuracy is requested.

Security Hotspots

1. Low: The IsSystemLocationSettingEnabled functions make platform-specific system calls to check location service status. Ensure the implementations cannot be abused.
2. Low: The new text in the permission dialog includes a link to an external site. Ensure the URL is safe and controlled by Brave.
3. Low: The renderer uses Mojo to notify the browser of high accuracy requests. Ensure this cannot be abused by a compromised renderer.

The changes overall look safe and well-contained. The main security consideration is ensuring the platform-specific code cannot enable location access improperly. The Mojo usage also warrants a careful review, but follows established patterns. Adding tests is good to see for a user-facing change like this.

[bsclifton]

Tried the different states - each worked great 😄👍

Nice cleanup on the placeholders too - thanks @goodov for the help

[simonhong]

It's finally fixed. thanks all for great review!

[rishiraj88]

Great thanks to you, @simonhong , for taking this humongous activity up. It has been as huge to fix as useful it is to end-users.

[mkarolin]

@simonhong, you can use translateable="false" attribute in the future if the string doesn't need to be translated.

[simonhong]

Got it, thanks!