Converted BookmarkStorage to use ImportantFileWriter

Also made BookmarkStorage completely responsible for
migration of bookmark data from history database.
Previously the logic crossed file and class boundaries a few
times. This made it a bit hard to follow. Now it should be
a bit more clear.

Made ImportantFileWriter also batch data serializations.

TEST=Make sure that bookmarks still work. Also test migrating bookmarks from history database.

http://crbug.com/10618

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

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

1 files changed, 55 insertions, 48 deletions

diff --git a/chrome/browser/bookmarks/bookmark_storage.h b/chrome/browser/bookmarks/bookmark_storage.h
index 7048928..61162eb 100644
--- a/chrome/browser/bookmarks/bookmark_storage.h
+++ b/chrome/browser/bookmarks/bookmark_storage.h
@@ -7,12 +7,13 @@
 
 #include "base/file_path.h"
 #include "base/ref_counted.h"
-#include "base/task.h"
+#include "chrome/common/important_file_writer.h"
+#include "chrome/common/notification_observer.h"
+#include "chrome/common/notification_registrar.h"
 
 class BookmarkModel;
-class BookmarkStorageBackend;
 class Profile;
-class MessageLoop;
+class Task;
 class Value;
 
 namespace base {
@@ -24,18 +25,16 @@ class Thread;
 // as notifying the BookmarkStorage every time the model changes.
 //
 // Internally BookmarkStorage uses BookmarkCodec to do the actual read/write.
-
-class BookmarkStorage : public base::RefCountedThreadSafe<BookmarkStorage> {
-  friend class BookmarkStorageBackend;
-
+class BookmarkStorage : public NotificationObserver,
+                        public ImportantFileWriter::DataSerializer,
+                        public base::RefCountedThreadSafe<BookmarkStorage> {
  public:
   // Creates a BookmarkStorage for the specified model
   BookmarkStorage(Profile* profile, BookmarkModel* model);
+  ~BookmarkStorage();
 
-  // Loads the bookmarks into the model, notifying the model when done. If
-  // load_from_history is true, the bookmarks are loaded from the file written
-  // by history (StarredURLDatabase).
-  void LoadBookmarks(bool load_from_history);
+  // Loads the bookmarks into the model, notifying the model when done.
+  void LoadBookmarks();
 
   // Schedules saving the bookmark bar model to disk.
   void ScheduleSave();
@@ -44,59 +43,67 @@ class BookmarkStorage : public base::RefCountedThreadSafe<BookmarkStorage> {
   // a pending save, it is saved immediately.
   void BookmarkModelDeleted();
 
+  // ImportantFileWriter::DataSerializer
+  virtual bool SerializeData(std::string* output);
+
  private:
+  class LoadTask;
+
   // Callback from backend with the results of the bookmark file.
-  void LoadedBookmarks(Value* root_value,
-                       bool bookmark_file_exists,
-                       bool loaded_from_history);
+  // This may be called multiple times, with different paths. This happens when
+  // we migrate bookmark data from database.
+  void OnLoadFinished(bool file_exists, const FilePath& path,
+                      Value* root_value);
 
-  // Schedules a save on the backend thread.
-  void SaveNow();
+  // Loads bookmark data from |file| and notifies the model when finished.
+  void DoLoadBookmarks(const FilePath& file);
 
-  // Returns the thread the backend is run on.
-  base::Thread* backend_thread() const { return backend_thread_; }
+  // Load bookmarks data from the file written by history (StarredURLDatabase).
+  void MigrateFromHistory();
 
-  // The model. The model is NULL once BookmarkModelDeleted has been invoked.
-  BookmarkModel* model_;
+  // Called when history has written the file with bookmarks data. Loads data
+  // from that file.
+  void OnHistoryFinishedWriting();
 
-  // Used to delay saves.
-  ScopedRunnableMethodFactory<BookmarkStorage> save_factory_;
+  // Called after we loaded file generated by history. Saves the data, deletes
+  // the temporary file, and notifies the model.
+  void FinishHistoryMigration();
 
-  // The backend handles actual reading/writing to disk.
-  scoped_refptr<BookmarkStorageBackend> backend_;
+  // NotificationObserver
+  void Observe(NotificationType type, const NotificationSource& source,
+               const NotificationDetails& details);
 
-  // Thread read/writing is run on. This comes from the profile, and is null
-  // during testing.
-  base::Thread* backend_thread_;
+  // Serializes the data and schedules save using ImportantFileWriter.
+  // Returns true on successful serialization.
+  bool SaveNow();
 
-  DISALLOW_COPY_AND_ASSIGN(BookmarkStorage);
-};
+  // Runs task on backend thread (or on current thread if backend thread
+  // is NULL). Takes ownership of |task|.
+  void RunTaskOnBackendThread(Task* task) const;
 
-// Used to save the bookmarks on the file thread.
-class BookmarkStorageBackend :
-    public base::RefCountedThreadSafe<BookmarkStorageBackend> {
- public:
-  explicit BookmarkStorageBackend(const FilePath& path,
-                                  const FilePath& tmp_history_path);
+  // Returns the thread the backend is run on.
+  const base::Thread* backend_thread() const { return backend_thread_; }
 
-  // Writes the specified value to disk. This takes ownership of |value| and
-  // deletes it when done.
-  void Write(Value* value);
+  // Keep the pointer to profile, we may need it for migration from history.
+  Profile* profile_;
 
-  // Reads the bookmarks from kBookmarksFileName. Notifies |service| with
-  // the results on the specified MessageLoop.
-  void Read(scoped_refptr<BookmarkStorage> service,
-            MessageLoop* message_loop,
-            bool load_from_history);
+  // The model. The model is NULL once BookmarkModelDeleted has been invoked.
+  BookmarkModel* model_;
 
- private:
-  // Path we read/write to.
-  const FilePath path_;
+  // Thread read/writing is run on. This comes from the profile, and is null
+  // during testing.
+  const base::Thread* backend_thread_;
+
+  // Helper to write bookmark data safely.
+  ImportantFileWriter writer_;
 
-  // Path bookmarks are read from if asked to load from history file.
+  // Helper to ensure that we unregister from notifications on destruction.
+  NotificationRegistrar notification_registrar_;
+
+  // Path to temporary file created during migrating bookmarks from history.
   const FilePath tmp_history_path_;
 
-  DISALLOW_COPY_AND_ASSIGN(BookmarkStorageBackend);
+  DISALLOW_COPY_AND_ASSIGN(BookmarkStorage);
 };
 
 #endif  // CHROME_BROWSER_BOOKMARKS_BOOKMARK_STORAGE_H_