Enable Android Location Settings Dialog by default.

This has now rolled out to 100% stable, so should enable by default.

Bug: 688208
Change-Id: I2b34824ca48072bad4df048451f2b8fc2c31136b
Reviewed-on: https://chromium-review.googlesource.com/625504Reviewed-by: Timothy Loh <timloh@chromium.org>
Commit-Queue: Ben Wells <benwells@chromium.org>
Cr-Commit-Position: refs/heads/master@{#496572}

chrome/browser/geolocation/geolocation_permission_context_android.h

@@ -69,6 +69,7 @@ class GeolocationPermissionContextAndroid
 
  private:
   friend class GeolocationPermissionContextTests;
+  friend class PermissionManagerTest;
 
   static void AddDayOffsetForTesting(int days);
   static void SetDSEOriginForTesting(const char* dse_origin);

chrome/browser/geolocation/geolocation_permission_context_unittest.cc

@@ -478,9 +478,6 @@ TEST_F(GeolocationPermissionContextTests, AndroidEnabledCantPrompt) {
 }
 
 TEST_F(GeolocationPermissionContextTests, SystemLocationOffLSDDisabled) {
-  base::test::ScopedFeatureList scoped_feature_list;
-  scoped_feature_list.InitAndEnableFeature(features::kLsdPermissionPrompt);
-
   GURL requesting_frame("https://www.example.com/geolocation");
   NavigateAndCommit(requesting_frame);
   RequestManagerDocumentLoadCompleted();
@@ -494,9 +491,6 @@ TEST_F(GeolocationPermissionContextTests, SystemLocationOffLSDDisabled) {
 }
 
 TEST_F(GeolocationPermissionContextTests, SystemLocationOnNoLSD) {
-  base::test::ScopedFeatureList scoped_feature_list;
-  scoped_feature_list.InitAndEnableFeature(features::kLsdPermissionPrompt);
-
   GURL requesting_frame("https://www.example.com/geolocation");
   NavigateAndCommit(requesting_frame);
   RequestManagerDocumentLoadCompleted();
@@ -512,8 +506,6 @@ TEST_F(GeolocationPermissionContextTests, SystemLocationOnNoLSD) {
 
 TEST_F(GeolocationPermissionContextTests, SystemLocationOffLSDAccept) {
   base::HistogramTester tester;
-  base::test::ScopedFeatureList scoped_feature_list;
-  scoped_feature_list.InitAndEnableFeature(features::kLsdPermissionPrompt);
 
   GURL requesting_frame("https://www.example.com/geolocation");
   NavigateAndCommit(requesting_frame);
@@ -538,8 +530,6 @@ TEST_F(GeolocationPermissionContextTests, SystemLocationOffLSDAccept) {
 
 TEST_F(GeolocationPermissionContextTests, SystemLocationOffLSDReject) {
   base::HistogramTester tester;
-  base::test::ScopedFeatureList scoped_feature_list;
-  scoped_feature_list.InitAndEnableFeature(features::kLsdPermissionPrompt);
 
   GURL requesting_frame("https://www.example.com/geolocation");
   NavigateAndCommit(requesting_frame);
@@ -564,8 +554,6 @@ TEST_F(GeolocationPermissionContextTests, SystemLocationOffLSDReject) {
 
 TEST_F(GeolocationPermissionContextTests, LSDBackOffDifferentSites) {
   base::HistogramTester tester;
-  base::test::ScopedFeatureList scoped_feature_list;
-  scoped_feature_list.InitAndEnableFeature(features::kLsdPermissionPrompt);
 
   GURL requesting_frame_1("https://www.example.com/geolocation");
   GURL requesting_frame_2("https://www.example-2.com/geolocation");
@@ -622,8 +610,6 @@ TEST_F(GeolocationPermissionContextTests, LSDBackOffDifferentSites) {
 
 TEST_F(GeolocationPermissionContextTests, LSDBackOffTiming) {
   base::HistogramTester tester;
-  base::test::ScopedFeatureList scoped_feature_list;
-  scoped_feature_list.InitAndEnableFeature(features::kLsdPermissionPrompt);
 
   GURL requesting_frame("https://www.example.com/geolocation");
   SetGeolocationContentSetting(requesting_frame, requesting_frame,
@@ -709,9 +695,6 @@ TEST_F(GeolocationPermissionContextTests, LSDBackOffTiming) {
 }
 
 TEST_F(GeolocationPermissionContextTests, LSDBackOffPermissionStatus) {
-  base::test::ScopedFeatureList scoped_feature_list;
-  scoped_feature_list.InitAndEnableFeature(features::kLsdPermissionPrompt);
-
   GURL requesting_frame("https://www.example.com/geolocation");
   SetGeolocationContentSetting(requesting_frame, requesting_frame,
                                CONTENT_SETTING_ALLOW);
@@ -738,9 +721,6 @@ TEST_F(GeolocationPermissionContextTests, LSDBackOffPermissionStatus) {
 }
 
 TEST_F(GeolocationPermissionContextTests, LSDBackOffAskPromptsDespiteBackOff) {
-  base::test::ScopedFeatureList scoped_feature_list;
-  scoped_feature_list.InitAndEnableFeature(features::kLsdPermissionPrompt);
-
   GURL requesting_frame("https://www.example.com/geolocation");
   SetGeolocationContentSetting(requesting_frame, requesting_frame,
                                CONTENT_SETTING_ALLOW);
@@ -769,9 +749,6 @@ TEST_F(GeolocationPermissionContextTests, LSDBackOffAskPromptsDespiteBackOff) {
 
 TEST_F(GeolocationPermissionContextTests,
        LSDBackOffAcceptPermissionResetsBackOff) {
-  base::test::ScopedFeatureList scoped_feature_list;
-  scoped_feature_list.InitAndEnableFeature(features::kLsdPermissionPrompt);
-
   GURL requesting_frame("https://www.example.com/geolocation");
   SetGeolocationContentSetting(requesting_frame, requesting_frame,
                                CONTENT_SETTING_ALLOW);
@@ -809,9 +786,6 @@ TEST_F(GeolocationPermissionContextTests,
 }
 
 TEST_F(GeolocationPermissionContextTests, LSDBackOffAcceptLSDResetsBackOff) {
-  base::test::ScopedFeatureList scoped_feature_list;
-  scoped_feature_list.InitAndEnableFeature(features::kLsdPermissionPrompt);
-
   GURL requesting_frame("https://www.example.com/geolocation");
   SetGeolocationContentSetting(requesting_frame, requesting_frame,
                                CONTENT_SETTING_ALLOW);
@@ -1136,40 +1110,7 @@ TEST_F(GeolocationPermissionContextTests, SearchGeolocationInIncognito) {
                 .content_setting);
 }
 
-TEST_F(GeolocationPermissionContextTests,
-       GeolocationStatusAndroidDisabledLegacy) {
-  GURL requesting_frame("https://www.example.com/geolocation");
-
-  // In these tests the Android permission status should not be taken into
-  // account, only the content setting.
-  SetGeolocationContentSetting(requesting_frame, requesting_frame,
-                               CONTENT_SETTING_ALLOW);
-  MockLocationSettings::SetLocationStatus(false /* android */,
-                                          true /* system */);
-  ASSERT_EQ(blink::mojom::PermissionStatus::GRANTED,
-            PermissionManager::Get(profile())->GetPermissionStatus(
-                content::PermissionType::GEOLOCATION, requesting_frame,
-                requesting_frame));
-
-  SetGeolocationContentSetting(requesting_frame, requesting_frame,
-                               CONTENT_SETTING_ASK);
-  ASSERT_EQ(blink::mojom::PermissionStatus::ASK,
-            PermissionManager::Get(profile())->GetPermissionStatus(
-                content::PermissionType::GEOLOCATION, requesting_frame,
-                requesting_frame));
-
-  SetGeolocationContentSetting(requesting_frame, requesting_frame,
-                               CONTENT_SETTING_BLOCK);
-  ASSERT_EQ(blink::mojom::PermissionStatus::DENIED,
-            PermissionManager::Get(profile())->GetPermissionStatus(
-                content::PermissionType::GEOLOCATION, requesting_frame,
-                requesting_frame));
-}
-
 TEST_F(GeolocationPermissionContextTests, GeolocationStatusAndroidDisabled) {
-  base::test::ScopedFeatureList scoped_feature_list;
-  scoped_feature_list.InitAndEnableFeature(features::kLsdPermissionPrompt);
-
   GURL requesting_frame("https://www.example.com/geolocation");
 
   // With the Android permission off, but location allowed for a domain, the
@@ -1202,40 +1143,7 @@ TEST_F(GeolocationPermissionContextTests, GeolocationStatusAndroidDisabled) {
                 requesting_frame));
 }
 
-TEST_F(GeolocationPermissionContextTests,
-       GeolocationStatusSystemDisabledLegacy) {
-  GURL requesting_frame("https://www.example.com/geolocation");
-
-  // In these tests the system permission status should not be taken into
-  // account, only the content setting.
-  SetGeolocationContentSetting(requesting_frame, requesting_frame,
-                               CONTENT_SETTING_ALLOW);
-  MockLocationSettings::SetLocationStatus(true /* android */,
-                                          false /* system */);
-  ASSERT_EQ(blink::mojom::PermissionStatus::GRANTED,
-            PermissionManager::Get(profile())->GetPermissionStatus(
-                content::PermissionType::GEOLOCATION, requesting_frame,
-                requesting_frame));
-
-  SetGeolocationContentSetting(requesting_frame, requesting_frame,
-                               CONTENT_SETTING_ASK);
-  ASSERT_EQ(blink::mojom::PermissionStatus::ASK,
-            PermissionManager::Get(profile())->GetPermissionStatus(
-                content::PermissionType::GEOLOCATION, requesting_frame,
-                requesting_frame));
-
-  SetGeolocationContentSetting(requesting_frame, requesting_frame,
-                               CONTENT_SETTING_BLOCK);
-  ASSERT_EQ(blink::mojom::PermissionStatus::DENIED,
-            PermissionManager::Get(profile())->GetPermissionStatus(
-                content::PermissionType::GEOLOCATION, requesting_frame,
-                requesting_frame));
-}
-
 TEST_F(GeolocationPermissionContextTests, GeolocationStatusSystemDisabled) {
-  base::test::ScopedFeatureList scoped_feature_list;
-  scoped_feature_list.InitAndEnableFeature(features::kLsdPermissionPrompt);
-
   GURL requesting_frame("https://www.example.com/geolocation");
 
   // With the system permission off, but location allowed for a domain, the

chrome/browser/permissions/permission_manager_unittest.cc

@@ -20,6 +20,11 @@
 #include "device/vr/features/features.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
+#if defined(OS_ANDROID)
+#include "chrome/browser/android/mock_location_settings.h"
+#include "chrome/browser/geolocation/geolocation_permission_context_android.h"
+#endif  // defined(OS_ANDROID)
+
 using blink::mojom::PermissionStatus;
 using content::PermissionType;
 
@@ -110,6 +115,17 @@ class PermissionManagerTest : public ChromeRenderViewHostTestHarness {
   void SetUp() override {
     ChromeRenderViewHostTestHarness::SetUp();
     profile_.reset(new PermissionManagerTestingProfile());
+#if defined(OS_ANDROID)
+    GeolocationPermissionContextAndroid* geolocation_permission_context_ =
+        static_cast<GeolocationPermissionContextAndroid*>(
+            GetPermissionManager()->GetPermissionContext(
+                CONTENT_SETTINGS_TYPE_GEOLOCATION));
+    geolocation_permission_context_->SetLocationSettingsForTesting(
+        std::unique_ptr<LocationSettings>(new MockLocationSettings()));
+    MockLocationSettings::SetLocationStatus(
+        true /* has_android_location_permission */,
+        true /* is_system_location_setting_enabled */);
+#endif
   }
 
   void TearDown() override {

chrome/common/chrome_features.cc

@@ -223,7 +223,7 @@ const base::Feature kLinuxObsoleteSystemIsEndOfTheLine{
 // Enables or disables the Location Settings Dialog (LSD). The LSD is an Android
 // system-level geolocation permission prompt.
 const base::Feature kLsdPermissionPrompt{"LsdPermissionPrompt",
-                                         base::FEATURE_DISABLED_BY_DEFAULT};
+                                         base::FEATURE_ENABLED_BY_DEFAULT};
 
 #if defined(OS_MACOSX)
 // Enables RTL layout in macOS top chrome.