Convert AutoBlocker static class to KeyedService.

chrome/browser/permissions/permission_decision_auto_blocker.h

 // Copyright 2016 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.
 
 #ifndef CHROME_BROWSER_PERMISSIONS_PERMISSION_DECISION_AUTO_BLOCKER_H_
 #define CHROME_BROWSER_PERMISSIONS_PERMISSION_DECISION_AUTO_BLOCKER_H_
 
 #include "base/callback.h"
 #include "base/macros.h"
 #include "base/memory/ref_counted.h"
+#include "base/memory/singleton.h"
+#include "components/keyed_service/content/browser_context_keyed_service_factory.h"
+#include "components/keyed_service/core/keyed_service.h"
 #include "content/public/browser/permission_type.h"
 #include "url/gurl.h"
 
 class GURL;
 class Profile;
 
 namespace content {
 class WebContents;
 }
 
 namespace safe_browsing {
 class SafeBrowsingDatabaseManager;
 }
 
 namespace base {
 class Time;
 }
 
 class HostContentSettingsMap;
 
 // The PermissionDecisionAutoBlocker decides whether or not a given origin
 // should be automatically blocked from requesting a permission. When an origin
 // is blocked, it is placed under an "embargo". Until the embargo expires, any
 // requests made by the origin are automatically blocked. Once the embargo is
 // lifted, the origin will be permitted to request a permission again, which may
 // result in it being placed under embargo again. Currently, an origin can be
 // placed under embargo if it appears on Safe Browsing's API blacklist, or if it
 // has a number of prior dismissals greater than a threshold.
