Dont start explanatory string animation until permission is determined

There is a period of time before macOS gives us a signal of our
location permission status. During this time we mostly assume that we
do not have permission. This is mostly not even noticed by the user as
it happens to quickly. However, if we dont have permission for location
we start an animation to let the user know. This animation does not
stop once permission is determined so it is very noticeable. This change
only starts that animation once permission is actually determined.

Bug: 1134741
Change-Id: I3af75da2b5fcfc80aa5f42d9e5aa88e8ee74d829
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2446023Reviewed-by: James Hollyer <jameshollyer@chromium.org>
Reviewed-by: Balazs Engedy <engedy@chromium.org>
Reviewed-by: Reilly Grant <reillyg@chromium.org>
Commit-Queue: James Hollyer <jameshollyer@chromium.org>
Cr-Commit-Position: refs/heads/master@{#814407}

chrome/browser/ui/content_settings/content_setting_image_model.cc

@@ -90,7 +90,8 @@ class ContentSettingGeolocationImageModel : public ContentSettingImageModel {
 
   bool IsGeolocationAccessed();
 #if defined(OS_MAC)
-  bool IsGeolocationBlockedOnASystemLevel();
+  bool IsGeolocationAllowedOnASystemLevel();
+  bool IsGeolocationPermissionDetermined();
 #endif  // defined(OS_MAC)
 
   std::unique_ptr<ContentSettingBubbleModel> CreateBubbleModelImpl(
@@ -513,13 +514,18 @@ bool ContentSettingGeolocationImageModel::UpdateAndGetVisibility(
           ::features::kMacCoreLocationImplementation)) {
     set_explanatory_string_id(0);
     if (is_allowed) {
-      if (IsGeolocationBlockedOnASystemLevel()) {
+      if (!IsGeolocationAllowedOnASystemLevel()) {
         set_icon(vector_icons::kLocationOnIcon,
                  vector_icons::kBlockedBadgeIcon);
         set_tooltip(l10n_util::GetStringUTF16(IDS_BLOCKED_GEOLOCATION_MESSAGE));
         if (content_settings->geolocation_was_just_granted_on_site_level())
           set_should_auto_open_bubble(true);
-        set_explanatory_string_id(IDS_GEOLOCATION_TURNED_OFF);
+        // At this point macOS may not have told us whether location permission
+        // has been allowed or blocked. Wait until the permission state is
+        // determined before displaying this message since it triggers an
+        // animation that cannot be cancelled
+        if (IsGeolocationPermissionDetermined())
+          set_explanatory_string_id(IDS_GEOLOCATION_TURNED_OFF);
         return true;
       }
     }
@@ -535,12 +541,20 @@ bool ContentSettingGeolocationImageModel::UpdateAndGetVisibility(
 }
 
 #if defined(OS_MAC)
-bool ContentSettingGeolocationImageModel::IsGeolocationBlockedOnASystemLevel() {
+bool ContentSettingGeolocationImageModel::IsGeolocationAllowedOnASystemLevel() {
   GeolocationSystemPermissionManager* permission_manager =
       g_browser_process->platform_part()->location_permission_manager();
   SystemPermissionStatus permission = permission_manager->GetSystemPermission();
 
-  return permission != SystemPermissionStatus::kAllowed;
+  return permission == SystemPermissionStatus::kAllowed;
+}
+
+bool ContentSettingGeolocationImageModel::IsGeolocationPermissionDetermined() {
+  GeolocationSystemPermissionManager* permission_manager =
+      g_browser_process->platform_part()->location_permission_manager();
+  SystemPermissionStatus permission = permission_manager->GetSystemPermission();
+
+  return permission != SystemPermissionStatus::kNotDetermined;
 }
 
 #endif  // defined(OS_MAC)

chrome/browser/ui/content_settings/content_setting_image_model_unittest.cc

@@ -132,7 +132,7 @@ class FakeSystemGeolocationPermissionsManager
   void SetStatus(SystemPermissionStatus status) { fake_status_ = status; }
 
  private:
-  SystemPermissionStatus fake_status_ = SystemPermissionStatus::kAllowed;
+  SystemPermissionStatus fake_status_ = SystemPermissionStatus::kNotDetermined;
 };
 #endif
 
@@ -318,6 +318,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->SetStatus(SystemPermissionStatus::kAllowed);
+
   settings_map->SetDefaultContentSetting(ContentSettingsType::GEOLOCATION,
                                          CONTENT_SETTING_ALLOW);
   content_settings->OnContentAllowed(ContentSettingsType::GEOLOCATION);
@@ -331,8 +333,6 @@ TEST_F(ContentSettingImageModelTest, GeolocationAccessPermissionsChanged) {
   settings_map->SetDefaultContentSetting(ContentSettingsType::GEOLOCATION,
                                          CONTENT_SETTING_BLOCK);
   content_settings->OnContentBlocked(ContentSettingsType::GEOLOCATION);
-  //   content_settings->OnGeolocationPermissionSet(requesting_origin,
-  //                                                /*allowed=*/false);
   content_setting_image_model->Update(web_contents());
   EXPECT_TRUE(content_setting_image_model->is_visible());
   EXPECT_TRUE(HasIcon(*content_setting_image_model));
@@ -358,6 +358,60 @@ TEST_F(ContentSettingImageModelTest, GeolocationAccessPermissionsChanged) {
   EXPECT_EQ(content_setting_image_model->explanatory_string_id(),
             IDS_GEOLOCATION_TURNED_OFF);
 }
+
+TEST_F(ContentSettingImageModelTest, GeolocationAccessPermissionsUndetermined) {
+  base::test::ScopedFeatureList feature_list;
+  feature_list.InitAndEnableFeature(features::kMacCoreLocationImplementation);
+  auto test_location_permission_manager =
+      std::make_unique<FakeSystemGeolocationPermissionsManager>();
+  TestingBrowserProcess::GetGlobal()
+      ->GetTestPlatformPart()
+      ->SetLocationPermissionManager(
+          std::move(test_location_permission_manager));
+
+  PageSpecificContentSettings::CreateForWebContents(
+      web_contents(),
+      std::make_unique<chrome::PageSpecificContentSettingsDelegate>(
+          web_contents()));
+  GURL requesting_origin = GURL("https://www.example.com");
+  NavigateAndCommit(controller_, requesting_origin);
+  PageSpecificContentSettings* content_settings =
+      PageSpecificContentSettings::GetForFrame(web_contents()->GetMainFrame());
+  HostContentSettingsMap* settings_map =
+      HostContentSettingsMapFactory::GetForProfile(profile());
+
+  auto content_setting_image_model =
+      ContentSettingImageModel::CreateForContentType(
+          ContentSettingImageModel::ImageType::GEOLOCATION);
+  EXPECT_FALSE(content_setting_image_model->is_visible());
+  EXPECT_TRUE(content_setting_image_model->get_tooltip().empty());
+
+  // When OS level permission is not determined the UI should show as if it is
+  // blocked. However, the explanatory string is not displayed since we aren't
+  // completely sure yet.
+  settings_map->SetDefaultContentSetting(ContentSettingsType::GEOLOCATION,
+                                         CONTENT_SETTING_ALLOW);
+  content_settings->OnContentAllowed(ContentSettingsType::GEOLOCATION);
+  content_setting_image_model->Update(web_contents());
+  EXPECT_TRUE(content_setting_image_model->is_visible());
+  EXPECT_FALSE(content_setting_image_model->get_tooltip().empty());
+  EXPECT_EQ(content_setting_image_model->get_tooltip(),
+            l10n_util::GetStringUTF16(IDS_BLOCKED_GEOLOCATION_MESSAGE));
+  EXPECT_EQ(content_setting_image_model->explanatory_string_id(), 0);
+
+  // When site permission is blocked it should not make any difference what the
+  // OS level permission is.
+  settings_map->SetDefaultContentSetting(ContentSettingsType::GEOLOCATION,
+                                         CONTENT_SETTING_BLOCK);
+  content_settings->OnContentBlocked(ContentSettingsType::GEOLOCATION);
+  content_setting_image_model->Update(web_contents());
+  EXPECT_TRUE(content_setting_image_model->is_visible());
+  EXPECT_TRUE(HasIcon(*content_setting_image_model));
+  EXPECT_FALSE(content_setting_image_model->get_tooltip().empty());
+  EXPECT_EQ(content_setting_image_model->get_tooltip(),
+            l10n_util::GetStringUTF16(IDS_BLOCKED_GEOLOCATION_MESSAGE));
+  EXPECT_EQ(content_setting_image_model->explanatory_string_id(), 0);
+}
 #endif
 
 // Regression test for https://crbug.com/955408