Use GeolocationManager in CoreLocationProvider

It has come to our attention that the creation of a CLLocationManager
while we have a pending update will remove location permission from the
browser. To fix we have created a one global instance of this object
which is owned by the BrowserProcess. This Object was originally just
created to determine current permission status. This change adds the
functionality to also use this object to determine the user's location.

This use of this object to determine user's location is still behind
the chrome://flags/#enable-core-location-backend flag.

Bug: 1051591
Change-Id: I5606fbfddc1cf7aeff43c8fa4709ada9abe227ed
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2779636
Commit-Queue: James Hollyer <jameshollyer@chromium.org>
Reviewed-by: Andrey Kosyakov <caseq@chromium.org>
Reviewed-by: Avi Drissman <avi@chromium.org>
Reviewed-by: Matt Reynolds <mattreynolds@chromium.org>
Cr-Commit-Position: refs/heads/master@{#873504}

chrome/browser/browser_process_platform_part_mac.h

@@ -17,7 +17,7 @@ class AppShimManager;
 }  // namespace apps
 
 namespace device {
-class GeolocationSystemPermissionManager;
+class GeolocationManager;
 }  // namespace device
 
 class BrowserProcessPlatformPart : public BrowserProcessPlatformPartBase {
@@ -33,15 +33,13 @@ class BrowserProcessPlatformPart : public BrowserProcessPlatformPartBase {
 
   AppShimListener* app_shim_listener();
   apps::AppShimManager* app_shim_manager();
-  device::GeolocationSystemPermissionManager* location_permission_manager();
+  device::GeolocationManager* geolocation_manager();
 
   void SetGeolocationManagerForTesting(
-      std::unique_ptr<device::GeolocationSystemPermissionManager>
-          fake_location_manager);
+      std::unique_ptr<device::GeolocationManager> fake_geolocation_manager);
 
  protected:
-  std::unique_ptr<device::GeolocationSystemPermissionManager>
-      location_permission_manager_;
+  std::unique_ptr<device::GeolocationManager> geolocation_manager_;
 
  private:
   std::unique_ptr<apps::AppShimManager> app_shim_manager_;

chrome/browser/browser_process_platform_part_mac.mm

@@ -14,7 +14,7 @@
 #include "chrome/browser/apps/platform_apps/extension_app_shim_manager_delegate_mac.h"
 #include "chrome/browser/chrome_browser_application_mac.h"
 #include "chrome/common/chrome_features.h"
-#include "services/device/public/cpp/geolocation/geolocation_system_permission_mac.h"
+#include "services/device/public/cpp/geolocation/geolocation_manager_impl_mac.h"
 
 BrowserProcessPlatformPart::BrowserProcessPlatformPart() {
 }
@@ -76,9 +76,8 @@
   DCHECK(!app_shim_listener_.get());
   app_shim_listener_ = new AppShimListener;
 
-  if (!location_permission_manager_) {
-    location_permission_manager_ =
-        device::GeolocationSystemPermissionManager::Create();
+  if (!geolocation_manager_) {
+    geolocation_manager_ = device::GeolocationManagerImpl::Create();
   }
 }
 
@@ -90,13 +89,11 @@
   return app_shim_listener_.get();
 }
 
-device::GeolocationSystemPermissionManager*
-BrowserProcessPlatformPart::location_permission_manager() {
-  return location_permission_manager_.get();
+device::GeolocationManager* BrowserProcessPlatformPart::geolocation_manager() {
+  return geolocation_manager_.get();
 }
 
 void BrowserProcessPlatformPart::SetGeolocationManagerForTesting(
-    std::unique_ptr<device::GeolocationSystemPermissionManager>
-        fake_location_manager) {
-  location_permission_manager_ = std::move(fake_location_manager);
-}
+    std::unique_ptr<device::GeolocationManager> fake_geolocation_manager) {
+  geolocation_manager_ = std::move(fake_geolocation_manager);
+}

chrome/browser/chrome_content_browser_client.cc

@@ -2850,10 +2850,10 @@ std::string ChromeContentBrowserClient::GetGeolocationApiKey() {
   return google_apis::GetAPIKey();
 }
 
