Revert "Take 2: Preload the visited link db on the file thread if"

This reverts commit r35991 due to a perf regression to New Tab Cold
on Mac.

TBR=thakis

Review URL: http://codereview.chromium.org/545024

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@35997 0039d316-1c4b-4281-b951-d872f2087c98

6 files changed, 91 insertions, 209 deletions

diff --git a/chrome/browser/browser_main.cc b/chrome/browser/browser_main.cc
index 8eb5409..0c57f3a 100644
--- a/chrome/browser/browser_main.cc
+++ b/chrome/browser/browser_main.cc
@@ -44,7 +44,6 @@
 #include "chrome/browser/plugin_service.h"
 #include "chrome/browser/process_singleton.h"
 #include "chrome/browser/profile_manager.h"
-#include "chrome/browser/profile.h"
 #include "chrome/browser/renderer_host/resource_dispatcher_host.h"
 #include "chrome/browser/shell_integration.h"
 #include "chrome/browser/user_data_manager.h"
@@ -101,6 +100,7 @@
 #include "chrome/browser/browser_trial.h"
 #include "chrome/browser/metrics/user_metrics.h"
 #include "chrome/browser/net/url_fixer_upper.h"
+#include "chrome/browser/profile.h"
 #include "chrome/browser/rlz/rlz.h"
 #include "chrome/browser/views/user_data_dir_dialog.h"
 #include "chrome/common/env_vars.h"
@@ -683,10 +683,6 @@ int BrowserMain(const MainFunctionParams& parameters) {
   CHECK(profile) << "Cannot get default profile.";
 #endif
 
-  // Now that we have a profile, try to load the visited link master on a
-  // background thread.  We want to start this as soon as we can.
-  profile->PreloadVisitedLinkMaster();
-
   PrefService* user_prefs = profile->GetPrefs();
   DCHECK(user_prefs);
 
diff --git a/chrome/browser/profile.cc b/chrome/browser/profile.cc
index fefbc8d..3231ede2 100644
--- a/chrome/browser/profile.cc
+++ b/chrome/browser/profile.cc
@@ -125,87 +125,6 @@ bool HasACacheSubdir(const FilePath &dir) {
 
 }  // namespace
 
-// This helper class owns the VisitedLinkMaster and exposes a way to load it
-// from on the file thread.
-class VisitedLinkCreator
-    : public base::RefCountedThreadSafe<VisitedLinkCreator> {
- public:
-  enum LoadState {
-    NOT_LOADED,
-    FILE_LOAD_FAILED,
-    LOAD_FINISHED
-  };
-
-  explicit VisitedLinkCreator(Profile* profile)
-      : listener_(new VisitedLinkEventListener()),
-        profile_(profile),
-        load_state_(NOT_LOADED) {
-    visited_link_master_.reset(new VisitedLinkMaster(listener_.get(),
-                                                     profile_));
-  }
-
-  // Preload the visited link master on the file thread.
-  void Preload() {
-    DCHECK(ChromeThread::CurrentlyOn(ChromeThread::UI));
-    ChromeThread::PostTask(ChromeThread::FILE, FROM_HERE, NewRunnableMethod(
-        this, &VisitedLinkCreator::LoadBackground));
-  }
-
-  // This method can return NULL if for some reason we can't initialize the
-  // visited link master.
-  VisitedLinkMaster* GetMaster() {
-    DCHECK(ChromeThread::CurrentlyOn(ChromeThread::UI));
-    // Go ahead and try to load the VisitedLink table.  If the data was
-    // preloaded, Load() does nothing.
-    AutoLock l(lock_);
-    if (LOAD_FINISHED != load_state_) {
-      bool success = false;
-      if (FILE_LOAD_FAILED == load_state_) {
-        success = visited_link_master_->InitFromScratch();
-      } else if (NOT_LOADED == load_state_) {
-        // We haven't tried to load from file yet, so go ahead and do that.
-        success = visited_link_master_->Init();
-      }
-
-      if (!success) {
-        NOTREACHED() << "Failed to init visited link master.";
-        visited_link_master_.reset();
-      }
-      load_state_ = LOAD_FINISHED;
-    }
-
-    // We don't need to lock here because after Load() has been called, we
-    // never change vistied_link_master_.
-    return visited_link_master_.get();
-  }
-
- private:
-  // Tries to load the visited link database from the UI thread.
-  void LoadBackground() {
-    DCHECK(ChromeThread::CurrentlyOn(ChromeThread::FILE));
-    AutoLock l(lock_);
-    if (NOT_LOADED != load_state_)
-      return;
-
-    // We only want to try to load the data from a file (it's not thread safe
-    // to InitFromScratch on the file thread).
-    load_state_ = visited_link_master_->InitFromFile() ? LOAD_FINISHED
-                                                       : FILE_LOAD_FAILED;
-  }
-
-  scoped_ptr<VisitedLinkMaster::Listener> listener_;
-  Profile* profile_;
-
-  // This lock protects visited_link_master_ and load_state_.
-  Lock lock_;
-  scoped_ptr<VisitedLinkMaster> visited_link_master_;
-  // Once created_ is true, we can stop trying to create and init the
-  // VisitedLinkMaster.
-  LoadState load_state_;
-
-  DISALLOW_COPY_AND_ASSIGN(VisitedLinkCreator);
-};
-
 // A pointer to the request context for the default profile.  See comments on
 // Profile::GetDefaultRequestContext.
 URLRequestContextGetter* Profile::default_request_context_;
@@ -295,9 +214,7 @@ class OffTheRecordProfileImpl : public Profile,
     return reinterpret_cast<ProfileId>(this);
   }
 
