Move InMemoryURLIndex Caching Operations to FILE Thread

chrome/browser/history/in_memory_url_index.h

Index: chrome/browser/history/in_memory_url_index.h
===================================================================
--- chrome/browser/history/in_memory_url_index.h	(revision 117518)
+++ chrome/browser/history/in_memory_url_index.h	(working copy)
@@ -15,30 +15,29 @@
 #include "base/basictypes.h"
 #include "base/file_path.h"
 #include "base/gtest_prod_util.h"
-#include "base/memory/linked_ptr.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/string16.h"
 #include "chrome/browser/autocomplete/autocomplete_match.h"
 #include "chrome/browser/autocomplete/history_provider_util.h"
+#include "chrome/browser/cancelable_request.h"
+#include "chrome/browser/history/history.h"
 #include "chrome/browser/history/history_types.h"
 #include "chrome/browser/history/in_memory_url_index_types.h"
-#include "chrome/browser/history/in_memory_url_index_cache.pb.h"
-#include "chrome/browser/history/url_index_private_data.h"
-#include "sql/connection.h"
+#include "content/public/browser/notification_observer.h"
+#include "content/public/browser/notification_registrar.h"
+#include "testing/gtest/include/gtest/gtest_prod.h"
 
-namespace base {
-class Time;
-}
+class HistoryQuickProviderTest;
+class Profile;
 
