Fetch extension blacklist states from SafeBrowsing server

chrome/browser/extensions/blacklist.h

Index: chrome/browser/extensions/blacklist.h
diff --git a/chrome/browser/extensions/blacklist.h b/chrome/browser/extensions/blacklist.h
index 200b5e6ff6c197970afe6329153481013dd0b1e3..85bdf2bf2941eb88c964c5bb405819c143808d22 100644
--- a/chrome/browser/extensions/blacklist.h
+++ b/chrome/browser/extensions/blacklist.h
@@ -5,12 +5,14 @@
 #ifndef CHROME_BROWSER_EXTENSIONS_BLACKLIST_H_
 #define CHROME_BROWSER_EXTENSIONS_BLACKLIST_H_
 
+#include <list>
 #include <map>
 #include <set>
 #include <string>
 #include <vector>
 
 #include "base/callback.h"
+#include "base/memory/scoped_ptr.h"
 #include "base/memory/weak_ptr.h"
 #include "base/observer_list.h"
 #include "chrome/browser/safe_browsing/database_manager.h"
@@ -19,6 +21,7 @@
 
 namespace extensions {
 
+class BlacklistFetcher;
 class Extension;
 class ExtensionPrefs;
 
@@ -53,14 +56,16 @@ class Blacklist : public content::NotificationObserver,
     DISALLOW_COPY_AND_ASSIGN(ScopedDatabaseManagerForTest);
   };
 
-  // The numeric values here match the values of the respective enum in proto
-  // received from SafeBrowsing server.
+  // The numeric values here match the values of the respective enum in
+  // ClientCRXListInfoResponse proto.
   enum BlacklistState {
     NOT_BLACKLISTED = 0,
     BLACKLISTED_MALWARE = 1,
     BLACKLISTED_SECURITY_VULNERABILITY = 2,
     BLACKLISTED_CWS_POLICY_VIOLATION = 3,
-    BLACKLISTED_POTENTIALLY_UNWANTED = 4
+    BLACKLISTED_POTENTIALLY_UNWANTED = 4,
+    BLACKLISTED_UNKNOWN  // Used when we couldn't connect to server,
+                         // e.g. when offline.
   };
 
   typedef std::map<std::string, BlacklistState> BlacklistStateMap;
@@ -99,6 +104,9 @@ class Blacklist : public content::NotificationObserver,
   void IsBlacklisted(const std::string& extension_id,
                      const IsBlacklistedCallback& callback);
 
+  // Used to mock BlacklistFetcher in unit tests.
+  void SetBlacklistFetcherForTest(BlacklistFetcher* fetcher);

[not at google - send to devlin, 2013/11/18 17:03:49]

could you make this part of the ScopedDatabaseManagerForTest class? Perhaps
rename that TestScope, then make it take 2 arguments, the database manager and
the fetcher?

[Oleg Eterevsky, 2013/11/20 12:58:45]

I am not sure, but I don't think it'll make the code simpler. The
DatabaseManager is a separate global object (g_database_manager), and if I
understand correctly, ScopedDatabaseManager is created to manage the temporary
stub. On the other hand, BlacklistFetcher always belongs to Blacklist and is
deleted when Blacklist object is destroyed in any case, so just replacing it
like this seems much more straightforward to me.

[not at google - send to devlin, 2013/11/20 23:44:48]

Right, good point.

+
   // Adds/removes an observer to the blacklist.
   void AddObserver(Observer* observer);
   void RemoveObserver(Observer* observer);
@@ -120,6 +128,8 @@ class Blacklist : public content::NotificationObserver,
   void RequestExtensionsBlacklistState(const std::set<std::string> ids,
                                        base::Callback<void()> callback);
 
+  void OnBlacklistStateReceived(std::string id, BlacklistState state);
+
   void ReturnBlacklistStateMap(const GetBlacklistedIDsCallback& callback,
                                const std::set<std::string>& blacklisted_ids);
 
@@ -129,6 +139,14 @@ class Blacklist : public content::NotificationObserver,
 
   BlacklistStateMap blacklist_state_cache_;
 
+  scoped_ptr<BlacklistFetcher> fetcher_;

[not at google - send to devlin, 2013/11/18 17:03:49]

state_fetcher? Otherwise it may imply that this is responsible for fetching the
actual whole blacklist.

[Oleg Eterevsky, 2013/11/20 12:58:45]

Done.

+
+  typedef std::list<std::pair<std::vector<std::string>,
+                              base::Callback<void()> > >
+      StateRequestsList;
+
+  StateRequestsList state_requests_;

[not at google - send to devlin, 2013/11/18 17:03:49]

The usage of |state_requests_| is non-trivial so a comment would be nice.

I suppose the same could be said for |blacklist_state_cache_|.

[Oleg Eterevsky, 2013/11/20 12:58:45]

Done.

+
   DISALLOW_COPY_AND_ASSIGN(Blacklist);
 };