Fetch incremental updates. Store new state in V4Store.

components/safe_browsing_db/v4_local_database_manager.cc

Index: components/safe_browsing_db/v4_local_database_manager.cc
diff --git a/components/safe_browsing_db/v4_local_database_manager.cc b/components/safe_browsing_db/v4_local_database_manager.cc
index 30812a01b3cf971d084c877d23d70708e5b844d3..71089dcf83bf252778c8e6a4f67150ea894a631a 100644
--- a/components/safe_browsing_db/v4_local_database_manager.cc
+++ b/components/safe_browsing_db/v4_local_database_manager.cc
@@ -152,27 +152,11 @@ void V4LocalDatabaseManager::StartOnIOThread(
 void V4LocalDatabaseManager::SetupUpdateProtocolManager(
     net::URLRequestContextGetter* request_context_getter,
     const V4ProtocolConfig& config) {
-#if defined(OS_WIN) || defined(OS_LINUX) || defined(OS_MACOSX)
-  // TODO(vakh): Remove this if/endif block when the V4Database is implemented.
-  // Filed as http://crbug.com/608075
-  UpdateListIdentifier update_list_identifier;
-#if defined(OS_WIN)
-  update_list_identifier.platform_type = WINDOWS_PLATFORM;
-#elif defined(OS_LINUX)
-  update_list_identifier.platform_type = LINUX_PLATFORM;
-#else
-  update_list_identifier.platform_type = OSX_PLATFORM;
-#endif
-  update_list_identifier.threat_entry_type = URL;
-  update_list_identifier.threat_type = MALWARE_THREAT;
-  current_list_states_[update_list_identifier] = "";
-#endif
-
   V4UpdateCallback callback = base::Bind(
       &V4LocalDatabaseManager::UpdateRequestCompleted, base::Unretained(this));
 
-  v4_update_protocol_manager_ = V4UpdateProtocolManager::Create(
-      request_context_getter, config, current_list_states_, callback);
+  v4_update_protocol_manager_ =
+      V4UpdateProtocolManager::Create(request_context_getter, config, callback);
 }
 
 void V4LocalDatabaseManager::SetupDatabase() {
@@ -187,16 +171,40 @@ void V4LocalDatabaseManager::SetupDatabase() {
         pool->GetSequenceToken(), base::SequencedWorkerPool::SKIP_ON_SHUTDOWN);
   }
 
-  // TODO(vakh): list_info_map should probably be a hard-coded map.
-  ListInfoMap list_info_map;
+  StoreFileNameMap store_file_name_map;
+  PopulateStoreFileNameMap(&store_file_name_map);

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

Could PopulateStoreFileNameMap() just return a map rather than requiring a
pointer?  And then it wouldn't be PopulateSFNM, but CreateSFNM?

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

Done.

 
   // Do not create the database on the IO thread since this may be an expensive
   // operation. Instead, do that on the task_runner and when the new database
   // has been created, swap it out on the IO thread.
+  DCHECK(!store_file_name_map.empty());
+  DatabaseUpdatedCallback db_updated_callback = base::Bind(
+      &V4LocalDatabaseManager::DatabaseUpdated, base::Unretained(this));
   NewDatabaseReadyCallback db_ready_callback = base::Bind(
       &V4LocalDatabaseManager::DatabaseReady, base::Unretained(this));
-  V4Database::Create(task_runner_, base_path_, list_info_map,
-                     db_ready_callback);
+  V4Database::Create(task_runner_, base_path_, store_file_name_map,
+                     db_updated_callback, db_ready_callback);
+}
+
+void V4LocalDatabaseManager::PopulateStoreFileNameMap(
+    StoreFileNameMap* store_file_name_map) {
+// TODO(vakh): Implement this to populate the map appopriately.
+// Filed as http://crbug.com/608075
+#if defined(OS_WIN)
+  PlatformType platform_type = WINDOWS_PLATFORM;
+#elif defined(OS_LINUX)
+  PlatformType platform_type = LINUX_PLATFORM;
+#else

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

I think there should be no default,
#elif defined(OS_OSX)

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

This code is getting compiled on Android for an unknown reason -- it should not.
I'll look into that later but in the mean time, in order to not break the
compilation, I defaulted to one of the platforms.

If I leave the platform_type empty, the server won't recognize the request and
return an error response which will pollute our UMA metrics so leaving it as-is
for now.

I know it is wrong, but probably less wrong than the alternative. Filed:
http://crbug.com/621647

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

Suggest having an explicit case for OS_OSX, plus an explicit #else case, with a
TODO(vakh) referencing that bug.  Perhaps even a comment about why the
X_PLATFORM you're choosing for the #else case won't pollute your metrics.

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

Done.

+  PlatformType platform_type = OSX_PLATFORM;
+#endif
+  // This is designed to fail compilation for other platforms because
+  // |platform_type| won't be declared.

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

I vote for no comment.  I think anyone debugging why platform_type isn't defined
would see that either their platform will need a new enum.

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

Done.

+  UpdateListIdentifier update_list_identifier(platform_type, URL,
+                                              MALWARE_THREAT);
+  (*store_file_name_map)[update_list_identifier] = "os_url_malware.store";
+
+  update_list_identifier.threat_type = SOCIAL_ENGINEERING_PUBLIC;

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

I am not entirely comfortable with this, because it makes assumptions about
update_list_identifier's implementation which may not hold over time.  Would it
make sense to have:

(*store_file_name_map)[UpdateListIdentifier(platform_type, URL,
SOCIAL_ENGINEERING_PUBLIC)] = "os_url_soceng.store"?

or malware_identifier and soceng_identifier separately.

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

Done.

+  (*store_file_name_map)[update_list_identifier] = "os_url_soceng.store";
 }
 
 void V4LocalDatabaseManager::DatabaseReady(
@@ -208,6 +216,9 @@ void V4LocalDatabaseManager::DatabaseReady(
   if (enabled_) {
     v4_database_ = std::move(v4_database);
 
+    v4_update_protocol_manager_->SetStoreStateMap(
+        v4_database_->store_state_map());
+
     // The database is in place. Start fetching updates now.
     v4_update_protocol_manager_->ScheduleNextUpdate();
   } else {
@@ -236,8 +247,10 @@ void V4LocalDatabaseManager::StopOnIOThread(bool shutdown) {
 void V4LocalDatabaseManager::UpdateRequestCompleted(
     const std::vector<ListUpdateResponse>& responses) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
+  v4_database_->ApplyUpdate(responses);

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

WRT my comment elsewhere with many store tasks versus a single db task, this is
the place I think would be cleaner if it was:
v4_database_->ApplyUpdate(responses, base::Bind(&V4LDM::DatabaseUpdated, this));
Then the reader would know looking only at this function where they need to look
next.

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

Still not sure about updating all stores at once, please see my comment in
v4_database.

Since the method to be called back is the same every time (without any
arguments), isn't it better to pass it at construction time?

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

It's more efficient, probably.  But a call like this:

  object->GoDoTheThing(parameter, parameter, base::Bind(&MyCallback, this));

makes it reasonably obvious what's happening.  As compared to:

  void SetUpt() {
    object->SetTheCallback(base::Bind(&MyCallback, this));
  }
  // hundreds of lines of code.
    object->GoDoTheThing(parameter, parameter);

In the latter case, it's not immediately clear that there even is a
flow-of-control thing, it reads like a synchronous operation.

If you do want to initialize up front, then make sure you review your other
callback use to see if they should all be regularized that way :-).

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

The other callbacks in this class also work the same way i.e. setup callback at
init and then keep getting called back.

[Scott Hess - ex-Googler, 2016/06/24 23:01:19]

AFAICT there's only that one callback which is stored as a member variable.  I
may be misreading something.

[vakh (use Gerrit instead), 2016/06/27 19:38:26]

Your reading is right. What I wrote is different than what I meant, and is
incorrect.
The other callback (NewDbReadyCallback) is only called back once not repeatedly.

[Scott Hess - ex-Googler, 2016/06/27 22:19:17]

OK, I guess I'll leave this to you.  I find the stored callback to be a little
undesirable in this case, so it might be worthwhile to have a comment on this
call saying "This is async, when complete DatabaseUpdated() will be called". 
Alternately, ApplyUpdate() could be renamed to imply that it's not synchronous,
but I can't think of a sensible name.

Hmm.  Another option would be to have it be ApplyUpdate(responses, callback),
and have the ApplyUpdate() implementation DCHECK() that the member callback
storage is null (no outstanding ApplyUpdate in process), then store the callback
for later, then clear the callback when sent.  That puts the pattern I like in
place, while still allowing multiple implementation options (loop in calling
thread posting multiple tasks, loop in background, or loop in background pumping
tasks).

[vakh (use Gerrit instead), 2016/06/27 23:58:52]

Done.

+}
 
-  // TODO(vakh): Updates downloaded. Store them on disk and record new state.
+void V4LocalDatabaseManager::DatabaseUpdated() {
   v4_update_protocol_manager_->ScheduleNextUpdate();
 }