-namespace in_memory_url_index {
-class InMemoryURLIndexCacheItem;
-}
-
 namespace history {
 
-namespace imui = in_memory_url_index;
-
+class InMemoryURLIndexTest;
 class URLDatabase;
+class URLIndexPrivateData;
+struct URLVisitedDetails;
+struct URLsModifiedDetails;
+struct URLsDeletedDetails;
 
 // The URL history source.
 // Holds portions of the URL database in memory in an indexed form.  Used to
@@ -59,48 +58,85 @@
 // will eliminate such words except in the case where a single character
 // is being searched on and which character occurs as the second char16 of a
 // multi-char16 instance.
-class InMemoryURLIndex {
+class InMemoryURLIndex : public base::RefCountedThreadSafe<InMemoryURLIndex>,

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

We chatted some in IM about avoiding this by vending weak pointers and deleting
on the UI thread.

It might be possible to use the existing |shutdown_| member to prevent any
indirect access to other threads, if you elect to go the DeleteSoon() route to
delete this on the UI thread.  Alternatively it might be possible to use the
call you've added to HistoryService::UnloadBackend() to delete this object
directly on the UI thread, if we can then prevent the object from getting
recreated.

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

Done. Notifications are now employed.

+                         public content::NotificationObserver {
  public:
-  // |history_dir| is a path to the directory containing the history database
-  // within the profile wherein the cache and transaction journals will be
-  // stored.
-  explicit InMemoryURLIndex(const FilePath& history_dir);
+  // Defines an abstract class which is notified upon completion of restoring
+  // the index's private data either by reading from the cache file or by
+  // rebuilding from the history database.
+  class RestoreCacheObserver {
+   public:
+    virtual ~RestoreCacheObserver();
+
+    // Callback that lets the observer know that the restore operation has
+    // completed. |succeeded| indicates if the restore was successful. This is
+    // called on the UI thread.
+    virtual void OnCacheRestoreFinished(bool succeeded) = 0;
+  };
+
+  // Defines an abstract class which is notified upon completion of saving
+  // the index's private data to the cache file.
+  class SaveCacheObserver {
+   public:
+    virtual ~SaveCacheObserver();
+
+    // Callback that lets the observer know that the save succeeded.
+    virtual void OnCacheSaveFinished() = 0;  // Is called on the IO thread.

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

Nit: Since this is subtle, it might make sense to put "IOThread" at the end of
the function name rather than in a comment, a la the HistoryDBTask functions. 
(2 places)

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

This has been postponed to the next CL when the FILE/resource pool is employed.

+
+    // Callback that lets the observer know that the save failed.
+    virtual void OnCacheSaveFailed() = 0;  // Is called on the UI thread.
+  };
+
+  // |profile|, which may be NULL during unit testing, is used to register for
+  // history changes. |history_dir| is a path to the directory containing the
+  // history database within the profile wherein the cache and transaction
+  // journals will be stored.
+  InMemoryURLIndex(Profile* profile, const FilePath& history_dir);
   virtual ~InMemoryURLIndex();
 
-  // Opens and indexes the URL history database. If the index private data
-  // cannot be restored from its cache file then it is rebuilt from the
-  // |history_db|. |languages| gives a list of language encodings by which URLs
-  // and omnibox searches are broken down into words and characters.
-  bool Init(URLDatabase* history_db, const std::string& languages);
+  // Opens and initializes the index from a cache file. If the cache file is
+  // not present or empty then the index will eventually be rebuilt from the
+  // history database associated with the profile. |languages| gives a list of
+  // language encodings with which the history URLs and omnibox searches are
+  // interpreted, i.e. when each is broken down into words and each word is
+  // broken down into characters.
+  void Init(const std::string& languages);
 
   // Signals that any outstanding initialization should be canceled and
   // flushes the cache to disk.
   void ShutDown();
 
-  // Reloads the history index from |history_db| ignoring any cache file that
-  // may be available, clears the cache and saves the cache after reloading.
-  bool ReloadFromHistory(history::URLDatabase* history_db);
-
   // Scans the history index and returns a vector with all scored, matching
   // history items. This entry point simply forwards the call on to the
   // URLIndexPrivateData class. For a complete description of this function
   // refer to that class.
   ScoredHistoryMatches HistoryItemsForTerms(const string16& term_string);
 
-  // Updates or adds an history item to the index if it meets the minimum
-  // 'quick' criteria.
-  void UpdateURL(URLID row_id, const URLRow& row);
+  // Sets the optional observers for completion of restoral and saving of the
+  // index's private data.
+  void set_restore_cache_observer(
+      RestoreCacheObserver* restore_cache_observer) {
+    restore_cache_observer_ = restore_cache_observer;
+  }
+  void set_save_cache_observer(SaveCacheObserver* save_cache_observer) {
+    save_cache_observer_ = save_cache_observer;
+  }
 
-  // Deletes indexing data for an history item. The item may not have actually
-  // been indexed (which is the case if it did not previously meet minimum
-  // 'quick' criteria).
-  void DeleteURL(URLID row_id);
+  // Forwards to the URLIndexPrivateData. For unit testing only.
+  void UpdateURL(const URLRow& row);
+  void DeleteURL(const GURL& url);
 
  private:
-  friend class InMemoryURLIndexTest;
+  friend class HistoryQuickProviderTest;
+  FRIEND_TEST_ALL_PREFIXES(InMemoryURLIndexTest, AddNewRows);

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

Nit: Does it make sense to build some base class for all the
InMemoryURLIndexTests, give it a bunch of passthrough accessors, and then just
friend that class here?

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

Done.

   FRIEND_TEST_ALL_PREFIXES(InMemoryURLIndexTest, CacheFilePath);
+  FRIEND_TEST_ALL_PREFIXES(InMemoryURLIndexTest, DeleteRows);
   FRIEND_TEST_ALL_PREFIXES(InMemoryURLIndexTest, CacheSaveRestore);
   FRIEND_TEST_ALL_PREFIXES(InMemoryURLIndexTest, HugeResultSet);
+  FRIEND_TEST_ALL_PREFIXES(InMemoryURLIndexTest, NonUniqueTermCharacterSets);
+  FRIEND_TEST_ALL_PREFIXES(InMemoryURLIndexTest, ProperStringMatching);
+  FRIEND_TEST_ALL_PREFIXES(InMemoryURLIndexTest, Retrieval);
+  FRIEND_TEST_ALL_PREFIXES(InMemoryURLIndexTest, TitleChange);
   FRIEND_TEST_ALL_PREFIXES(InMemoryURLIndexTest, TitleSearch);
   FRIEND_TEST_ALL_PREFIXES(InMemoryURLIndexTest, TypedCharacterCaching);
   FRIEND_TEST_ALL_PREFIXES(InMemoryURLIndexTest, WhitelistedURLs);
@@ -109,16 +145,98 @@
   // Creating one of me without a history path is not allowed (tests excepted).
   InMemoryURLIndex();
 
+  // HistoryDBTask used to rebuild our private data from the history database.
+  class RebuildPrivateDataFromHistoryDBTask : public HistoryDBTask {
+   public:
+    // We take ownership of the |private_data|.

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

Nit: |private_data| is not a variable defined here (or, by this name, below)

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

Done.

+    explicit RebuildPrivateDataFromHistoryDBTask(InMemoryURLIndex* index);
+

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

Nit: No newline

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

Done.

+    virtual ~RebuildPrivateDataFromHistoryDBTask();
+
+    virtual bool RunOnDBThread(HistoryBackend* backend,

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

Nit: Optional: "// HistoryDBTask"

+                               history::HistoryDatabase* db) OVERRIDE;
+

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

Nit: Newline unnecessary

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

Done.

+    virtual void DoneRunOnMainThread() OVERRIDE;
+
+   private:
+    InMemoryURLIndex* index_;  // Call back to this index at completion.
+    bool succeeded_;  // Indicates if the rebuild was successful.
+    scoped_ptr<URLIndexPrivateData> data_;  // The rebuilt private data.
+
+    DISALLOW_COPY_AND_ASSIGN(RebuildPrivateDataFromHistoryDBTask);
+  };
+
   // Initializes all index data members in preparation for restoring the index
   // from the cache or a complete rebuild from the history database.
   void ClearPrivateData();
 
+  // Utility functions supporting RestoreFromCache and SaveToCache.
+
   // Construct a file path for the cache file within the same directory where

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

Nit: Construct -> Constructs (while you're here)

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

Done.

   // the history database is kept and saves that path to |file_path|. Returns
   // true if |file_path| can be successfully constructed. (This function
   // provided as a hook for unit testing.)
   bool GetCacheFilePath(FilePath* file_path);
 
+  // Restores the index's private data from the cache file stored in the
+  // profile directory.
+  void RestoreFromCacheFile();
+
+  // Restores private_data_ from the given |path|. Runs on the UI thread.

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

Nit: You might want an explicit comment here or above clarifying what the
difference between these two functions is.  (2 places)

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

Done.

+  void DoRestoreFromCacheFile(const FilePath& path);
+
+  // Called by DoRestoreFromCacheFile and retrieves the private data from the
+  // given |path|. If the restore fails then spawn a task to rebuild the
+  // private data from the history database.
+  void ReadPrivateDataFromCacheFile(const FilePath path);
+
+  // Caches the index private data and writes the cache file to the profile
+  // directory.
+  void SaveToCacheFile();
+
+  // Saves private_data_ to the given |path|. Runs on the UI thread.
+  void DoSaveToCacheFile(const FilePath& path);
+
+  // Called by DoSaveToCacheFile and saves |private_data| to the given |path|.

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

Nit: and saves -> to save

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

Postponed to the FILE/pool change.

+  // Runs on the FILE thread.
+  void WritePrivateDataToCacheFile(const FilePath path,
+                                   URLIndexPrivateData* private_data);
+
+  // Called by DoSaveToCacheFile to delete any old cache file at |path| when
+  // there is no private data to save. Runs on the FILE thread.
+  static void DeleteCacheFile(const FilePath path);
+
+  // Callback used by RebuildPrivateDataFromHistoryDBTask to signal completion
+  // or rebuilding our private data from the history database. |succeeded|
+  // will be true if the rebuild was successful. |data| will point to a new
+  // instanceof the private data just rebuilt.

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

Nit: Missing space

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

Done.

+  void DoneRebuidingPrivateDataFromHistoryDB(bool succeeded,
+                                             URLIndexPrivateData* data);
+
+  // Callback function that sets the private data from the just-restored-from-
+  // file |private_data| if |succeeded| otherwise clears the private data.
+  // Notifies any |restore_cache_observer_| of success status.
+  void OnCacheRestored(URLIndexPrivateData* private_data, bool succeeded);
+
+  // Notifications -------------------------------------------------------------

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

Nit: Kinda weird to have ---- here but not above (e.g. on "// Utility functions
...", and no divider below (above RebuildFromHistory() or something).

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

Done.

+
+  // Handle notifications of history loading, and all history changes.
+  virtual void Observe(int notification_type,
+                       const content::NotificationSource& source,
+                       const content::NotificationDetails& details) OVERRIDE;
+
+  // Notification handlers.
+  void OnURLVisited(const URLVisitedDetails* details);
+  void OnURLsModified(const URLsModifiedDetails* details);
+  void OnURLsDeleted(const URLsDeletedDetails* details);
+
+  // Rebuilds the history index from the history database in |history_db|.
+  // Used for unit testing only.
+  void RebuildFromHistory(URLDatabase* history_db);
+
+  // The profile, may be null when testing.
+  Profile* profile_;
+
   // Directory where cache file resides. This is, except when unit testing,
   // the same directory in which the profile's history database is found. It
   // should never be empty.
@@ -127,12 +245,25 @@
   // The index's durable private data.
   scoped_ptr<URLIndexPrivateData> private_data_;
 
-  // Set to true at shutdown when the cache has been written to disk. Used
-  // as a temporary safety check to insure that the cache is saved before
-  // the index has been destructed.
+  // Observers to notify upon restoral or save of the private data cache.

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

Nit: restoral or save -> restoration or saving

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

Postponed to the FILE/pool change.

+  RestoreCacheObserver* restore_cache_observer_;
+  SaveCacheObserver* save_cache_observer_;
+
+  CancelableRequestConsumer cache_reader_consumer_;
+
+  // Remember to unregister from notifications on destruction.

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

Nit: Comment unnecessary

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

Done.

+  content::NotificationRegistrar registrar_;
+
+  // Set to true once the shutdown process has begun.
+  bool shutdown_;
+
+  // Set to true when changes to the index have been made and the index needs
+  // to be cached. Set to false when the index has been cached. Used as a
+  // temporary safety check to insure that the cache is saved before the
+  // index has been destructed.
   // TODO(mrossetti): Eliminate once the transition to SQLite has been done.
   // http://crbug.com/83659
-  bool cached_at_shutdown_;
+  bool needs_to_be_cached_;
 
   DISALLOW_COPY_AND_ASSIGN(InMemoryURLIndex);
 };