Move InMemoryURLIndex Caching Operations to FILE Thread

chrome/browser/history/in_memory_url_index.cc

Index: chrome/browser/history/in_memory_url_index.cc
===================================================================
--- chrome/browser/history/in_memory_url_index.cc	(revision 125451)
+++ chrome/browser/history/in_memory_url_index.cc	(working copy)
@@ -4,23 +4,52 @@
 
 #include "chrome/browser/history/in_memory_url_index.h"
 
+#include "base/file_util.h"
 #include "base/utf_string_conversions.h"
 #include "chrome/browser/history/history_notifications.h"
 #include "chrome/browser/history/url_database.h"
 #include "chrome/browser/history/url_index_private_data.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/common/chrome_notification_types.h"
+#include "chrome/common/url_constants.h"
+#include "content/public/browser/browser_thread.h"
 #include "content/public/browser/notification_details.h"
+#include "content/public/browser/notification_service.h"
 #include "content/public/browser/notification_source.h"
 
 using in_memory_url_index::InMemoryURLIndexCacheItem;
 
 namespace history {
 
+// Called by DoSaveToCacheFile to delete any old cache file at |path| when
+// there is no private data to save. Runs on the FILE thread.
+void DeleteCacheFile(const FilePath& path) {
+  DCHECK(!content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
+  file_util::Delete(path, false);
+}
+
+// RefCountedBool --------------------------------------------------------------
+
+RefCountedBool::~RefCountedBool() {}
+
+// Restore/SaveCacheObserver ---------------------------------------------------
+
+InMemoryURLIndex::RestoreCacheObserver::~RestoreCacheObserver() {}
+
+InMemoryURLIndex::SaveCacheObserver::~SaveCacheObserver() {}
+
+// RebuildPrivateDataFromHistoryDBTask -----------------------------------------
+
 InMemoryURLIndex::RebuildPrivateDataFromHistoryDBTask::
-    RebuildPrivateDataFromHistoryDBTask(InMemoryURLIndex* index)
+    RebuildPrivateDataFromHistoryDBTask(
+        InMemoryURLIndex* index,
+        const std::string& languages,
+        const std::set<std::string>& scheme_whitelist)
     : index_(index),
-      succeeded_(false) {}
+      languages_(languages),
+      scheme_whitelist_(scheme_whitelist),
+      succeeded_(false) {
+}
 
 InMemoryURLIndex::RebuildPrivateDataFromHistoryDBTask::
     ~RebuildPrivateDataFromHistoryDBTask() {}
@@ -28,28 +57,33 @@
 bool InMemoryURLIndex::RebuildPrivateDataFromHistoryDBTask::RunOnDBThread(
     HistoryBackend* backend,
     HistoryDatabase* db) {
-  data_.reset(URLIndexPrivateData::RebuildFromHistory(db));
-  succeeded_ = data_.get() && !data_->history_info_map_.empty();
+  data_ = URLIndexPrivateData::RebuildFromHistory(db, languages_,
+                                                  scheme_whitelist_);
+  succeeded_ = data_.get() && !data_->Empty();
   if (!succeeded_)
-    data_.reset();
+    data_->Clear();
   return true;
 }
 
 void InMemoryURLIndex::RebuildPrivateDataFromHistoryDBTask::
     DoneRunOnMainThread() {
-  if (succeeded_)
-    index_->DoneRebuidingPrivateDataFromHistoryDB(data_.release());
+  index_->DoneRebuidingPrivateDataFromHistoryDB(succeeded_, data_.release());
 }
 
+// InMemoryURLIndex ------------------------------------------------------------
+
 InMemoryURLIndex::InMemoryURLIndex(Profile* profile,
                                    const FilePath& history_dir,
                                    const std::string& languages)
     : profile_(profile),
       history_dir_(history_dir),
+      languages_(languages),
       private_data_(new URLIndexPrivateData),
+      restore_cache_observer_(NULL),
+      save_cache_observer_(NULL),
       shutdown_(false),
       needs_to_be_cached_(false) {
-  private_data_->set_languages(languages);
+  InMemoryURLIndex::InitializeSchemeWhitelist(&scheme_whitelist_);
   if (profile) {
     // TODO(mrossetti): Register for language change notifications.
     content::Source<Profile> source(profile);
@@ -64,8 +98,11 @@
 InMemoryURLIndex::InMemoryURLIndex()
     : profile_(NULL),
       private_data_(new URLIndexPrivateData),
+      restore_cache_observer_(NULL),
+      save_cache_observer_(NULL),
       shutdown_(false),
       needs_to_be_cached_(false) {
+  InMemoryURLIndex::InitializeSchemeWhitelist(&scheme_whitelist_);
 }
 
 InMemoryURLIndex::~InMemoryURLIndex() {
@@ -75,14 +112,14 @@
 }
 
 void InMemoryURLIndex::Init() {
-  RestoreFromCacheFile();
+  PostRestoreFromCacheFileTask();
 }
 
 void InMemoryURLIndex::ShutDown() {
   registrar_.RemoveAll();
   cache_reader_consumer_.CancelAllRequests();
   shutdown_ = true;
-  SaveToCacheFile();
+  PostSaveToCacheFileTask();
   needs_to_be_cached_ = false;
 }
 
@@ -121,12 +158,11 @@
       OnURLsDeleted(
           content::Details<history::URLsDeletedDetails>(details).ptr());
       break;
-    case chrome::NOTIFICATION_HISTORY_LOADED: {
+    case chrome::NOTIFICATION_HISTORY_LOADED:
       registrar_.Remove(this, chrome::NOTIFICATION_HISTORY_LOADED,
                         content::Source<Profile>(profile_));
       ScheduleRebuildFromHistory();
       break;
-    }
     default:
       // For simplicity, the unit tests send us all notifications, even when
       // we haven't registered for them, so don't assert here.
@@ -135,13 +171,15 @@
 }
 
 void InMemoryURLIndex::OnURLVisited(const URLVisitedDetails* details) {
-  needs_to_be_cached_ |= private_data_->UpdateURL(details->row);
+  needs_to_be_cached_ |=
+      private_data_->UpdateURL(details->row, languages_, scheme_whitelist_);
 }
 
 void InMemoryURLIndex::OnURLsModified(const URLsModifiedDetails* details) {
   for (URLRows::const_iterator row = details->changed_urls.begin();
        row != details->changed_urls.end(); ++row)
-    needs_to_be_cached_ |= private_data_->UpdateURL(*row);
+    needs_to_be_cached_ |=
+        private_data_->UpdateURL(*row, languages_, scheme_whitelist_);
 }
 
 void InMemoryURLIndex::OnURLsDeleted(const URLsDeletedDetails* details) {
@@ -157,24 +195,42 @@
 
 // Restoring from Cache --------------------------------------------------------
 
-void InMemoryURLIndex::RestoreFromCacheFile() {
+void InMemoryURLIndex::PostRestoreFromCacheFileTask() {
   FilePath path;
-  if (GetCacheFilePath(&path) && !shutdown_)
-    DoRestoreFromCacheFile(path);
+  if (!GetCacheFilePath(&path) || shutdown_)
+    return;
+  scoped_refptr<URLIndexPrivateData> restored_private_data =
+      new URLIndexPrivateData;
+  content::BrowserThread::PostTaskAndReply(
+      content::BrowserThread::FILE, FROM_HERE,
+      base::Bind(&URLIndexPrivateData::RestoreFromFileTask, path,
+          restored_private_data),
+      base::Bind(&InMemoryURLIndex::OnCacheLoadDone, AsWeakPtr(),
+          restored_private_data));
 }
 
-void InMemoryURLIndex::DoRestoreFromCacheFile(const FilePath& path) {
-  if (private_data_->RestoreFromFile(path))
-    return;
-
-  // When unable to restore from the cache file we must rebuild from the
-  // history database.
-  HistoryService* service = profile_->GetHistoryServiceWithoutCreating();
-  if (service && service->backend_loaded())
-    ScheduleRebuildFromHistory();
-  // We must wait to rebuild until the history backend has been loaded.
-  registrar_.Add(this, chrome::NOTIFICATION_HISTORY_LOADED,
-                 content::Source<Profile>(profile_));
+void InMemoryURLIndex::OnCacheLoadDone(
+    scoped_refptr<URLIndexPrivateData> private_data) {
+  if (private_data.get() && !private_data->Empty()) {
+    private_data_ = private_data;
+    if (restore_cache_observer_)
+      restore_cache_observer_->OnCacheRestoreFinished(true);
+  } else if (profile_) {
+    // When unable to restore from the cache file delete the cache file, if
+    // it exists, and then rebuild from the history database if it's available,
+    // otherwise wait until the history database loaded and then rebuild.
+    FilePath path;
+    if (!GetCacheFilePath(&path) || shutdown_)
+      return;
+    content::BrowserThread::PostBlockingPoolTask(
+        FROM_HERE, base::Bind(DeleteCacheFile, path));
+    HistoryService* service = profile_->GetHistoryServiceWithoutCreating();
+    if (service && service->backend_loaded())
+      ScheduleRebuildFromHistory();
+    else

[brettw, 2012/03/13 23:33:52]

Style nit: need {} for this if/else block since you have >1 line in one of the
arms.

[mrossetti, 2012/03/14 17:49:19]

Done.

+      registrar_.Add(this, chrome::NOTIFICATION_HISTORY_LOADED,
+                     content::Source<Profile>(profile_));
+  }
 }
 
 // Restoring from the History DB -----------------------------------------------
@@ -183,34 +239,79 @@
   HistoryService* service =
       profile_->GetHistoryService(Profile::EXPLICIT_ACCESS);
   service->ScheduleDBTask(
-      new InMemoryURLIndex::RebuildPrivateDataFromHistoryDBTask(this),
+      new InMemoryURLIndex::RebuildPrivateDataFromHistoryDBTask(
+          this, languages_, scheme_whitelist_),
       &cache_reader_consumer_);
 }
 
 void InMemoryURLIndex::DoneRebuidingPrivateDataFromHistoryDB(
+    bool succeeded,
     URLIndexPrivateData* data) {
-  scoped_ptr<URLIndexPrivateData> private_data(data);
-  private_data_.swap(private_data);
-  // Cache the newly rebuilt index.
-  FilePath cache_file_path;
-  if (GetCacheFilePath(&cache_file_path))
-    private_data_->SaveToFile(cache_file_path);
+  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
+  scoped_refptr<URLIndexPrivateData> private_data(data);
+  if (succeeded) {
+    private_data_ = private_data;
+    PostSaveToCacheFileTask();  // Cache the newly rebuilt index.
+  } else {
+    private_data_->Clear();  // Dump the old private data.
+    // There is no need to do anything with the cache file as it was deleted
+    // when the rebuild from the history operation was kicked off.
+  }
+  if (restore_cache_observer_)
+    restore_cache_observer_->OnCacheRestoreFinished(succeeded);
 }
 
 void InMemoryURLIndex::RebuildFromHistory(HistoryDatabase* history_db) {
-  private_data_.reset(URLIndexPrivateData::RebuildFromHistory(history_db));
+  private_data_ = URLIndexPrivateData::RebuildFromHistory(history_db,
+                                                          languages_,
+                                                          scheme_whitelist_);
 }
 
 // Saving to Cache -------------------------------------------------------------
 
-void InMemoryURLIndex::SaveToCacheFile() {
+void InMemoryURLIndex::PostSaveToCacheFileTask() {
   FilePath path;
-  if (GetCacheFilePath(&path))
-    DoSaveToCacheFile(path);
+  if (!GetCacheFilePath(&path))
+    return;
+  // If there is anything in our private data then make a copy of it and tell
+  // it to save itself to a file.
+  URLIndexPrivateData* private_data = private_data_.get();
+  if (private_data && !private_data->Empty()) {
+    // Note that ownership of the copy of our private data is passed to the
+    // completion closure below.
+    scoped_refptr<URLIndexPrivateData> private_data_copy =

[brettw, 2012/03/13 23:33:52]

Doesn't this leak a reference? Duplicate is returning you a ref, and this
scoped_refptr will take another one.

I think it will be best to return a scoped_refptr.

[mrossetti, 2012/03/14 17:49:19]

Gah! Of course, you're right about the leak. I'll get it, eventually. ;^)

On 2012/03/13 23:33:52, brettw wrote:

+        private_data->Duplicate();
+    scoped_refptr<RefCountedBool> succeeded(new RefCountedBool(false));
+    content::BrowserThread::PostTaskAndReply(
+        content::BrowserThread::FILE, FROM_HERE,
+        base::Bind(&URLIndexPrivateData::WritePrivateDataToCacheFileTask,
+                   private_data_copy, path, succeeded),
+        base::Bind(&InMemoryURLIndex::OnCacheSaveDone, AsWeakPtr(), succeeded));
+  } else {
+    // If there is no data in our index then delete any existing cache file.
+    content::BrowserThread::PostBlockingPoolTask(
+        FROM_HERE,
+        base::Bind(DeleteCacheFile, path));
+  }
 }
 
-void InMemoryURLIndex::DoSaveToCacheFile(const FilePath& path) {
-  private_data_->SaveToFile(path);
+void InMemoryURLIndex::OnCacheSaveDone(
+    scoped_refptr<RefCountedBool> succeeded) {
+  if (save_cache_observer_)
+    save_cache_observer_->OnCacheSaveFinished(succeeded->value());
 }
 
+// static
+void InMemoryURLIndex::InitializeSchemeWhitelist(
+    std::set<std::string>* whitelist) {
+  DCHECK(whitelist);
+  whitelist->insert(std::string(chrome::kAboutScheme));
+  whitelist->insert(std::string(chrome::kChromeUIScheme));
+  whitelist->insert(std::string(chrome::kFileScheme));
+  whitelist->insert(std::string(chrome::kFtpScheme));
+  whitelist->insert(std::string(chrome::kHttpScheme));
+  whitelist->insert(std::string(chrome::kHttpsScheme));
+  whitelist->insert(std::string(chrome::kMailToScheme));
+}
+
 }  // namespace history