Do not open file on UI thread in VisitedLinkMaster

chrome/browser/visitedlink/visitedlink_master.h

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #ifndef CHROME_BROWSER_VISITEDLINK_VISITEDLINK_MASTER_H_
 #define CHROME_BROWSER_VISITEDLINK_VISITEDLINK_MASTER_H_
 #pragma once
 
 #if defined(OS_WIN)
 #include <windows.h>
@@ skipping 99 matching lines @@
     rebuild_complete_task_ = task;
   }
 
   // returns the number of items in the table for testing verification
   int32 GetUsedCount() const {
     return used_items_;
   }
 
   // Call to cause the entire database file to be re-written from scratch
   // to disk. Used by the performance tester.
-  bool RewriteFile() {
-    return WriteFullTable();
+  void RewriteFile() {
+    WriteFullTable();
   }
 #endif
 
  private:
   FRIEND_TEST_ALL_PREFIXES(VisitedLinkTest, Delete);
   FRIEND_TEST_ALL_PREFIXES(VisitedLinkTest, BigDelete);
   FRIEND_TEST_ALL_PREFIXES(VisitedLinkTest, BigImport);
 
   // Object to rebuild the table on the history thread (see the .cc file).
   class TableBuilder;
 
+  typedef FILE* FilePtr;

[brettw, 2012/07/27 22:11:14]

I think I prefer just seeing FILE** in the various places since then there's
nothing to look up what it means. So I'd probably delete this. Can you also add
that this class owns the pointer.

+
   // Byte offsets of values in the header.
   static const int32 kFileHeaderSignatureOffset;
   static const int32 kFileHeaderVersionOffset;
   static const int32 kFileHeaderLengthOffset;
   static const int32 kFileHeaderUsedOffset;
   static const int32 kFileHeaderSaltOffset;
 
   // The signature at the beginning of a file.
   static const int32 kFileSignature;
 
@@ skipping 19 matching lines @@
   // inserted fingerprint or null_hash_ on failure.
   Hash TryToAddURL(const GURL& url);
 
   // File I/O functions
   // ------------------
 
   // Posts the given task to the blocking worker pool with our options.
   void PostIOTask(const tracked_objects::Location& from_here,
                   const base::Closure& task);
 
-  // Writes the entire table to disk, returning true on success. It will leave
-  // the table file open and the handle to it in file_
-  bool WriteFullTable();
+  // Writes the entire table to disk. It will leave the table file open and
+  // 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.
   bool InitFromFile();
 
   // 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.
   //
   // 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]);
 
   // Fills *filename with the name of the link database filename
   bool GetDatabaseFileName(FilePath* filename);
 
   // Wrapper around Window's WriteFile using asynchronous I/O. This will proxy
   // the write to a background thread.
-  void WriteToFile(FILE* hfile, off_t offset, void* data, int32 data_size);
+  void WriteToFile(FilePtr* hfile, off_t offset, void* data, int32 data_size);
 
   // Helper function to schedule and asynchronous write of the used count to
   // disk (this is a common operation).
   void WriteUsedItemCountToFile();
 
   // Helper function to schedule an asynchronous write of the given range of
   // hash functions to disk. The range is inclusive on both ends. The range can
   // wrap around at 0 and this function will handle it.
   void WriteHashRangeToFile(Hash first_hash, Hash last_hash);
 
@@ skipping 126 matching lines @@
   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_;
 
   // 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. Writing to the file may
   // be safely ignored in this case.
-  FILE* file_;
+  FilePtr* file_;

[brettw, 2012/07/27 22:11:14]

Can you clarify the comment here? There are now three cases, file_ is NULL
(rebuilding and haven't written a new version as in the old comment), and that
it exists but points to NULL (because it's being opened on a background thread).

 
   // Shared memory consists of a SharedHeader followed by the table.
   base::SharedMemory *shared_memory_;
 
   // When we generate new tables, we increment the serial number of the
   // shared memory object.
   int32 shared_memory_serial_;
 
   // Number of non-empty items in the table, used to compute fullness.
   int32 used_items_;
@@ skipping 36 matching lines @@
   int32 used_count = 0;
   for (int32 i = 0; i < table_length_; i++) {
     if (hash_table_[i])
       used_count++;
   }
   DCHECK_EQ(used_count, used_items_);
 }
 #endif
 
 #endif  // CHROME_BROWSER_VISITEDLINK_VISITEDLINK_MASTER_H_