Parse the hash prefix map from the V4Store on disk.

components/safe_browsing_db/v4_store.cc

Index: components/safe_browsing_db/v4_store.cc
diff --git a/components/safe_browsing_db/v4_store.cc b/components/safe_browsing_db/v4_store.cc
index 7f375d69a3df15203567ee1213406cd922776105..e6195767d0161575bb9f81d616a3f581bc318b3c 100644
--- a/components/safe_browsing_db/v4_store.cc
+++ b/components/safe_browsing_db/v4_store.cc
@@ -40,6 +40,12 @@ void RecordApplyUpdateResult(ApplyUpdateResult result) {
                             APPLY_UPDATE_RESULT_MAX);
 }
 
+void RecordApplyUpdateResultWhenReadingFromDisk(ApplyUpdateResult result) {
+  UMA_HISTOGRAM_ENUMERATION(
+      "SafeBrowsing.V4ApplyUpdateResultWhenReadingFromDisk", result,

[Nathan Parker, 2016/07/14 22:36:12]

Is this really an update?  It's more just a load.

[vakh (use Gerrit instead), 2016/07/15 00:07:30]

Yes, it depends on the naming strategy in the reader's head.
I chose this name because it tells me that the result to expect is an enum
ApplyUpdateResult for the operation WhenReadingFromDisk.

The enum should probably be named differently.

What happens when reading from disk is a subset of what happens when an update
is applied so the enum is named after the super set operation ApplyUpdate.

+      APPLY_UPDATE_RESULT_MAX);
+}
+
 // Returns the name of the temporary file used to buffer data for
 // |filename|.  Exported for unit tests.
 const base::FilePath TemporaryFileForFilename(const base::FilePath& filename) {
@@ -359,10 +365,17 @@ StoreReadResult V4Store::ReadFromDisk() {
     return HASH_PREFIX_INFO_MISSING_FAILURE;
   }
 
-  ListUpdateResponse list_update_response = file_format.list_update_response();
-  state_ = list_update_response.new_client_state();
-  // TODO(vakh): Do more with what's read from the disk.
+  const ListUpdateResponse& response = file_format.list_update_response();
+  HashPrefixMap read_from_disk_map;
+  ApplyUpdateResult apply_update_result = UpdateHashPrefixMapFromAdditions(

[Nathan Parker, 2016/07/14 22:36:12]

While I think this will work, it does a lot of extra work to look in the current
hash_prefix_map_ for every single hash that's getting loaded.  It would be good
to short-circuit that since we know hash_prefix_map_ is empty and there are no
removals.  This will save a chunk of CPU at startup (where it's most at a
premium).

You might want a separate function here that would just copy the proto's
string(s) straight into the map, assuming they aren't compressed.  If they are
compressed, you could do a streaming-decompress into the string owned by
hash_prefix_map_.

Or, maybe within UpdateHashPrefixMapFromAdditions() you could have
short-circuits to do this: If removals is empty and some hash_prefix_map_
entries are empty, you can just move the corresponding update result string
directly into the hash_prefix_map_. I think that may be more complicated than a
separate function. 


OH and I found some useful proto functions that let you do zero-copy add/remove
of big strings:
  void set_allocated_foo(string* value);
  string* release_foo();

[vakh (use Gerrit instead), 2016/07/14 23:51:57]

Umm, that doesn't happen. I may have misunderstood you so please correct me if
that's the case.
The UpdateHashPrefixMapFromAdditions doesn't look at the hash_prefix_map_ at
all. It is a static method.

All it does is take an instance of additions() and generate a hash_prefix_map
out of it *without merging*.
(Merge is done for partial updates by MergeUpdate() method.)
Could you be confusing UpdateHashPrefixMapFromAdditions with ApplyUpdate() or
MergeUpdate() ?
Thanks. I'll investigate their use and make changes in a separate CL.

[Nathan Parker, 2016/07/15 00:35:31]

Blarg. Indeed, that's exactly how I was reading it. Sorry for the confusion,
ignore that comment.

+      response.additions(), &read_from_disk_map);
+  RecordApplyUpdateResultWhenReadingFromDisk(apply_update_result);
+  if (apply_update_result != APPLY_UPDATE_SUCCESS) {
+    return HASH_PREFIX_MAP_GENERATION_FAILURE;
+  }
 
+  hash_prefix_map_ = read_from_disk_map;

[Nathan Parker, 2016/07/14 22:36:12]

This does a copy of the map.

[vakh (use Gerrit instead), 2016/07/14 23:51:57]

Done.

+  state_ = response.new_client_state();
   return READ_SUCCESS;
 }