SafeBrowising: Read and write V4Store from/to disk

components/safe_browsing_db/v4_store.h

Index: components/safe_browsing_db/v4_store.h
diff --git a/components/safe_browsing_db/v4_store.h b/components/safe_browsing_db/v4_store.h
index 2d83f393c04a900828f79474ebc398db1a8b1790..0060e8a00a8f184acf6bbedd23aac411d67cdd45 100644
--- a/components/safe_browsing_db/v4_store.h
+++ b/components/safe_browsing_db/v4_store.h
@@ -18,6 +18,67 @@ class V4Store;
 typedef base::Callback<void(std::unique_ptr<V4Store>)>
     UpdatedStoreReadyCallback;
 
+// Enumerate different failure events while parsing the file read from disk for
+// histogramming purposes.  DO NOT CHANGE THE ORDERING OF THESE VALUES.
+enum StoreReadResult {
+  // Reserved for errors in parsing this enum.
+  UNEXPECTED_READ_FAILURE = 0,
+
+  // No errors.
+  READ_SUCCESS = 1,

[Scott Hess - ex-Googler, 2016/06/22 22:58:39]

There are many failures, but only one success - suggest putting success as 0. 
Same for writes.

[Here and histograms.xml.  Or ignore my OCD.]

[vakh (use Gerrit instead), 2016/06/23 23:09:32]

It's a valid point. Done.

+
+  // The contents of the file could not be read.
+  FILE_UNREADABLE_FAILURE = 2,
+
+  // The file was found to be empty.
+  FILE_EMPTY_FAILURE = 3,
+
+  // The contents of the file could not be interpreted as a valid
+  // V4StoreFileFormat proto.
+  PROTO_PARSING_FAILURE = 4,
+
+  // The magic number didn't match. We're most likely trying to read a file
+  // that doesn't contain hash-prefixes.
+  UNEXPECTED_MAGIC_NUMBER_FAILURE = 5,
+
+  // The version of the file is different from expected and Chromium doesn't
+  // know how to interpret this version of the file.
+  FILE_VERSION_INCOMPATIBLE_FAILURE = 6,
+
+  // The rest of the file could not be parsed as a ListUpdateResponse protobuf.
+  // This can happen if the machine crashed before the file was fully written to
+  // disk or if there was disk corruption.
+  HASH_PREFIX_INFO_MISSING_FAILURE = 7,
+
+  // Memory space for histograms is determined by the max.  ALWAYS
+  // ADD NEW VALUES BEFORE THIS ONE.
+  STORE_READ_RESULT_MAX
+};
+
+// Enumerate different failure events while writing the file to disk after
+// applying updates for histogramming purposes.
+// DO NOT CHANGE THE ORDERING OF THESE VALUES.
+enum StoreWriteResult {
+  // Reserved for errors in parsing this enum.
+  UNEXPECTED_WRITE_FAILURE = 0,
+
+  // No errors.
+  WRITE_SUCCESS = 1,
+
+  // The proto being written to disk wasn't a FULL_UPDATE proto.
+  INVALID_RESPONSE_TYPE_FAILURE = 2,
+
+  // Number of bytes written to disk was different from the size of the proto.
+  UNEXPECTED_BYTES_WRITTEN_FAILURE = 3,
+
+  // Renaming the temporary file to store file failed.
+  UNABLE_TO_RENAME_FAILURE = 4,
+
+  // Memory space for histograms is determined by the max.  ALWAYS
+  // ADD NEW VALUES BEFORE THIS ONE.
+  STORE_WRITE_RESULT_MAX
+};
+
 // Factory for creating V4Store. Tests implement this factory to create fake
 // stores for testing.
 class V4StoreFactory {
@@ -47,6 +108,8 @@ class V4Store {
 
   std::string DebugString() const;
 
+  void Initialize();

[Scott Hess - ex-Googler, 2016/06/22 22:58:39]

Seems like this must be something different from the constructor?  Maybe it
could use a comment.

[vakh (use Gerrit instead), 2016/06/23 23:09:32]

Done.

+
   // Reset internal state and delete the backing file.
   virtual bool Reset();
 
@@ -54,6 +117,26 @@ class V4Store {
   // The state of the store as returned by the PVer4 server in the last applied
   // update response.
   std::string state_;
+
+ private:
+  // Reads the state of the store from the file on disk and returns the reason
+  // for the failure or reports success.
+  StoreReadResult ReadFromDisk();
+
+  // Writes the FULL_UPDATE |response| to disk as a V4StoreFileFormat proto.
+  StoreWriteResult WriteToDisk(const ListUpdateResponse& response) const;
+
+  FRIEND_TEST_ALL_PREFIXES(V4StoreTest, TestReadFromEmptyFile);
+  FRIEND_TEST_ALL_PREFIXES(V4StoreTest, TestReadFromAbsentFile);
+  FRIEND_TEST_ALL_PREFIXES(V4StoreTest, TestReadFromInvalidContentsFile);
+  FRIEND_TEST_ALL_PREFIXES(V4StoreTest, TestReadFromUnexpectedMagicFile);
+  FRIEND_TEST_ALL_PREFIXES(V4StoreTest, TestReadFromLowVersionFile);
+  FRIEND_TEST_ALL_PREFIXES(V4StoreTest, TestReadFromNoHashPrefixInfoFile);
+  FRIEND_TEST_ALL_PREFIXES(V4StoreTest, TestReadFromNoHashPrefixesFile);
+  FRIEND_TEST_ALL_PREFIXES(V4StoreTest, TestWriteNoResponseType);
+  FRIEND_TEST_ALL_PREFIXES(V4StoreTest, TestWritePartialResponseType);
+  FRIEND_TEST_ALL_PREFIXES(V4StoreTest, TestWriteFullResponseType);

[Scott Hess - ex-Googler, 2016/06/22 22:58:39]

I'd put these above the private functions.

[vakh (use Gerrit instead), 2016/06/23 23:09:32]

Done.

+
   const base::FilePath store_path_;
   const scoped_refptr<base::SequencedTaskRunner> task_runner_;
 };