From a51201050aa488692d889a8f1ca6edffee871405 Mon Sep 17 00:00:00 2001 From: Simon Hong Date: Wed, 24 Apr 2024 15:29:07 +0900 Subject: [PATCH 01/19] Update Geolocation permission dialog fix https://github.com/brave/brave-browser/issues/16897 Added more information to users for getting precise location. --- app/brave_generated_resources.grd | 9 ++ browser/ui/BUILD.gn | 7 + .../ui/geolocation/geolocation_utils_win.cc | 52 ++++++++ .../ui/geolocation/geolocation_utils_win.h | 16 +++ .../permission_prompt_bubble_base_view.cc | 123 +++++++++++++++++- 5 files changed, 206 insertions(+), 1 deletion(-) create mode 100644 browser/ui/geolocation/geolocation_utils_win.cc create mode 100644 browser/ui/geolocation/geolocation_utils_win.h diff --git a/app/brave_generated_resources.grd b/app/brave_generated_resources.grd index 1d5c1e1894df..e46522252cb3 100644 --- a/app/brave_generated_resources.grd +++ b/app/brave_generated_resources.grd @@ -557,6 +557,15 @@ Or change later at $2brave://settings/ext this setting + + + + You will share your $1exact location$2 with this site. Location services are active, providing detailed data. Trust this site before proceeding. $3Learn more$4 + + + You will share your $1general location$2 with this site. Location services are limited, enhancing privacy by only providing broad location data. $3Learn more$4 + + Close all diff --git a/browser/ui/BUILD.gn b/browser/ui/BUILD.gn index d95c3c2d637a..5c7cbde5b073 100644 --- a/browser/ui/BUILD.gn +++ b/browser/ui/BUILD.gn @@ -609,6 +609,13 @@ source_set("ui") { ] } + if (is_win) { + sources += [ + "geolocation/geolocation_utils_win.cc", + "geolocation/geolocation_utils_win.h", + ] + } + # brave's obsolete infobar is only used on Windows and Mac now. if (is_win || is_mac) { sources += [ diff --git a/browser/ui/geolocation/geolocation_utils_win.cc b/browser/ui/geolocation/geolocation_utils_win.cc new file mode 100644 index 000000000000..e4444bea5714 --- /dev/null +++ b/browser/ui/geolocation/geolocation_utils_win.cc @@ -0,0 +1,52 @@ +/* Copyright (c) 2024 The Brave Authors. All rights reserved. + * This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this file, + * You can obtain one at https://mozilla.org/MPL/2.0/. */ + +#include "brave/browser/ui/geolocation/geolocation_utils_win.h" + +#include + +#include +#include +#include + +#include "base/logging.h" +#include "base/win/core_winrt_util.h" + +using ABI::Windows::Devices::Enumeration::DeviceAccessStatus; +using ABI::Windows::Devices::Enumeration::DeviceClass; +using ABI::Windows::Devices::Enumeration::IDeviceAccessInformation; +using ABI::Windows::Devices::Enumeration::IDeviceAccessInformationStatics; +using Microsoft::WRL::ComPtr; + +namespace geolocation::win { + +// Copied from services/device/geolocation/win/location_provider_winrt.cc +bool IsSystemLocationSettingEnabled() { + ComPtr dev_access_info_statics; + HRESULT hr = base::win::GetActivationFactory< + IDeviceAccessInformationStatics, + RuntimeClass_Windows_Devices_Enumeration_DeviceAccessInformation>( + &dev_access_info_statics); + if (FAILED(hr)) { + VLOG(1) << "IDeviceAccessInformationStatics failed: " << hr; + return true; + } + + ComPtr dev_access_info; + hr = dev_access_info_statics->CreateFromDeviceClass( + DeviceClass::DeviceClass_Location, &dev_access_info); + if (FAILED(hr)) { + VLOG(1) << "IDeviceAccessInformation failed: " << hr; + return true; + } + + auto status = DeviceAccessStatus::DeviceAccessStatus_Unspecified; + dev_access_info->get_CurrentStatus(&status); + + return !(status == DeviceAccessStatus::DeviceAccessStatus_DeniedBySystem || + status == DeviceAccessStatus::DeviceAccessStatus_DeniedByUser); +} + +} // namespace geolocation::win diff --git a/browser/ui/geolocation/geolocation_utils_win.h b/browser/ui/geolocation/geolocation_utils_win.h new file mode 100644 index 000000000000..178754df8788 --- /dev/null +++ b/browser/ui/geolocation/geolocation_utils_win.h @@ -0,0 +1,16 @@ +/* Copyright (c) 2024 The Brave Authors. All rights reserved. + * This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this file, + * You can obtain one at https://mozilla.org/MPL/2.0/. */ + +#ifndef BRAVE_BROWSER_UI_GEOLOCATION_GEOLOCATION_UTILS_WIN_H_ +#define BRAVE_BROWSER_UI_GEOLOCATION_GEOLOCATION_UTILS_WIN_H_ + +namespace geolocation::win { + +// True when system location service is available to applications. +bool IsSystemLocationSettingEnabled(); + +} // namespace geolocation::win + +#endif // BRAVE_BROWSER_UI_GEOLOCATION_GEOLOCATION_UTILS_WIN_H_ diff --git a/chromium_src/chrome/browser/ui/views/permissions/permission_prompt_bubble_base_view.cc b/chromium_src/chrome/browser/ui/views/permissions/permission_prompt_bubble_base_view.cc index 9b4529c3c8db..d4d88da202d5 100644 --- a/chromium_src/chrome/browser/ui/views/permissions/permission_prompt_bubble_base_view.cc +++ b/chromium_src/chrome/browser/ui/views/permissions/permission_prompt_bubble_base_view.cc @@ -3,12 +3,17 @@ * License, v. 2.0. If a copy of the MPL was not distributed with this file, * You can obtain one at https://mozilla.org/MPL/2.0/. */ +#include "chrome/browser/ui/views/permissions/permission_prompt_bubble_base_view.h" + #include #include "base/feature_list.h" #include "base/memory/raw_ptr.h" #include "base/memory/raw_ref.h" +#include "base/strings/string_util.h" +#include "base/task/thread_pool.h" #include "brave/browser/ui/views/dialog_footnote_utils.h" +#include "brave/browser/ui/views/infobars/custom_styled_label.h" #include "brave/components/constants/url_constants.h" #include "brave/components/constants/webui_url_constants.h" #include "brave/components/l10n/common/localization_util.h" @@ -28,6 +33,7 @@ #include "components/permissions/request_type.h" #include "components/strings/grit/components_strings.h" #include "third_party/widevine/cdm/buildflags.h" +#include "ui/base/l10n/l10n_util.h" #include "ui/base/metadata/metadata_header_macros.h" #include "ui/base/metadata/metadata_impl_macros.h" #include "ui/base/models/combobox_model.h" @@ -42,10 +48,26 @@ #include "ui/views/window/dialog_client_view.h" #include "ui/views/window/dialog_delegate.h" +#if BUILDFLAG(IS_WIN) +#include "brave/browser/ui/geolocation/geolocation_utils_win.h" +#endif + #if BUILDFLAG(ENABLE_WIDEVINE) #include "brave/browser/widevine/widevine_permission_request.h" #endif +constexpr char kLearnMoreURL[] = +#if BUILDFLAG(IS_WIN) + "https://support.microsoft.com/en-us/windows/" + "windows-location-service-and-privacy-3a8eee0a-5b0b-dc07-eede-" + "2a5ca1c49088"; +#elif BUILDFLAG(IS_MAC) + "https://support.apple.com/guide/mac-help/" + "allow-apps-to-detect-the-location-of-your-mac-mh35873/mac"; +#else + "https://help.ubuntu.com/stable/ubuntu-help/privacy-location.html"; +#endif + namespace { #if BUILDFLAG(ENABLE_WIDEVINE) @@ -176,6 +198,104 @@ class PermissionLifetimeCombobox : public views::Combobox, BEGIN_METADATA(PermissionLifetimeCombobox) END_METADATA +void AddGeolocationDescription( + views::BubbleDialogDelegateView* dialog_delegate_view, + Browser* browser, + bool use_exact_location_label) { + auto container = std::make_unique(); + + constexpr int kPadding = 12; + container->SetLayoutManager(std::make_unique()) + ->set_inside_border_insets(gfx::Insets::TLBR(kPadding, 0, 0, 0)); + + // We put placeholders to get target offsets. + const std::vector placeholders{u"$1", u"$2", u"$3", u"$4"}; + std::vector offsets; + std::u16string contents_text = l10n_util::GetStringFUTF16( + use_exact_location_label + ? IDS_GEOLOCATION_PERMISSION_BUBBLE_EXACT_LOCATION_LABEL + : IDS_GEOLOCATION_PERMISSION_BUBBLE_GENERAL_LOCATION_LABEL, + placeholders, &offsets); + CHECK(offsets.size() == placeholders.size()); + + // Remove placeholers. It's used to get offsets. + for (const auto& placeholder : placeholders) { + base::RemoveChars(contents_text, placeholder, &contents_text); + } + + // Need to adjust offests after removing placeholders. + int offset_diff = 0; + for (size_t i = 0; i < offsets.size(); i++) { + offsets[i] -= offset_diff; + offset_diff += placeholders[i].length(); + } + + auto* contents_label = + container->AddChildView(std::make_unique()); + contents_label->SetTextContext(views::style::CONTEXT_LABEL); + contents_label->SetDefaultTextStyle(views::style::STYLE_PRIMARY); + contents_label->SetHorizontalAlignment(gfx::ALIGN_LEFT); + contents_label->SetText(contents_text); + views::StyledLabel::RangeStyleInfo part_style; + part_style.text_style = views::style::STYLE_EMPHASIZED; + contents_label->AddStyleRange(gfx::Range(offsets[0], offsets[1]), part_style); + + // It's ok to use |browser| in the link's callback as bubbble is tied with + // that |browser| and bubble is destroyed earlier than browser. + views::StyledLabel::RangeStyleInfo learn_more_style = + views::StyledLabel::RangeStyleInfo::CreateForLink(base::BindRepeating( + [](Browser* browser) { + chrome::AddSelectedTabWithURL(browser, GURL(kLearnMoreURL), + ui::PAGE_TRANSITION_AUTO_TOPLEVEL); + }, + browser)); + + contents_label->AddStyleRange(gfx::Range(offsets[2], offsets[3]), + learn_more_style); + + dialog_delegate_view->AddChildView(std::move(container)); +} + +void AddGeolocationDescriptionIfNeeded( + PermissionPromptBubbleBaseView* bubble_base_view, + permissions::PermissionPrompt::Delegate* delegate, + Browser* browser) { + auto requests = delegate->Requests(); + + // Geolocation permission is not grouped with others. + if (requests.empty() || + requests[0]->request_type() != permissions::RequestType::kGeolocation) { + return; + } + +#if BUILDFLAG(IS_WIN) + // IsSystemLocationSettingEnabled() uses COM. + base::ThreadPool::CreateCOMSTATaskRunner({base::MayBlock()}) + ->PostTaskAndReplyWithResult( + FROM_HERE, + base::BindOnce(&geolocation::win::IsSystemLocationSettingEnabled), + base::BindOnce( + [](base::WeakPtr widget_delegate_weak_ptr, + PermissionPromptBubbleBaseView* bubble_base_view, + Browser* browser, bool settings_enabled) { + if (!widget_delegate_weak_ptr) { + // Bubble is already gone. + return; + } + AddGeolocationDescription( + bubble_base_view, browser, + /*use_exact_location_label*/ settings_enabled); + + // To update widget layout after adding another child view. + bubble_base_view->UpdateAnchorPosition(); + }, + bubble_base_view->AsWeakPtr(), bubble_base_view, browser)); +#else + AddGeolocationDescription(bubble_base_view, browser, + /*use_exact_location_label*/ false); +#endif +} + views::View* AddPermissionLifetimeComboboxIfNeeded( views::BubbleDialogDelegateView* dialog_delegate_view, permissions::PermissionPrompt::Delegate* delegate) { @@ -257,7 +377,8 @@ void AddFootnoteViewIfNeeded( permission_lifetime_view->GetPreferredSize().width()) + \ margins().width()); \ set_should_ignore_snapping(true); \ - } + } \ + AddGeolocationDescriptionIfNeeded(this, delegate_.get(), browser_); #include "src/chrome/browser/ui/views/permissions/permission_prompt_bubble_base_view.cc" #undef BRAVE_PERMISSION_PROMPT_BUBBLE_BASE_VIEW From 736f3f9366f915d1e67d205c1034f1358e92723a Mon Sep 17 00:00:00 2001 From: Simon Hong Date: Mon, 29 Apr 2024 16:42:40 +0900 Subject: [PATCH 02/19] Consider enableHighAccuracy bit from web site Geolocation permission bubble consdiers enableHighAccuracy bit. --- browser/brave_content_browser_client.cc | 13 +++++ ...brave_geolocation_permission_tab_helper.cc | 52 +++++++++++++++++++ .../brave_geolocation_permission_tab_helper.h | 45 ++++++++++++++++ browser/brave_tab_helpers.cc | 2 + browser/sources.gni | 8 +++ .../permission_prompt_bubble_base_view.cc | 23 ++++++-- .../blink/renderer/modules/geolocation/DEPS | 3 ++ .../modules/geolocation/geolocation.cc | 50 ++++++++++++++++++ .../common/BUILD.gn | 14 +++++ .../common/brave_geolocation_permission.mojom | 10 ++++ ...r-modules-geolocation-geolocation.cc.patch | 12 +++++ third_party/blink/renderer/BUILD.gn | 5 +- 12 files changed, 232 insertions(+), 5 deletions(-) create mode 100644 browser/brave_geolocation_permission_tab_helper.cc create mode 100644 browser/brave_geolocation_permission_tab_helper.h create mode 100644 chromium_src/third_party/blink/renderer/modules/geolocation/DEPS create mode 100644 chromium_src/third_party/blink/renderer/modules/geolocation/geolocation.cc create mode 100644 components/brave_geolocation_permission/common/BUILD.gn create mode 100644 components/brave_geolocation_permission/common/brave_geolocation_permission.mojom create mode 100644 patches/third_party-blink-renderer-modules-geolocation-geolocation.cc.patch diff --git a/browser/brave_content_browser_client.cc b/browser/brave_content_browser_client.cc index 51297818dfe1..ab2869acbdf2 100644 --- a/browser/brave_content_browser_client.cc +++ b/browser/brave_content_browser_client.cc @@ -219,6 +219,7 @@ using extensions::ChromeContentBrowserClientExtensionsPart; #endif // BUILDFLAG(IS_ANDROID) #if !BUILDFLAG(IS_ANDROID) +#include "brave/browser/brave_geolocation_permission_tab_helper.h" #include "brave/browser/new_tab/new_tab_shows_navigation_throttle.h" #include "brave/browser/ui/webui/brave_news_internals/brave_news_internals_ui.h" #include "brave/browser/ui/webui/brave_rewards/rewards_panel_ui.h" @@ -564,6 +565,18 @@ void BraveContentBrowserClient:: &render_frame_host)); #endif // BUILDFLAG(ENABLE_WIDEVINE) +#if !BUILDFLAG(IS_ANDROID) + associated_registry.AddInterface< + geolocation::mojom::BraveGeolocationPermission>(base::BindRepeating( + [](content::RenderFrameHost* render_frame_host, + mojo::PendingAssociatedReceiver< + geolocation::mojom::BraveGeolocationPermission> receiver) { + BraveGeolocationPermissionTabHelper::BindBraveGeolocationPermission( + std::move(receiver), render_frame_host); + }, + &render_frame_host)); +#endif // BUILDFLAG(ENABLE_WIDEVINE) + associated_registry.AddInterface< brave_shields::mojom::BraveShieldsHost>(base::BindRepeating( [](content::RenderFrameHost* render_frame_host, diff --git a/browser/brave_geolocation_permission_tab_helper.cc b/browser/brave_geolocation_permission_tab_helper.cc new file mode 100644 index 000000000000..8b0fd0023dcf --- /dev/null +++ b/browser/brave_geolocation_permission_tab_helper.cc @@ -0,0 +1,52 @@ +/* Copyright (c) 2024 The Brave Authors. All rights reserved. + * This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this file, + * You can obtain one at https://mozilla.org/MPL/2.0/. */ + +#include "brave/browser/brave_geolocation_permission_tab_helper.h" + +#include + +#include "content/public/browser/navigation_handle.h" +#include "content/public/browser/web_contents.h" + +BraveGeolocationPermissionTabHelper::BraveGeolocationPermissionTabHelper( + content::WebContents* contents) + : WebContentsObserver(contents), + content::WebContentsUserData( + *contents), + brave_geolocation_permission_receivers_(contents, this) {} + +BraveGeolocationPermissionTabHelper::~BraveGeolocationPermissionTabHelper() = + default; + +// static +void BraveGeolocationPermissionTabHelper::BindBraveGeolocationPermission( + mojo::PendingAssociatedReceiver< + geolocation::mojom::BraveGeolocationPermission> receiver, + content::RenderFrameHost* rfh) { + auto* web_contents = content::WebContents::FromRenderFrameHost(rfh); + if (!web_contents) { + return; + } + + auto* tab_helper = + BraveGeolocationPermissionTabHelper::FromWebContents(web_contents); + if (!tab_helper) { + return; + } + tab_helper->brave_geolocation_permission_receivers_.Bind(rfh, + std::move(receiver)); +} + +void BraveGeolocationPermissionTabHelper::PrimaryPageChanged( + content::Page& page) { + enable_high_accuracy_ = false; +} + +void BraveGeolocationPermissionTabHelper::SetEnableHighAccuracy( + bool enable_high_accuracy) { + enable_high_accuracy_ = enable_high_accuracy; +} + +WEB_CONTENTS_USER_DATA_KEY_IMPL(BraveGeolocationPermissionTabHelper); diff --git a/browser/brave_geolocation_permission_tab_helper.h b/browser/brave_geolocation_permission_tab_helper.h new file mode 100644 index 000000000000..ceb4d586bc82 --- /dev/null +++ b/browser/brave_geolocation_permission_tab_helper.h @@ -0,0 +1,45 @@ +/* Copyright (c) 2024 The Brave Authors. All rights reserved. + * This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this file, + * You can obtain one at https://mozilla.org/MPL/2.0/. */ + +#ifndef BRAVE_BROWSER_BRAVE_GEOLOCATION_PERMISSION_TAB_HELPER_H_ +#define BRAVE_BROWSER_BRAVE_GEOLOCATION_PERMISSION_TAB_HELPER_H_ + +#include "brave/components/brave_geolocation_permission/common/brave_geolocation_permission.mojom.h" +#include "content/public/browser/render_frame_host_receiver_set.h" +#include "content/public/browser/web_contents_observer.h" +#include "content/public/browser/web_contents_user_data.h" + +class BraveGeolocationPermissionTabHelper final + : public content::WebContentsObserver, + public geolocation::mojom::BraveGeolocationPermission, + public content::WebContentsUserData { + public: + explicit BraveGeolocationPermissionTabHelper(content::WebContents* contents); + ~BraveGeolocationPermissionTabHelper() override; + + static void BindBraveGeolocationPermission( + mojo::PendingAssociatedReceiver< + geolocation::mojom::BraveGeolocationPermission> receiver, + content::RenderFrameHost* rfh); + + // content::WebContentsObserver + void PrimaryPageChanged(content::Page& page) override; + + // geolocation::mojom::BraveGeolocationPermission overrides: + void SetEnableHighAccuracy(bool high_accuracy) override; + + bool enable_high_accuracy() const { return enable_high_accuracy_; } + + WEB_CONTENTS_USER_DATA_KEY_DECL(); + + private: + content::RenderFrameHostReceiverSet< + geolocation::mojom::BraveGeolocationPermission> + brave_geolocation_permission_receivers_; + + bool enable_high_accuracy_ = false; +}; + +#endif // BRAVE_BROWSER_BRAVE_GEOLOCATION_PERMISSION_TAB_HELPER_H_ diff --git a/browser/brave_tab_helpers.cc b/browser/brave_tab_helpers.cc index 0006f81eb71c..a3d8d65cb52b 100644 --- a/browser/brave_tab_helpers.cc +++ b/browser/brave_tab_helpers.cc @@ -55,6 +55,7 @@ #endif #if !BUILDFLAG(IS_ANDROID) +#include "brave/browser/brave_geolocation_permission_tab_helper.h" #include "brave/browser/ui/brave_shields_data_controller.h" #include "chrome/browser/ui/thumbnails/thumbnail_tab_helper.h" #endif @@ -131,6 +132,7 @@ void AttachTabHelpers(content::WebContents* web_contents) { BraveBookmarkTabHelper::CreateForWebContents(web_contents); brave_shields::BraveShieldsDataController::CreateForWebContents(web_contents); ThumbnailTabHelper::CreateForWebContents(web_contents); + BraveGeolocationPermissionTabHelper::CreateForWebContents(web_contents); #endif brave_rewards::RewardsTabHelper::CreateForWebContents(web_contents); diff --git a/browser/sources.gni b/browser/sources.gni index 290a9755c978..764a6491900a 100644 --- a/browser/sources.gni +++ b/browser/sources.gni @@ -399,6 +399,14 @@ if (enable_brave_wayback_machine) { brave_chrome_browser_deps += [ "//brave/components/brave_wayback_machine" ] } +if (!is_android) { + brave_chrome_browser_sources += [ + "//brave/browser/brave_geolocation_permission_tab_helper.cc", + "//brave/browser/brave_geolocation_permission_tab_helper.h", + ] + brave_chrome_browser_deps += [ "//brave/components/brave_geolocation_permission/common:brave_geolocation_permission" ] +} + if (enable_widevine) { brave_chrome_browser_sources += [ "//brave/browser/brave_drm_tab_helper.cc", diff --git a/chromium_src/chrome/browser/ui/views/permissions/permission_prompt_bubble_base_view.cc b/chromium_src/chrome/browser/ui/views/permissions/permission_prompt_bubble_base_view.cc index d4d88da202d5..f5ad396aa350 100644 --- a/chromium_src/chrome/browser/ui/views/permissions/permission_prompt_bubble_base_view.cc +++ b/chromium_src/chrome/browser/ui/views/permissions/permission_prompt_bubble_base_view.cc @@ -12,6 +12,7 @@ #include "base/memory/raw_ref.h" #include "base/strings/string_util.h" #include "base/task/thread_pool.h" +#include "brave/browser/brave_geolocation_permission_tab_helper.h" #include "brave/browser/ui/views/dialog_footnote_utils.h" #include "brave/browser/ui/views/infobars/custom_styled_label.h" #include "brave/components/constants/url_constants.h" @@ -201,6 +202,7 @@ END_METADATA void AddGeolocationDescription( views::BubbleDialogDelegateView* dialog_delegate_view, Browser* browser, + bool enable_high_accuracy, bool use_exact_location_label) { auto container = std::make_unique(); @@ -212,7 +214,7 @@ void AddGeolocationDescription( const std::vector placeholders{u"$1", u"$2", u"$3", u"$4"}; std::vector offsets; std::u16string contents_text = l10n_util::GetStringFUTF16( - use_exact_location_label + (use_exact_location_label && enable_high_accuracy) ? IDS_GEOLOCATION_PERMISSION_BUBBLE_EXACT_LOCATION_LABEL : IDS_GEOLOCATION_PERMISSION_BUBBLE_GENERAL_LOCATION_LABEL, placeholders, &offsets); @@ -268,6 +270,15 @@ void AddGeolocationDescriptionIfNeeded( return; } + bool enable_high_accuracy = false; + if (auto* web_contents = delegate->GetAssociatedWebContents()) { + if (auto* geolocation_tab_helper = + BraveGeolocationPermissionTabHelper::FromWebContents( + web_contents)) { + enable_high_accuracy = geolocation_tab_helper->enable_high_accuracy(); + } + } + #if BUILDFLAG(IS_WIN) // IsSystemLocationSettingEnabled() uses COM. base::ThreadPool::CreateCOMSTATaskRunner({base::MayBlock()}) @@ -277,22 +288,26 @@ void AddGeolocationDescriptionIfNeeded( base::BindOnce( [](base::WeakPtr widget_delegate_weak_ptr, PermissionPromptBubbleBaseView* bubble_base_view, - Browser* browser, bool settings_enabled) { + Browser* browser, bool enable_high_accuracy, + bool settings_enabled) { if (!widget_delegate_weak_ptr) { // Bubble is already gone. return; } AddGeolocationDescription( bubble_base_view, browser, + /*enable_high_accuracy*/ enable_high_accuracy, /*use_exact_location_label*/ settings_enabled); // To update widget layout after adding another child view. bubble_base_view->UpdateAnchorPosition(); }, - bubble_base_view->AsWeakPtr(), bubble_base_view, browser)); + bubble_base_view->AsWeakPtr(), bubble_base_view, browser, + enable_high_accuracy)); #else AddGeolocationDescription(bubble_base_view, browser, - /*use_exact_location_label*/ false); + /*use_exact_location_label*/ false, + /*enable_high_accuracy*/ false); #endif } diff --git a/chromium_src/third_party/blink/renderer/modules/geolocation/DEPS b/chromium_src/third_party/blink/renderer/modules/geolocation/DEPS new file mode 100644 index 000000000000..992f5164a20c --- /dev/null +++ b/chromium_src/third_party/blink/renderer/modules/geolocation/DEPS @@ -0,0 +1,3 @@ +include_rules = [ + "+brave/components/brave_geolocation_permission/common", +] diff --git a/chromium_src/third_party/blink/renderer/modules/geolocation/geolocation.cc b/chromium_src/third_party/blink/renderer/modules/geolocation/geolocation.cc new file mode 100644 index 000000000000..5a03d8811675 --- /dev/null +++ b/chromium_src/third_party/blink/renderer/modules/geolocation/geolocation.cc @@ -0,0 +1,50 @@ +/* Copyright (c) 2024 The Brave Authors. All rights reserved. + * This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this file, + * You can obtain one at https://mozilla.org/MPL/2.0/. */ + +#include "brave/components/brave_geolocation_permission/common/brave_geolocation_permission.mojom-blink.h" +#include "mojo/public/cpp/bindings/associated_remote.h" +#include "third_party/blink/public/common/associated_interfaces/associated_interface_provider.h" +#include "third_party/blink/renderer/core/frame/local_frame_client.h" + +namespace blink { +namespace { + +void SetEnableHighAccuracy(LocalFrame* frame, bool enable_high_accuracy); + +} // namespace +} // namespace blink + +// Pass enabledHighAccuracy bit to browser to make geolocation permission +// bubble gives more detailed infos. +// Renderer uses |Geolocation| mojo interface and it's used |WebContentsImpl|. +// It means it's in internal content layer impls so hard to get about it from +// client layer. Instead of touching |WebContents|, |Geolocation|, +// |GeolocationContext| interfaces, it would be more simple to pass via +// separated mojo interface. +#define BRAVE_START_REQUEST \ + SetEnableHighAccuracy(GetFrame(), notifier->Options()->enableHighAccuracy()); + +#include "src/third_party/blink/renderer/modules/geolocation/geolocation.cc" + +#undef BRAVE_START_REQUEST + +namespace blink { +namespace { + +void SetEnableHighAccuracy(LocalFrame* frame, bool enable_high_accuracy) { + if (frame->Client()->GetRemoteNavigationAssociatedInterfaces()) { + mojo::AssociatedRemote< + geolocation::mojom::blink::BraveGeolocationPermission> + brave_geolocation_permission_binding; + frame->Client()->GetRemoteNavigationAssociatedInterfaces()->GetInterface( + &brave_geolocation_permission_binding); + CHECK(brave_geolocation_permission_binding.is_bound()); + brave_geolocation_permission_binding->SetEnableHighAccuracy( + enable_high_accuracy); + } +} + +} // namespace +} // namespace blink diff --git a/components/brave_geolocation_permission/common/BUILD.gn b/components/brave_geolocation_permission/common/BUILD.gn new file mode 100644 index 000000000000..970e54d35bd6 --- /dev/null +++ b/components/brave_geolocation_permission/common/BUILD.gn @@ -0,0 +1,14 @@ +# Copyright (c) 2024 The Brave Authors. All rights reserved. +# This Source Code Form is subject to the terms of the Mozilla Public +# License, v. 2.0. If a copy of the MPL was not distributed with this file, +# You can obtain one at https://mozilla.org/MPL/2.0/. + +import("//mojo/public/tools/bindings/mojom.gni") + +mojom("brave_geolocation_permission") { + sources = [ "brave_geolocation_permission.mojom" ] + + export_class_attribute_blink = "CORE_EXPORT" + export_define_blink = "BLINK_CORE_IMPLEMENTATION=1" + export_header_blink = "third_party/blink/renderer/core/core_export.h" +} diff --git a/components/brave_geolocation_permission/common/brave_geolocation_permission.mojom b/components/brave_geolocation_permission/common/brave_geolocation_permission.mojom new file mode 100644 index 000000000000..7c538bdf7995 --- /dev/null +++ b/components/brave_geolocation_permission/common/brave_geolocation_permission.mojom @@ -0,0 +1,10 @@ +// Copyright (c) 2024 The Brave Authors. All rights reserved. +// This Source Code Form is subject to the terms of the Mozilla Public +// License, v. 2.0. If a copy of the MPL was not distributed with this file, +// You can obtain one at https://mozilla.org/MPL/2.0/. + +module geolocation.mojom; + +interface BraveGeolocationPermission { + SetEnableHighAccuracy(bool high_accuracy); +}; diff --git a/patches/third_party-blink-renderer-modules-geolocation-geolocation.cc.patch b/patches/third_party-blink-renderer-modules-geolocation-geolocation.cc.patch new file mode 100644 index 000000000000..ad3bcbdae0f6 --- /dev/null +++ b/patches/third_party-blink-renderer-modules-geolocation-geolocation.cc.patch @@ -0,0 +1,12 @@ +diff --git a/third_party/blink/renderer/modules/geolocation/geolocation.cc b/third_party/blink/renderer/modules/geolocation/geolocation.cc +index 1051a55224e0665e6b6672e53bdb423bd398f480..0488005f08dd53e277978856ed955a8fd06ae351 100644 +--- a/third_party/blink/renderer/modules/geolocation/geolocation.cc ++++ b/third_party/blink/renderer/modules/geolocation/geolocation.cc +@@ -257,6 +257,7 @@ void Geolocation::StartRequest(GeoNotifier* notifier) { + if (HaveSuitableCachedPosition(notifier->Options())) { + notifier->SetUseCachedPosition(); + } else { ++ BRAVE_START_REQUEST + StartUpdating(notifier); + } + } diff --git a/third_party/blink/renderer/BUILD.gn b/third_party/blink/renderer/BUILD.gn index 53f5c6283141..baef94fa8607 100644 --- a/third_party/blink/renderer/BUILD.gn +++ b/third_party/blink/renderer/BUILD.gn @@ -10,7 +10,10 @@ component("renderer") { "brave_font_whitelist.h", ] - deps = [ "//brave/components/brave_drm:brave_drm_blink" ] + deps = [ + "//brave/components/brave_drm:brave_drm_blink", + "//brave/components/brave_geolocation_permission/common:brave_geolocation_permission_blink", + ] defines = [ "BLINK_IMPLEMENTATION=1" ] From ed62798cde88e922d6f8387f095444792e14d640 Mon Sep 17 00:00:00 2001 From: Simon Hong Date: Tue, 30 Apr 2024 12:06:43 +0900 Subject: [PATCH 03/19] Updated bubble text and added icon based on high accuracy state --- app/brave_generated_resources.grd | 11 ++- .../permission_prompt_bubble_base_view.cc | 91 ++++++++++++++----- 2 files changed, 77 insertions(+), 25 deletions(-) diff --git a/app/brave_generated_resources.grd b/app/brave_generated_resources.grd index e46522252cb3..123815147287 100644 --- a/app/brave_generated_resources.grd +++ b/app/brave_generated_resources.grd @@ -559,11 +559,14 @@ Or change later at $2brave://settings/ext - - You will share your $1exact location$2 with this site. Location services are active, providing detailed data. Trust this site before proceeding. $3Learn more$4 + + This site has requested your $1general location$2. $3Learn more$4 - - You will share your $1general location$2 with this site. Location services are limited, enhancing privacy by only providing broad location data. $3Learn more$4 + + This site has requested your $1precise location$2. $3Learn more$4 + + + This site has requested your $1precise location$2. Brave will only able to provide $3general location$4 data due to $5Location Services$6 being disabled. $7Learn more$8 diff --git a/chromium_src/chrome/browser/ui/views/permissions/permission_prompt_bubble_base_view.cc b/chromium_src/chrome/browser/ui/views/permissions/permission_prompt_bubble_base_view.cc index f5ad396aa350..70342d243368 100644 --- a/chromium_src/chrome/browser/ui/views/permissions/permission_prompt_bubble_base_view.cc +++ b/chromium_src/chrome/browser/ui/views/permissions/permission_prompt_bubble_base_view.cc @@ -20,6 +20,7 @@ #include "brave/components/l10n/common/localization_util.h" #include "brave/components/permissions/permission_lifetime_utils.h" #include "brave/components/permissions/permission_widevine_utils.h" +#include "brave/components/vector_icons/vector_icons.h" #include "brave/grit/brave_generated_resources.h" #include "chrome/browser/ui/browser.h" #include "chrome/browser/ui/browser_tabstrip.h" @@ -38,10 +39,12 @@ #include "ui/base/metadata/metadata_header_macros.h" #include "ui/base/metadata/metadata_impl_macros.h" #include "ui/base/models/combobox_model.h" +#include "ui/base/models/image_model.h" #include "ui/gfx/text_constants.h" #include "ui/views/bubble/bubble_dialog_delegate_view.h" #include "ui/views/controls/button/checkbox.h" #include "ui/views/controls/combobox/combobox.h" +#include "ui/views/controls/image_view.h" #include "ui/views/controls/label.h" #include "ui/views/controls/styled_label.h" #include "ui/views/layout/box_layout.h" @@ -199,25 +202,25 @@ class PermissionLifetimeCombobox : public views::Combobox, BEGIN_METADATA(PermissionLifetimeCombobox) END_METADATA -void AddGeolocationDescription( - views::BubbleDialogDelegateView* dialog_delegate_view, +std::unique_ptr CreateGeolocationDescLabel( Browser* browser, bool enable_high_accuracy, - bool use_exact_location_label) { - auto container = std::make_unique(); - - constexpr int kPadding = 12; - container->SetLayoutManager(std::make_unique()) - ->set_inside_border_insets(gfx::Insets::TLBR(kPadding, 0, 0, 0)); - - // We put placeholders to get target offsets. - const std::vector placeholders{u"$1", u"$2", u"$3", u"$4"}; + bool location_service_is_on) { + // We put placeholders raw string to get target offsets. + std::vector placeholders{u"$1", u"$2", u"$3", u"$4"}; + if (enable_high_accuracy && !location_service_is_on) { + placeholders.insert(placeholders.end(), {u"$5", u"$6", u"$7", u"$8"}); + } + int string_id = IDS_GEOLOCATION_PERMISSION_BUBBLE_LOW_ACCURACY_LABEL; + if (enable_high_accuracy) { + string_id = + location_service_is_on + ? IDS_GEOLOCATION_PERMISSION_BUBBLE_HIGH_ACCURACY_WITH_LOCATION_SERVICE_LABEL + : IDS_GEOLOCATION_PERMISSION_BUBBLE_HIGH_ACCURACY_WITHOUT_LOCATION_SERVICE_LABEL; + } std::vector offsets; - std::u16string contents_text = l10n_util::GetStringFUTF16( - (use_exact_location_label && enable_high_accuracy) - ? IDS_GEOLOCATION_PERMISSION_BUBBLE_EXACT_LOCATION_LABEL - : IDS_GEOLOCATION_PERMISSION_BUBBLE_GENERAL_LOCATION_LABEL, - placeholders, &offsets); + std::u16string contents_text = + l10n_util::GetStringFUTF16(string_id, placeholders, &offsets); CHECK(offsets.size() == placeholders.size()); // Remove placeholers. It's used to get offsets. @@ -232,15 +235,21 @@ void AddGeolocationDescription( offset_diff += placeholders[i].length(); } - auto* contents_label = - container->AddChildView(std::make_unique()); + auto contents_label = std::make_unique(); contents_label->SetTextContext(views::style::CONTEXT_LABEL); contents_label->SetDefaultTextStyle(views::style::STYLE_PRIMARY); contents_label->SetHorizontalAlignment(gfx::ALIGN_LEFT); contents_label->SetText(contents_text); views::StyledLabel::RangeStyleInfo part_style; part_style.text_style = views::style::STYLE_EMPHASIZED; - contents_label->AddStyleRange(gfx::Range(offsets[0], offsets[1]), part_style); + const int part_count_except_learn_more = placeholders.size() / 2 - 1; + for (int i = 0; i < part_count_except_learn_more; ++i) { + // Each part has start/end offset pair. + const int part_start_offset = i * 2; + contents_label->AddStyleRange( + gfx::Range(offsets[part_start_offset], offsets[part_start_offset + 1]), + part_style); + } // It's ok to use |browser| in the link's callback as bubbble is tied with // that |browser| and bubble is destroyed earlier than browser. @@ -252,9 +261,49 @@ void AddGeolocationDescription( }, browser)); - contents_label->AddStyleRange(gfx::Range(offsets[2], offsets[3]), - learn_more_style); + // Learn more is last part. + const int learn_more_offset = placeholders.size() - 2; + contents_label->AddStyleRange( + gfx::Range(offsets[learn_more_offset], offsets[learn_more_offset + 1]), + learn_more_style); + constexpr int kFixedLabelWidth = 290; + contents_label->SizeToFit(kFixedLabelWidth); + return contents_label; +} +std::unique_ptr CreateGeolocationDescIcon( + bool enable_high_accuracy, + bool location_service_is_on) { + const bool use_high_accuracy = enable_high_accuracy && location_service_is_on; + constexpr int kIconSize = 16; + auto icon_view = + std::make_unique(ui::ImageModel::FromVectorIcon( + use_high_accuracy ? kLeoWarningTriangleOutlineIcon + : kLeoInfoOutlineIcon, + ui::ColorIds::kColorMenuIcon, kIconSize)); + // To align with text more precisely. + icon_view->SetBorder(views::CreateEmptyBorder(gfx::Insets::TLBR(2, 0, 0, 0))); + return icon_view; +} + +void AddGeolocationDescription( + views::BubbleDialogDelegateView* dialog_delegate_view, + Browser* browser, + bool enable_high_accuracy, + bool location_service_is_on) { + auto container = std::make_unique(); + constexpr int kPadding = 12; + constexpr int kChildSpacing = 6; + container + ->SetLayoutManager(std::make_unique( + views::BoxLayout::Orientation::kHorizontal, + gfx::Insets::TLBR(kPadding, 0, 0, 0), kChildSpacing)) + ->set_cross_axis_alignment(views::BoxLayout::CrossAxisAlignment::kStart); + + container->AddChildView( + CreateGeolocationDescIcon(enable_high_accuracy, location_service_is_on)); + container->AddChildView(CreateGeolocationDescLabel( + browser, enable_high_accuracy, location_service_is_on)); dialog_delegate_view->AddChildView(std::move(container)); } From 4dea3bf2fc1226d0c5faea6b9278d74fb5773402 Mon Sep 17 00:00:00 2001 From: Simon Hong Date: Tue, 30 Apr 2024 15:53:32 +0900 Subject: [PATCH 04/19] WIP: Check location service status on macOS --- browser/ui/BUILD.gn | 15 +++++++-- browser/ui/geolocation/geolocation_utils.cc | 18 +++++++++++ ...cation_utils_win.h => geolocation_utils.h} | 10 +++--- .../ui/geolocation/geolocation_utils_mac.mm | 32 +++++++++++++++++++ .../ui/geolocation/geolocation_utils_win.cc | 6 ++-- .../permission_prompt_bubble_base_view.cc | 12 +++---- 6 files changed, 74 insertions(+), 19 deletions(-) create mode 100644 browser/ui/geolocation/geolocation_utils.cc rename browser/ui/geolocation/{geolocation_utils_win.h => geolocation_utils.h} (60%) create mode 100644 browser/ui/geolocation/geolocation_utils_mac.mm diff --git a/browser/ui/BUILD.gn b/browser/ui/BUILD.gn index 5c7cbde5b073..522994a6d33a 100644 --- a/browser/ui/BUILD.gn +++ b/browser/ui/BUILD.gn @@ -609,11 +609,20 @@ source_set("ui") { ] } - if (is_win) { + if (is_win || is_mac || is_linux) { sources += [ - "geolocation/geolocation_utils_win.cc", - "geolocation/geolocation_utils_win.h", + "geolocation/geolocation_utils.cc", + "geolocation/geolocation_utils.h", ] + + if (is_win) { + sources += [ "geolocation/geolocation_utils_win.cc" ] + } + + if (is_mac) { + sources += [ "geolocation/geolocation_utils_mac.mm" ] + frameworks = [ "CoreLocation.framework" ] + } } # brave's obsolete infobar is only used on Windows and Mac now. diff --git a/browser/ui/geolocation/geolocation_utils.cc b/browser/ui/geolocation/geolocation_utils.cc new file mode 100644 index 000000000000..94cd3f606100 --- /dev/null +++ b/browser/ui/geolocation/geolocation_utils.cc @@ -0,0 +1,18 @@ +/* Copyright (c) 2024 The Brave Authors. All rights reserved. + * This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this file, + * You can obtain one at https://mozilla.org/MPL/2.0/. */ + +#include "brave/browser/ui/geolocation/geolocation_utils.h" + +#include "build/build_config.h" + +namespace geolocation { + +#if BUILDFLAG(IS_LINUX) +bool IsSystemLocationSettingEnabled() { + return false; +} +#endif + +} // namespace geolocation diff --git a/browser/ui/geolocation/geolocation_utils_win.h b/browser/ui/geolocation/geolocation_utils.h similarity index 60% rename from browser/ui/geolocation/geolocation_utils_win.h rename to browser/ui/geolocation/geolocation_utils.h index 178754df8788..093234cb0745 100644 --- a/browser/ui/geolocation/geolocation_utils_win.h +++ b/browser/ui/geolocation/geolocation_utils.h @@ -3,14 +3,14 @@ * License, v. 2.0. If a copy of the MPL was not distributed with this file, * You can obtain one at https://mozilla.org/MPL/2.0/. */ -#ifndef BRAVE_BROWSER_UI_GEOLOCATION_GEOLOCATION_UTILS_WIN_H_ -#define BRAVE_BROWSER_UI_GEOLOCATION_GEOLOCATION_UTILS_WIN_H_ +#ifndef BRAVE_BROWSER_UI_GEOLOCATION_GEOLOCATION_UTILS_H_ +#define BRAVE_BROWSER_UI_GEOLOCATION_GEOLOCATION_UTILS_H_ -namespace geolocation::win { +namespace geolocation { // True when system location service is available to applications. bool IsSystemLocationSettingEnabled(); -} // namespace geolocation::win +} // namespace geolocation -#endif // BRAVE_BROWSER_UI_GEOLOCATION_GEOLOCATION_UTILS_WIN_H_ +#endif // BRAVE_BROWSER_UI_GEOLOCATION_GEOLOCATION_UTILS_H_ diff --git a/browser/ui/geolocation/geolocation_utils_mac.mm b/browser/ui/geolocation/geolocation_utils_mac.mm new file mode 100644 index 000000000000..bc1a17f11dce --- /dev/null +++ b/browser/ui/geolocation/geolocation_utils_mac.mm @@ -0,0 +1,32 @@ +/* Copyright (c) 2024 The Brave Authors. All rights reserved. + * This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this file, + * You can obtain one at https://mozilla.org/MPL/2.0/. */ + +#include "brave/browser/ui/geolocation/geolocation_utils.h" + +#import + +#include "base/logging.h" + +namespace geolocation { + +bool IsSystemLocationSettingEnabled() { + // Service is off globally. + if (![CLLocationManager locationServicesEnabled]) { + return false; + } + + CLLocationManager* manager = [[CLLocationManager alloc] init]; + if (@available(macOS 11.0, *)) { + auto status = [manager authorizationStatus]; + LOG(ERROR) << __func__ << " ##### " << (int)status; + if (status == kCLAuthorizationStatusAuthorized) { + return true; + } + } + + return false; +} + +} // namespace geolocation diff --git a/browser/ui/geolocation/geolocation_utils_win.cc b/browser/ui/geolocation/geolocation_utils_win.cc index e4444bea5714..563cd12b8867 100644 --- a/browser/ui/geolocation/geolocation_utils_win.cc +++ b/browser/ui/geolocation/geolocation_utils_win.cc @@ -3,7 +3,7 @@ * License, v. 2.0. If a copy of the MPL was not distributed with this file, * You can obtain one at https://mozilla.org/MPL/2.0/. */ -#include "brave/browser/ui/geolocation/geolocation_utils_win.h" +#include "brave/browser/ui/geolocation/geolocation_utils.h" #include @@ -20,7 +20,7 @@ using ABI::Windows::Devices::Enumeration::IDeviceAccessInformation; using ABI::Windows::Devices::Enumeration::IDeviceAccessInformationStatics; using Microsoft::WRL::ComPtr; -namespace geolocation::win { +namespace geolocation { // Copied from services/device/geolocation/win/location_provider_winrt.cc bool IsSystemLocationSettingEnabled() { @@ -49,4 +49,4 @@ bool IsSystemLocationSettingEnabled() { status == DeviceAccessStatus::DeviceAccessStatus_DeniedByUser); } -} // namespace geolocation::win +} // namespace geolocation diff --git a/chromium_src/chrome/browser/ui/views/permissions/permission_prompt_bubble_base_view.cc b/chromium_src/chrome/browser/ui/views/permissions/permission_prompt_bubble_base_view.cc index 70342d243368..1b8f0b98da44 100644 --- a/chromium_src/chrome/browser/ui/views/permissions/permission_prompt_bubble_base_view.cc +++ b/chromium_src/chrome/browser/ui/views/permissions/permission_prompt_bubble_base_view.cc @@ -13,6 +13,7 @@ #include "base/strings/string_util.h" #include "base/task/thread_pool.h" #include "brave/browser/brave_geolocation_permission_tab_helper.h" +#include "brave/browser/ui/geolocation/geolocation_utils.h" #include "brave/browser/ui/views/dialog_footnote_utils.h" #include "brave/browser/ui/views/infobars/custom_styled_label.h" #include "brave/components/constants/url_constants.h" @@ -52,10 +53,6 @@ #include "ui/views/window/dialog_client_view.h" #include "ui/views/window/dialog_delegate.h" -#if BUILDFLAG(IS_WIN) -#include "brave/browser/ui/geolocation/geolocation_utils_win.h" -#endif - #if BUILDFLAG(ENABLE_WIDEVINE) #include "brave/browser/widevine/widevine_permission_request.h" #endif @@ -333,7 +330,7 @@ void AddGeolocationDescriptionIfNeeded( base::ThreadPool::CreateCOMSTATaskRunner({base::MayBlock()}) ->PostTaskAndReplyWithResult( FROM_HERE, - base::BindOnce(&geolocation::win::IsSystemLocationSettingEnabled), + base::BindOnce(&geolocation::IsSystemLocationSettingEnabled), base::BindOnce( [](base::WeakPtr widget_delegate_weak_ptr, PermissionPromptBubbleBaseView* bubble_base_view, @@ -354,9 +351,8 @@ void AddGeolocationDescriptionIfNeeded( bubble_base_view->AsWeakPtr(), bubble_base_view, browser, enable_high_accuracy)); #else - AddGeolocationDescription(bubble_base_view, browser, - /*use_exact_location_label*/ false, - /*enable_high_accuracy*/ false); + AddGeolocationDescription(bubble_base_view, browser, enable_high_accuracy, + geolocation::IsSystemLocationSettingEnabled()); #endif } From 3373a2db3328ecc9de3929158ac3595d6b86b7c3 Mon Sep 17 00:00:00 2001 From: Simon Hong Date: Tue, 30 Apr 2024 22:02:03 +0900 Subject: [PATCH 05/19] Fixing upstream test failure Browser could be nullptr. --- .../permission_prompt_bubble_base_view.cc | 21 ++++++++++++------- 1 file changed, 14 insertions(+), 7 deletions(-) diff --git a/chromium_src/chrome/browser/ui/views/permissions/permission_prompt_bubble_base_view.cc b/chromium_src/chrome/browser/ui/views/permissions/permission_prompt_bubble_base_view.cc index 1b8f0b98da44..0e2b983849ea 100644 --- a/chromium_src/chrome/browser/ui/views/permissions/permission_prompt_bubble_base_view.cc +++ b/chromium_src/chrome/browser/ui/views/permissions/permission_prompt_bubble_base_view.cc @@ -308,6 +308,11 @@ void AddGeolocationDescriptionIfNeeded( PermissionPromptBubbleBaseView* bubble_base_view, permissions::PermissionPrompt::Delegate* delegate, Browser* browser) { + // Could be nullptr in unit test. + if (!browser) { + return; + } + auto requests = delegate->Requests(); // Geolocation permission is not grouped with others. @@ -332,23 +337,25 @@ void AddGeolocationDescriptionIfNeeded( FROM_HERE, base::BindOnce(&geolocation::IsSystemLocationSettingEnabled), base::BindOnce( - [](base::WeakPtr widget_delegate_weak_ptr, - PermissionPromptBubbleBaseView* bubble_base_view, - Browser* browser, bool enable_high_accuracy, + [](base::WeakPtr widget_delegate, + base::WeakPtr browser, bool enable_high_accuracy, bool settings_enabled) { - if (!widget_delegate_weak_ptr) { - // Bubble is already gone. + // Browser or Bubble is already gone. + if (!browser || !widget_delegate) { return; } + PermissionPromptBubbleBaseView* bubble_base_view = + static_cast( + widget_delegate.get()); AddGeolocationDescription( - bubble_base_view, browser, + bubble_base_view, browser.get(), /*enable_high_accuracy*/ enable_high_accuracy, /*use_exact_location_label*/ settings_enabled); // To update widget layout after adding another child view. bubble_base_view->UpdateAnchorPosition(); }, - bubble_base_view->AsWeakPtr(), bubble_base_view, browser, + bubble_base_view->AsWeakPtr(), browser->AsWeakPtr(), enable_high_accuracy)); #else AddGeolocationDescription(bubble_base_view, browser, enable_high_accuracy, From a9490068418b79a23ff17598142d279f1a8e9cd1 Mon Sep 17 00:00:00 2001 From: Simon Hong Date: Wed, 1 May 2024 08:44:31 +0900 Subject: [PATCH 06/19] Added platform guards --- browser/ui/BUILD.gn | 9 +++-- browser/ui/geolocation/geolocation_utils.h | 2 + ...on_utils.cc => geolocation_utils_linux.cc} | 8 ++-- .../ui/geolocation/geolocation_utils_mac.mm | 11 ++++-- .../ui/geolocation/geolocation_utils_win.cc | 4 ++ .../permission_prompt_bubble_base_view.cc | 4 ++ .../modules/geolocation/geolocation.cc | 38 +++++++++---------- third_party/blink/renderer/BUILD.gn | 9 +++-- 8 files changed, 50 insertions(+), 35 deletions(-) rename browser/ui/geolocation/{geolocation_utils.cc => geolocation_utils_linux.cc} (87%) diff --git a/browser/ui/BUILD.gn b/browser/ui/BUILD.gn index 522994a6d33a..3e4ea3b7e004 100644 --- a/browser/ui/BUILD.gn +++ b/browser/ui/BUILD.gn @@ -610,10 +610,7 @@ source_set("ui") { } if (is_win || is_mac || is_linux) { - sources += [ - "geolocation/geolocation_utils.cc", - "geolocation/geolocation_utils.h", - ] + sources += [ "geolocation/geolocation_utils.h" ] if (is_win) { sources += [ "geolocation/geolocation_utils_win.cc" ] @@ -623,6 +620,10 @@ source_set("ui") { sources += [ "geolocation/geolocation_utils_mac.mm" ] frameworks = [ "CoreLocation.framework" ] } + + if (is_linux) { + sources += [ "geolocation/geolocation_utils_linux.cc" ] + } } # brave's obsolete infobar is only used on Windows and Mac now. diff --git a/browser/ui/geolocation/geolocation_utils.h b/browser/ui/geolocation/geolocation_utils.h index 093234cb0745..8186325f4eae 100644 --- a/browser/ui/geolocation/geolocation_utils.h +++ b/browser/ui/geolocation/geolocation_utils.h @@ -11,6 +11,8 @@ namespace geolocation { // True when system location service is available to applications. bool IsSystemLocationSettingEnabled(); +bool CanGiveDetailedGeolocationRequestInfo(); + } // namespace geolocation #endif // BRAVE_BROWSER_UI_GEOLOCATION_GEOLOCATION_UTILS_H_ diff --git a/browser/ui/geolocation/geolocation_utils.cc b/browser/ui/geolocation/geolocation_utils_linux.cc similarity index 87% rename from browser/ui/geolocation/geolocation_utils.cc rename to browser/ui/geolocation/geolocation_utils_linux.cc index 94cd3f606100..585327440f20 100644 --- a/browser/ui/geolocation/geolocation_utils.cc +++ b/browser/ui/geolocation/geolocation_utils_linux.cc @@ -5,14 +5,14 @@ #include "brave/browser/ui/geolocation/geolocation_utils.h" -#include "build/build_config.h" - namespace geolocation { -#if BUILDFLAG(IS_LINUX) bool IsSystemLocationSettingEnabled() { return false; } -#endif + +bool CanGiveDetailedGeolocationRequestInfo() { + return false; +} } // namespace geolocation diff --git a/browser/ui/geolocation/geolocation_utils_mac.mm b/browser/ui/geolocation/geolocation_utils_mac.mm index bc1a17f11dce..0066ad49ed6e 100644 --- a/browser/ui/geolocation/geolocation_utils_mac.mm +++ b/browser/ui/geolocation/geolocation_utils_mac.mm @@ -7,8 +7,6 @@ #import -#include "base/logging.h" - namespace geolocation { bool IsSystemLocationSettingEnabled() { @@ -20,7 +18,6 @@ bool IsSystemLocationSettingEnabled() { CLLocationManager* manager = [[CLLocationManager alloc] init]; if (@available(macOS 11.0, *)) { auto status = [manager authorizationStatus]; - LOG(ERROR) << __func__ << " ##### " << (int)status; if (status == kCLAuthorizationStatusAuthorized) { return true; } @@ -29,4 +26,12 @@ bool IsSystemLocationSettingEnabled() { return false; } +bool CanGiveDetailedGeolocationRequestInfo() { + if (@available(macOS 11.0, *)) { + return true; + } + + return false; +} + } // namespace geolocation diff --git a/browser/ui/geolocation/geolocation_utils_win.cc b/browser/ui/geolocation/geolocation_utils_win.cc index 563cd12b8867..e07af9100839 100644 --- a/browser/ui/geolocation/geolocation_utils_win.cc +++ b/browser/ui/geolocation/geolocation_utils_win.cc @@ -49,4 +49,8 @@ bool IsSystemLocationSettingEnabled() { status == DeviceAccessStatus::DeviceAccessStatus_DeniedByUser); } +bool CanGiveDetailedGeolocationRequestInfo() { + return true; +} + } // namespace geolocation diff --git a/chromium_src/chrome/browser/ui/views/permissions/permission_prompt_bubble_base_view.cc b/chromium_src/chrome/browser/ui/views/permissions/permission_prompt_bubble_base_view.cc index 0e2b983849ea..7226c7e25808 100644 --- a/chromium_src/chrome/browser/ui/views/permissions/permission_prompt_bubble_base_view.cc +++ b/chromium_src/chrome/browser/ui/views/permissions/permission_prompt_bubble_base_view.cc @@ -308,6 +308,10 @@ void AddGeolocationDescriptionIfNeeded( PermissionPromptBubbleBaseView* bubble_base_view, permissions::PermissionPrompt::Delegate* delegate, Browser* browser) { + if (!geolocation::CanGiveDetailedGeolocationRequestInfo()) { + return; + } + // Could be nullptr in unit test. if (!browser) { return; diff --git a/chromium_src/third_party/blink/renderer/modules/geolocation/geolocation.cc b/chromium_src/third_party/blink/renderer/modules/geolocation/geolocation.cc index 5a03d8811675..79a47703296d 100644 --- a/chromium_src/third_party/blink/renderer/modules/geolocation/geolocation.cc +++ b/chromium_src/third_party/blink/renderer/modules/geolocation/geolocation.cc @@ -4,14 +4,28 @@ * You can obtain one at https://mozilla.org/MPL/2.0/. */ #include "brave/components/brave_geolocation_permission/common/brave_geolocation_permission.mojom-blink.h" +#include "build/build_config.h" #include "mojo/public/cpp/bindings/associated_remote.h" #include "third_party/blink/public/common/associated_interfaces/associated_interface_provider.h" +#include "third_party/blink/renderer/core/frame/local_frame.h" #include "third_party/blink/renderer/core/frame/local_frame_client.h" +#if !BUILDFLAG(IS_ANDROID) && !BUILDFLAG(IS_IOS) namespace blink { namespace { -void SetEnableHighAccuracy(LocalFrame* frame, bool enable_high_accuracy); +void SetEnableHighAccuracy(LocalFrame* frame, bool enable_high_accuracy) { + if (frame->Client()->GetRemoteNavigationAssociatedInterfaces()) { + mojo::AssociatedRemote< + geolocation::mojom::blink::BraveGeolocationPermission> + brave_geolocation_permission_binding; + frame->Client()->GetRemoteNavigationAssociatedInterfaces()->GetInterface( + &brave_geolocation_permission_binding); + CHECK(brave_geolocation_permission_binding.is_bound()); + brave_geolocation_permission_binding->SetEnableHighAccuracy( + enable_high_accuracy); + } +} } // namespace } // namespace blink @@ -25,26 +39,10 @@ void SetEnableHighAccuracy(LocalFrame* frame, bool enable_high_accuracy); // separated mojo interface. #define BRAVE_START_REQUEST \ SetEnableHighAccuracy(GetFrame(), notifier->Options()->enableHighAccuracy()); +#else +#define BRAVE_START_REQUEST +#endif #include "src/third_party/blink/renderer/modules/geolocation/geolocation.cc" #undef BRAVE_START_REQUEST - -namespace blink { -namespace { - -void SetEnableHighAccuracy(LocalFrame* frame, bool enable_high_accuracy) { - if (frame->Client()->GetRemoteNavigationAssociatedInterfaces()) { - mojo::AssociatedRemote< - geolocation::mojom::blink::BraveGeolocationPermission> - brave_geolocation_permission_binding; - frame->Client()->GetRemoteNavigationAssociatedInterfaces()->GetInterface( - &brave_geolocation_permission_binding); - CHECK(brave_geolocation_permission_binding.is_bound()); - brave_geolocation_permission_binding->SetEnableHighAccuracy( - enable_high_accuracy); - } -} - -} // namespace -} // namespace blink diff --git a/third_party/blink/renderer/BUILD.gn b/third_party/blink/renderer/BUILD.gn index baef94fa8607..0f555f613764 100644 --- a/third_party/blink/renderer/BUILD.gn +++ b/third_party/blink/renderer/BUILD.gn @@ -10,10 +10,11 @@ component("renderer") { "brave_font_whitelist.h", ] - deps = [ - "//brave/components/brave_drm:brave_drm_blink", - "//brave/components/brave_geolocation_permission/common:brave_geolocation_permission_blink", - ] + deps = [ "//brave/components/brave_drm:brave_drm_blink" ] + + if (!is_android && !is_ios) { + deps += [ "//brave/components/brave_geolocation_permission/common:brave_geolocation_permission_blink" ] + } defines = [ "BLINK_IMPLEMENTATION=1" ] From 7459f4b04d97ba7f4c464e3e5ace6389fe97f321 Mon Sep 17 00:00:00 2001 From: Simon Hong Date: Wed, 1 May 2024 11:26:50 +0900 Subject: [PATCH 07/19] Cleanup --- browser/brave_content_browser_client.cc | 2 +- browser/brave_tab_helpers.cc | 2 +- browser/sources.gni | 8 -------- browser/ui/BUILD.gn | 8 +++++++- .../brave_geolocation_permission_tab_helper.cc | 2 +- .../brave_geolocation_permission_tab_helper.h | 6 +++--- .../permissions/permission_prompt_bubble_base_view.cc | 5 +++-- 7 files changed, 16 insertions(+), 17 deletions(-) rename browser/{ => ui/geolocation}/brave_geolocation_permission_tab_helper.cc (95%) rename browser/{ => ui/geolocation}/brave_geolocation_permission_tab_helper.h (87%) diff --git a/browser/brave_content_browser_client.cc b/browser/brave_content_browser_client.cc index ab2869acbdf2..b90b221c8726 100644 --- a/browser/brave_content_browser_client.cc +++ b/browser/brave_content_browser_client.cc @@ -219,8 +219,8 @@ using extensions::ChromeContentBrowserClientExtensionsPart; #endif // BUILDFLAG(IS_ANDROID) #if !BUILDFLAG(IS_ANDROID) -#include "brave/browser/brave_geolocation_permission_tab_helper.h" #include "brave/browser/new_tab/new_tab_shows_navigation_throttle.h" +#include "brave/browser/ui/geolocation/brave_geolocation_permission_tab_helper.h" #include "brave/browser/ui/webui/brave_news_internals/brave_news_internals_ui.h" #include "brave/browser/ui/webui/brave_rewards/rewards_panel_ui.h" #include "brave/browser/ui/webui/brave_rewards/tip_panel_ui.h" diff --git a/browser/brave_tab_helpers.cc b/browser/brave_tab_helpers.cc index a3d8d65cb52b..bf4f21270b47 100644 --- a/browser/brave_tab_helpers.cc +++ b/browser/brave_tab_helpers.cc @@ -55,8 +55,8 @@ #endif #if !BUILDFLAG(IS_ANDROID) -#include "brave/browser/brave_geolocation_permission_tab_helper.h" #include "brave/browser/ui/brave_shields_data_controller.h" +#include "brave/browser/ui/geolocation/brave_geolocation_permission_tab_helper.h" #include "chrome/browser/ui/thumbnails/thumbnail_tab_helper.h" #endif diff --git a/browser/sources.gni b/browser/sources.gni index 764a6491900a..290a9755c978 100644 --- a/browser/sources.gni +++ b/browser/sources.gni @@ -399,14 +399,6 @@ if (enable_brave_wayback_machine) { brave_chrome_browser_deps += [ "//brave/components/brave_wayback_machine" ] } -if (!is_android) { - brave_chrome_browser_sources += [ - "//brave/browser/brave_geolocation_permission_tab_helper.cc", - "//brave/browser/brave_geolocation_permission_tab_helper.h", - ] - brave_chrome_browser_deps += [ "//brave/components/brave_geolocation_permission/common:brave_geolocation_permission" ] -} - if (enable_widevine) { brave_chrome_browser_sources += [ "//brave/browser/brave_drm_tab_helper.cc", diff --git a/browser/ui/BUILD.gn b/browser/ui/BUILD.gn index 3e4ea3b7e004..525a53f31a5d 100644 --- a/browser/ui/BUILD.gn +++ b/browser/ui/BUILD.gn @@ -610,7 +610,13 @@ source_set("ui") { } if (is_win || is_mac || is_linux) { - sources += [ "geolocation/geolocation_utils.h" ] + sources += [ + "geolocation/brave_geolocation_permission_tab_helper.cc", + "geolocation/brave_geolocation_permission_tab_helper.h", + "geolocation/geolocation_utils.h", + ] + + deps += [ "//brave/components/brave_geolocation_permission/common:brave_geolocation_permission" ] if (is_win) { sources += [ "geolocation/geolocation_utils_win.cc" ] diff --git a/browser/brave_geolocation_permission_tab_helper.cc b/browser/ui/geolocation/brave_geolocation_permission_tab_helper.cc similarity index 95% rename from browser/brave_geolocation_permission_tab_helper.cc rename to browser/ui/geolocation/brave_geolocation_permission_tab_helper.cc index 8b0fd0023dcf..bfde8c5dcb89 100644 --- a/browser/brave_geolocation_permission_tab_helper.cc +++ b/browser/ui/geolocation/brave_geolocation_permission_tab_helper.cc @@ -3,7 +3,7 @@ * License, v. 2.0. If a copy of the MPL was not distributed with this file, * You can obtain one at https://mozilla.org/MPL/2.0/. */ -#include "brave/browser/brave_geolocation_permission_tab_helper.h" +#include "brave/browser/ui/geolocation/brave_geolocation_permission_tab_helper.h" #include diff --git a/browser/brave_geolocation_permission_tab_helper.h b/browser/ui/geolocation/brave_geolocation_permission_tab_helper.h similarity index 87% rename from browser/brave_geolocation_permission_tab_helper.h rename to browser/ui/geolocation/brave_geolocation_permission_tab_helper.h index ceb4d586bc82..a9490b8e0314 100644 --- a/browser/brave_geolocation_permission_tab_helper.h +++ b/browser/ui/geolocation/brave_geolocation_permission_tab_helper.h @@ -3,8 +3,8 @@ * License, v. 2.0. If a copy of the MPL was not distributed with this file, * You can obtain one at https://mozilla.org/MPL/2.0/. */ -#ifndef BRAVE_BROWSER_BRAVE_GEOLOCATION_PERMISSION_TAB_HELPER_H_ -#define BRAVE_BROWSER_BRAVE_GEOLOCATION_PERMISSION_TAB_HELPER_H_ +#ifndef BRAVE_BROWSER_UI_GEOLOCATION_BRAVE_GEOLOCATION_PERMISSION_TAB_HELPER_H_ +#define BRAVE_BROWSER_UI_GEOLOCATION_BRAVE_GEOLOCATION_PERMISSION_TAB_HELPER_H_ #include "brave/components/brave_geolocation_permission/common/brave_geolocation_permission.mojom.h" #include "content/public/browser/render_frame_host_receiver_set.h" @@ -42,4 +42,4 @@ class BraveGeolocationPermissionTabHelper final bool enable_high_accuracy_ = false; }; -#endif // BRAVE_BROWSER_BRAVE_GEOLOCATION_PERMISSION_TAB_HELPER_H_ +#endif // BRAVE_BROWSER_UI_GEOLOCATION_BRAVE_GEOLOCATION_PERMISSION_TAB_HELPER_H_ diff --git a/chromium_src/chrome/browser/ui/views/permissions/permission_prompt_bubble_base_view.cc b/chromium_src/chrome/browser/ui/views/permissions/permission_prompt_bubble_base_view.cc index 7226c7e25808..853101f70d4c 100644 --- a/chromium_src/chrome/browser/ui/views/permissions/permission_prompt_bubble_base_view.cc +++ b/chromium_src/chrome/browser/ui/views/permissions/permission_prompt_bubble_base_view.cc @@ -12,7 +12,7 @@ #include "base/memory/raw_ref.h" #include "base/strings/string_util.h" #include "base/task/thread_pool.h" -#include "brave/browser/brave_geolocation_permission_tab_helper.h" +#include "brave/browser/ui/geolocation/brave_geolocation_permission_tab_helper.h" #include "brave/browser/ui/geolocation/geolocation_utils.h" #include "brave/browser/ui/views/dialog_footnote_utils.h" #include "brave/browser/ui/views/infobars/custom_styled_label.h" @@ -66,7 +66,8 @@ constexpr char kLearnMoreURL[] = "https://support.apple.com/guide/mac-help/" "allow-apps-to-detect-the-location-of-your-mac-mh35873/mac"; #else - "https://help.ubuntu.com/stable/ubuntu-help/privacy-location.html"; + // Not used now. Set proper link when detailed bubble is enabled on linux. + "https://www.brave.com/"; #endif namespace { From a10895e04d920fbd78bcf3b9bf251824f54caef2 Mon Sep 17 00:00:00 2001 From: Simon Hong Date: Wed, 1 May 2024 12:21:53 +0900 Subject: [PATCH 08/19] Added test --- browser/ui/geolocation/BUILD.gn | 24 ++++++ .../brave_geolocation_browsertest.cc | 80 +++++++++++++++++++ test/BUILD.gn | 1 + 3 files changed, 105 insertions(+) create mode 100644 browser/ui/geolocation/BUILD.gn create mode 100644 browser/ui/geolocation/brave_geolocation_browsertest.cc diff --git a/browser/ui/geolocation/BUILD.gn b/browser/ui/geolocation/BUILD.gn new file mode 100644 index 000000000000..933cb423765e --- /dev/null +++ b/browser/ui/geolocation/BUILD.gn @@ -0,0 +1,24 @@ +# Copyright (c) 2024 The Brave Authors. All rights reserved. +# This Source Code Form is subject to the terms of the Mozilla Public +# License, v. 2.0. If a copy of the MPL was not distributed with this file, +# You can obtain one at https://mozilla.org/MPL/2.0/. + +assert(!is_android) + +source_set("browser_tests") { + testonly = true + defines = [ "HAS_OUT_OF_PROC_TEST_RUNNER" ] + + sources = [ "brave_geolocation_browsertest.cc" ] + deps = [ + "//base", + "//brave/components/constants", + "//chrome/browser", + "//chrome/browser/ui", + "//chrome/common:constants", + "//chrome/test:test_support_ui", + "//content/test:test_support", + "//net:test_support", + "//url", + ] +} diff --git a/browser/ui/geolocation/brave_geolocation_browsertest.cc b/browser/ui/geolocation/brave_geolocation_browsertest.cc new file mode 100644 index 000000000000..f89fc8668bff --- /dev/null +++ b/browser/ui/geolocation/brave_geolocation_browsertest.cc @@ -0,0 +1,80 @@ +/* Copyright (c) 2024 The Brave Authors. All rights reserved. + * This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this file, + * You can obtain one at https://mozilla.org/MPL/2.0/. */ + +#include + +#include "base/path_service.h" +#include "brave/browser/ui/geolocation/brave_geolocation_permission_tab_helper.h" +#include "brave/components/constants/brave_paths.h" +#include "chrome/browser/ssl/cert_verifier_browser_test.h" +#include "chrome/browser/ui/browser.h" +#include "chrome/browser/ui/browser_commands.h" +#include "chrome/browser/ui/tabs/tab_strip_model.h" +#include "chrome/common/chrome_paths.h" +#include "chrome/test/base/in_process_browser_test.h" +#include "chrome/test/base/ui_test_utils.h" +#include "content/public/test/browser_test.h" +#include "content/public/test/browser_test_utils.h" +#include "content/public/test/test_navigation_observer.h" +#include "content/public/test/test_utils.h" +#include "net/dns/mock_host_resolver.h" +#include "url/gurl.h" + +class GeolocationPermissionRequestBrowserTest : public CertVerifierBrowserTest { + public: + GeolocationPermissionRequestBrowserTest() + : https_server_(net::EmbeddedTestServer::TYPE_HTTPS) {} + + void SetUpOnMainThread() override { + CertVerifierBrowserTest::SetUpOnMainThread(); + host_resolver()->AddRule("*", "127.0.0.1"); + brave::RegisterPathProvider(); + base::FilePath test_data_dir = + base::PathService::CheckedGet(brave::DIR_TEST_DATA); + https_server_.ServeFilesFromDirectory(test_data_dir); + mock_cert_verifier()->set_default_result(net::OK); + + ASSERT_TRUE(https_server_.Start()); + } + + content::WebContents* active_contents() { + return browser()->tab_strip_model()->GetActiveWebContents(); + } + + protected: + net::EmbeddedTestServer https_server_; +}; + +IN_PROC_BROWSER_TEST_F(GeolocationPermissionRequestBrowserTest, + SetEnableHighAccuracyTest) { + GURL url = https_server_.GetURL("a.com", "/simple.html"); + ASSERT_TRUE(ui_test_utils::NavigateToURL(browser(), url)); + + auto* tab_helper = + BraveGeolocationPermissionTabHelper::FromWebContents(active_contents()); + EXPECT_FALSE(tab_helper->enable_high_accuracy()); + + const std::string get_current_position_js_with_high = + "navigator.geolocation.getCurrentPosition(() => {}, () => {}, { " + "enableHighAccuracy : true })"; + ASSERT_EQ(nullptr, content::EvalJs(active_contents(), + get_current_position_js_with_high)); + content::RunAllTasksUntilIdle(); + EXPECT_TRUE(tab_helper->enable_high_accuracy()); + + // Reload clear high_accuracy bit from tab helper. + content::TestNavigationObserver observer(active_contents()); + chrome::Reload(browser(), WindowOpenDisposition::CURRENT_TAB); + observer.Wait(); + EXPECT_FALSE(tab_helper->enable_high_accuracy()); + + const std::string get_current_position_js_without_high = + "navigator.geolocation.getCurrentPosition(() => {}, () => {}, { " + "enableHighAccuracy : false })"; + ASSERT_EQ(nullptr, content::EvalJs(active_contents(), + get_current_position_js_without_high)); + content::RunAllTasksUntilIdle(); + EXPECT_FALSE(tab_helper->enable_high_accuracy()); +} diff --git a/test/BUILD.gn b/test/BUILD.gn index 85fab137a0af..a40e8f1e4cc4 100644 --- a/test/BUILD.gn +++ b/test/BUILD.gn @@ -1124,6 +1124,7 @@ test("brave_browser_tests") { "//brave/app/theme:brave_theme_resources_grit", "//brave/app/theme:brave_unscaled_resources_grit", "//brave/browser/sharing_hub:browser_tests", + "//brave/browser/ui/geolocation:browser_tests", "//brave/browser/ui/whats_new:browser_test", "//brave/components/brave_wallet/browser:test_support", "//chrome/browser/apps/app_service:app_service", From 2627f7dfd3e6f3abd18b10532d1677abc7a4e395 Mon Sep 17 00:00:00 2001 From: Simon Hong Date: Fri, 3 May 2024 10:35:43 +0900 Subject: [PATCH 09/19] Update browser/brave_content_browser_client.cc Co-authored-by: Brian Clifton --- browser/brave_content_browser_client.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/browser/brave_content_browser_client.cc b/browser/brave_content_browser_client.cc index b90b221c8726..a6f432435d0d 100644 --- a/browser/brave_content_browser_client.cc +++ b/browser/brave_content_browser_client.cc @@ -575,7 +575,7 @@ void BraveContentBrowserClient:: std::move(receiver), render_frame_host); }, &render_frame_host)); -#endif // BUILDFLAG(ENABLE_WIDEVINE) +#endif // !BUILDFLAG(IS_ANDROID) associated_registry.AddInterface< brave_shields::mojom::BraveShieldsHost>(base::BindRepeating( From 4532a5d64a46139c5de2292fcfd27b1ecaa923c1 Mon Sep 17 00:00:00 2001 From: Simon Hong Date: Fri, 3 May 2024 03:52:59 +0900 Subject: [PATCH 10/19] Fixed build failure on Android --- .../blink/renderer/modules/geolocation/geolocation.cc | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/chromium_src/third_party/blink/renderer/modules/geolocation/geolocation.cc b/chromium_src/third_party/blink/renderer/modules/geolocation/geolocation.cc index 79a47703296d..8ae7ebd2dc03 100644 --- a/chromium_src/third_party/blink/renderer/modules/geolocation/geolocation.cc +++ b/chromium_src/third_party/blink/renderer/modules/geolocation/geolocation.cc @@ -3,13 +3,16 @@ * License, v. 2.0. If a copy of the MPL was not distributed with this file, * You can obtain one at https://mozilla.org/MPL/2.0/. */ -#include "brave/components/brave_geolocation_permission/common/brave_geolocation_permission.mojom-blink.h" #include "build/build_config.h" #include "mojo/public/cpp/bindings/associated_remote.h" #include "third_party/blink/public/common/associated_interfaces/associated_interface_provider.h" #include "third_party/blink/renderer/core/frame/local_frame.h" #include "third_party/blink/renderer/core/frame/local_frame_client.h" +#if !BUILDFLAG(IS_ANDROID) && !BUILDFLAG(IS_IOS) +#include "brave/components/brave_geolocation_permission/common/brave_geolocation_permission.mojom-blink.h" +#endif + #if !BUILDFLAG(IS_ANDROID) && !BUILDFLAG(IS_IOS) namespace blink { namespace { From 502dae72a7c270a11bc6da6bbcfb4944bb5c2cff Mon Sep 17 00:00:00 2001 From: Simon Hong Date: Mon, 6 May 2024 10:12:21 +0900 Subject: [PATCH 11/19] Use kCLAuthorizationStatusAuthorizedAlways kCLAuthorizationStatusAuthorized is deprecated. --- browser/ui/geolocation/geolocation_utils_mac.mm | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/browser/ui/geolocation/geolocation_utils_mac.mm b/browser/ui/geolocation/geolocation_utils_mac.mm index 0066ad49ed6e..b211a5bfd13e 100644 --- a/browser/ui/geolocation/geolocation_utils_mac.mm +++ b/browser/ui/geolocation/geolocation_utils_mac.mm @@ -15,10 +15,10 @@ bool IsSystemLocationSettingEnabled() { return false; } - CLLocationManager* manager = [[CLLocationManager alloc] init]; if (@available(macOS 11.0, *)) { - auto status = [manager authorizationStatus]; - if (status == kCLAuthorizationStatusAuthorized) { + CLLocationManager* manager = [[CLLocationManager alloc] init]; + if ([manager authorizationStatus] == + kCLAuthorizationStatusAuthorizedAlways) { return true; } } From 6b172330733a559d33c57874f988539a3a6b7990 Mon Sep 17 00:00:00 2001 From: Simon Hong Date: Mon, 6 May 2024 11:50:09 +0900 Subject: [PATCH 12/19] Refactored to have separated api for handling placeholders in string --- .../permission_prompt_bubble_base_view.cc | 45 ++++++++----------- components/l10n/common/localization_util.cc | 26 +++++++++++ components/l10n/common/localization_util.h | 8 ++++ .../l10n/common/localization_util_unittest.cc | 21 +++++++++ .../resources/brave_components_strings.grd | 4 ++ 5 files changed, 78 insertions(+), 26 deletions(-) diff --git a/chromium_src/chrome/browser/ui/views/permissions/permission_prompt_bubble_base_view.cc b/chromium_src/chrome/browser/ui/views/permissions/permission_prompt_bubble_base_view.cc index 853101f70d4c..72b36db08bb6 100644 --- a/chromium_src/chrome/browser/ui/views/permissions/permission_prompt_bubble_base_view.cc +++ b/chromium_src/chrome/browser/ui/views/permissions/permission_prompt_bubble_base_view.cc @@ -10,7 +10,6 @@ #include "base/feature_list.h" #include "base/memory/raw_ptr.h" #include "base/memory/raw_ref.h" -#include "base/strings/string_util.h" #include "base/task/thread_pool.h" #include "brave/browser/ui/geolocation/brave_geolocation_permission_tab_helper.h" #include "brave/browser/ui/geolocation/geolocation_utils.h" @@ -36,7 +35,6 @@ #include "components/permissions/request_type.h" #include "components/strings/grit/components_strings.h" #include "third_party/widevine/cdm/buildflags.h" -#include "ui/base/l10n/l10n_util.h" #include "ui/base/metadata/metadata_header_macros.h" #include "ui/base/metadata/metadata_impl_macros.h" #include "ui/base/models/combobox_model.h" @@ -204,35 +202,30 @@ std::unique_ptr CreateGeolocationDescLabel( Browser* browser, bool enable_high_accuracy, bool location_service_is_on) { - // We put placeholders raw string to get target offsets. - std::vector placeholders{u"$1", u"$2", u"$3", u"$4"}; - if (enable_high_accuracy && !location_service_is_on) { - placeholders.insert(placeholders.end(), {u"$5", u"$6", u"$7", u"$8"}); - } + // The text shown in dialog is different depending on if person has location + // services enabled or disabled. This code finds which placeholders should + // show. int string_id = IDS_GEOLOCATION_PERMISSION_BUBBLE_LOW_ACCURACY_LABEL; - if (enable_high_accuracy) { + std::vector placeholders{u"$1", u"$2", u"$3", u"$4"}; + if (enable_high_accuracy && location_service_is_on) { string_id = - location_service_is_on - ? IDS_GEOLOCATION_PERMISSION_BUBBLE_HIGH_ACCURACY_WITH_LOCATION_SERVICE_LABEL - : IDS_GEOLOCATION_PERMISSION_BUBBLE_HIGH_ACCURACY_WITHOUT_LOCATION_SERVICE_LABEL; - } - std::vector offsets; - std::u16string contents_text = - l10n_util::GetStringFUTF16(string_id, placeholders, &offsets); - CHECK(offsets.size() == placeholders.size()); - - // Remove placeholers. It's used to get offsets. - for (const auto& placeholder : placeholders) { - base::RemoveChars(contents_text, placeholder, &contents_text); + IDS_GEOLOCATION_PERMISSION_BUBBLE_HIGH_ACCURACY_WITH_LOCATION_SERVICE_LABEL; + } else if (enable_high_accuracy) { + string_id = + IDS_GEOLOCATION_PERMISSION_BUBBLE_HIGH_ACCURACY_WITHOUT_LOCATION_SERVICE_LABEL; + placeholders.insert(placeholders.end(), {u"$5", u"$6", u"$7", u"$8"}); } - // Need to adjust offests after removing placeholders. - int offset_diff = 0; - for (size_t i = 0; i < offsets.size(); i++) { - offsets[i] -= offset_diff; - offset_diff += placeholders[i].length(); - } + // This code will get the actual strings so we know the length/offset of each + // styles (bold, italics, etc) can be applied with a range (begin offset / end + // offset). + std::vector offsets; + const std::u16string contents_text = + brave_l10n::GetStringFUTF16WithPlaceHolders(string_id, placeholders, + offsets); + CHECK(!contents_text.empty()); + // Insert the placeholder text with styles. auto contents_label = std::make_unique(); contents_label->SetTextContext(views::style::CONTEXT_LABEL); contents_label->SetDefaultTextStyle(views::style::STYLE_PRIMARY); diff --git a/components/l10n/common/localization_util.cc b/components/l10n/common/localization_util.cc index eb447359aea8..8a963a5f7ad0 100644 --- a/components/l10n/common/localization_util.cc +++ b/components/l10n/common/localization_util.cc @@ -5,7 +5,9 @@ #include "brave/components/l10n/common/localization_util.h" +#include "base/strings/string_util.h" #include "base/strings/utf_string_conversions.h" +#include "ui/base/l10n/l10n_util.h" #include "ui/base/resource/resource_bundle.h" namespace brave_l10n { @@ -18,4 +20,28 @@ std::u16string GetLocalizedResourceUTF16String(const int resource_id) { return base::UTF8ToUTF16(resource); } +std::u16string GetStringFUTF16WithPlaceHolders( + int resource_id, + const std::vector& placeholders, + std::vector& offsets) { + std::u16string contents_text = + l10n_util::GetStringFUTF16(resource_id, placeholders, &offsets); + CHECK(offsets.size() == placeholders.size()); + + // Remove placeholder text from the message. They were only inserted + // to find the offsets of each placeholder. + for (const auto& placeholder : placeholders) { + base::RemoveChars(contents_text, placeholder, &contents_text); + } + + // Need to adjust offests after removing placeholders. + int offset_diff = 0; + for (size_t i = 0; i < offsets.size(); i++) { + offsets[i] -= offset_diff; + offset_diff += placeholders[i].length(); + } + + return contents_text; +} + } // namespace brave_l10n diff --git a/components/l10n/common/localization_util.h b/components/l10n/common/localization_util.h index 9f0a426948e0..96baca203ff1 100644 --- a/components/l10n/common/localization_util.h +++ b/components/l10n/common/localization_util.h @@ -7,12 +7,20 @@ #define BRAVE_COMPONENTS_L10N_COMMON_LOCALIZATION_UTIL_H_ #include +#include namespace brave_l10n { // Return localized resource as a UTF-16 string for the given |resource_id|. std::u16string GetLocalizedResourceUTF16String(int resource_id); +// Returns string and |offsets| for |string_id| after removing place holders +// from original strings. +std::u16string GetStringFUTF16WithPlaceHolders( + int resource_id, + const std::vector& placeholders, + std::vector& offsets); + } // namespace brave_l10n #endif // BRAVE_COMPONENTS_L10N_COMMON_LOCALIZATION_UTIL_H_ diff --git a/components/l10n/common/localization_util_unittest.cc b/components/l10n/common/localization_util_unittest.cc index f793e04a5c19..b139ca980dfd 100644 --- a/components/l10n/common/localization_util_unittest.cc +++ b/components/l10n/common/localization_util_unittest.cc @@ -32,4 +32,25 @@ TEST(LocalizationUtilTest, EXPECT_TRUE(localized_resource.empty()); } +TEST(LocalizationUtilTest, GetLocalizedResourceUTF16StringWithPlaceholders) { + int string_id = IDS_TEST_STRING_FOR_PLACEHOLDERS; + std::vector placeholders{u"$1", u"$2", u"$3", u"$4"}; + std::vector offsets; + const std::u16string text = brave_l10n::GetStringFUTF16WithPlaceHolders( + string_id, placeholders, offsets); + EXPECT_EQ(placeholders.size(), offsets.size()); + + // Check |text| doesn't include place holder strings. + // grd file has above place holders and GetStringFUTF16WithPlaceHolders() + // returns string w/o them. + const std::u16string expected_text = + u"Test string for place holders. Learn more"; + EXPECT_EQ(expected_text, text); + EXPECT_EQ(offsets[0], + 16ul); // start offset of "place holders" part in the above. + EXPECT_EQ(offsets[1], 29ul); // end offset of "plce holders" part. + EXPECT_EQ(offsets[2], 31ul); // start offset of "Learn more" part. + EXPECT_EQ(offsets[3], 41ul); // end offset of "Learn more" part. +} + } // namespace brave_l10n diff --git a/components/resources/brave_components_strings.grd b/components/resources/brave_components_strings.grd index 9cb58669e64e..3e20582c3353 100644 --- a/components/resources/brave_components_strings.grd +++ b/components/resources/brave_components_strings.grd @@ -797,6 +797,10 @@ You're viewing a WebTorrent page + + Test string for $1place holders$2. $3Learn more$4 + + From 79fe0d607b7747c268ddf26d9b5721b2f89a1c14 Mon Sep 17 00:00:00 2001 From: Simon Hong Date: Tue, 7 May 2024 10:47:57 +0900 Subject: [PATCH 13/19] Removed geolocation.cc patch --- .../renderer/modules/geolocation/geolocation.cc | 13 ++++++++----- ...enderer-modules-geolocation-geolocation.cc.patch | 12 ------------ 2 files changed, 8 insertions(+), 17 deletions(-) delete mode 100644 patches/third_party-blink-renderer-modules-geolocation-geolocation.cc.patch diff --git a/chromium_src/third_party/blink/renderer/modules/geolocation/geolocation.cc b/chromium_src/third_party/blink/renderer/modules/geolocation/geolocation.cc index 8ae7ebd2dc03..dcbbb14b5804 100644 --- a/chromium_src/third_party/blink/renderer/modules/geolocation/geolocation.cc +++ b/chromium_src/third_party/blink/renderer/modules/geolocation/geolocation.cc @@ -5,6 +5,7 @@ #include "build/build_config.h" #include "mojo/public/cpp/bindings/associated_remote.h" +#include "services/device/public/mojom/geolocation.mojom-blink.h" #include "third_party/blink/public/common/associated_interfaces/associated_interface_provider.h" #include "third_party/blink/renderer/core/frame/local_frame.h" #include "third_party/blink/renderer/core/frame/local_frame_client.h" @@ -17,7 +18,7 @@ namespace blink { namespace { -void SetEnableHighAccuracy(LocalFrame* frame, bool enable_high_accuracy) { +bool SetEnableHighAccuracy(LocalFrame* frame, bool enable_high_accuracy) { if (frame->Client()->GetRemoteNavigationAssociatedInterfaces()) { mojo::AssociatedRemote< geolocation::mojom::blink::BraveGeolocationPermission> @@ -28,6 +29,8 @@ void SetEnableHighAccuracy(LocalFrame* frame, bool enable_high_accuracy) { brave_geolocation_permission_binding->SetEnableHighAccuracy( enable_high_accuracy); } + + return enable_high_accuracy; } } // namespace @@ -40,12 +43,12 @@ void SetEnableHighAccuracy(LocalFrame* frame, bool enable_high_accuracy) { // client layer. Instead of touching |WebContents|, |Geolocation|, // |GeolocationContext| interfaces, it would be more simple to pass via // separated mojo interface. -#define BRAVE_START_REQUEST \ - SetEnableHighAccuracy(GetFrame(), notifier->Options()->enableHighAccuracy()); +#define SetHighAccuracy(is_high_accuracy) \ + SetHighAccuracy(SetEnableHighAccuracy(GetFrame(), is_high_accuracy)); #else -#define BRAVE_START_REQUEST +#define SetHighAccuracy(is_high_accuracy) SetHighAccuracy(is_high_accuracy) #endif #include "src/third_party/blink/renderer/modules/geolocation/geolocation.cc" -#undef BRAVE_START_REQUEST +#undef SetHighAccuracy diff --git a/patches/third_party-blink-renderer-modules-geolocation-geolocation.cc.patch b/patches/third_party-blink-renderer-modules-geolocation-geolocation.cc.patch deleted file mode 100644 index ad3bcbdae0f6..000000000000 --- a/patches/third_party-blink-renderer-modules-geolocation-geolocation.cc.patch +++ /dev/null @@ -1,12 +0,0 @@ -diff --git a/third_party/blink/renderer/modules/geolocation/geolocation.cc b/third_party/blink/renderer/modules/geolocation/geolocation.cc -index 1051a55224e0665e6b6672e53bdb423bd398f480..0488005f08dd53e277978856ed955a8fd06ae351 100644 ---- a/third_party/blink/renderer/modules/geolocation/geolocation.cc -+++ b/third_party/blink/renderer/modules/geolocation/geolocation.cc -@@ -257,6 +257,7 @@ void Geolocation::StartRequest(GeoNotifier* notifier) { - if (HaveSuitableCachedPosition(notifier->Options())) { - notifier->SetUseCachedPosition(); - } else { -+ BRAVE_START_REQUEST - StartUpdating(notifier); - } - } From e4b32f74e6c3d09a31498c26efd8770647a61568 Mon Sep 17 00:00:00 2001 From: Simon Hong Date: Tue, 7 May 2024 10:56:28 +0900 Subject: [PATCH 14/19] Updated learn more url constants --- .../permissions/permission_prompt_bubble_base_view.cc | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/chromium_src/chrome/browser/ui/views/permissions/permission_prompt_bubble_base_view.cc b/chromium_src/chrome/browser/ui/views/permissions/permission_prompt_bubble_base_view.cc index 72b36db08bb6..602245c9de34 100644 --- a/chromium_src/chrome/browser/ui/views/permissions/permission_prompt_bubble_base_view.cc +++ b/chromium_src/chrome/browser/ui/views/permissions/permission_prompt_bubble_base_view.cc @@ -55,7 +55,9 @@ #include "brave/browser/widevine/widevine_permission_request.h" #endif -constexpr char kLearnMoreURL[] = +namespace { + +constexpr char kGeolocationPermissionLearnMoreURL[] = #if BUILDFLAG(IS_WIN) "https://support.microsoft.com/en-us/windows/" "windows-location-service-and-privacy-3a8eee0a-5b0b-dc07-eede-" @@ -68,8 +70,6 @@ constexpr char kLearnMoreURL[] = "https://www.brave.com/"; #endif -namespace { - #if BUILDFLAG(ENABLE_WIDEVINE) class DontAskAgainCheckbox : public views::Checkbox { METADATA_HEADER(DontAskAgainCheckbox, views::Checkbox) @@ -247,8 +247,9 @@ std::unique_ptr CreateGeolocationDescLabel( views::StyledLabel::RangeStyleInfo learn_more_style = views::StyledLabel::RangeStyleInfo::CreateForLink(base::BindRepeating( [](Browser* browser) { - chrome::AddSelectedTabWithURL(browser, GURL(kLearnMoreURL), - ui::PAGE_TRANSITION_AUTO_TOPLEVEL); + chrome::AddSelectedTabWithURL( + browser, GURL(kGeolocationPermissionLearnMoreURL), + ui::PAGE_TRANSITION_AUTO_TOPLEVEL); }, browser)); From 9cb97cd9dbf2c2fd7f0bc783e12ecb6c1ae95ddb Mon Sep 17 00:00:00 2001 From: Simon Hong Date: Tue, 7 May 2024 11:37:13 +0900 Subject: [PATCH 15/19] Updated IsSystemLocationSettingEnabled() --- browser/ui/geolocation/geolocation_utils.h | 7 ++- .../ui/geolocation/geolocation_utils_linux.cc | 4 +- .../ui/geolocation/geolocation_utils_mac.mm | 12 ++++- .../ui/geolocation/geolocation_utils_win.cc | 17 +++++- .../permission_prompt_bubble_base_view.cc | 52 +++++++------------ 5 files changed, 53 insertions(+), 39 deletions(-) diff --git a/browser/ui/geolocation/geolocation_utils.h b/browser/ui/geolocation/geolocation_utils.h index 8186325f4eae..7990c831d007 100644 --- a/browser/ui/geolocation/geolocation_utils.h +++ b/browser/ui/geolocation/geolocation_utils.h @@ -6,10 +6,13 @@ #ifndef BRAVE_BROWSER_UI_GEOLOCATION_GEOLOCATION_UTILS_H_ #define BRAVE_BROWSER_UI_GEOLOCATION_GEOLOCATION_UTILS_H_ +#include "base/functional/callback_forward.h" + namespace geolocation { -// True when system location service is available to applications. -bool IsSystemLocationSettingEnabled(); +// Run |callback| with true when system location service is available to +// applications. +void IsSystemLocationSettingEnabled(base::OnceCallback callback); bool CanGiveDetailedGeolocationRequestInfo(); diff --git a/browser/ui/geolocation/geolocation_utils_linux.cc b/browser/ui/geolocation/geolocation_utils_linux.cc index 585327440f20..df95809b2def 100644 --- a/browser/ui/geolocation/geolocation_utils_linux.cc +++ b/browser/ui/geolocation/geolocation_utils_linux.cc @@ -7,9 +7,7 @@ namespace geolocation { -bool IsSystemLocationSettingEnabled() { - return false; -} +void IsSystemLocationSettingEnabled(base::OnceCallback callback) {} bool CanGiveDetailedGeolocationRequestInfo() { return false; diff --git a/browser/ui/geolocation/geolocation_utils_mac.mm b/browser/ui/geolocation/geolocation_utils_mac.mm index b211a5bfd13e..ca736c0b8ca0 100644 --- a/browser/ui/geolocation/geolocation_utils_mac.mm +++ b/browser/ui/geolocation/geolocation_utils_mac.mm @@ -7,9 +7,13 @@ #import +#include + namespace geolocation { -bool IsSystemLocationSettingEnabled() { +namespace { + +bool GetSystemLocationSettingEnabled() { // Service is off globally. if (![CLLocationManager locationServicesEnabled]) { return false; @@ -26,6 +30,12 @@ bool IsSystemLocationSettingEnabled() { return false; } +} // namespace + +void IsSystemLocationSettingEnabled(base::OnceCallback callback) { + std::move(callback).Run(GetSystemLocationSettingEnabled()); +} + bool CanGiveDetailedGeolocationRequestInfo() { if (@available(macOS 11.0, *)) { return true; diff --git a/browser/ui/geolocation/geolocation_utils_win.cc b/browser/ui/geolocation/geolocation_utils_win.cc index e07af9100839..2bb3a1adec61 100644 --- a/browser/ui/geolocation/geolocation_utils_win.cc +++ b/browser/ui/geolocation/geolocation_utils_win.cc @@ -5,13 +5,17 @@ #include "brave/browser/ui/geolocation/geolocation_utils.h" +#include + #include #include #include #include +#include "base/functional/callback.h" #include "base/logging.h" +#include "base/task/thread_pool.h" #include "base/win/core_winrt_util.h" using ABI::Windows::Devices::Enumeration::DeviceAccessStatus; @@ -22,8 +26,10 @@ using Microsoft::WRL::ComPtr; namespace geolocation { +namespace { + // Copied from services/device/geolocation/win/location_provider_winrt.cc -bool IsSystemLocationSettingEnabled() { +bool GetSystemLocationSettingEnabled() { ComPtr dev_access_info_statics; HRESULT hr = base::win::GetActivationFactory< IDeviceAccessInformationStatics, @@ -49,6 +55,15 @@ bool IsSystemLocationSettingEnabled() { status == DeviceAccessStatus::DeviceAccessStatus_DeniedByUser); } +} // namespace + +void IsSystemLocationSettingEnabled(base::OnceCallback callback) { + base::ThreadPool::CreateCOMSTATaskRunner({base::MayBlock()}) + ->PostTaskAndReplyWithResult( + FROM_HERE, base::BindOnce(&GetSystemLocationSettingEnabled), + std::move(callback)); +} + bool CanGiveDetailedGeolocationRequestInfo() { return true; } diff --git a/chromium_src/chrome/browser/ui/views/permissions/permission_prompt_bubble_base_view.cc b/chromium_src/chrome/browser/ui/views/permissions/permission_prompt_bubble_base_view.cc index 602245c9de34..fdedaeea2415 100644 --- a/chromium_src/chrome/browser/ui/views/permissions/permission_prompt_bubble_base_view.cc +++ b/chromium_src/chrome/browser/ui/views/permissions/permission_prompt_bubble_base_view.cc @@ -10,7 +10,6 @@ #include "base/feature_list.h" #include "base/memory/raw_ptr.h" #include "base/memory/raw_ref.h" -#include "base/task/thread_pool.h" #include "brave/browser/ui/geolocation/brave_geolocation_permission_tab_helper.h" #include "brave/browser/ui/geolocation/geolocation_utils.h" #include "brave/browser/ui/views/dialog_footnote_utils.h" @@ -329,37 +328,26 @@ void AddGeolocationDescriptionIfNeeded( } } -#if BUILDFLAG(IS_WIN) - // IsSystemLocationSettingEnabled() uses COM. - base::ThreadPool::CreateCOMSTATaskRunner({base::MayBlock()}) - ->PostTaskAndReplyWithResult( - FROM_HERE, - base::BindOnce(&geolocation::IsSystemLocationSettingEnabled), - base::BindOnce( - [](base::WeakPtr widget_delegate, - base::WeakPtr browser, bool enable_high_accuracy, - bool settings_enabled) { - // Browser or Bubble is already gone. - if (!browser || !widget_delegate) { - return; - } - PermissionPromptBubbleBaseView* bubble_base_view = - static_cast( - widget_delegate.get()); - AddGeolocationDescription( - bubble_base_view, browser.get(), - /*enable_high_accuracy*/ enable_high_accuracy, - /*use_exact_location_label*/ settings_enabled); - - // To update widget layout after adding another child view. - bubble_base_view->UpdateAnchorPosition(); - }, - bubble_base_view->AsWeakPtr(), browser->AsWeakPtr(), - enable_high_accuracy)); -#else - AddGeolocationDescription(bubble_base_view, browser, enable_high_accuracy, - geolocation::IsSystemLocationSettingEnabled()); -#endif + geolocation::IsSystemLocationSettingEnabled(base::BindOnce( + [](base::WeakPtr widget_delegate, + base::WeakPtr browser, bool enable_high_accuracy, + bool settings_enabled) { + // Browser or Bubble is already gone. + if (!browser || !widget_delegate) { + return; + } + PermissionPromptBubbleBaseView* bubble_base_view = + static_cast(widget_delegate.get()); + AddGeolocationDescription( + bubble_base_view, browser.get(), + /*enable_high_accuracy*/ enable_high_accuracy, + /*use_exact_location_label*/ settings_enabled); + + // To update widget layout after adding another child view. + bubble_base_view->UpdateAnchorPosition(); + }, + bubble_base_view->AsWeakPtr(), browser->AsWeakPtr(), + enable_high_accuracy)); } views::View* AddPermissionLifetimeComboboxIfNeeded( From e4ad5fad70d2b35ebfef548931ee5011d0d58d7a Mon Sep 17 00:00:00 2001 From: Simon Hong Date: Tue, 7 May 2024 12:20:44 +0900 Subject: [PATCH 16/19] Updated strings desc --- app/brave_generated_resources.grd | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/app/brave_generated_resources.grd b/app/brave_generated_resources.grd index 123815147287..259e0ca4fe78 100644 --- a/app/brave_generated_resources.grd +++ b/app/brave_generated_resources.grd @@ -559,13 +559,13 @@ Or change later at $2brave://settings/ext - + This site has requested your $1general location$2. $3Learn more$4 - + This site has requested your $1precise location$2. $3Learn more$4 - + This site has requested your $1precise location$2. Brave will only able to provide $3general location$4 data due to $5Location Services$6 being disabled. $7Learn more$8 From 98f92e02f5c1888a539f5d395bcb6ed073c660ca Mon Sep 17 00:00:00 2001 From: Simon Hong Date: Tue, 7 May 2024 12:40:33 +0900 Subject: [PATCH 17/19] fixup! Updated IsSystemLocationSettingEnabled() --- browser/ui/geolocation/geolocation_utils_linux.cc | 2 ++ browser/ui/geolocation/geolocation_utils_mac.mm | 2 ++ browser/ui/geolocation/geolocation_utils_win.cc | 4 ++-- 3 files changed, 6 insertions(+), 2 deletions(-) diff --git a/browser/ui/geolocation/geolocation_utils_linux.cc b/browser/ui/geolocation/geolocation_utils_linux.cc index df95809b2def..c689c6082c57 100644 --- a/browser/ui/geolocation/geolocation_utils_linux.cc +++ b/browser/ui/geolocation/geolocation_utils_linux.cc @@ -5,6 +5,8 @@ #include "brave/browser/ui/geolocation/geolocation_utils.h" +#include "base/functional/callback.h" + namespace geolocation { void IsSystemLocationSettingEnabled(base::OnceCallback callback) {} diff --git a/browser/ui/geolocation/geolocation_utils_mac.mm b/browser/ui/geolocation/geolocation_utils_mac.mm index ca736c0b8ca0..60eb9c99a0bb 100644 --- a/browser/ui/geolocation/geolocation_utils_mac.mm +++ b/browser/ui/geolocation/geolocation_utils_mac.mm @@ -9,6 +9,8 @@ #include +#include "base/functional/callback.h" + namespace geolocation { namespace { diff --git a/browser/ui/geolocation/geolocation_utils_win.cc b/browser/ui/geolocation/geolocation_utils_win.cc index 2bb3a1adec61..90a55037c56a 100644 --- a/browser/ui/geolocation/geolocation_utils_win.cc +++ b/browser/ui/geolocation/geolocation_utils_win.cc @@ -5,14 +5,14 @@ #include "brave/browser/ui/geolocation/geolocation_utils.h" -#include - #include #include #include #include +#include + #include "base/functional/callback.h" #include "base/logging.h" #include "base/task/thread_pool.h" From 8ec14a55d1450411fc3a904f8035b7a5487694e9 Mon Sep 17 00:00:00 2001 From: Simon Hong Date: Tue, 7 May 2024 21:41:59 +0900 Subject: [PATCH 18/19] Deleted GetStringFUTF16WithPlaceHolders() --- app/brave_generated_resources.grd | 6 ++--- .../permission_prompt_bubble_base_view.cc | 13 +++++----- components/l10n/common/localization_util.cc | 26 ------------------- components/l10n/common/localization_util.h | 8 ------ .../l10n/common/localization_util_unittest.cc | 21 --------------- 5 files changed, 10 insertions(+), 64 deletions(-) diff --git a/app/brave_generated_resources.grd b/app/brave_generated_resources.grd index 259e0ca4fe78..73605ef8f1bc 100644 --- a/app/brave_generated_resources.grd +++ b/app/brave_generated_resources.grd @@ -560,13 +560,13 @@ Or change later at $2brave://settings/ext - This site has requested your $1general location$2. $3Learn more$4 + This site has requested your $1general location$1. $1Learn more$1 - This site has requested your $1precise location$2. $3Learn more$4 + This site has requested your $1precise location$1. $1Learn more$1 - This site has requested your $1precise location$2. Brave will only able to provide $3general location$4 data due to $5Location Services$6 being disabled. $7Learn more$8 + This site has requested your $1precise location$1. Brave will only able to provide $1general location$1 data due to $1Location Services$1 being disabled. $1Learn more$1 diff --git a/chromium_src/chrome/browser/ui/views/permissions/permission_prompt_bubble_base_view.cc b/chromium_src/chrome/browser/ui/views/permissions/permission_prompt_bubble_base_view.cc index fdedaeea2415..ac3c558c0014 100644 --- a/chromium_src/chrome/browser/ui/views/permissions/permission_prompt_bubble_base_view.cc +++ b/chromium_src/chrome/browser/ui/views/permissions/permission_prompt_bubble_base_view.cc @@ -34,6 +34,7 @@ #include "components/permissions/request_type.h" #include "components/strings/grit/components_strings.h" #include "third_party/widevine/cdm/buildflags.h" +#include "ui/base/l10n/l10n_util.h" #include "ui/base/metadata/metadata_header_macros.h" #include "ui/base/metadata/metadata_impl_macros.h" #include "ui/base/models/combobox_model.h" @@ -205,14 +206,14 @@ std::unique_ptr CreateGeolocationDescLabel( // services enabled or disabled. This code finds which placeholders should // show. int string_id = IDS_GEOLOCATION_PERMISSION_BUBBLE_LOW_ACCURACY_LABEL; - std::vector placeholders{u"$1", u"$2", u"$3", u"$4"}; + size_t expected_offset_size = 4; if (enable_high_accuracy && location_service_is_on) { string_id = IDS_GEOLOCATION_PERMISSION_BUBBLE_HIGH_ACCURACY_WITH_LOCATION_SERVICE_LABEL; } else if (enable_high_accuracy) { string_id = IDS_GEOLOCATION_PERMISSION_BUBBLE_HIGH_ACCURACY_WITHOUT_LOCATION_SERVICE_LABEL; - placeholders.insert(placeholders.end(), {u"$5", u"$6", u"$7", u"$8"}); + expected_offset_size = 8; } // This code will get the actual strings so we know the length/offset of each @@ -220,9 +221,9 @@ std::unique_ptr CreateGeolocationDescLabel( // offset). std::vector offsets; const std::u16string contents_text = - brave_l10n::GetStringFUTF16WithPlaceHolders(string_id, placeholders, - offsets); + l10n_util::GetStringFUTF16(string_id, {u""}, &offsets); CHECK(!contents_text.empty()); + CHECK_EQ(expected_offset_size, offsets.size()); // Insert the placeholder text with styles. auto contents_label = std::make_unique(); @@ -232,7 +233,7 @@ std::unique_ptr CreateGeolocationDescLabel( contents_label->SetText(contents_text); views::StyledLabel::RangeStyleInfo part_style; part_style.text_style = views::style::STYLE_EMPHASIZED; - const int part_count_except_learn_more = placeholders.size() / 2 - 1; + const int part_count_except_learn_more = offsets.size() / 2 - 1; for (int i = 0; i < part_count_except_learn_more; ++i) { // Each part has start/end offset pair. const int part_start_offset = i * 2; @@ -253,7 +254,7 @@ std::unique_ptr CreateGeolocationDescLabel( browser)); // Learn more is last part. - const int learn_more_offset = placeholders.size() - 2; + const int learn_more_offset = offsets.size() - 2; contents_label->AddStyleRange( gfx::Range(offsets[learn_more_offset], offsets[learn_more_offset + 1]), learn_more_style); diff --git a/components/l10n/common/localization_util.cc b/components/l10n/common/localization_util.cc index 8a963a5f7ad0..eb447359aea8 100644 --- a/components/l10n/common/localization_util.cc +++ b/components/l10n/common/localization_util.cc @@ -5,9 +5,7 @@ #include "brave/components/l10n/common/localization_util.h" -#include "base/strings/string_util.h" #include "base/strings/utf_string_conversions.h" -#include "ui/base/l10n/l10n_util.h" #include "ui/base/resource/resource_bundle.h" namespace brave_l10n { @@ -20,28 +18,4 @@ std::u16string GetLocalizedResourceUTF16String(const int resource_id) { return base::UTF8ToUTF16(resource); } -std::u16string GetStringFUTF16WithPlaceHolders( - int resource_id, - const std::vector& placeholders, - std::vector& offsets) { - std::u16string contents_text = - l10n_util::GetStringFUTF16(resource_id, placeholders, &offsets); - CHECK(offsets.size() == placeholders.size()); - - // Remove placeholder text from the message. They were only inserted - // to find the offsets of each placeholder. - for (const auto& placeholder : placeholders) { - base::RemoveChars(contents_text, placeholder, &contents_text); - } - - // Need to adjust offests after removing placeholders. - int offset_diff = 0; - for (size_t i = 0; i < offsets.size(); i++) { - offsets[i] -= offset_diff; - offset_diff += placeholders[i].length(); - } - - return contents_text; -} - } // namespace brave_l10n diff --git a/components/l10n/common/localization_util.h b/components/l10n/common/localization_util.h index 96baca203ff1..9f0a426948e0 100644 --- a/components/l10n/common/localization_util.h +++ b/components/l10n/common/localization_util.h @@ -7,20 +7,12 @@ #define BRAVE_COMPONENTS_L10N_COMMON_LOCALIZATION_UTIL_H_ #include -#include namespace brave_l10n { // Return localized resource as a UTF-16 string for the given |resource_id|. std::u16string GetLocalizedResourceUTF16String(int resource_id); -// Returns string and |offsets| for |string_id| after removing place holders -// from original strings. -std::u16string GetStringFUTF16WithPlaceHolders( - int resource_id, - const std::vector& placeholders, - std::vector& offsets); - } // namespace brave_l10n #endif // BRAVE_COMPONENTS_L10N_COMMON_LOCALIZATION_UTIL_H_ diff --git a/components/l10n/common/localization_util_unittest.cc b/components/l10n/common/localization_util_unittest.cc index b139ca980dfd..f793e04a5c19 100644 --- a/components/l10n/common/localization_util_unittest.cc +++ b/components/l10n/common/localization_util_unittest.cc @@ -32,25 +32,4 @@ TEST(LocalizationUtilTest, EXPECT_TRUE(localized_resource.empty()); } -TEST(LocalizationUtilTest, GetLocalizedResourceUTF16StringWithPlaceholders) { - int string_id = IDS_TEST_STRING_FOR_PLACEHOLDERS; - std::vector placeholders{u"$1", u"$2", u"$3", u"$4"}; - std::vector offsets; - const std::u16string text = brave_l10n::GetStringFUTF16WithPlaceHolders( - string_id, placeholders, offsets); - EXPECT_EQ(placeholders.size(), offsets.size()); - - // Check |text| doesn't include place holder strings. - // grd file has above place holders and GetStringFUTF16WithPlaceHolders() - // returns string w/o them. - const std::u16string expected_text = - u"Test string for place holders. Learn more"; - EXPECT_EQ(expected_text, text); - EXPECT_EQ(offsets[0], - 16ul); // start offset of "place holders" part in the above. - EXPECT_EQ(offsets[1], 29ul); // end offset of "plce holders" part. - EXPECT_EQ(offsets[2], 31ul); // start offset of "Learn more" part. - EXPECT_EQ(offsets[3], 41ul); // end offset of "Learn more" part. -} - } // namespace brave_l10n From 17ba9845d06515106ea3221f19c3192e4da9e1bd Mon Sep 17 00:00:00 2001 From: Simon Hong Date: Tue, 7 May 2024 22:15:31 +0900 Subject: [PATCH 19/19] Added brave_geolocation_permission_blink target via brave_blink_renderer_core_deps --- third_party/blink/renderer/BUILD.gn | 4 ---- third_party/blink/renderer/includes.gni | 4 ++++ 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/third_party/blink/renderer/BUILD.gn b/third_party/blink/renderer/BUILD.gn index 0f555f613764..53f5c6283141 100644 --- a/third_party/blink/renderer/BUILD.gn +++ b/third_party/blink/renderer/BUILD.gn @@ -12,10 +12,6 @@ component("renderer") { deps = [ "//brave/components/brave_drm:brave_drm_blink" ] - if (!is_android && !is_ios) { - deps += [ "//brave/components/brave_geolocation_permission/common:brave_geolocation_permission_blink" ] - } - defines = [ "BLINK_IMPLEMENTATION=1" ] output_name = "brave_blink_renderer_addon" diff --git a/third_party/blink/renderer/includes.gni b/third_party/blink/renderer/includes.gni index 6778a79eb488..587ead1e4dc1 100644 --- a/third_party/blink/renderer/includes.gni +++ b/third_party/blink/renderer/includes.gni @@ -37,6 +37,10 @@ brave_blink_renderer_core_sources = [ brave_blink_renderer_core_deps = [ "//brave/components/brave_shields/core/common/" ] +if (!is_android && !is_ios) { + brave_blink_renderer_core_deps += [ "//brave/components/brave_geolocation_permission/common:brave_geolocation_permission_blink" ] +} + brave_blink_renderer_core_public_deps += brave_page_graph_core_public_deps brave_blink_renderer_core_sources += brave_page_graph_core_sources brave_blink_renderer_core_deps += brave_page_graph_core_deps