Load the table of visited links from database file asynchronously.

components/visitedlink/browser/visitedlink_master.h

Index: components/visitedlink/browser/visitedlink_master.h
diff --git a/components/visitedlink/browser/visitedlink_master.h b/components/visitedlink/browser/visitedlink_master.h
index d154c8067c72163720e85f834d1e240e5c7017a9..fcc515e16a8eb566b24c7f023b10e98c4d470d8a 100644
--- a/components/visitedlink/browser/visitedlink_master.h
+++ b/components/visitedlink/browser/visitedlink_master.h
@@ -15,7 +15,9 @@
 #include "base/callback_forward.h"
 #include "base/files/file_path.h"
 #include "base/gtest_prod_util.h"
+#include "base/memory/ref_counted.h"
 #include "base/memory/shared_memory.h"
+#include "base/memory/weak_ptr.h"
 #include "base/threading/sequenced_worker_pool.h"
 #include "components/visitedlink/common/visitedlink_common.h"
 
@@ -56,8 +58,9 @@ class VisitedLinkMaster : public VisitedLinkCommon {
     virtual void Add(Fingerprint fingerprint) = 0;
 
     // Called when link coloring state has been reset. This may occur when
-    // entire or parts of history were deleted.
-    virtual void Reset() = 0;
+    // entire or parts of history were deleted. Also this may occur when
+    // the table was rebuilt or loaded.

[brettw, 2015/12/08 00:54:42]

Can this comment explain at a higher-level explicitly what invalidate_hashes
means? I assume this is because the salt can change and everything needs to be
recomputed.

+    virtual void Reset(bool invalidate_hashes) = 0;
   };
 
   VisitedLinkMaster(content::BrowserContext* browser_context,
@@ -98,7 +101,7 @@ class VisitedLinkMaster : public VisitedLinkCommon {
   void AddURL(const GURL& url);
 
   // Adds a set of URLs to the table.
-  void AddURLs(const std::vector<GURL>& url);
+  void AddURLs(const std::vector<GURL>& urls);
 
   // See DeleteURLs.
   class URLIterator {
@@ -160,6 +163,14 @@ class VisitedLinkMaster : public VisitedLinkCommon {
   FRIEND_TEST_ALL_PREFIXES(VisitedLinkTest, BigDelete);
   FRIEND_TEST_ALL_PREFIXES(VisitedLinkTest, BigImport);
 
+  // Keeps the result of loading the table from the database file to the UI
+  // thread.
+  struct LoadFromFileResult;
+
+  using TableLoadCompleteCallback = base::Callback<void(
+      bool success,
+      scoped_refptr<LoadFromFileResult> load_from_file_result)>;
+
   // Object to rebuild the table on the history thread (see the .cc file).
   class TableBuilder;
 
@@ -207,19 +218,39 @@ class VisitedLinkMaster : public VisitedLinkCommon {
   // the handle to it will be stored in file_.
   void WriteFullTable();
 
-  // Try to load the table from the database file. If the file doesn't exist or
-  // is corrupt, this will return failure.
+  // Tries to load asynchronously the table from the database file.
   bool InitFromFile();
 
+  // Load the table from the database file. Calls |callback| when completed. It
+  // is called from the background thread. It must be first in the sequence of
+  // background operations with the database file.
+  static void LoadFromFile(const base::FilePath& filename,
+                           const TableLoadCompleteCallback& callback);
+
+  // Load the table from the database file. Returns true on success.
+  // Fills parameter |load_from_file_result| on success. It is called from
+  // the background thread.
+  static bool LoadApartFromFile(
+      const base::FilePath& filename,
+      scoped_refptr<LoadFromFileResult>* load_from_file_result);
+
+  // It is called from the background thread and executed on the UI
+  // thread.
+  void OnTableLoadComplete(
+      bool success,
+      scoped_refptr<LoadFromFileResult> load_from_file_result);
+
   // Reads the header of the link coloring database from disk. Assumes the
-  // file pointer is at the beginning of the file and that there are no pending
-  // asynchronous I/O operations.
+  // file pointer is at the beginning of the file and that it is the first
+  // asynchronous I/O operation on the background thread.
   //
   // Returns true on success and places the size of the table in num_entries
   // and the number of nonzero fingerprints in used_count. This will fail if
   // the version of the file is not the current version of the database.
-  bool ReadFileHeader(FILE* hfile, int32* num_entries, int32* used_count,
-                      uint8 salt[LINK_SALT_LENGTH]);
+  static bool ReadFileHeader(FILE* hfile,
+                             int32* num_entries,
+                             int32* used_count,
+                             uint8 salt[LINK_SALT_LENGTH]);
 
   // Fills *filename with the name of the link database filename
   bool GetDatabaseFileName(base::FilePath* filename);
@@ -237,9 +268,13 @@ class VisitedLinkMaster : public VisitedLinkCommon {
   // wrap around at 0 and this function will handle it.
   void WriteHashRangeToFile(Hash first_hash, Hash last_hash);
 
-  // Synchronous read from the file. Assumes there are no pending asynchronous
-  // I/O functions. Returns true if the entire buffer was successfully filled.
-  bool ReadFromFile(FILE* hfile, off_t offset, void* data, size_t data_size);
+  // Synchronous read from the file. Assumes that it is the first asynchronous
+  // I/O operation in the background thread. Returns true if the entire buffer
+  // was successfully filled.
+  static bool ReadFromFile(FILE* hfile,
+                           off_t offset,
+                           void* data,
+                           size_t data_size);
 
   // General table handling
   // ----------------------
@@ -269,12 +304,16 @@ class VisitedLinkMaster : public VisitedLinkCommon {
   // database and for unit tests.
   bool InitFromScratch(bool suppress_rebuild);
 
-  // Allocates the Fingerprint structure and length. When init_to_empty is set,
-  // the table will be filled with 0s and used_items_ will be set to 0 as well.
-  // If the flag is not set, these things are untouched and it is the
-  // responsibility of the caller to fill them (like when we are reading from
-  // a file).
-  bool CreateURLTable(int32 num_entries, bool init_to_empty);
+  // Allocates the Fingerprint structure and length. Structure is filled with 0s
+  // and shared header with salt and used_items_ is set to 0.
+  bool CreateURLTable(int32 num_entries);
+
+  // Allocates the Fingerprint structure and length. Returns true on success.
+  // Structure is filled with 0s and shared header with salt.
+  static bool CreateApartURLTable(int32 num_entries,
+                                  const uint8 salt[LINK_SALT_LENGTH],
+                                  scoped_ptr<base::SharedMemory>* shared_memory,
+                                  VisitedLinkCommon::Fingerprint** hash_table);

[brettw, 2015/12/08 00:54:42]

In the comment above, can you make more explicit how this works, especially for
memory management.

 
   // A wrapper for CreateURLTable, this will allocate a new table, initialized
   // to empty. The caller is responsible for saving the shared memory pointer
@@ -299,6 +338,9 @@ class VisitedLinkMaster : public VisitedLinkCommon {
   // current count.
   void ResizeTable(int32 new_size);
 
+  // Returns the default table size. It can be overrided in unit tests.
+  uint32 DefaultTableSize() const;
+
   // Returns the desired table size for |item_count| URLs.
   uint32 NewTableSizeForCount(int32 item_count) const;
 
@@ -337,15 +379,6 @@ class VisitedLinkMaster : public VisitedLinkCommon {
     return hash - 1;
   }
 
-#ifndef NDEBUG
-  // Indicates whether any asynchronous operation has ever been completed.
-  // We do some synchronous reads that require that no asynchronous operations
-  // are pending, yet we don't track whether they have been completed. This
-  // flag is a sanity check that these reads only happen before any
-  // asynchronous writes have been fired.
-  bool posted_asynchronous_operation_;
-#endif
-
   // Reference to the browser context that this object belongs to
   // (it knows the path to where the data is stored)
   content::BrowserContext* browser_context_;
@@ -370,10 +403,10 @@ class VisitedLinkMaster : public VisitedLinkCommon {
   std::set<Fingerprint> added_since_rebuild_;
   std::set<Fingerprint> deleted_since_rebuild_;
 
-  // TODO(brettw) Support deletion, we need to track whether anything was
-  // deleted during the rebuild here. Then we should delete any of these
-  // entries from the complete table later.
-  // std::vector<Fingerprint> removed_since_rebuild_;
+  // Indicates URLs added and deleted since we started loading the table.
+  // It can be only url because after loading table the salt will be changed.
+  std::set<GURL> added_since_load_;
+  std::set<GURL> deleted_since_load_;
 
   // The currently open file with the table in it. This may be NULL if we're
   // rebuilding and haven't written a new version yet or if |persist_to_disk_|
@@ -400,6 +433,9 @@ class VisitedLinkMaster : public VisitedLinkCommon {
   // Number of non-empty items in the table, used to compute fullness.
   int32 used_items_;
 
+  // We set this to true to avoid writing to the database file.
+  bool table_is_loading_from_file_;
+
   // Testing values -----------------------------------------------------------
   //
   // The following fields exist for testing purposes. They are not used in
@@ -423,6 +459,8 @@ class VisitedLinkMaster : public VisitedLinkCommon {
   // will be false in production.
   bool suppress_rebuild_;
 
+  base::WeakPtrFactory<VisitedLinkMaster> weak_ptr_factory_;
+
   DISALLOW_COPY_AND_ASSIGN(VisitedLinkMaster);
 };