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.
(cherry picked from commit 69a1223a698a27e2179abea00f4f88232f89a7e3)
Bug: 1134741
Change-Id: I3af75da2b5fcfc80aa5f42d9e5aa88e8ee74d829
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2446023
Reviewed-by: James Hollyer <[email protected]>
Reviewed-by: Balazs Engedy <[email protected]>
Reviewed-by: Reilly Grant <[email protected]>
Commit-Queue: James Hollyer <[email protected]>
Cr-Original-Commit-Position: refs/heads/master@{#814407}
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2472907
Cr-Commit-Position: refs/branch-heads/4280@{#398}
Cr-Branched-From: ea420fb963f9658c9969b6513c56b8f47efa1a2a-refs/heads/master@{#812852}
diff --git a/chrome/browser/ui/content_settings/content_setting_image_model.cc b/chrome/browser/ui/content_settings/content_setting_image_model.cc
index 0c23009..4f8182d2 100644
--- a/chrome/browser/ui/content_settings/content_setting_image_model.cc
+++ b/chrome/browser/ui/content_settings/content_setting_image_model.cc
@@ -89,7 +89,8 @@
bool IsGeolocationAccessed();
#if defined(OS_MAC)
- bool IsGeolocationBlockedOnASystemLevel();
+ bool IsGeolocationAllowedOnASystemLevel();
+ bool IsGeolocationPermissionDetermined();
#endif // defined(OS_MAC)
std::unique_ptr<ContentSettingBubbleModel> CreateBubbleModelImpl(
@@ -512,13 +513,18 @@
::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;
}
}
@@ -534,12 +540,20 @@
}
#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)
diff --git a/chrome/browser/ui/content_settings/content_setting_image_model_unittest.cc b/chrome/browser/ui/content_settings/content_setting_image_model_unittest.cc
index 3c05b64..d1b60945 100644
--- a/chrome/browser/ui/content_settings/content_setting_image_model_unittest.cc
+++ b/chrome/browser/ui/content_settings/content_setting_image_model_unittest.cc
@@ -132,7 +132,7 @@
void SetStatus(SystemPermissionStatus status) { fake_status_ = status; }
private:
- SystemPermissionStatus fake_status_ = SystemPermissionStatus::kAllowed;
+ SystemPermissionStatus fake_status_ = SystemPermissionStatus::kNotDetermined;
};
#endif
@@ -318,6 +318,8 @@
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 @@
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 @@
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
