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..2b9632d76ad4bb68e5ba843949b170346419a5ba 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 BlacklistStateFetcher;
 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,

[mattm, 2013/12/03 02:55:45]

It's pretty confusing that 0 is UNKNOWN in the proto but NOT_BLACKLISTED here,
and further there is a different value of UNKNOWN below.

It would be better to just use the enum from the proto everywhere rather than
have a duplicate definition here. (You could just put the extra
"unknown"/uninitialized value in the proto with a comment that the server should
never set that value or something).

[Oleg Eterevsky, 2013/12/04 10:45:10]

I understand your concern. The reason for different unknown's is as follows: the
proto was first added on server side. 'unknown' there basically mean that the
extension in question is not in the blacklist. Then in Chrome we had to use some
state when we failed to connect to server, and I added another UNKNOWN.

How about I write to Nav, who wrote the server-side, and ask him to change proto
there, and once it's done, I'll change it here?

Also, I do not like the idea of directly referring everywhere to an enum from
some proto, since it should be visible and used only by the protocol. Still I
agree that it would be nice to have this enum and proto synced.

WDYT?

[mattm, 2013/12/10 11:09:40]

sounds good

[Oleg Eterevsky, 2013/12/10 17:10:44]

Done.

     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 BlacklistStateFetcher in unit tests.
+  void SetBlacklistStateFetcherForTest(BlacklistStateFetcher* fetcher);
+
   // Adds/removes an observer to the blacklist.
   void AddObserver(Observer* observer);
   void RemoveObserver(Observer* observer);
@@ -117,8 +125,10 @@ class Blacklist : public content::NotificationObserver,
   void GetBlacklistStateForIDs(const GetBlacklistedIDsCallback& callback,
                                const std::set<std::string>& blacklisted_ids);
 
-  void RequestExtensionsBlacklistState(const std::set<std::string> ids,
-                                       base::Callback<void()> callback);
+  void RequestExtensionsBlacklistState(const std::set<std::string>& ids,
+                                       const base::Callback<void()>& callback);
+
+  void OnBlacklistStateReceived(const std::string& id, BlacklistState state);
 
   void ReturnBlacklistStateMap(const GetBlacklistedIDsCallback& callback,
                                const std::set<std::string>& blacklisted_ids);
@@ -127,8 +137,21 @@ class Blacklist : public content::NotificationObserver,
 
   content::NotificationRegistrar registrar_;
 
+  // The cached BlacklistState's, received from BlacklistStateFetcher.
   BlacklistStateMap blacklist_state_cache_;
 
+  scoped_ptr<BlacklistStateFetcher> state_fetcher_;
+
+  typedef std::list<std::pair<std::vector<std::string>,
+                              base::Callback<void()> > >
+      StateRequestsList;
+
+  // The list of ongoing requests for blacklist states that couldn't be
+  // served directly from the cache. A new request is created in
+  // GetBlacklistedIDs and deleted when the callback is called from
+  // OnBlacklistStateReceived.
+  StateRequestsList state_requests_;
+
   DISALLOW_COPY_AND_ASSIGN(Blacklist);
 };