Convert AutoBlocker static class to KeyedService.

chrome/browser/permissions/permission_decision_auto_blocker.cc

Index: chrome/browser/permissions/permission_decision_auto_blocker.cc
diff --git a/chrome/browser/permissions/permission_decision_auto_blocker.cc b/chrome/browser/permissions/permission_decision_auto_blocker.cc
index b53fe83b542dc18bf3a3578bf989007440dabfcf..f585758dd198eede7bc49806532c999fe3b449b2 100644
--- a/chrome/browser/permissions/permission_decision_auto_blocker.cc
+++ b/chrome/browser/permissions/permission_decision_auto_blocker.cc
@@ -10,15 +10,19 @@
 #include "base/logging.h"
 #include "base/memory/ptr_util.h"
 #include "base/strings/string_number_conversions.h"
-#include "base/time/time.h"
+#include "base/time/default_clock.h"
 #include "base/values.h"
+#include "chrome/browser/browser_process.h"
 #include "chrome/browser/content_settings/host_content_settings_map_factory.h"
 #include "chrome/browser/permissions/permission_blacklist_client.h"
 #include "chrome/browser/permissions/permission_util.h"
-#include "chrome/browser/permissions/permission_blacklist_client.h"
-#include "chrome/browser/permissions/permission_util.h"
+#include "chrome/browser/profiles/incognito_helpers.h"
+#include "chrome/browser/profiles/profile.h"
+#include "chrome/browser/safe_browsing/safe_browsing_service.h"
 #include "chrome/common/chrome_features.h"
 #include "components/content_settings/core/browser/host_content_settings_map.h"
+#include "components/keyed_service/content/browser_context_dependency_manager.h"
+#include "components/safe_browsing_db/database_manager.h"
 #include "components/variations/variations_associated_data.h"
 #include "content/public/browser/permission_type.h"
 #include "content/public/browser/web_contents.h"
@@ -38,6 +42,8 @@ int g_blacklist_embargo_days = 7;
 // permission due to repeated dismissals.
 int g_dismissal_embargo_days = 7;
 
+// TODO(meredithl): Migrate to a new and more fitting type, once metrics have
+// been gathered, and deprecate CONTENT_SETTINGS_TYPE_PROMPT_NO_DECISION_COUNT.
 std::unique_ptr<base::DictionaryValue> GetOriginDict(
     HostContentSettingsMap* settings,
     const GURL& origin_url) {
@@ -104,6 +110,8 @@ int GetActionCount(const GURL& url,
 
 }  // namespace
 
+// PermissionDecisionAutoBlocker -----------------------------------------------
+
 // static
 const char PermissionDecisionAutoBlocker::kPromptDismissCountKey[] =
     "dismiss_count";
@@ -120,12 +128,35 @@ const char PermissionDecisionAutoBlocker::kPermissionBlacklistEmbargoKey[] =
 const char PermissionDecisionAutoBlocker::kPermissionDismissalEmbargoKey[] =
     "dismissal_embargo_days";
 
-// static
+// Maximum time in milliseconds to wait for safe browsing service to check a
+// url for blacklisting. After this amount of time, the check will be aborted
+// and the url will be treated as not safe.
+// TODO(meredithl): Revisit this once UMA metrics have data about request time.
+const int kCheckUrlTimeoutMs = 2000;
+
+PermissionDecisionAutoBlocker::PermissionDecisionAutoBlocker(Profile* profile)
+    : profile_(profile),
+      safe_browsing_timeout_(kCheckUrlTimeoutMs),
+      clock_(new base::DefaultClock()) {
+  if (!db_manager_) {

[raymes, 2017/01/24 03:31:08]

The db_manager_ will always be null at the start, so this check is unncessary
(actually right now it is undefined, so this could result in a bug). 

To fix:
-Initialize db_manager_ to null in the initializer list
-Remove this line

[meredithl, 2017/01/24 04:52:25]

Done. But that means I still need to add a null check before calling the client
method, yes? Again, I'm not sure under which circumstances, if any, caling
database_manager() will return a nullptr.

+    safe_browsing::SafeBrowsingService* sb_service =
+        g_browser_process->safe_browsing_service();
+    if (sb_service)
+      db_manager_ = sb_service->database_manager();
+  }
+}
+
+PermissionDecisionAutoBlocker::~PermissionDecisionAutoBlocker() {}
+
+PermissionDecisionAutoBlocker* PermissionDecisionAutoBlocker::GetForProfile(

[raymes, 2017/01/24 03:31:08]

nit: // static

[meredithl, 2017/01/24 04:52:25]

Done.

+    Profile* profile) {
+  return PermissionDecisionAutoBlocker::Factory::GetForProfile(profile);
+}
+
 void PermissionDecisionAutoBlocker::RemoveCountsByUrl(
-    Profile* profile,
     base::Callback<bool(const GURL& url)> filter) {
   HostContentSettingsMap* map =
-      HostContentSettingsMapFactory::GetForProfile(profile);
+      HostContentSettingsMapFactory::GetForProfile(profile_);
 
   std::unique_ptr<ContentSettingsForOneType> settings(
       new ContentSettingsForOneType);
@@ -143,62 +174,39 @@ void PermissionDecisionAutoBlocker::RemoveCountsByUrl(
   }
 }
 
-// static
 int PermissionDecisionAutoBlocker::GetDismissCount(
     const GURL& url,
-    content::PermissionType permission,
-    Profile* profile) {
-  return GetActionCount(url, permission, kPromptDismissCountKey, profile);
+    content::PermissionType permission) {
+  return GetActionCount(url, permission, kPromptDismissCountKey, profile_);
 }
 
-// static
 int PermissionDecisionAutoBlocker::GetIgnoreCount(
     const GURL& url,
-    content::PermissionType permission,
-    Profile* profile) {
-  return GetActionCount(url, permission, kPromptIgnoreCountKey, profile);
+    content::PermissionType permission) {
+  return GetActionCount(url, permission, kPromptIgnoreCountKey, profile_);
 }
 
-// static
 bool PermissionDecisionAutoBlocker::RecordDismissAndEmbargo(
     const GURL& url,
-    content::PermissionType permission,
-    Profile* profile,
-    base::Time current_time) {
+    content::PermissionType permission) {
   int current_dismissal_count = RecordActionInWebsiteSettings(
-      url, permission, kPromptDismissCountKey, profile);
+      url, permission, kPromptDismissCountKey, profile_);
+
   if (base::FeatureList::IsEnabled(features::kBlockPromptsIfDismissedOften) &&
       current_dismissal_count >= g_prompt_dismissals_before_block) {
-    HostContentSettingsMap* map =
-        HostContentSettingsMapFactory::GetForProfile(profile);
-    PlaceUnderEmbargo(permission, url, map, current_time,
-                      kPermissionDismissalEmbargoKey);
+    PlaceUnderEmbargo(permission, url,
+                      HostContentSettingsMapFactory::GetForProfile(profile_),
+                      clock_->Now(), kPermissionDismissalEmbargoKey);
     return true;
   }
   return false;
 }
 
