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

chrome/browser/safe_browsing/safe_browsing_database.h

 // Copyright (c) 2011 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_SAFE_BROWSING_SAFE_BROWSING_DATABASE_H_
 #define CHROME_BROWSER_SAFE_BROWSING_SAFE_BROWSING_DATABASE_H_
 
 #include <set>
 #include <vector>
 
@@ skipping 250 matching lines @@
   // A SafeBrowsing whitelist contains a list of whitelisted full-hashes (stored
   // in a sorted vector) as well as a boolean flag indicating whether all
   // lookups in the whitelist should be considered matches for safety.
   typedef std::pair<std::vector<SBFullHash>, bool> SBWhitelist;
 
   // Returns true if the whitelist is disabled or if any of the given hashes
   // matches the whitelist.
   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.
   void LoadPrefixSet();
 
   // Writes the current prefix set to disk.
   void WritePrefixSet();
 
   // Loads the given full-length hashes to the given whitelist.  If the number
   // of hashes is too large or if the kill switch URL is on the whitelist
@@ skipping 10 matching lines @@
   // |corruption_detected_|, |HandleCorruptDatabase()| posts
   // |OnHandleCorruptDatabase()| to the current thread, to be run
   // after the current task completes.
   // TODO(shess): Wire things up to entirely abort the update
   // transaction when this happens.
   void HandleCorruptDatabase();
   void OnHandleCorruptDatabase();
 
   // 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();
   void UpdateWhitelistStore(const FilePath& store_filename,
                             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_;
 
   // Lock for protecting access to variables that may be used on the
   // IO thread.  This includes |prefix_set_|, |full_browse_hashes_|,
   // |pending_browse_hashes_|, |prefix_miss_cache_|, |csd_whitelist_|,
   // and |csd_whitelist_all_urls_|.
   base::Lock lookup_lock_;
 
@@ skipping 33 matching lines @@
   std::set<SBPrefix> prefix_miss_cache_;
 
   // Used to schedule resetting the database because of corruption.
   base::WeakPtrFactory<SafeBrowsingDatabaseNew> reset_factory_;
 
   // Set if corruption is detected during the course of an update.
   // Causes the update functions to fail with no side effects, until
   // 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_;
   scoped_ptr<safe_browsing::PrefixSet> prefix_set_;
 };
 
 #endif  // CHROME_BROWSER_SAFE_BROWSING_SAFE_BROWSING_DATABASE_H_