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

chrome/browser/safe_browsing/safe_browsing_database.h

Index: chrome/browser/safe_browsing/safe_browsing_database.h
diff --git a/chrome/browser/safe_browsing/safe_browsing_database.h b/chrome/browser/safe_browsing/safe_browsing_database.h
index 07346c9b77015c73e872df7be2d6a41ad8ced76f..4664a8589eba9f0efb660dc3cc9935a61ba5623f 100644
--- a/chrome/browser/safe_browsing/safe_browsing_database.h
+++ b/chrome/browser/safe_browsing/safe_browsing_database.h
@@ -268,11 +268,11 @@ class SafeBrowsingDatabaseNew : public SafeBrowsingDatabase {
   bool ContainsWhitelistedHashes(const SBWhitelist& whitelist,
                                  const std::vector<SBFullHash>& hashes);
 
-  // Return the browse_store_, download_store_, download_whitelist_store or
-  // csd_whitelist_store_ based on list_id.
+  // Return the browse_store_, download_store_, download_whitelist_store,
+  // or csd_whitelist_store_ based on list_id.

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

Egregious formatting change is egregious.

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

Done.

   SafeBrowsingStore* GetStore(int list_id);
 
-    // Deletes the files on disk.
+  // Deletes the files on disk.

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

Though this one isn't.

   bool Delete();
 
   // Load the prefix set off disk, if available.
@@ -303,9 +303,11 @@ class SafeBrowsingDatabaseNew : public SafeBrowsingDatabase {
 
   // Helpers for InsertChunks().
   void InsertAdd(int chunk, SBPrefix host, const SBEntry* entry, int list_id);
-  void InsertAddChunks(int list_id, const SBChunkList& chunks);
+  void InsertAddChunks(safe_browsing_util::ListType list_id,
+                       const SBChunkList& chunks);
   void InsertSub(int chunk, SBPrefix host, const SBEntry* entry, int list_id);
-  void InsertSubChunks(int list_id, const SBChunkList& chunks);
+  void InsertSubChunks(safe_browsing_util::ListType list_id,
+                       const SBChunkList& chunks);
 
   void UpdateDownloadStore();
   void UpdateBrowseStore();
@@ -313,15 +315,6 @@ class SafeBrowsingDatabaseNew : public SafeBrowsingDatabase {
                             SafeBrowsingStore* store,
                             SBWhitelist* whitelist);
 
-  // Helper function to compare addprefixes in download_store_ with |prefixes|.
-  // The |list_bit| indicates which list (download url or download hash)
-  // to compare.
-  // Returns true if there is a match, |*prefix_hits| will contain the actual
-  // matching prefixes.
-  bool MatchDownloadAddPrefixes(int list_bit,
-                                const std::vector<SBPrefix>& prefixes,
-                                std::vector<SBPrefix>* prefix_hits);
-
   // Used to verify that various calls are made from the thread the
   // object was created on.
   MessageLoop* creation_loop_;
@@ -375,9 +368,8 @@ class SafeBrowsingDatabaseNew : public SafeBrowsingDatabase {
   // the next call to |UpdateStarted()|.
   bool corruption_detected_;
 
-  // Set to true if any chunks are added or deleted during an update.
-  // Used to optimize away database update.
-  bool change_detected_;
+  // Contains the IDs of stores which changed during an update.
+  std::set<safe_browsing_util::ListType> changes_detected_;

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

As with the earlier CL we were discussing, I still don't agree with this change.
 The fact that the database is split across multiple stores is not something the
caller should be aware of, and I'd prefer not to start making it aware of such
things unless it's really necessary.

From what I recall, you wanted that information so that someone could skip
rebuilding a data structure.  Note that all of the other data doesn't have an
external data structure which gets out of sync, because those data structures
are internal to the database structure.

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

The "external data structure" for extensions is the set of extensions for that
profile (ExtensionService::extensions()) so it's not really a question of
maintaining that data structure in here, if that's what you mean.

I'd just like to avoid having to loop over all installed extensions every time
some other list changes. Admittedly I can't predict how often the list is going
to be updated compared to others, but instinctively it's going to be much less
frequently.

So - if it's exposing the list type - another option would be to add a very
specific event like OnExtensionsBlacklistUpdated, and not need to expose the
actual ListType enum values to callers.

Either way, sounds like I should pull this change out of this patch. It's only
here because I was trying to make my next patch pure cargo cult as requested.

[Scott Hess - ex-Googler, 2013/01/17 19:24:42]

The update frequency is once every 45 minutes, and something like half the
updates are empty.  Non-empty updates already involve a somewhat substantial
operation to update the database, which is likely to very much dominate the
costs.  Furthermore, the iterate-over-extensions code already has to be correct
and should be reasonably fast (like if the state of matching didn't change,
there should be no work for that extension).  So mostly I don't want to add
additional API knobs to preemptively optimize against something which shouldn't
anyhow be a notable fraction of performance costs.

 
   // Used to check if a prefix was in the database.
   FilePath prefix_set_filename_;