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 125078)
+++ chrome/browser/history/in_memory_url_index.cc	(working copy)
@@ -4,23 +4,35 @@
 
 #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 "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 {
 
+// Restore/SaveCacheObserver ---------------------------------------------------
+
+InMemoryURLIndex::RestoreCacheObserver::~RestoreCacheObserver() {}
+
+InMemoryURLIndex::SaveCacheObserver::~SaveCacheObserver() {}
+
+// RebuildPrivateDataFromHistoryDBTask -----------------------------------------
+
 InMemoryURLIndex::RebuildPrivateDataFromHistoryDBTask::
     RebuildPrivateDataFromHistoryDBTask(InMemoryURLIndex* index)
     : index_(index),
-      succeeded_(false) {}
+      succeeded_(false) {
+}
 
 InMemoryURLIndex::RebuildPrivateDataFromHistoryDBTask::
     ~RebuildPrivateDataFromHistoryDBTask() {}
@@ -37,16 +49,19 @@
 
 void InMemoryURLIndex::RebuildPrivateDataFromHistoryDBTask::
     DoneRunOnMainThread() {
-  if (succeeded_)
-    index_->DoneRebuidingPrivateDataFromHistoryDB(data_.release());
+    index_->DoneRebuidingPrivateDataFromHistoryDB(succeeded_, data_.release());

[brettw, 2012/03/08 22:03:54]

Style: outdent 2 sp

[mrossetti, 2012/03/10 00:03:46]

Done.

 }
 
+// InMemoryURLIndex ------------------------------------------------------------
+
 InMemoryURLIndex::InMemoryURLIndex(Profile* profile,
                                    const FilePath& history_dir,
                                    const std::string& languages)
     : profile_(profile),
       history_dir_(history_dir),
       private_data_(new URLIndexPrivateData),
+      restore_cache_observer_(NULL),
+      save_cache_observer_(NULL),
       shutdown_(false),
       needs_to_be_cached_(false) {
   private_data_->set_languages(languages);
@@ -64,6 +79,8 @@
 InMemoryURLIndex::InMemoryURLIndex()
     : profile_(NULL),
       private_data_(new URLIndexPrivateData),
+      restore_cache_observer_(NULL),
+      save_cache_observer_(NULL),
       shutdown_(false),
       needs_to_be_cached_(false) {
 }
@@ -75,14 +92,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 +138,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.
@@ -157,24 +173,50 @@
 
 // Restoring from Cache --------------------------------------------------------
 
-void InMemoryURLIndex::RestoreFromCacheFile() {
+void InMemoryURLIndex::PostRestoreFromCacheFileTask() {
   FilePath path;
-  if (GetCacheFilePath(&path) && !shutdown_)
-    DoRestoreFromCacheFile(path);
+  if (!GetCacheFilePath(&path) || shutdown_)
+    return;
+  URLIndexPrivateData* restored_private_data = NULL;

[brettw, 2012/03/08 22:03:54]

Unless I'm missing something obvious, this code doesn't work at all. Have you
tested this patch?

[mrossetti, 2012/03/10 00:03:46]

Yes, but I'm sure it passes the unit test only as a fluke. :^[

Fixed by using a special scoped_refptr class.

On 2012/03/08 22:03:54, brettw wrote:

+  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;
+void InMemoryURLIndex::OnCacheLoadDone(URLIndexPrivateData* private_data) {
+  if (private_data) {
+    OnCacheRestored(private_data, 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(InMemoryURLIndex::DeleteCacheFile, path));
+    HistoryService* service = profile_->GetHistoryServiceWithoutCreating();
+    if (service && service->backend_loaded())
+      ScheduleRebuildFromHistory();
+    else
+      registrar_.Add(this, chrome::NOTIFICATION_HISTORY_LOADED,
+                     content::Source<Profile>(profile_));
+  }
+}
 
-  // 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::OnCacheRestored(URLIndexPrivateData* private_data,
+                                       bool succeeded) {
+  // TODO(mrossetti): Take care of remembering any visit transactions while
+  // we were away rebuilding the index and then apply them here.
+  if (succeeded)
+    private_data_.reset(private_data);
+  else
+    private_data_->Clear();
+  if (restore_cache_observer_)
+    restore_cache_observer_->OnCacheRestoreFinished(succeeded);
 }
 
 // Restoring from the History DB -----------------------------------------------
@@ -188,13 +230,20 @@
 }
 
 void InMemoryURLIndex::DoneRebuidingPrivateDataFromHistoryDB(
+    bool succeeded,
     URLIndexPrivateData* data) {
+  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
   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);
+  if (succeeded) {
+    private_data_.swap(private_data);
+    PostSaveToCacheFileTask(); // Cache the newly rebuilt index.

[mrossetti, 2012/03/06 03:49:30]

I'll fix the lint complaint about needing two spaces before the comment before
committing.

+  } 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) {
@@ -203,14 +252,42 @@
 
 // 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_ptr<URLIndexPrivateData> private_data_copy(
+        new URLIndexPrivateData(*(private_data)));

[brettw, 2012/03/08 22:03:54]

Style: normally we wouldn't put parens after the * here.

[mrossetti, 2012/03/10 00:03:46]

Done.

+    scoped_refptr<RefCountedBool> succeeded(new RefCountedBool(false));
+    content::BrowserThread::PostTaskAndReply(
+        content::BrowserThread::FILE, FROM_HERE,
+        base::Bind(&URLIndexPrivateData::WritePrivateDataToCacheFileTask,
+                   base::Passed(&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(InMemoryURLIndex::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::DeleteCacheFile(const FilePath& path) {
+  DCHECK(!content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
+  file_util::Delete(path, false);
+}
+
 }  // namespace history