Fetch extension blacklist states from SafeBrowsing server

chrome/browser/extensions/blacklist.h

 // 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.
 
 #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"
 #include "content/public/browser/notification_observer.h"
 #include "content/public/browser/notification_registrar.h"
 
 namespace extensions {
 
+class BlacklistFetcher;
 class Extension;
 class ExtensionPrefs;
 
 // The blacklist of extensions backed by safe browsing.
 class Blacklist : public content::NotificationObserver,
                   public base::SupportsWeakPtr<Blacklist> {
  public:
   class Observer {
    public:
     // Observes |blacklist| on construction and unobserves on destruction.
@@ skipping 14 matching lines @@
         scoped_refptr<SafeBrowsingDatabaseManager> database_manager);
 
     ~ScopedDatabaseManagerForTest();
 
    private:
     scoped_refptr<SafeBrowsingDatabaseManager> original_;
 
     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;
 
   typedef base::Callback<void(const BlacklistStateMap&)>
       GetBlacklistedIDsCallback;
 
   typedef base::Callback<void(const std::set<std::string>&)>
       GetMalwareIDsCallback;
 
@@ skipping 18 matching lines @@
   // marked in the blacklist as BLACKLISTED_MALWARE and asynchronously pass
   // to |callback|. Basically, will call GetBlacklistedIDs and filter its
   // results.
   void GetMalwareIDs(const std::set<std::string>& ids,
                      const GetMalwareIDsCallback& callback);
 
   // More convenient form of GetBlacklistedIDs for checking a single extension.
   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);
 
  private:
   // Use via ScopedDatabaseManagerForTest.
   static void SetDatabaseManager(
       scoped_refptr<SafeBrowsingDatabaseManager> database_manager);
   static scoped_refptr<SafeBrowsingDatabaseManager> GetDatabaseManager();
 
   // content::NotificationObserver
   virtual void Observe(int type,
                        const content::NotificationSource& source,
                        const content::NotificationDetails& details) OVERRIDE;
 
   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 OnBlacklistStateReceived(std::string id, BlacklistState state);
+
   void ReturnBlacklistStateMap(const GetBlacklistedIDsCallback& callback,
                                const std::set<std::string>& blacklisted_ids);
 
   ObserverList<Observer> observers_;
 
   content::NotificationRegistrar registrar_;
 
   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);
 };
 
 }  // namespace extensions
 
 #endif  // CHROME_BROWSER_EXTENSIONS_BLACKLIST_H_