Limit the amount the Location Settings Dialog will be shown to users.

chrome/browser/geolocation/geolocation_permission_context_android.cc

Index: chrome/browser/geolocation/geolocation_permission_context_android.cc
diff --git a/chrome/browser/geolocation/geolocation_permission_context_android.cc b/chrome/browser/geolocation/geolocation_permission_context_android.cc
index 6630c11551b958b53999b8efdd988ee65775d884..f2d3d8869bc7d9e089e1d5d52c9470cb435bfde3 100644
--- a/chrome/browser/geolocation/geolocation_permission_context_android.cc
+++ b/chrome/browser/geolocation/geolocation_permission_context_android.cc
@@ -18,7 +18,10 @@
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/search_engines/template_url_service_factory.h"
 #include "chrome/common/chrome_features.h"
+#include "chrome/common/pref_names.h"
 #include "components/infobars/core/infobar.h"
+#include "components/prefs/pref_registry_simple.h"
+#include "components/prefs/pref_service.h"
 #include "components/search_engines/template_url.h"
 #include "components/search_engines/template_url_service.h"
 #include "content/public/browser/browser_thread.h"
@@ -26,6 +29,28 @@
 #include "content/public/browser/web_contents.h"
 #include "url/gurl.h"
 
+namespace {
+
+int gDayOffsetForTesting = 0;
+
+bool gHasDSEOriginForTesting = false;

[raymes, 2017/03/15 04:28:25]

Could you just use 
if (!gDSEOriginForTesting.empty())

instead of adding this variable?

[benwells, 2017/03/15 23:10:48]

Done.

+GURL gDSEOriginForTesting;

[raymes, 2017/03/15 04:28:25]

I think we avoid non-POD globals because it leads to static initializers? You
may just want to make this an instance variable? That would avoid test lifetime
issues too.

[benwells, 2017/03/15 23:10:48]

Done.

+
+base::Time GetTimeNow() {
+  return base::Time::Now() + base::TimeDelta::FromDays(gDayOffsetForTesting);

[raymes, 2017/03/15 04:28:25]

A more standard pattern is to store a base::Clock in a member variable. You can
override the clock for testing and then set the time as needed. See, e.g.
https://cs.chromium.org/chromium/src/chrome/browser/engagement/site_engagemen...

[benwells, 2017/03/15 23:10:48]

Yep, I know. I think I even added the clock to the site engagement. In this case
I thought it was less code to allow a simple day offset.

+}
+
+}  // namespace
+
+// static
+void GeolocationPermissionContextAndroid::RegisterProfilePrefs(
+    PrefRegistrySimple* registry) {
+  registry->RegisterIntegerPref(prefs::kLocationSettingsBackoffLevelDSE, 0);
+  registry->RegisterIntegerPref(prefs::kLocationSettingsBackoffLevelDefault, 0);
+  registry->RegisterInt64Pref(prefs::kLocationSettingsNextShowDSE, 0);
+  registry->RegisterInt64Pref(prefs::kLocationSettingsNextShowDefault, 0);
+}
+
 GeolocationPermissionContextAndroid::
     GeolocationPermissionContextAndroid(Profile* profile)
     : GeolocationPermissionContext(profile),
@@ -77,6 +102,18 @@ ContentSetting GeolocationPermissionContextAndroid::GetPermissionStatusInternal(
   return value;
 }
 
