Fetch incremental updates. Store new state in V4Store.

components/safe_browsing_db/v4_database.h

Index: components/safe_browsing_db/v4_database.h
diff --git a/components/safe_browsing_db/v4_database.h b/components/safe_browsing_db/v4_database.h
index 8e634492e6537955f1b31f736bec2fddfe6a74e7..8767a1303a26f391427cb8f241e7507724888749 100644
--- a/components/safe_browsing_db/v4_database.h
+++ b/components/safe_browsing_db/v4_database.h
@@ -5,6 +5,7 @@
 #ifndef COMPONENTS_SAFE_BROWSING_DB_V4_DATABASE_H_
 #define COMPONENTS_SAFE_BROWSING_DB_V4_DATABASE_H_
 
+#include "base/callback.h"
 #include "base/files/file_path.h"
 #include "base/memory/ref_counted.h"
 #include "base/sequenced_task_runner.h"
@@ -19,15 +20,10 @@ class V4Database;
 typedef base::Callback<void(std::unique_ptr<V4Database>)>
     NewDatabaseReadyCallback;
 
-// This defines a hash_map that is used to create the backing files for the
-// stores that contain the hash-prefixes. The map key identifies the list that
-// we're interested in and the value represents the ASCII file-name that'll be
-// created on-disk to store the hash-prefixes for that list. This file is
-// created inside the user's profile directory.
-// For instance, the UpdateListIdentifier could be for URL expressions for UwS
-// on Windows platform, and the corresponding file on disk could be named:
-// "uws_win_url.store"
-typedef base::hash_map<UpdateListIdentifier, std::string> ListInfoMap;
+// This callback is Run once the database has finished processing the update

[Scott Hess - ex-Googler, 2016/06/17 22:53:42]

I don't think Run is a proper noun in this context.

[vakh (use Gerrit instead), 2016/06/20 22:28:42]

Changed to scheduled.

[Scott Hess - ex-Googler, 2016/06/21 21:03:44]

:-).  Should have been more clear, I meant that "Run" is written like a proper
noun, like Mexico or Larry, but here it should be "run".  Not that it was not
_the_ proper noun to put there!  Scheduled is fine, too.

[vakh (use Gerrit instead), 2016/06/21 23:19:35]

I had a hunch that's what you meant but wasn't sure. "scheduled" is also more
accurate :)

+// requests for all stores and is ready to process the next set of update
+// requests.
+typedef base::Callback<void()> DatabaseUpdatedCallback;
 
 // This hash_map maps the UpdateListIdentifiers to their corresponding in-memory
 // stores, which contain the hash prefixes for that UpdateListIdentifier as well
@@ -42,7 +38,7 @@ class V4DatabaseFactory {
   virtual V4Database* CreateV4Database(
       const scoped_refptr<base::SequencedTaskRunner>& db_task_runner,
       const base::FilePath& base_dir_path,
-      const ListInfoMap& list_info_map) = 0;
+      const StoreFileNameMap& store_file_name_map) = 0;
 };
 
 // The on-disk databases are shared among all profiles, as it doesn't contain