-// static
 int PermissionDecisionAutoBlocker::RecordIgnore(
     const GURL& url,
-    content::PermissionType permission,
-    Profile* profile) {
+    content::PermissionType permission) {
   return RecordActionInWebsiteSettings(url, permission, kPromptIgnoreCountKey,
-                                       profile);
-}
-
-// static
-bool PermissionDecisionAutoBlocker::ShouldChangeDismissalToBlock(
-    const GURL& url,
-    content::PermissionType permission,
-    Profile* profile) {
-  int current_dismissal_count =
-      RecordDismissAndEmbargo(url, permission, profile, base::Time::Now());
-
-  if (!base::FeatureList::IsEnabled(features::kBlockPromptsIfDismissedOften))
-    return false;
-
-  return current_dismissal_count >= g_prompt_dismissals_before_block;
+                                       profile_);
 }
 
 // static
@@ -230,30 +238,24 @@ void PermissionDecisionAutoBlocker::UpdateFromVariations() {
   }
 }
 
-// static
-// TODO(meredithl): Have PermissionDecisionAutoBlocker handle the database
-// manager, rather than passing it in.
 void PermissionDecisionAutoBlocker::UpdateEmbargoedStatus(
-    scoped_refptr<safe_browsing::SafeBrowsingDatabaseManager> db_manager,
     content::PermissionType permission,
     const GURL& request_origin,
     content::WebContents* web_contents,
-    int timeout,
-    Profile* profile,
-    base::Time current_time,
     base::Callback<void(bool)> callback) {
   // Check if origin is currently under embargo for the requested permission.
-  if (IsUnderEmbargo(permission, profile, request_origin, current_time)) {
+  if (IsUnderEmbargo(permission, request_origin)) {
     callback.Run(true /* permission_blocked */);
     return;
   }
 
   if (base::FeatureList::IsEnabled(features::kPermissionsBlacklist) &&
-      db_manager) {
+      db_manager_) {
     PermissionBlacklistClient::CheckSafeBrowsingBlacklist(

[raymes, 2017/01/24 03:31:08]

nit: I discussed about lifetimes with Dom. Could you add a comment here, as well
as in the header for this function, that the callback passed in will not be
called if the profile is destroyed in the process. Something like

// The CheckSafeBrowsingResult callback won't be called if the profile is
destroyed before a result is received. In that case this object will have been
destroyed by that point.

And in the header for CheckSafeBrowsingBlacklist
// The callback passed in will not be called if the |web_contents| passed in is
destroyed. Thus if the callback is run, the profile associated with
|web_contents| is guaranteed to be alive.

We should think about how to make those guarantees clearer by better API design
at some point (but not now :)

[meredithl, 2017/01/24 04:52:25]

Done.

-        db_manager, permission, request_origin, web_contents, timeout,
+        db_manager_, permission, request_origin, web_contents,
+        safe_browsing_timeout_,
         base::Bind(&PermissionDecisionAutoBlocker::CheckSafeBrowsingResult,
-                   permission, profile, request_origin, current_time,
+                   permission, profile_, request_origin, clock_->Now(),
                    callback));
     return;
   }
@@ -261,14 +263,11 @@ void PermissionDecisionAutoBlocker::UpdateEmbargoedStatus(
   callback.Run(false /* permission blocked */);
 }
 
-// static
 bool PermissionDecisionAutoBlocker::IsUnderEmbargo(
     content::PermissionType permission,
-    Profile* profile,
-    const GURL& request_origin,
-    base::Time current_time) {
+    const GURL& request_origin) {
   HostContentSettingsMap* map =
-      HostContentSettingsMapFactory::GetForProfile(profile);
+      HostContentSettingsMapFactory::GetForProfile(profile_);
   std::unique_ptr<base::DictionaryValue> dict =
       GetOriginDict(map, request_origin);
   base::DictionaryValue* permission_dict = GetOrCreatePermissionDict(
@@ -276,6 +275,7 @@ bool PermissionDecisionAutoBlocker::IsUnderEmbargo(
   double embargo_date = -1;
   bool is_under_dismiss_embargo = false;
   bool is_under_blacklist_embargo = false;
+  base::Time current_time = clock_->Now();
   if (base::FeatureList::IsEnabled(features::kPermissionsBlacklist) &&
       permission_dict->GetDouble(kPermissionBlacklistEmbargoKey,
                                  &embargo_date)) {
@@ -295,24 +295,9 @@ bool PermissionDecisionAutoBlocker::IsUnderEmbargo(
       is_under_dismiss_embargo = true;
     }
   }
-  // If either embargoes is still in effect, return true.
-  return is_under_dismiss_embargo || is_under_blacklist_embargo;
-}
 
-void PermissionDecisionAutoBlocker::PlaceUnderEmbargo(
-    content::PermissionType permission,
-    const GURL& request_origin,
-    HostContentSettingsMap* map,
-    base::Time current_time,
-    const char* key) {
-  std::unique_ptr<base::DictionaryValue> dict =
-      GetOriginDict(map, request_origin);
-  base::DictionaryValue* permission_dict = GetOrCreatePermissionDict(
-      dict.get(), PermissionUtil::GetPermissionString(permission));
-  permission_dict->SetDouble(key, current_time.ToInternalValue());
-  map->SetWebsiteSettingDefaultScope(
-      request_origin, GURL(), CONTENT_SETTINGS_TYPE_PROMPT_NO_DECISION_COUNT,
-      std::string(), std::move(dict));
+  // If either embargo is still in effect, return true.
+  return is_under_dismiss_embargo || is_under_blacklist_embargo;
 }
 
 // static
@@ -334,73 +319,6 @@ void PermissionDecisionAutoBlocker::CheckSafeBrowsingResult(
 }
 
 // static
-// TODO(meredithl): Have PermissionDecisionAutoBlocker handle the database
-// manager, rather than passing it in.
-void PermissionDecisionAutoBlocker::UpdateEmbargoedStatus(
-    scoped_refptr<safe_browsing::SafeBrowsingDatabaseManager> db_manager,
-    content::PermissionType permission,
-    const GURL& request_origin,
-    content::WebContents* web_contents,
-    int timeout,
-    Profile* profile,
-    base::Time current_time,
-    base::Callback<void(bool)> callback) {
-  // Check if origin is currently under embargo for the requested permission.
-  if (IsUnderEmbargo(permission, profile, request_origin, current_time)) {
-    callback.Run(true /* permission_blocked */);
-    return;
-  }
-
-  if (base::FeatureList::IsEnabled(features::kPermissionsBlacklist) &&
-      db_manager) {
-    PermissionBlacklistClient::CheckSafeBrowsingBlacklist(
-        db_manager, permission, request_origin, web_contents, timeout,
-        base::Bind(&PermissionDecisionAutoBlocker::CheckSafeBrowsingResult,
-                   permission, profile, request_origin, current_time,
-                   callback));
-  }
-
-  callback.Run(false /* permission blocked */);
-}
-
-// static
-bool PermissionDecisionAutoBlocker::IsUnderEmbargo(
-    content::PermissionType permission,
-    Profile* profile,
-    const GURL& request_origin,
-    base::Time current_time) {
-  HostContentSettingsMap* map =
-      HostContentSettingsMapFactory::GetForProfile(profile);
-  std::unique_ptr<base::DictionaryValue> dict =
-      GetOriginDict(map, request_origin);
-  base::DictionaryValue* permission_dict = GetOrCreatePermissionDict(
-      dict.get(), PermissionUtil::GetPermissionString(permission));
-  double embargo_date = -1;
-  bool is_under_dismiss_embargo = false;
-  bool is_under_blacklist_embargo = false;
-  if (base::FeatureList::IsEnabled(features::kPermissionsBlacklist) &&
-      permission_dict->GetDouble(kPermissionBlacklistEmbargoKey,
-                                 &embargo_date)) {
-    if (current_time <
-        base::Time::FromInternalValue(embargo_date) +
-            base::TimeDelta::FromDays(g_blacklist_embargo_days)) {
-      is_under_blacklist_embargo = true;
-    }
-  }
-
-  if (base::FeatureList::IsEnabled(features::kBlockPromptsIfDismissedOften) &&
-      permission_dict->GetDouble(kPermissionDismissalEmbargoKey,
-                                 &embargo_date)) {
-    if (current_time <
-        base::Time::FromInternalValue(embargo_date) +
-            base::TimeDelta::FromDays(g_dismissal_embargo_days)) {
-      is_under_dismiss_embargo = true;
-    }
-  }
-  // If either embargoes is still in effect, return true.
-  return is_under_dismiss_embargo || is_under_blacklist_embargo;
-}
-
 void PermissionDecisionAutoBlocker::PlaceUnderEmbargo(
     content::PermissionType permission,
     const GURL& request_origin,
@@ -417,20 +335,49 @@ void PermissionDecisionAutoBlocker::PlaceUnderEmbargo(
       std::string(), std::move(dict));
 }
 
+void PermissionDecisionAutoBlocker::
+    SetSafeBrowsingDatabaseManagerAndTimeoutForTesting(
+        scoped_refptr<safe_browsing::SafeBrowsingDatabaseManager> db_manager,
+        int timeout) {
+  db_manager_ = db_manager;
+  safe_browsing_timeout_ = timeout;
+}
+
+void PermissionDecisionAutoBlocker::SetClockForTesting(
+    std::unique_ptr<base::Clock> clock) {
+  clock_ = std::move(clock);
+}
+
+// PermissionDecisionAutoBlocker::Factory --------------------------------------

[raymes, 2017/01/24 03:31:08]

nit: this section should probably move to the top to match the header.

[meredithl, 2017/01/24 04:52:25]

Done.

+
 // static
-void PermissionDecisionAutoBlocker::CheckSafeBrowsingResult(
-    content::PermissionType permission,
-    Profile* profile,
-    const GURL& request_origin,
-    base::Time current_time,
-    base::Callback<void(bool)> callback,
-    bool should_be_embargoed) {
-  if (should_be_embargoed) {
-    // Requesting site is blacklisted for this permission, update the content
-    // setting to place it under embargo.
-    PlaceUnderEmbargo(permission, request_origin,
-                      HostContentSettingsMapFactory::GetForProfile(profile),
-                      current_time, kPermissionBlacklistEmbargoKey);
-  }
-  callback.Run(should_be_embargoed /* permission blocked */);
+PermissionDecisionAutoBlocker*
+PermissionDecisionAutoBlocker::Factory::GetForProfile(Profile* profile) {
+  return static_cast<PermissionDecisionAutoBlocker*>(
+      GetInstance()->GetServiceForBrowserContext(profile, true));
+}
+
+// static
+PermissionDecisionAutoBlocker::Factory*
+PermissionDecisionAutoBlocker::Factory::GetInstance() {
+  return base::Singleton<PermissionDecisionAutoBlocker::Factory>::get();
+}
+
+PermissionDecisionAutoBlocker::Factory::Factory()
+    : BrowserContextKeyedServiceFactory(
+          "PermissionDecisionAutoBlocker",
+          BrowserContextDependencyManager::GetInstance()) {}
+
+PermissionDecisionAutoBlocker::Factory::~Factory() {}
+
+KeyedService* PermissionDecisionAutoBlocker::Factory::BuildServiceInstanceFor(
+    content::BrowserContext* context) const {
+  Profile* profile = static_cast<Profile*>(context);
+  return new PermissionDecisionAutoBlocker(profile);
+}
+
+content::BrowserContext*
+PermissionDecisionAutoBlocker::Factory::GetBrowserContextToUse(
+    content::BrowserContext* context) const {
+  return chrome::GetBrowserContextOwnInstanceInIncognito(context);
 }