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 117518)
+++ chrome/browser/history/in_memory_url_index.cc	(working copy)
@@ -4,63 +4,98 @@
 
 #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_database.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_source.h"
 
-using in_memory_url_index::InMemoryURLIndexCacheItem;
+using content::BrowserThread;
 
 namespace history {
 
-InMemoryURLIndex::InMemoryURLIndex(const FilePath& history_dir)
-    : history_dir_(history_dir),
+InMemoryURLIndex::RestoreCacheObserver::~RestoreCacheObserver() {}
+
+InMemoryURLIndex::SaveCacheObserver::~SaveCacheObserver() {}
+
+InMemoryURLIndex::RebuildPrivateDataFromHistoryDBTask::
+    RebuildPrivateDataFromHistoryDBTask(InMemoryURLIndex* index)
+    : index_(index),
+      succeeded_(false) {}
+
+InMemoryURLIndex::RebuildPrivateDataFromHistoryDBTask::
+    ~RebuildPrivateDataFromHistoryDBTask() {}
+
+bool InMemoryURLIndex::RebuildPrivateDataFromHistoryDBTask::RunOnDBThread(
+    HistoryBackend* backend,
+    HistoryDatabase* db) {
+  data_.reset(URLIndexPrivateData::RebuildFromHistory(db));
+  succeeded_ = data_.get() && !data_->history_info_map_.empty();
+  if (!succeeded_)
+    data_.reset();
+  return true;

[Peter Kasting, 2012/01/14 00:12:49]

Idle curiosity... what would be the consequences of just never notifying the
IMUI on failure?

[mrossetti, 2012/03/03 05:05:56]

The index data would still have been cleared so there are no ramifications. So
in the following DoneRunOnMainThread() function I'll just not call the
DoneRebuidingPrivateDataFromHistoryDB(...) function and simplify the
implementation thereof.

+}
+
+void InMemoryURLIndex::RebuildPrivateDataFromHistoryDBTask::
+    DoneRunOnMainThread() {
+  index_->DoneRebuidingPrivateDataFromHistoryDB(succeeded_, data_.release());
+}
+
+InMemoryURLIndex::InMemoryURLIndex(Profile* profile,
+                                   const FilePath& history_dir)
+    : profile_(profile),
+      history_dir_(history_dir),
       private_data_(new URLIndexPrivateData),
-      cached_at_shutdown_(false) {
+      restore_cache_observer_(NULL),
+      save_cache_observer_(NULL),
+      shutdown_(false),
+      needs_to_be_cached_(false) {
+  if (profile) {
+    // TODO(mrossetti): Register for profile/language change notifications.

[Peter Kasting, 2012/01/14 00:12:49]

What is a profile change notification?  We don't ever change the profile for a
browser object.

[mrossetti, 2012/03/03 05:05:56]

Comment changes to only refer to language changes.

+    content::Source<Profile> source(profile);
+    registrar_.Add(this, chrome::NOTIFICATION_HISTORY_LOADED, source);
+    registrar_.Add(this, chrome::NOTIFICATION_HISTORY_URL_VISITED, source);
+    registrar_.Add(this, chrome::NOTIFICATION_HISTORY_TYPED_URLS_MODIFIED,
+                   source);
+    registrar_.Add(this, chrome::NOTIFICATION_HISTORY_URLS_DELETED, source);
+  }
 }
 
 // Called only by unit tests.
 InMemoryURLIndex::InMemoryURLIndex()
-    : private_data_(new URLIndexPrivateData),
-      cached_at_shutdown_(false) {
+    : profile_(NULL),
+      private_data_(new URLIndexPrivateData),
+      restore_cache_observer_(NULL),
+      save_cache_observer_(NULL),
+      shutdown_(false),
+      needs_to_be_cached_(false) {
 }
 
 InMemoryURLIndex::~InMemoryURLIndex() {
   // If there was a history directory (which there won't be for some unit tests)
   // then insure that the cache has already been saved.
-  DCHECK(history_dir_.empty() || cached_at_shutdown_);
+  DCHECK(history_dir_.empty() || !needs_to_be_cached_);
 }
 
-bool InMemoryURLIndex::Init(URLDatabase* history_db,
-                            const std::string& languages) {
-  // TODO(mrossetti): Register for profile/language change notifications.
+void InMemoryURLIndex::Init(const std::string& languages) {
   private_data_->set_languages(languages);
-  FilePath cache_file_path;
-  return (GetCacheFilePath(&cache_file_path) &&
-          private_data_->RestoreFromFile(cache_file_path)) ||
-      ReloadFromHistory(history_db);
+  RestoreFromCacheFile();
 }
 
-bool InMemoryURLIndex::ReloadFromHistory(history::URLDatabase* history_db) {
-  if (!private_data_->ReloadFromHistory(history_db))
-    return false;
-  FilePath cache_file_path;
-  if (GetCacheFilePath(&cache_file_path))
-    private_data_->SaveToFile(cache_file_path);
-  return true;
-}
-
 void InMemoryURLIndex::ShutDown() {
-  // Write our cache.
-  FilePath cache_file_path;
-  if (!GetCacheFilePath(&cache_file_path))
-    return;
-  private_data_->SaveToFile(cache_file_path);
-  cached_at_shutdown_ = true;
+  registrar_.RemoveAll();
+  cache_reader_consumer_.CancelAllRequests();
+  shutdown_ = true;
+  SaveToCacheFile();
+  needs_to_be_cached_ = false;
 }
 
-void InMemoryURLIndex::ClearPrivateData() {
-  private_data_->Clear();
-}
-
 bool InMemoryURLIndex::GetCacheFilePath(FilePath* file_path) {
   if (history_dir_.empty())
     return false;
@@ -68,19 +103,173 @@
   return true;
 }
 
-// Querying and Updating -------------------------------------------------------
+void InMemoryURLIndex::ClearPrivateData() {
+  private_data_->Clear();
+}
 
+// Saving to Cache -------------------------------------------------------------
+
+void InMemoryURLIndex::SaveToCacheFile() {
+  FilePath path;
+  if (GetCacheFilePath(&path))
+    DoSaveToCacheFile(path);
+}
+
+void InMemoryURLIndex::DoSaveToCacheFile(const FilePath& path) {
+  URLIndexPrivateData* private_data = private_data_.get();
+  // If there is anything in our private data then make a copy of it and tell
+  // it to save itself to a file.
+  if (private_data && private_data->word_list_.size()) {
+    URLIndexPrivateData* private_data_copy(
+        new URLIndexPrivateData(*(private_data)));
+    BrowserThread::PostTask(
+        BrowserThread::FILE, FROM_HERE,

[Peter Kasting, 2012/01/14 00:12:49]

Nit: You could put these on the above line if you want.  (Several places)

[mrossetti, 2012/03/03 05:05:56]

This part of the change has been postponed. I'll address this in the next CL
where I add the FILE/pool handling.

[brettw, 2012/03/05 22:13:05]

It's not clear what you mean here? What part has been postponed?

+        base::Bind(&InMemoryURLIndex::WritePrivateDataToCacheFile, this,
+                   path, private_data_copy));
+  } else {
+    // If there is no data in our index then delete any existing cache file.
+    BrowserThread::PostTask(
+        BrowserThread::FILE, FROM_HERE,
+        base::Bind(InMemoryURLIndex::DeleteCacheFile, path));
+  }
+}
+
+void InMemoryURLIndex::WritePrivateDataToCacheFile(
+    const FilePath path,
+    URLIndexPrivateData* private_data) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
+  scoped_ptr<history::URLIndexPrivateData> data(private_data);
+  data->SaveToFile(path);
+  if (save_cache_observer_)
+    save_cache_observer_->OnCacheSaveFinished();
+}
+
+// static
+void InMemoryURLIndex::DeleteCacheFile(const FilePath path) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
+  file_util::Delete(path, false);
+}
+
+// Restoring from Cache --------------------------------------------------------
+
+void InMemoryURLIndex::RestoreFromCacheFile() {
+  FilePath path;
+  if (GetCacheFilePath(&path) && !shutdown_)
+    DoRestoreFromCacheFile(path);
+}
+
+void InMemoryURLIndex::DoRestoreFromCacheFile(const FilePath& path) {
+  BrowserThread::PostTask(
+      BrowserThread::FILE, FROM_HERE,
+      base::Bind(&InMemoryURLIndex::ReadPrivateDataFromCacheFile, this, path));
+}
+
+void InMemoryURLIndex::ReadPrivateDataFromCacheFile(const FilePath path) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
+  history::URLIndexPrivateData* data(
+      history::URLIndexPrivateData::RestoreFromFile(path));
+  if (!profile_ || (data && !data->history_info_map_.empty())) {
+    BrowserThread::PostTask(
+        BrowserThread::UI, FROM_HERE,
+        base::Bind(&history::InMemoryURLIndex::OnCacheRestored, this, data,
+            profile_ != NULL));
+  } else if (profile_) {
+    profile_->GetHistoryServiceWithoutCreating()->ScheduleDBTask(
+        new InMemoryURLIndex::RebuildPrivateDataFromHistoryDBTask(this),
+        &cache_reader_consumer_);
+  }
+}
+
+void InMemoryURLIndex::OnCacheRestored(URLIndexPrivateData* private_data,
+                                       bool succeeded) {
+  if (succeeded)
+    private_data_.reset(private_data);
+  else
+    private_data_->Clear();

[Peter Kasting, 2012/01/14 00:12:49]

Nit: Would it make sense to allow |private_data_| to be NULL when there is no
private data?  (This would mean we wouldn't need to create an empty object in
the IMUI initializer list, and we could eliminate the "else" clause and the bool
here if |private_data| is NULL on failure.  Actually we could probably do the
latter regardless.)  Or would that mean that if restoration fails then we will
not overwrite the disk file with whatever new data we collect this run?  (Will
we do that currently?)  This also makes me wonder if we have some sort of
dataloss of navigations that happen while we're busy restoring the cache.

[mrossetti, 2012/03/03 05:05:56]

I've been thinking about this actually. The approach I was considering (for a
future CL) was to stack up the visit/delete notifications (that are being
implemented in this CL) while the cache was being restored. We could certainly
trigger the need to do so off of the |private_data_| being NULL.

In the case of failure to restore from the cache we could leave |private_data_|
null but then if the successive attempt to rebuild from the history database
failed we'd need to have a |private_data_| so that we can start collecting
visits afresh. This would be a pretty serious failure.

+  if (restore_cache_observer_)
+    restore_cache_observer_->OnCacheRestoreFinished(succeeded);
+}
+
+// Restoring from the History DB -----------------------------------------------
+
+void InMemoryURLIndex::DoneRebuidingPrivateDataFromHistoryDB(

[Peter Kasting, 2012/01/14 00:12:49]

This function seems basically identical to OnCacheRestored().  One deletes its
arg and one doesn't, which seems odd (is that a bug?).  Also they arbitrarily
have different result ordering.  Can we combine these?

[mrossetti, 2012/03/03 05:05:56]

Possibly. I'll consider this for the next change as the FILE/pool work is being
dealt with in a separate CL.

+    bool succeeded,
+    URLIndexPrivateData* data) {
+  scoped_ptr<URLIndexPrivateData> private_data(data);
+  if (succeeded)
+    private_data_.swap(private_data);
+  else
+    private_data_->Clear();  // Dump the old private data.
+  if (restore_cache_observer_)
+    restore_cache_observer_->OnCacheRestoreFinished(succeeded);
+}
+
+void InMemoryURLIndex::RebuildFromHistory(URLDatabase* history_db) {
+  private_data_.reset(URLIndexPrivateData::RebuildFromHistory(history_db));
+}
+
+// Querying --------------------------------------------------------------------
+
 ScoredHistoryMatches InMemoryURLIndex::HistoryItemsForTerms(
     const string16& term_string) {
   return private_data_->HistoryItemsForTerms(term_string);
 }
 