@@ -55,12 +51,19 @@ class V4DatabaseFactory {
 class V4Database {
  public:
   // Factory method to create a V4Database. It creates the database on the
-  // provided |db_task_runner|. When the database creation is complete, it calls
-  // the NewDatabaseReadyCallback on the same thread as it was called.
-  static void Create(
+  // provided |db_task_runner| containing stores for which the filename has
+  // been specified in \store_file_name_map\. The created V4Database runs

[Scott Hess - ex-Googler, 2016/06/17 22:53:42]

Why the backslashes for this case?

What does it mean for the filename to _not_ be specified in store_file_name_map?
 Or are they just not in the map?  If StoreFileNameMap is the thing describing
the filenames for stores, maybe just "containing stores in
|store_file_name_map|"?

[vakh (use Gerrit instead), 2016/06/20 22:28:42]

Done.

+  // |db_updated_callback| after processing updates passed to the ApplyUpdate
+  // method. When the database creation is complete, it runs the

[Scott Hess - ex-Googler, 2016/06/17 22:53:43]

The ApplyUpdate snippet feels pretty contrived, like maybe you need to split
this into "The part about the setup process" and "The part about the things that
are setup".

[vakh (use Gerrit instead), 2016/06/20 22:28:42]

Removed the ApplyUpdate part of the comment since that is described by the
comment about ApplyUpdate itself.

+  // NewDatabaseReadyCallback on the same thread as it was called. If the
+  // |base_path| argument isn't an absolute path or if the
+  // |store_file_name_map| is empty, it returns false right away and doesn't
+  // attempt to create the database at all.

[Scott Hess - ex-Googler, 2016/06/17 22:53:42]

This last bit seems like an API mis-use, rather than something to return false
for.  Empty store_file_name_map is unexpected, so DCHECK makes sense, but it
seems like things should work just fine if that case happens (I mean, it won't
_work_ work, but it can just do nothing successfully, I wouldn't optimize for
that case in any way).

Actually, I guess I kind of feel the same about non-absolute base_path.  It
would be reasonable to DCHECK, but have the code in general drop it on the floor
and spin up a noop instance.

[vakh (use Gerrit instead), 2016/06/20 22:28:42]

Done.

+  static bool Create(
       const scoped_refptr<base::SequencedTaskRunner>& db_task_runner,
       const base::FilePath& base_path,
-      const ListInfoMap& list_info_map,
+      const StoreFileNameMap& store_file_name_map,
+      DatabaseUpdatedCallback db_updated_callback,
       NewDatabaseReadyCallback callback);
 
   // Destroys the provided v4_database on its task_runner since this may be a
@@ -69,12 +72,19 @@ class V4Database {
 
   virtual ~V4Database();
 
+  // Updates the stores with the response received from the SafeBrowsing service
+  // and calls the db_updated_callback_ when done.
+  void ApplyUpdate(const std::vector<ListUpdateResponse>& response);
+
+  const StoreStateMap* store_state_map() { return &store_state_map_; }

[Scott Hess - ex-Googler, 2016/06/17 22:53:42]

This feels like it shouldn't be exposed to public:.

[vakh (use Gerrit instead), 2016/06/21 00:29:36]

Done.

+
   // Deletes the current database and creates a new one.
   virtual bool ResetDatabase();
 
  protected:
   V4Database(const scoped_refptr<base::SequencedTaskRunner>& db_task_runner,
-             std::unique_ptr<StoreMap> store_map);
+             std::unique_ptr<StoreMap> store_map,
+             DatabaseUpdatedCallback db_updated_callback);
 
  private:
   friend class SafeBrowsingV4DatabaseTest;
@@ -82,6 +92,12 @@ class V4Database {
                            TestSetupDatabaseWithFakeStores);
   FRIEND_TEST_ALL_PREFIXES(SafeBrowsingV4DatabaseTest,
                            TestSetupDatabaseWithFakeStoresFailsReset);
+  FRIEND_TEST_ALL_PREFIXES(SafeBrowsingV4DatabaseTest,
+                           TestApplyUpdateWithNewStates);
+  FRIEND_TEST_ALL_PREFIXES(SafeBrowsingV4DatabaseTest,
+                           TestApplyUpdateWithNoNewState);
+  FRIEND_TEST_ALL_PREFIXES(SafeBrowsingV4DatabaseTest,
+                           TestApplyUpdateWithEmptyUpdate);
 
   // Makes the passed |factory| the factory used to instantiate a V4Store. Only
   // for tests.
@@ -94,18 +110,38 @@ class V4Database {
   static void CreateOnTaskRunner(
       const scoped_refptr<base::SequencedTaskRunner>& db_task_runner,
       const base::FilePath& base_path,
-      const ListInfoMap& list_info_map,
+      const StoreFileNameMap& store_file_name_map,
+      DatabaseUpdatedCallback db_updated_callback,
       const scoped_refptr<base::SingleThreadTaskRunner>& callback_task_runner,
       NewDatabaseReadyCallback callback);
 
+  // Callback called when a new store has been created and is ready to be used.
+  // This method updates the store_map_ to point to the new store, which causes
+  // the old store to get deleted.
+  void UpdatedStoreReady(UpdateListIdentifier identifier,
+                         std::unique_ptr<V4Store> store);
+
+  // Updates the store state map after reading the stores from disk on startup.

[Scott Hess - ex-Googler, 2016/06/17 22:53:42]

This is called on startup?

[vakh (use Gerrit instead), 2016/06/20 22:28:42]

Updated to say: "when the database is being created"

+  void UpdateStoreStateMap();
+
   const scoped_refptr<base::SequencedTaskRunner> db_task_runner_;
 
   // Map of UpdateListIdentifier to the V4Store.
   const std::unique_ptr<StoreMap> store_map_;
 
+  // Map of UpdateListIdentifier to the state of the V4Store it represents.
+  StoreStateMap store_state_map_;
+
+  DatabaseUpdatedCallback db_updated_callback_;
+
   // The factory that controls the creation of V4Store objects.
   static V4StoreFactory* factory_;
 
+  // The number of stores for which the update request is pending. When this
+  // goes down to 0, that indicates that the database has updated all the stores
+  // that needed updating and is ready for the next update.
+  unsigned pending_store_updates_;

[Scott Hess - ex-Googler, 2016/06/17 22:53:42]

I think countable things are recommended to be int or size_t.

That said, I wonder about tracking "count of outstanding stores to update"
versus just tracking the actual outstanding stores to update.

[vakh (use Gerrit instead), 2016/06/20 22:28:42]

Changed to int.
I thought of that at first, but couldn't figure out any advantage of doing that,
over jut keeping a counter. The only thing that I could come up with was to help
debug or track how long each store update took or if there are any that are not
getting updated at all for some reason. Finally decided to keep it simple
because those cases aren't really worthy enough.

[Scott Hess - ex-Googler, 2016/06/21 21:03:44]

OK.  Mostly just worried about whether counting the same thing in multiple
places can get out of sync.

[vakh (use Gerrit instead), 2016/06/21 23:19:35]

Yes, I agree but I think in this particular case, we are using thread
synchronization to ensure that all the increments happen before all the
decrements so I it's not risky.

+
   DISALLOW_COPY_AND_ASSIGN(V4Database);
 };