+// static
+void GeolocationPermissionContextAndroid::AddDayOffsetForTesting(int days) {
+  gDayOffsetForTesting += days;
+}
+
+// static
+void GeolocationPermissionContextAndroid::SetDSEOriginForTesting(
+    const GURL& dse_origin) {
+  gHasDSEOriginForTesting = true;
+  gDSEOriginForTesting = dse_origin;
+}
+
 void GeolocationPermissionContextAndroid::RequestPermission(
     content::WebContents* web_contents,
     const PermissionRequestID& id,
@@ -129,6 +166,24 @@ void GeolocationPermissionContextAndroid::CancelPermissionRequest(
   GeolocationPermissionContext::CancelPermissionRequest(web_contents, id);
 }
 
+void GeolocationPermissionContextAndroid::PermissionDecided(
+    const PermissionRequestID& id,
+    const GURL& requesting_origin,
+    const GURL& embedding_origin,
+    bool user_gesture,
+    const BrowserPermissionCallback& callback,
+    bool persist,
+    ContentSetting content_setting) {
+  // If the user has accepted geolocation, reset the location settings dialog
+  // backoff.
+  if (content_setting == CONTENT_SETTING_ALLOW)
+    ResetLocationSettingsBackOff(requesting_origin);
+
+  GeolocationPermissionContext::PermissionDecided(
+      id, requesting_origin, embedding_origin, user_gesture, callback, persist,
+      content_setting);
+}
+
 void GeolocationPermissionContextAndroid::NotifyPermissionSet(
     const PermissionRequestID& id,
     const GURL& requesting_origin,
@@ -139,7 +194,15 @@ void GeolocationPermissionContextAndroid::NotifyPermissionSet(
   if (content_setting == CONTENT_SETTING_ALLOW &&
       !location_settings_->IsSystemLocationSettingEnabled()) {
     // There is no need to check CanShowLocationSettingsDialog here again, as it
-    // must have been checked to get this far.
+    // must have been checked to get this far. But, the backoff will not have
+    // been checked, so check that.

[raymes, 2017/03/15 04:28:25]

nit: maybe explain why the backoff hasn't been checked yet because I think it's
subtle. I think it's because the user may have been prompted and so we only
really know if we're in backoff after we try to prompt the user.

[benwells, 2017/03/15 23:10:48]

Done.

+    if (IsInLocationSettingsBackOff(requesting_origin)) {
+      FinishNotifyPermissionSet(id, requesting_origin, embedding_origin,
+                                callback, false /* persist */,
+                                CONTENT_SETTING_BLOCK);
+      return;
+    }
+
     content::WebContents* web_contents =
         content::WebContents::FromRenderFrameHost(
             content::RenderFrameHost::FromID(id.render_process_id(),
@@ -170,8 +233,11 @@ GeolocationPermissionContextAndroid::UpdatePermissionStatusWithDeviceStatus(
       // gesture here. If there is a possibility of PROMPT (i.e. if there is a
       // user gesture attached to the later request) that should be returned,
       // not BLOCK.
-      if (CanShowLocationSettingsDialog(requesting_origin,
-                                        true /* user_gesture */)) {
+      // If the permission is in the ASK state, backoff is ignored.

[raymes, 2017/03/15 04:28:25]

nit: maybe explain why this is the case here? I think it's because we want it
such that if the user sees an infobar and accepts it, then we still display LSD.
Basically, if we were going to display an infobar anyway, it's ok to show them
the LSD.

[benwells, 2017/03/15 23:10:48]

Done.

+      if (CanShowLocationSettingsDialog(
+              requesting_origin, true /* user_gesture */,
+              result.content_setting ==
+                  CONTENT_SETTING_ASK /* ignore_backoff */)) {
         result.content_setting = CONTENT_SETTING_ASK;
       } else {
         result.content_setting = CONTENT_SETTING_BLOCK;
@@ -191,6 +257,70 @@ GeolocationPermissionContextAndroid::UpdatePermissionStatusWithDeviceStatus(
   return result;
 }
 
+std::string
+GeolocationPermissionContextAndroid::LocationSettingsBackOffLevelPref(
+    const GURL& requesting_origin) const {
+  if (GetLocationSettingsDialogContext(requesting_origin) == SEARCH)
+    return prefs::kLocationSettingsBackoffLevelDSE;
+
+  return prefs::kLocationSettingsBackoffLevelDefault;
+}
+
+std::string GeolocationPermissionContextAndroid::LocationSettingsNextShowPref(
+    const GURL& requesting_origin) const {
+  if (GetLocationSettingsDialogContext(requesting_origin) == SEARCH)
+    return prefs::kLocationSettingsNextShowDSE;
+
+  return prefs::kLocationSettingsNextShowDefault;
+}
+
+bool GeolocationPermissionContextAndroid::IsInLocationSettingsBackOff(
+    const GURL& requesting_origin) const {
+  base::Time next_show =
+      base::Time::FromInternalValue(profile()->GetPrefs()->GetInt64(
+          LocationSettingsNextShowPref(requesting_origin)));
+
+  return GetTimeNow() < next_show;
+}
+
+void GeolocationPermissionContextAndroid::ResetLocationSettingsBackOff(
+    const GURL& requesting_origin) {
+  PrefService* prefs = profile()->GetPrefs();
+  prefs->ClearPref(LocationSettingsNextShowPref(requesting_origin));
+  prefs->ClearPref(LocationSettingsBackOffLevelPref(requesting_origin));
+}
+
+void GeolocationPermissionContextAndroid::SetLocationSettingsBackOff(

[raymes, 2017/03/15 04:28:25]

nit: maybe this should be ApplyLocationSettingsBackOff or
UpdateLocationSettingsBackOff?

[benwells, 2017/03/15 23:10:49]

Done.

+    const GURL& requesting_origin) {
+  // Backoff levels:
+  // 0: 1 week
+  // 1: 1 month
+  // 2: 3 months
+  PrefService* prefs = profile()->GetPrefs();
+  int backoff_level =
+      prefs->GetInteger(LocationSettingsBackOffLevelPref(requesting_origin));
+
+  base::Time next_show = GetTimeNow();
+  switch (backoff_level) {
+    case 0:
+      next_show += base::TimeDelta::FromDays(7);
+      break;
+    case 1:
+      next_show += base::TimeDelta::FromDays(30);
+      break;
+    default:
+      next_show += base::TimeDelta::FromDays(90);
+  }
+
+  if (backoff_level < 2)
+    backoff_level++;
+
+  prefs->SetInteger(LocationSettingsBackOffLevelPref(requesting_origin),
+                    backoff_level);
+  prefs->SetInt64(LocationSettingsNextShowPref(requesting_origin),
+                  next_show.ToInternalValue());
+}
+
 bool GeolocationPermissionContextAndroid::IsLocationAccessPossible(
     content::WebContents* web_contents,
     const GURL& requesting_origin,
@@ -199,25 +329,32 @@ bool GeolocationPermissionContextAndroid::IsLocationAccessPossible(
           location_settings_->CanPromptForAndroidLocationPermission(
               web_contents)) &&
          (location_settings_->IsSystemLocationSettingEnabled() ||
-          CanShowLocationSettingsDialog(requesting_origin, user_gesture));
+          CanShowLocationSettingsDialog(requesting_origin, user_gesture,
+                                        true /* ignore_backoff */));
 }
 
 LocationSettingsDialogContext
 GeolocationPermissionContextAndroid::GetLocationSettingsDialogContext(

[raymes, 2017/03/15 04:28:25]

nit: I wonder if this is more clearly framed as 
IsRequestingOriginDSE ?

since "Context" is overloaded in this context :) Only one callsite passes on the
return value.

[benwells, 2017/03/15 23:10:48]

Done.

     const GURL& requesting_origin) const {
   bool is_dse_origin = false;
-  TemplateURLService* template_url_service =
-      TemplateURLServiceFactory::GetForProfile(profile());
-  if (template_url_service) {
-    const TemplateURL* template_url =
-        template_url_service->GetDefaultSearchProvider();
-    if (template_url) {
-      GURL search_url = template_url->GenerateSearchURL(
-          template_url_service->search_terms_data());
-      is_dse_origin = url::IsSameOriginWith(requesting_origin, search_url);
+  GURL dse_url;
+
+  if (gHasDSEOriginForTesting) {
+    dse_url = gDSEOriginForTesting;
+  } else {
+    TemplateURLService* template_url_service =
+        TemplateURLServiceFactory::GetForProfile(profile());
+    if (template_url_service) {
+      const TemplateURL* template_url =
+          template_url_service->GetDefaultSearchProvider();
+      if (template_url) {
+        dse_url = template_url->GenerateSearchURL(
+            template_url_service->search_terms_data());
+      }
     }
   }
 
+  is_dse_origin = url::IsSameOriginWith(requesting_origin, dse_url);
   return is_dse_origin ? SEARCH : DEFAULT;
 }
 
@@ -239,7 +376,8 @@ void GeolocationPermissionContextAndroid::HandleUpdateAndroidPermissions(
 
 bool GeolocationPermissionContextAndroid::CanShowLocationSettingsDialog(
     const GURL& requesting_origin,
-    bool user_gesture) const {
+    bool user_gesture,
+    bool ignore_backoff) const {
   if (!base::FeatureList::IsEnabled(features::kLsdPermissionPrompt))
     return false;
 
@@ -249,7 +387,9 @@ bool GeolocationPermissionContextAndroid::CanShowLocationSettingsDialog(
     return false;
   }
 
-  // TODO(benwells): Also check backoff for |requesting_origin|.
+  if (!ignore_backoff && IsInLocationSettingsBackOff(requesting_origin))
+    return false;
+
   return location_settings_->CanPromptToEnableSystemLocationSetting();
 }
 
@@ -261,7 +401,10 @@ void GeolocationPermissionContextAndroid::OnLocationSettingsDialogShown(
     bool persist,
     ContentSetting content_setting,
     LocationSettingsDialogOutcome prompt_outcome) {
-  if (prompt_outcome != GRANTED) {
+  if (prompt_outcome == GRANTED) {
+    ResetLocationSettingsBackOff(requesting_origin);
+  } else {
+    SetLocationSettingsBackOff(requesting_origin);
     content_setting = CONTENT_SETTING_BLOCK;
     persist = false;
   }