-void InMemoryURLIndex::UpdateURL(URLID row_id, const URLRow& row) {
-  private_data_->UpdateURL(row_id, row);
+
+// Updating --------------------------------------------------------------------
+
+void InMemoryURLIndex::Observe(int notification_type,
+                               const content::NotificationSource& source,
+                               const content::NotificationDetails& details) {
+  switch (notification_type) {
+    case chrome::NOTIFICATION_HISTORY_URL_VISITED:
+      OnURLVisited(content::Details<URLVisitedDetails>(details).ptr());
+      break;
+    case chrome::NOTIFICATION_HISTORY_TYPED_URLS_MODIFIED:
+      OnURLsModified(
+          content::Details<history::URLsModifiedDetails>(details).ptr());
+      break;
+    case chrome::NOTIFICATION_HISTORY_URLS_DELETED:
+      OnURLsDeleted(
+          content::Details<history::URLsDeletedDetails>(details).ptr());
+      break;
+    default:
+      // For simplicity, the unit tests send us all notifications, even when
+      // we haven't registered for them, so don't assert here.
+      break;
+  }
 }
 
-void InMemoryURLIndex::DeleteURL(URLID row_id) {
-  private_data_->DeleteURL(row_id);
+void InMemoryURLIndex::OnURLVisited(const URLVisitedDetails* details) {
+  private_data_->UpdateURL(details->row);

[Peter Kasting, 2012/01/14 00:12:49]

Nit: Should we be calling IMUI::UpdateURL() or DeleteURL() instead in these next
three functions?  (I don't know if that's in any way clearer or more
future-proof if you want to make updating more complex)

[mrossetti, 2012/03/03 05:05:56]

Done.

+  needs_to_be_cached_ = true;

[Peter Kasting, 2012/01/14 00:12:49]

Nit: Does this need to be after the above line?  Doing it before would be
consistent with the next two functions.

[mrossetti, 2012/03/03 05:05:56]

I'll move them to the bottom for now for consistency. I'm also going to have the
UpdateURL and DeleteURL functions return a bool to indicate if there was an
actual change. If there was then needs_to_be_cached_ will be set to true.

 }
 
+void InMemoryURLIndex::OnURLsModified(const URLsModifiedDetails* details) {
+  needs_to_be_cached_ = true;
+  for (std::vector<history::URLRow>::const_iterator row =

[Peter Kasting, 2012/01/14 00:12:49]

Nit: Is there a typedef somewhere (history.h?  I dunno) for vector<URLRow>? 
Should there be?  (2 places)

[mrossetti, 2012/03/03 05:05:56]

Done. Added 'URLRows' to history_types.h. Will file a separate CL for changing
all of the unrelated-to-this-CL uses.

+           details->changed_urls.begin();
+       row != details->changed_urls.end(); ++row)
+    private_data_->UpdateURL(*row);
+}
+
+void InMemoryURLIndex::OnURLsDeleted(const URLsDeletedDetails* details) {
+  needs_to_be_cached_ = true;
+  if (details->all_history) {
+    ClearPrivateData();
+  } else {
+    for (std::vector<URLRow>::const_iterator row = details->rows.begin();
+         row != details->rows.end(); ++row)
+      private_data_->DeleteURL(row->url());
+  }
+}
+
+void InMemoryURLIndex::UpdateURL(const URLRow& row) {
+  private_data_->UpdateURL(row);
+}
+
+void InMemoryURLIndex::DeleteURL(const GURL& url) {
+  private_data_->DeleteURL(url);
+}
+
 }  // namespace history