-class PermissionDecisionAutoBlocker {
+class PermissionDecisionAutoBlocker : public KeyedService {
  public:
-  // Removes any recorded counts for urls which match |filter| under |profile|.
-  static void RemoveCountsByUrl(Profile* profile,
-                                base::Callback<bool(const GURL& url)> filter);
+  explicit PermissionDecisionAutoBlocker(Profile* profile);
+  ~PermissionDecisionAutoBlocker() override;

[raymes, 2017/01/23 06:14:23]

You should be able to make the constructor/destrutor private and the factory
could be a friend class or it could just be a nested class (see e.g.
https://cs.chromium.org/chromium/src/chrome/browser/android/search_geolocatio...)

[meredithl, 2017/01/24 01:20:46]

Done.

+
+  static PermissionDecisionAutoBlocker* GetForProfile(Profile* profile);
+
+  // Removes any recorded counts for urls which match |filter|.
+  void RemoveCountsByUrl(base::Callback<bool(const GURL& url)> filter);
 
   // Returns the current number of dismisses recorded for |permission| type at
   // |url|.
-  static int GetDismissCount(const GURL& url,
-                             content::PermissionType permission,
-                             Profile* profile);
+  int GetDismissCount(const GURL& url, content::PermissionType permission);
 
   // Returns the current number of ignores recorded for |permission|
   // type at |url|.
-  static int GetIgnoreCount(const GURL& url,
-                            content::PermissionType permission,
-                            Profile* profile);
+  int GetIgnoreCount(const GURL& url, content::PermissionType permission);
 
   // Records that a dismissal of a prompt for |permission| was made. If the
   // total number of dismissals exceeds a threshhold and
   // features::kBlockPromptsIfDismissedOften is enabled it will place |url|
   // under embargo for |permission|.
-  static bool RecordDismissAndEmbargo(const GURL& url,
-                                      content::PermissionType permission,
-                                      Profile* profile,
-                                      base::Time current_time);
+  bool RecordDismissAndEmbargo(const GURL& url,
+                               content::PermissionType permission,
+                               base::Time current_time);
 
   // Records that an ignore of a prompt for |permission| was made.
-  static int RecordIgnore(const GURL& url,
-                          content::PermissionType permission,
-                          Profile* profile);
-
-  // Records that a dismissal of a prompt for |permission| was made, and returns
-  // true if this dismissal should be considered a block. False otherwise.
-  // TODO(meredithl): Remove in favour of embargoing on repeated dismissals.
-  static bool ShouldChangeDismissalToBlock(const GURL& url,
-                                           content::PermissionType permission,
-                                           Profile* profile);
+  int RecordIgnore(const GURL& url, content::PermissionType permission);
 
   // Updates the threshold to start blocking prompts from the field trial.
   static void UpdateFromVariations();
 
   // Checks if |request_origin| is under embargo for |permission|. Internally,
   // this will make a call to IsUnderEmbargo to check the content setting first,
-  // but may also make a call to Safe Browsing to check if |request_origin| is
-  // blacklisted for |permission|, which is performed asynchronously.
-  static void 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);
+  // but may also make a call to Safe Browsing to check their API blacklist,

[raymes, 2017/01/23 06:14:23]

their->the

[meredithl, 2017/01/24 01:20:46]

Done.

+  // which is performed asynchronously.
+  void UpdateEmbargoedStatus(content::PermissionType permission,
+                             const GURL& request_origin,
+                             content::WebContents* web_contents,
+                             base::Time current_time,
+                             base::Callback<void(bool)> callback);
 
   // Checks the status of the content setting to determine if |request_origin|
   // is under embargo for |permission|. This checks both embargo for Permissions
   // Blacklisting and repeated dismissals.
-  static bool IsUnderEmbargo(content::PermissionType permission,
-                             Profile* profile,
-                             const GURL& request_origin,
-                             base::Time current_time);
+  bool IsUnderEmbargo(content::PermissionType permission,
+                      const GURL& request_origin,
+                      base::Time current_time);
+
+  void SetSafeBrowsingDatabaseManagerAndTimeoutForTesting(
+      scoped_refptr<safe_browsing::SafeBrowsingDatabaseManager> db_manager,
+      int timeout);

[raymes, 2017/01/23 06:14:23]

nit: this should be private

[meredithl, 2017/01/24 01:20:46]

Done.

 
  private:
   friend class PermissionContextBaseTests;
   friend class PermissionDecisionAutoBlockerUnitTest;
 
   static void CheckSafeBrowsingResult(content::PermissionType permission,
                                       Profile* profile,
                                       const GURL& request_origin,
                                       base::Time current_time,
                                       base::Callback<void(bool)> callback,
                                       bool should_be_embargoed);
 
   static void PlaceUnderEmbargo(content::PermissionType permission,
                                 const GURL& request_origin,
                                 HostContentSettingsMap* map,
                                 base::Time current_time,
                                 const char* key);

[raymes, 2017/01/23 06:14:23]

nit: convert these to member functions or just standalone functions that live in
the anonymous namespace of the implementation

[meredithl, 2017/01/24 01:20:46]

CheckSafeBrowsingResult is static to avoid having to pass the client a bound
method, so it has safer lifetime semantics. Internally, this can call
PlaceUnderEmbargo, so this was made also static.

These were both in the anonymous namespace originally, which is why I was using
the PlaceUnderEmbargoForTest methods in the autoblocker to still access them for
testing, which was removed in a previous CL in favour of the current way, so as
to only test the public API. 

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

I don't think it is actually safer from a lifetime perspective. The lifetime of
this object is tied to the profile's lifetime so either way we need to make sure
the profile is still around. I think it might be slightly nicer to make these
member functions because it means that the test isn't accessing stuff without
getting a handle on this object first.

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

Done.

 
   // Keys used for storing count data in a website setting.
   static const char kPromptDismissCountKey[];
   static const char kPromptIgnoreCountKey[];
   static const char kPermissionDismissalEmbargoKey[];
   static const char kPermissionBlacklistEmbargoKey[];
 
+  Profile* profile_;
+  int safe_browsing_timeout_;

[raymes, 2017/01/23 06:14:23]

nit: please document the units that the timeout is in

[meredithl, 2017/01/24 01:20:46]

Done.

+  scoped_refptr<safe_browsing::SafeBrowsingDatabaseManager> db_manager_;
+
   DISALLOW_IMPLICIT_CONSTRUCTORS(PermissionDecisionAutoBlocker);
 };
 
+class PermissionDecisionAutoBlockerFactory
+    : public BrowserContextKeyedServiceFactory {
+ public:
+  static PermissionDecisionAutoBlocker* GetForProfile(Profile* profile);
+  static PermissionDecisionAutoBlockerFactory* GetInstance();
+
+ private:
+  friend struct base::DefaultSingletonTraits<
+      PermissionDecisionAutoBlockerFactory>;
+
+  PermissionDecisionAutoBlockerFactory();
+  ~PermissionDecisionAutoBlockerFactory() override;
+
+  // BrowserContextKeyedServiceFactory
+  KeyedService* BuildServiceInstanceFor(
+      content::BrowserContext* context) const override;
+
+  content::BrowserContext* GetBrowserContextToUse(
+      content::BrowserContext* context) const override;
+};
 #endif  // CHROME_BROWSER_PERMISSIONS_PERMISSION_DECISION_AUTO_BLOCKER_H_