-  virtual FilePath GetPath() {
-    return profile_->GetPath();
-  }
+  virtual FilePath GetPath() { return profile_->GetPath(); }
 
   virtual bool IsOffTheRecord() {
     return true;
@@ -328,8 +245,6 @@ class OffTheRecordProfileImpl : public Profile,
     return NULL;
   }
 
-  virtual void PreloadVisitedLinkMaster() {}
-
   virtual ExtensionsService* GetExtensionsService() {
     return NULL;
   }
@@ -652,6 +567,7 @@ class OffTheRecordProfileImpl : public Profile,
 
 ProfileImpl::ProfileImpl(const FilePath& path)
     : path_(path),
+      visited_link_event_listener_(new VisitedLinkEventListener()),
       extension_devtools_manager_(NULL),
       request_context_(NULL),
       media_request_context_(NULL),
@@ -898,16 +814,15 @@ webkit_database::DatabaseTracker* ProfileImpl::GetDatabaseTracker() {
 }
 
 VisitedLinkMaster* ProfileImpl::GetVisitedLinkMaster() {
-  if (!visited_link_creator_.get())
-    visited_link_creator_ = new VisitedLinkCreator(this);
-  return visited_link_creator_->GetMaster();
-}
-
-void ProfileImpl::PreloadVisitedLinkMaster() {
-  if (!visited_link_creator_.get()) {
-    visited_link_creator_ = new VisitedLinkCreator(this);
-    visited_link_creator_->Preload();
+  if (!visited_link_master_.get()) {
+    scoped_ptr<VisitedLinkMaster> visited_links(
+      new VisitedLinkMaster(visited_link_event_listener_.get(), this));
+    if (!visited_links->Init())
+      return NULL;
+    visited_link_master_.swap(visited_links);
   }
+
+  return visited_link_master_.get();
 }
 
 ExtensionsService* ProfileImpl::GetExtensionsService() {
diff --git a/chrome/browser/profile.h b/chrome/browser/profile.h
index 95f76b0..1d15753 100644
--- a/chrome/browser/profile.h
+++ b/chrome/browser/profile.h
@@ -64,8 +64,8 @@ class ThemeProvider;
 class ThumbnailStore;
 class URLRequestContextGetter;
 class UserScriptMaster;
-class VisitedLinkCreator;
 class VisitedLinkMaster;
+class VisitedLinkEventListener;
 class WebDataService;
 class WebKitContext;
 class WebResourceService;
@@ -148,10 +148,6 @@ class Profile {
   // that this method is called.
   virtual VisitedLinkMaster* GetVisitedLinkMaster() = 0;
 
-  // Loads the visited link master on the file thread.  It's safe to call
-  // GetVisitedLinkMaster without calling this in advance.
-  virtual void PreloadVisitedLinkMaster() = 0;
-
   // Retrieves a pointer to the ExtensionsService associated with this
   // profile. The ExtensionsService is created at startup.
   virtual ExtensionsService* GetExtensionsService() = 0;
@@ -410,7 +406,6 @@ class ProfileImpl : public Profile,
   virtual Profile* GetOriginalProfile();
   virtual webkit_database::DatabaseTracker* GetDatabaseTracker();
   virtual VisitedLinkMaster* GetVisitedLinkMaster();
-  virtual void PreloadVisitedLinkMaster();
   virtual UserScriptMaster* GetUserScriptMaster();
   virtual SSLHostState* GetSSLHostState();
   virtual net::TransportSecurityState* GetTransportSecurityState();
@@ -496,7 +491,8 @@ class ProfileImpl : public Profile,
 
   FilePath path_;
   FilePath base_cache_path_;
-  scoped_refptr<VisitedLinkCreator> visited_link_creator_;
+  scoped_ptr<VisitedLinkEventListener> visited_link_event_listener_;
+  scoped_ptr<VisitedLinkMaster> visited_link_master_;
   scoped_refptr<ExtensionsService> extensions_service_;
   scoped_refptr<UserScriptMaster> user_script_master_;
   scoped_refptr<ExtensionDevToolsManager> extension_devtools_manager_;
diff --git a/chrome/browser/visitedlink_master.cc b/chrome/browser/visitedlink_master.cc
index 58769dc..c2aa3fe 100644
--- a/chrome/browser/visitedlink_master.cc
+++ b/chrome/browser/visitedlink_master.cc
@@ -24,6 +24,7 @@
 #include "base/rand_util.h"
 #include "base/stack_container.h"
 #include "base/string_util.h"
+#include "chrome/browser/browser_process.h"
 #include "chrome/browser/chrome_thread.h"
 #include "chrome/browser/history/history.h"
 #include "chrome/browser/profile.h"
@@ -143,24 +144,6 @@ class AsyncCloseHandle : public Task {
   DISALLOW_COPY_AND_ASSIGN(AsyncCloseHandle);
 };
 
-// A helper class for clearing |profile_pointer_| then re-setting it when the
-// object goes out of scope.  This helps to make sure we don't accidentally
-// use the pointer when we're not supposed to.
-class ScopedNullProfile {
- public:
-  ScopedNullProfile(Profile** profile)
-      : profile_(*profile),
-        profile_pointer_(profile) {
-    *profile_pointer_ = NULL;
-  }
-  ~ScopedNullProfile() {
-    *profile_pointer_ = profile_;
-  }
- private:
-  Profile* profile_;
-  Profile** profile_pointer_;
-};
-
 }  // namespace
 
 // TableBuilder ---------------------------------------------------------------
@@ -265,9 +248,6 @@ void VisitedLinkMaster::InitMembers(Listener* listener, Profile* profile) {
   suppress_rebuild_ = false;
   profile_ = profile;
 
-  if (profile_)
-    profile_dir_ = profile_->GetPath();
-
 #ifndef NDEBUG
   posted_asynchronous_operation_ = false;
 #endif
@@ -275,74 +255,10 @@ void VisitedLinkMaster::InitMembers(Listener* listener, Profile* profile) {
 
 bool VisitedLinkMaster::Init() {
   if (!InitFromFile())
-    return InitFromScratch();
-  return true;
-}
-
-bool VisitedLinkMaster::InitFromFile() {
-  // We allow running this method from background threads, so be careful not to
-  // access profile_ (which may already be deleted) from this method.
-  ScopedNullProfile scoped(&profile_);
-  DCHECK(file_ == NULL);
-  DCHECK(profile_ == NULL);
-
-  FilePath filename;
-  GetDatabaseFileName(&filename);
-  ScopedFILE file_closer(OpenFile(filename, "rb+"));
-  if (!file_closer.get())
-    return false;
-
-  int32 num_entries, used_count;
-  if (!ReadFileHeader(file_closer.get(), &num_entries, &used_count, salt_))
-    return false;  // Header isn't valid.
-
-  // Allocate and read the table.
-  if (!CreateURLTable(num_entries, false))
-    return false;
-  if (!ReadFromFile(file_closer.get(), kFileHeaderSize,
-                    hash_table_, num_entries * sizeof(Fingerprint))) {
-    FreeURLTable();
-    return false;
-  }
-  used_items_ = used_count;
-
-#ifndef NDEBUG
-  DebugValidate();
-#endif
-
-  file_ = file_closer.release();
+    return InitFromScratch(suppress_rebuild_);
   return true;
 }
 
-bool VisitedLinkMaster::InitFromScratch() {
-  int32 table_size = kDefaultTableSize;
-  if (table_size_override_)
-    table_size = table_size_override_;
-
-  // The salt must be generated before the table so that it can be copied to
-  // the shared memory.
-  GenerateSalt(salt_);
-  if (!CreateURLTable(table_size, true))
-    return false;
-
-#ifndef NDEBUG
-  DebugValidate();
-#endif
-
-  if (suppress_rebuild_) {
-    // When we disallow rebuilds (normally just unit tests), just use the
-    // current empty table.
-    return WriteFullTable();
-  }
-
-  // This will build the table from history. On the first run, history will
-  // be empty, so this will be correct. This will also write the new table
-  // to disk. We don't want to save explicitly here, since the rebuild may
-  // not complete, leaving us with an empty but valid visited link database.
-  // In the future, we won't know we need to try rebuilding again.
-  return RebuildTableFromHistory();
-}
-
 VisitedLinkMaster::Hash VisitedLinkMaster::TryToAddURL(const GURL& url) {
   // Extra check that we are not off the record. This should not happen.
   if (profile_ && profile_->IsOffTheRecord()) {
@@ -621,6 +537,66 @@ bool VisitedLinkMaster::WriteFullTable() {
   return true;
 }
 
+bool VisitedLinkMaster::InitFromFile() {
+  DCHECK(file_ == NULL);
+
+  FilePath filename;
+  GetDatabaseFileName(&filename);
+  ScopedFILE file_closer(OpenFile(filename, "rb+"));
+  if (!file_closer.get())
+    return false;
+
+  int32 num_entries, used_count;
+  if (!ReadFileHeader(file_closer.get(), &num_entries, &used_count, salt_))
+    return false;  // Header isn't valid.
+
+  // Allocate and read the table.
+  if (!CreateURLTable(num_entries, false))
+    return false;
+  if (!ReadFromFile(file_closer.get(), kFileHeaderSize,
+                    hash_table_, num_entries * sizeof(Fingerprint))) {
+    FreeURLTable();
+    return false;
+  }
+  used_items_ = used_count;
+
+#ifndef NDEBUG
+  DebugValidate();
+#endif
+
+  file_ = file_closer.release();
+  return true;
+}
+
+bool VisitedLinkMaster::InitFromScratch(bool suppress_rebuild) {
+  int32 table_size = kDefaultTableSize;
+  if (table_size_override_)
+    table_size = table_size_override_;
+
+  // The salt must be generated before the table so that it can be copied to
+  // the shared memory.
+  GenerateSalt(salt_);
+  if (!CreateURLTable(table_size, true))
+    return false;
+
+#ifndef NDEBUG
+  DebugValidate();
+#endif
+
+  if (suppress_rebuild) {
+    // When we disallow rebuilds (normally just unit tests), just use the
+    // current empty table.
+    return WriteFullTable();
+  }
+
+  // This will build the table from history. On the first run, history will
+  // be empty, so this will be correct. This will also write the new table
+  // to disk. We don't want to save explicitly here, since the rebuild may
+  // not complete, leaving us with an empty but valid visited link database.
+  // In the future, we won't know we need to try rebuilding again.
+  return RebuildTableFromHistory();
+}
+
 bool VisitedLinkMaster::ReadFileHeader(FILE* file,
                                        int32* num_entries,
                                        int32* used_count,
@@ -678,10 +654,11 @@ bool VisitedLinkMaster::GetDatabaseFileName(FilePath* filename) {
     return true;
   }
 
-  if (profile_dir_.empty())
+  if (!profile_ || profile_->GetPath().empty())
     return false;
 
-  *filename = profile_dir_.Append(FILE_PATH_LITERAL("Visited Links"));
+  FilePath profile_dir = profile_->GetPath();
+  *filename = profile_dir.Append(FILE_PATH_LITERAL("Visited Links"));
   return true;
 }
 
diff --git a/chrome/browser/visitedlink_master.h b/chrome/browser/visitedlink_master.h
index 14ba9d8..fc35ff9 100644
--- a/chrome/browser/visitedlink_master.h
+++ b/chrome/browser/visitedlink_master.h
@@ -77,18 +77,9 @@ class VisitedLinkMaster : public VisitedLinkCommon {
 
   // Must be called immediately after object creation. Nothing else will work
   // until this is called. Returns true on success, false means that this
-  // object won't work. You can also use InitFromFile() and InitFromScratch()
-  // if you need more control over loading the visited link information.
+  // object won't work.
   bool Init();
 
-  // Try to load the table from the database file. If the file doesn't exist or
-  // is corrupt, this will return failure. This method may be called from a
-  // background thread, but it isn't thread safe.
-  bool InitFromFile();
-
-  // Creates a new empty table, call if InitFromFile() fails.
-  bool InitFromScratch();
-
   base::SharedMemory* shared_memory() { return shared_memory_; }
 
   // Adds a URL to the table.
@@ -176,6 +167,10 @@ class VisitedLinkMaster : public VisitedLinkCommon {
   // the table file open and the handle to it in file_
   bool 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.
@@ -227,6 +222,13 @@ class VisitedLinkMaster : public VisitedLinkCommon {
   // fingerprint was deleted, false if it was not in the table to delete.
   bool DeleteFingerprint(Fingerprint fingerprint, bool update_file);
 
+  // Creates a new empty table, call if InitFromFile() fails. Normally, when
+  // |suppress_rebuild| is false, the table will be rebuilt from history,
+  // keeping us in sync. When |suppress_rebuild| is true, the new table will be
+  // empty and we will not consult history. This is used when clearing the
+  // 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
@@ -367,9 +369,6 @@ class VisitedLinkMaster : public VisitedLinkCommon {
   // will be false in production.
   bool suppress_rebuild_;
 
-  // Keep a copy of the profile_dir in case we outlive the profile.
-  FilePath profile_dir_;
-
   DISALLOW_EVIL_CONSTRUCTORS(VisitedLinkMaster);
 };
 
diff --git a/chrome/test/testing_profile.h b/chrome/test/testing_profile.h
index 7d32356..77fdd40 100644
--- a/chrome/test/testing_profile.h
+++ b/chrome/test/testing_profile.h
@@ -79,7 +79,6 @@ class TestingProfile : public Profile {
   virtual Profile* GetOriginalProfile() { return this; }
   virtual webkit_database::DatabaseTracker* GetDatabaseTracker();
   virtual VisitedLinkMaster* GetVisitedLinkMaster() { return NULL; }
-  virtual void PreloadVisitedLinkMaster() {}
   virtual ExtensionsService* GetExtensionsService() { return NULL; }
   virtual UserScriptMaster* GetUserScriptMaster() { return NULL; }
   virtual ExtensionDevToolsManager* GetExtensionDevToolsManager() {