Small modifications to safebrowsing code to make it simpler to add the extension

chrome/browser/safe_browsing/database_manager.h

Index: chrome/browser/safe_browsing/database_manager.h
diff --git a/chrome/browser/safe_browsing/database_manager.h b/chrome/browser/safe_browsing/database_manager.h
index 9bfefa261440ab3ea11e9cd04dd2892b0e667748..8ea0874dcbaa3a11072f4725ea0aa7ef6e005ed9 100644
--- a/chrome/browser/safe_browsing/database_manager.h
+++ b/chrome/browser/safe_browsing/database_manager.h
@@ -9,6 +9,7 @@
 #define CHROME_BROWSER_SAFE_BROWSING_DATABASE_MANAGER_H_
 
 #include <deque>
+#include <map>
 #include <set>
 #include <string>
 #include <vector>
@@ -47,20 +48,29 @@ class SafeBrowsingDatabaseManager
  public:
   class Client;
 
-  // Bundle of SafeBrowsing state for one URL or hash prefix check.
+  // Bundle of SafeBrowsing state while performing a URL or hash prefix check.
   struct SafeBrowsingCheck {
-    SafeBrowsingCheck();
+    // |check_type| should correspond to the type of item that is being checked,
+    // either a URL or a binary hash/URL. This is largely for accounting.
+    //
+    // NOTE: since we actually just query strings and are returned a threat
+    // type, this cannot be used to predict the response type (e.g. we can
+    // query the MALWARE list but may be given results for phishing).
+    explicit SafeBrowsingCheck(safe_browsing_util::ListType check_type);

[Scott Hess - ex-Googler, 2013/01/11 23:44:05]

Have you talked to the server people about this?  AFAICT, this only matters if
there is a full-hash collision, which should be very unlikely.  If it's deemed
likely enough to be worth protecting against, then we need to allow per-list
queries.

[not at google - send to devlin, 2013/01/14 23:00:55]

As discussed (err, I think) this is just for knowing which of Client's callback
methods to run. If we just combined all the callbacks into a single ohe, this
would be moot (though you'd go back to needing that "is_download" thing for
accounting in ProtocolManager). I'm going to leave it the way it is here but if
you would prefer me to change it (e.g. combine the callbacks, make it a callback
itself, etc) then I'm happy to change it. In the meantime I re-wrote the
comment.

     ~SafeBrowsingCheck();
 
-    // Either |urls| or |prefix| is used to lookup database.
+    // Either |urls| or |full_hashes| is used to lookup database.
     std::vector<GURL> urls;
-    scoped_ptr<SBFullHash> full_hash;
+    std::vector<SBFullHash> full_hashes;
+
+    // Results from either |urls| or |full_hashes|.
+    std::map<GURL, SBThreatType> url_threats;
+    std::map<SBFullHash, SBThreatType> full_hash_threats;

[Scott Hess - ex-Googler, 2013/01/11 23:44:05]

Having these maps seems over-engineered.  I think you could either:
 - use parallel vectors<SBThreadType> pre-filled with SB_THREAT_TYPE_SAFE.
 - make the vectors above be maps, with default value SB_THREAD_TYPE_SAFE.
 - make the above vectors of pairs, like vector<pair<GURL,SBThreatType>>.

Of course, the latter is exactly like the map version, just more efficient for
storage, and less general for searching.  But it's not clear if searching is
very important for these structures, and they're short.

[not at google - send to devlin, 2013/01/14 23:00:55]

I like the vector<pair<>> one, but it's much simpler to implement this stuff
with parallel vectors so I'll just do it that way.

 
     Client* client;
     bool need_get_hash;
     base::TimeTicks start;  // When check was sent to SB service.
-    SBThreatType threat_type;
-    bool is_download;  // If this check for download url or hash.
+    safe_browsing_util::ListType check_type;  // See comment in constructor.
     std::vector<SBPrefix> prefix_hits;
     std::vector<SBFullHashResult> full_hits;
 
@@ -188,6 +198,7 @@ class SafeBrowsingDatabaseManager
 
   // Clients that we've queued up for checking later once the database is ready.
   struct QueuedCheck {
+    safe_browsing_util::ListType check_type;
     Client* client;
     GURL url;
     base::TimeTicks start;  // When check was queued.
@@ -295,12 +306,12 @@ class SafeBrowsingDatabaseManager
   void SafeBrowsingCheckDone(SafeBrowsingCheck* check);
 
   // Helper function to set |check| with default values and start a safe
-  // browsing check with timeout of |timeout_ms|. |task| will be called upon
+  // browsing check with timeout of |timeout|. |task| will be called on
   // success, otherwise TimeoutCallback will be called.
-  void StartDownloadCheck(SafeBrowsingCheck* check,
-                          Client* client,
-                          const base::Closure& task,
-                          int64 timeout_ms);
+  void StartSafeBrowsingCheck(SafeBrowsingCheck* check,
+                              Client* client,
+                              const base::Closure& task,
+                              const base::TimeDelta& timeout);
 
   // SafeBrowsingProtocolManageDelegate override
   virtual void ResetDatabase() OVERRIDE;
@@ -363,10 +374,13 @@ class SafeBrowsingDatabaseManager
 
   // When download url check takes this long, client's callback will be called
   // without waiting for the result.
-  int64 download_urlcheck_timeout_ms_;
+  base::TimeDelta download_url_check_timeout_;
+
+  // Similar to |download_urlcheck_timeout_| but for download hash checks.
+  base::TimeDelta download_hash_check_timeout_;
 
-  // Similar to |download_urlcheck_timeout_ms_|, but for download hash checks.
-  int64 download_hashcheck_timeout_ms_;
+  // Similar to |download_urlcheck_timeout_| but for extension ID checks.
+  base::TimeDelta extension_id_check_timeout_;
 
   DISALLOW_COPY_AND_ASSIGN(SafeBrowsingDatabaseManager);
 };