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

chrome/browser/geolocation/geolocation_permission_context_android.cc

 // Copyright 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/geolocation/geolocation_permission_context_android.h"
 
 #include <utility>
 #include <vector>
 
 #include "base/bind.h"
 #include "base/feature_list.h"
 #include "chrome/browser/android/location_settings.h"
 #include "chrome/browser/android/location_settings_impl.h"
 #include "chrome/browser/android/search_geolocation/search_geolocation_disclosure_tab_helper.h"
 #include "chrome/browser/android/search_geolocation/search_geolocation_service.h"
 #include "chrome/browser/permissions/permission_request_id.h"
 #include "chrome/browser/permissions/permission_update_infobar_delegate_android.h"
 #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"
 #include "content/public/browser/render_frame_host.h"
 #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),
       location_settings_(new LocationSettingsImpl()),
       permission_update_infobar_(nullptr),
       weak_factory_(this) {
 }
 
 GeolocationPermissionContextAndroid::~GeolocationPermissionContextAndroid() {
 }
@@ skipping 31 matching lines @@
       } else if (!profile()->IsOffTheRecord()) {
         // Otherwise, return ALLOW only if this is not incognito.
         value = CONTENT_SETTING_ALLOW;
       }
     }
   }
 
   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,
     const GURL& requesting_frame_origin,
     bool user_gesture,
     const BrowserPermissionCallback& callback) {
   if (!IsLocationAccessPossible(web_contents, requesting_frame_origin,
                                 user_gesture)) {
     PermissionDecided(id, requesting_frame_origin,
                       web_contents->GetLastCommittedURL().GetOrigin(),
@@ skipping 32 matching lines @@
     content::WebContents* web_contents,
     const PermissionRequestID& id) {
   if (permission_update_infobar_) {
     permission_update_infobar_->RemoveSelf();
     permission_update_infobar_ = nullptr;
   }
 
   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,
     const GURL& embedding_origin,
     const BrowserPermissionCallback& callback,
     bool persist,
     ContentSetting content_setting) {
   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(),
                                              id.render_frame_id()));
     location_settings_->PromptToEnableSystemLocationSetting(
         GetLocationSettingsDialogContext(requesting_origin), web_contents,
         base::BindOnce(
             &GeolocationPermissionContextAndroid::OnLocationSettingsDialogShown,
             weak_factory_.GetWeakPtr(), id, requesting_origin, embedding_origin,
             callback, persist, content_setting));
@@ skipping 10 matching lines @@
     const GURL& requesting_origin,
     const GURL& embedding_origin) const {
   if (base::FeatureList::IsEnabled(features::kLsdPermissionPrompt) &&
       result.content_setting != CONTENT_SETTING_BLOCK) {
     if (!location_settings_->IsSystemLocationSettingEnabled()) {
       // As this is returning the status for possible future permission
       // requests, whose gesture status is unknown, pretend there is a user
       // 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;
       }
       result.source = PermissionStatusSource::UNSPECIFIED;
     }
 
     if (result.content_setting != CONTENT_SETTING_BLOCK &&
         !location_settings_->HasAndroidLocationPermission()) {
       // TODO(benwells): plumb through the RFH and use the associated
       // WebContents to check that the android location can be prompted for.
       result.content_setting = CONTENT_SETTING_ASK;
       result.source = PermissionStatusSource::UNSPECIFIED;
     }
   }
 
   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,
     bool user_gesture) {
   return (location_settings_->HasAndroidLocationPermission() ||
           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;
 }
 
 void GeolocationPermissionContextAndroid::HandleUpdateAndroidPermissions(
     const PermissionRequestID& id,
     const GURL& requesting_frame_origin,
     const GURL& embedding_origin,
     const BrowserPermissionCallback& callback,
     bool permissions_updated) {
   permission_update_infobar_ = nullptr;
 
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
   ContentSetting new_setting = permissions_updated
       ? CONTENT_SETTING_ALLOW : CONTENT_SETTING_BLOCK;
 
   NotifyPermissionSet(id, requesting_frame_origin, embedding_origin, callback,
                       false /* persist */, new_setting);
 }
 
 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;
 
   // If this isn't the default search engine, a gesture is needed.
   if (GetLocationSettingsDialogContext(requesting_origin) == DEFAULT &&
       !user_gesture) {
     return false;
   }
 
-  // TODO(benwells): Also check backoff for |requesting_origin|.
+  if (!ignore_backoff && IsInLocationSettingsBackOff(requesting_origin))
+    return false;
+
   return location_settings_->CanPromptToEnableSystemLocationSetting();
 }
 
 void GeolocationPermissionContextAndroid::OnLocationSettingsDialogShown(
     const PermissionRequestID& id,
     const GURL& requesting_origin,
     const GURL& embedding_origin,
     const BrowserPermissionCallback& callback,
     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;
   }
 
   FinishNotifyPermissionSet(id, requesting_origin, embedding_origin, callback,
                             persist, content_setting);
 }
 
 void GeolocationPermissionContextAndroid::FinishNotifyPermissionSet(
     const PermissionRequestID& id,
@@ skipping 22 matching lines @@
     // The tab helper can be null in tests.
     if (disclosure_helper)
       disclosure_helper->MaybeShowDisclosure(requesting_origin);
   }
 }
 
 void GeolocationPermissionContextAndroid::SetLocationSettingsForTesting(
     std::unique_ptr<LocationSettings> settings) {
   location_settings_ = std::move(settings);
 }