-device::GeolocationSystemPermissionManager*
-ChromeContentBrowserClient::GetLocationPermissionManager() {
+device::GeolocationManager*
+ChromeContentBrowserClient::GetGeolocationManager() {
 #if defined(OS_MAC)
-  return g_browser_process->platform_part()->location_permission_manager();
+  return g_browser_process->platform_part()->geolocation_manager();
 #else
   return nullptr;
 #endif

chrome/browser/chrome_content_browser_client.h

@@ -288,8 +288,7 @@ class ChromeContentBrowserClient : public content::ContentBrowserClient {
   GetSystemSharedURLLoaderFactory() override;
   network::mojom::NetworkContext* GetSystemNetworkContext() override;
   std::string GetGeolocationApiKey() override;
-  device::GeolocationSystemPermissionManager* GetLocationPermissionManager()
-      override;
+  device::GeolocationManager* GetGeolocationManager() override;
 
 #if defined(OS_ANDROID)
   bool ShouldUseGmsCoreGeolocationProvider() override;

chrome/browser/flag-metadata.json

@@ -1474,7 +1474,7 @@
   {
     "name": "enable-core-location-backend",
     "owners": [ "jameshollyer@google.com" ],
-    "expiry_milestone": 90
+    "expiry_milestone": 100
   },
   {
     "name": "enable-core-location-implementation",

chrome/browser/ui/content_settings/content_setting_bubble_model.cc

@@ -71,7 +71,6 @@
 #include "mojo/public/cpp/bindings/associated_remote.h"
 #include "ppapi/buildflags/buildflags.h"
 #include "services/device/public/cpp/device_features.h"
-#include "services/device/public/cpp/geolocation/geolocation_system_permission_mac.h"
 #include "services/device/public/cpp/geolocation/location_system_permission_status.h"
 #include "services/network/public/cpp/is_potentially_trustworthy.h"
 #include "third_party/blink/public/common/associated_interfaces/associated_interface_provider.h"
@@ -86,6 +85,7 @@
 #if defined(OS_MAC)
 #include "chrome/browser/browser_process_platform_part.h"
 #include "chrome/browser/media/webrtc/system_media_capture_permissions_mac.h"
+#include "services/device/public/cpp/geolocation/geolocation_manager.h"
 #endif
 
 using base::UserMetricsAction;
@@ -1237,10 +1237,10 @@ ContentSettingGeolocationBubbleModel::ContentSettingGeolocationBubbleModel(
     bool is_allowed =
         content_settings->IsContentAllowed(ContentSettingsType::GEOLOCATION);
 
-    device::GeolocationSystemPermissionManager* permission_delegate =
-        g_browser_process->platform_part()->location_permission_manager();
+    device::GeolocationManager* geolocation_manager =
+        g_browser_process->platform_part()->geolocation_manager();
     LocationSystemPermissionStatus permission =
-        permission_delegate->GetSystemPermission();
+        geolocation_manager->GetSystemPermission();
     if (permission != LocationSystemPermissionStatus::kAllowed && is_allowed) {
       // If the permission is turned off in MacOS system preferences, overwrite
       // the bubble to enable the user to trigger the system dialog.

chrome/browser/ui/content_settings/content_setting_bubble_model_unittest.cc

@@ -46,12 +46,15 @@
 #include "content/public/browser/web_contents.h"
 #include "content/public/test/web_contents_tester.h"
 #include "services/device/public/cpp/device_features.h"
-#include "services/device/public/cpp/geolocation/geolocation_system_permission_mac.h"
-#include "services/device/public/cpp/geolocation/location_system_permission_status.h"
-#include "services/device/public/cpp/test/fake_geolocation_system_permission.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "ui/base/l10n/l10n_util.h"
 
+#if defined(OS_MAC)
+#include "services/device/public/cpp/geolocation/geolocation_manager.h"
+#include "services/device/public/cpp/geolocation/location_system_permission_status.h"
+#include "services/device/public/cpp/test/fake_geolocation_manager.h"
+#endif
+
 using content::WebContentsTester;
 using content_settings::PageSpecificContentSettings;
 
@@ -735,14 +738,13 @@ TEST_F(GeolocationContentSettingBubbleModelTest, Geolocation) {
   ASSERT_TRUE(profile()->CreateHistoryService());
 
 #if defined(OS_MAC)
-  auto fake_geolocation_permission_manager =
-      std::make_unique<FakeSystemGeolocationPermissionsManager>();
-  FakeSystemGeolocationPermissionsManager* geolocation_permission_manager =
-      fake_geolocation_permission_manager.get();
+  auto fake_geolocation_manager =
+      std::make_unique<device::FakeGeolocationManager>();
+  device::FakeGeolocationManager* geolocation_manager =
+      fake_geolocation_manager.get();
   TestingBrowserProcess::GetGlobal()
       ->GetTestPlatformPart()
-      ->SetLocationPermissionManager(
-          std::move(fake_geolocation_permission_manager));
+      ->SetGeolocationManager(std::move(fake_geolocation_manager));
 #endif  // defined(OS_MAC)
 
   WebContentsTester::For(web_contents())
@@ -786,7 +788,7 @@ TEST_F(GeolocationContentSettingBubbleModelTest, Geolocation) {
         FakeOwner::Create(*content_setting_bubble_model, 0);
     const auto& bubble_content = content_setting_bubble_model->bubble_content();
 
-    geolocation_permission_manager->set_status(
+    geolocation_manager->SetSystemPermission(
         device::LocationSystemPermissionStatus::kAllowed);
 
     EXPECT_EQ(bubble_content.title,

chrome/browser/ui/content_settings/content_setting_image_model.cc

@@ -39,7 +39,6 @@
 #include "components/vector_icons/vector_icons.h"
 #include "content/public/browser/web_contents.h"
 #include "services/device/public/cpp/device_features.h"
-#include "services/device/public/cpp/geolocation/geolocation_system_permission_mac.h"
 #include "services/device/public/cpp/geolocation/location_system_permission_status.h"
 #include "ui/base/l10n/l10n_util.h"
 #include "ui/base/pointer/touch_ui_controller.h"
@@ -52,6 +51,7 @@
 #if defined(OS_MAC)
 #include "chrome/browser/browser_process_platform_part.h"
 #include "chrome/browser/media/webrtc/system_media_capture_permissions_mac.h"
+#include "services/device/public/cpp/geolocation/geolocation_manager.h"
 #endif
 
 using content::WebContents;
@@ -514,19 +514,19 @@ bool ContentSettingGeolocationImageModel::UpdateAndGetVisibility(
 
 #if defined(OS_MAC)
 bool ContentSettingGeolocationImageModel::IsGeolocationAllowedOnASystemLevel() {
-  device::GeolocationSystemPermissionManager* permission_manager =
-      g_browser_process->platform_part()->location_permission_manager();
+  device::GeolocationManager* geolocation_manager =
+      g_browser_process->platform_part()->geolocation_manager();
   device::LocationSystemPermissionStatus permission =
-      permission_manager->GetSystemPermission();
+      geolocation_manager->GetSystemPermission();
 
   return permission == device::LocationSystemPermissionStatus::kAllowed;
 }
 
 bool ContentSettingGeolocationImageModel::IsGeolocationPermissionDetermined() {
-  device::GeolocationSystemPermissionManager* permission_manager =
-      g_browser_process->platform_part()->location_permission_manager();
+  device::GeolocationManager* geolocation_manager =
+      g_browser_process->platform_part()->geolocation_manager();
   device::LocationSystemPermissionStatus permission =
-      permission_manager->GetSystemPermission();
+      geolocation_manager->GetSystemPermission();
 
   return permission != device::LocationSystemPermissionStatus::kNotDetermined;
 }

chrome/browser/ui/content_settings/content_setting_image_model_unittest.cc

@@ -45,15 +45,17 @@
 #include "content/public/test/web_contents_tester.h"
 #include "net/cookies/cookie_options.h"
 #include "services/device/public/cpp/device_features.h"
-#include "services/device/public/cpp/geolocation/geolocation_system_permission_mac.h"
-#include "services/device/public/cpp/geolocation/location_system_permission_status.h"
-#include "services/device/public/cpp/test/fake_geolocation_system_permission.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "ui/base/l10n/l10n_util.h"
 #include "ui/gfx/color_palette.h"
 
+#if defined(OS_MAC)
+#include "services/device/public/cpp/geolocation/geolocation_manager.h"
+#include "services/device/public/cpp/geolocation/location_system_permission_status.h"
+#include "services/device/public/cpp/test/fake_geolocation_manager.h"
+#endif
+
 using content_settings::PageSpecificContentSettings;
-using device::LocationSystemPermissionStatus;
 
 namespace {
 
@@ -276,14 +278,13 @@ TEST_F(ContentSettingImageModelTest, SensorAccessed) {
 TEST_F(ContentSettingImageModelTest, GeolocationAccessPermissionsChanged) {
   base::test::ScopedFeatureList feature_list;
   feature_list.InitAndEnableFeature(features::kMacCoreLocationImplementation);
-  auto test_location_permission_manager =
-      std::make_unique<FakeSystemGeolocationPermissionsManager>();
-  FakeSystemGeolocationPermissionsManager* location_permission_manager =
-      test_location_permission_manager.get();
+  auto test_geolocation_manager =
+      std::make_unique<device::FakeGeolocationManager>();
+  device::FakeGeolocationManager* geolocation_manager =
+      test_geolocation_manager.get();
   TestingBrowserProcess::GetGlobal()
       ->GetTestPlatformPart()
-      ->SetLocationPermissionManager(
-          std::move(test_location_permission_manager));
+      ->SetGeolocationManager(std::move(test_geolocation_manager));
 
   PageSpecificContentSettings::CreateForWebContents(
       web_contents(),
@@ -302,8 +303,8 @@ TEST_F(ContentSettingImageModelTest, GeolocationAccessPermissionsChanged) {
   EXPECT_FALSE(content_setting_image_model->is_visible());
   EXPECT_TRUE(content_setting_image_model->get_tooltip().empty());
 
-  location_permission_manager->set_status(
-      LocationSystemPermissionStatus::kAllowed);
+  geolocation_manager->SetSystemPermission(
+      device::LocationSystemPermissionStatus::kAllowed);
 
   settings_map->SetDefaultContentSetting(ContentSettingsType::GEOLOCATION,
                                          CONTENT_SETTING_ALLOW);
@@ -326,8 +327,8 @@ TEST_F(ContentSettingImageModelTest, GeolocationAccessPermissionsChanged) {
             l10n_util::GetStringUTF16(IDS_BLOCKED_GEOLOCATION_MESSAGE));
   EXPECT_EQ(content_setting_image_model->explanatory_string_id(), 0);
 
-  location_permission_manager->set_status(
-      LocationSystemPermissionStatus::kDenied);
+  geolocation_manager->SetSystemPermission(
+      device::LocationSystemPermissionStatus::kDenied);
   content_setting_image_model->Update(web_contents());
   EXPECT_TRUE(content_setting_image_model->is_visible());
   EXPECT_FALSE(content_setting_image_model->get_tooltip().empty());
@@ -348,14 +349,13 @@ TEST_F(ContentSettingImageModelTest, GeolocationAccessPermissionsChanged) {
 TEST_F(ContentSettingImageModelTest, GeolocationAccessPermissionsUndetermined) {
   base::test::ScopedFeatureList feature_list;
   feature_list.InitAndEnableFeature(features::kMacCoreLocationImplementation);
-  auto test_location_permission_manager =
-      std::make_unique<FakeSystemGeolocationPermissionsManager>();
-  test_location_permission_manager->set_status(
-      LocationSystemPermissionStatus::kNotDetermined);
+  auto test_geolocation_manager =
+      std::make_unique<device::FakeGeolocationManager>();
+  test_geolocation_manager->SetSystemPermission(
+      device::LocationSystemPermissionStatus::kNotDetermined);
   TestingBrowserProcess::GetGlobal()
       ->GetTestPlatformPart()
-      ->SetLocationPermissionManager(
-          std::move(test_location_permission_manager));
+      ->SetGeolocationManager(std::move(test_geolocation_manager));
 
   PageSpecificContentSettings::CreateForWebContents(
       web_contents(),

chrome/browser/ui/views/location_bar/location_bar_view.cc

@@ -169,7 +169,7 @@ LocationBarView::LocationBarView(Browser* browser,
 
 #if defined(OS_MAC)
     geolocation_permission_observation_.Observe(
-        g_browser_process->platform_part()->location_permission_manager());
+        g_browser_process->platform_part()->geolocation_manager());
 #endif
   }
 }
@@ -791,7 +791,7 @@ LocationBarView::GetContentSettingBubbleModelDelegate() {
   return delegate_->GetContentSettingBubbleModelDelegate();
 }
 
-void LocationBarView::OnSystemPermissionUpdate(
+void LocationBarView::OnSystemPermissionUpdated(
     device::LocationSystemPermissionStatus new_status) {
   UpdateContentSettingsIcons();
 }

chrome/browser/ui/views/location_bar/location_bar_view.h

@@ -29,7 +29,7 @@
 #include "chrome/browser/ui/views/omnibox/omnibox_view_views.h"
 #include "chrome/browser/ui/views/page_action/page_action_icon_view.h"
 #include "components/security_state/core/security_state.h"
-#include "services/device/public/cpp/geolocation/geolocation_system_permission_mac.h"
+#include "services/device/public/cpp/geolocation/geolocation_manager.h"
 #include "ui/base/pointer/touch_ui_controller.h"
 #include "ui/gfx/animation/slide_animation.h"
 #include "ui/gfx/font.h"
@@ -76,8 +76,7 @@ class LocationBarView : public LocationBar,
                         public LocationIconView::Delegate,
                         public ContentSettingImageView::Delegate,
                         public PageActionIconView::Delegate,
-                        public device::GeolocationSystemPermissionManager::
-                            GeolocationPermissionObserver {
+                        public device::GeolocationManager::PermissionObserver {
  public:
   METADATA_HEADER(LocationBarView);
 
@@ -207,8 +206,8 @@ class LocationBarView : public LocationBar,
   ContentSettingBubbleModelDelegate* GetContentSettingBubbleModelDelegate()
       override;
 
-  // GeolocationSystemPermissionManager::GeolocationPermissionObserver
-  void OnSystemPermissionUpdate(
+  // GeolocationManager::PermissionObserver:
+  void OnSystemPermissionUpdated(
       device::LocationSystemPermissionStatus new_status) override;
 
   static bool IsVirtualKeyboardVisible(views::Widget* widget);
@@ -251,11 +250,10 @@ class LocationBarView : public LocationBar,
   using ContentSettingViews = std::vector<ContentSettingImageView*>;
 
 #if defined(OS_MAC)
-  // Manage a subscription to GeolocationSystemPermissionManager, which may
+  // Manage a subscription to GeolocationManager, which may
   // outlive this object.
-  base::ScopedObservation<
-      device::GeolocationSystemPermissionManager,
-      device::GeolocationSystemPermissionManager::GeolocationPermissionObserver>
+  base::ScopedObservation<device::GeolocationManager,
+                          device::GeolocationManager::PermissionObserver>
       geolocation_permission_observation_{this};
 #endif
 

chrome/test/base/in_process_browser_test.cc

@@ -80,7 +80,7 @@
 #if defined(OS_MAC)
 #include "base/mac/scoped_nsautorelease_pool.h"
 #include "chrome/test/base/scoped_bundle_swizzler_mac.h"
-#include "services/device/public/cpp/test/fake_geolocation_system_permission.h"
+#include "services/device/public/cpp/test/fake_geolocation_manager.h"
 #endif
 
 #if defined(OS_WIN)
@@ -179,14 +179,14 @@ class ChromeBrowserMainExtraPartsBrowserProcessInjection
 
   // ChromeBrowserMainExtraParts implementation
   void PreMainMessageLoopStart() override {
-    // The real SystemGeolocationPermissionsManager initializes a
-    // CLLocationManager. It has been observed that when thousands of instances
-    // of this object are created, as happens when running browser tests, the
-    // CoreLocationAgent process uses lots of CPU. This makes test execution
-    // slower and causes jobs to time out. We therefore insert a fake.
+    // The real GeolocationManager initializes a CLLocationManager. It has
+    // been observed that when thousands of instances of this object are
+    // created, as happens when running browser tests, the CoreLocationAgent
+    // process uses lots of CPU. This makes test execution slower and causes
+    // jobs to time out. We therefore insert a fake.
     auto fake_geolocation_manager =
-        std::make_unique<FakeSystemGeolocationPermissionsManager>();
-    fake_geolocation_manager->set_status(
+        std::make_unique<device::FakeGeolocationManager>();
+    fake_geolocation_manager->SetSystemPermission(
         device::LocationSystemPermissionStatus::kAllowed);
     g_browser_process->platform_part()->SetGeolocationManagerForTesting(
         std::move(fake_geolocation_manager));