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

9 files changed, 351 insertions, 314 deletions

diff --git a/chrome/browser/bookmarks/bookmark_model.cc b/chrome/browser/bookmarks/bookmark_model.cc
index e944419..646aa3f 100644
--- a/chrome/browser/bookmarks/bookmark_model.cc
+++ b/chrome/browser/bookmarks/bookmark_model.cc
@@ -109,7 +109,6 @@ BookmarkModel::BookmarkModel(Profile* profile)
       bookmark_bar_node_(NULL),
       other_node_(NULL),
       observers_(ObserverList<BookmarkModelObserver>::NOTIFY_EXISTING_ONLY),
-      waiting_for_history_load_(false),
       loaded_signal_(TRUE, FALSE)
 {
   // Create the bookmark bar and other bookmarks folders. These always exist.
@@ -135,11 +134,6 @@ BookmarkModel::~BookmarkModel() {
         this, NotificationType::FAVICON_CHANGED, Source<Profile>(profile_));
   }
 
-  if (waiting_for_history_load_) {
-    NotificationService::current()->RemoveObserver(
-        this, NotificationType::HISTORY_LOADED, Source<Profile>(profile_));
-  }
-
   FOR_EACH_OBSERVER(BookmarkModelObserver, observers_,
                     BookmarkModelBeingDeleted(this));
 
@@ -167,7 +161,7 @@ void BookmarkModel::Load() {
 
   // Load the bookmarks. BookmarkStorage notifies us when done.
   store_ = new BookmarkStorage(profile_, this);
-  store_->LoadBookmarks(false);
+  store_->LoadBookmarks();
 }
 
 BookmarkNode* BookmarkModel::GetParentForNewNodes() {
@@ -437,65 +431,9 @@ void BookmarkModel::RemoveNode(BookmarkNode* node,
     RemoveNode(node->GetChild(i), removed_urls);
 }
 
-void BookmarkModel::OnBookmarkStorageLoadedBookmarks(
-    bool file_exists,
-    bool loaded_from_history) {
-  if (loaded_) {
-    NOTREACHED();
-    return;
-  }
-
-  LOG(INFO) << "Loaded bookmarks, file_exists=" << file_exists <<
-      " from_history=" << loaded_from_history;
-
-  if (file_exists || loaded_from_history || !profile_ ||
-      !profile_->GetHistoryService(Profile::EXPLICIT_ACCESS)) {
-    // The file exists, we're loaded.
-    DoneLoading();
-
-    if (loaded_from_history) {
-      // We were just populated from the historical file. Schedule a save so
-      // that the main file is up to date.
-      store_->ScheduleSave();
-    }
-    return;
-  }
-
-  // The file doesn't exist. This means one of two things:
-  // 1. A clean profile.
-  // 2. The user is migrating from an older version where bookmarks were saved
-  //    in history.
-  // We assume step 2. If history had the bookmarks, history will write the
-  // bookmarks to a file for us. We need to wait until history has finished
-  // loading before reading from that file.
-  HistoryService* history =
-      profile_->GetHistoryService(Profile::EXPLICIT_ACCESS);
-  if (!history->backend_loaded()) {
-    // The backend isn't finished loading. Wait for it.
-    LOG(INFO) << " waiting for history to finish";
-
-    waiting_for_history_load_ = true;
-    NotificationService::current()->AddObserver(
-        this, NotificationType::HISTORY_LOADED, Source<Profile>(profile_));
-  } else {
-    OnHistoryDone();
-  }
-}
-
-void BookmarkModel::OnHistoryDone() {
-  if (loaded_) {
-    NOTREACHED();
-    return;
-  }
-
-  LOG(INFO) << " history done, reloading";
-
-  // If the bookmarks were stored in the db the db will have migrated them to
-  // a file now. Try loading from the file.
-  store_->LoadBookmarks(true);
-}
-
 void BookmarkModel::DoneLoading() {
+  DCHECK(!loaded_);
+
   {
     AutoLock url_lock(url_lock_);
     // Update nodes_ordered_by_url_set_ from the nodes.
@@ -718,18 +656,6 @@ void BookmarkModel::Observe(NotificationType type,
       break;
     }
 
-    case NotificationType::HISTORY_LOADED: {
-      if (waiting_for_history_load_) {
-        waiting_for_history_load_ = false;
-        NotificationService::current()->RemoveObserver(
-            this,NotificationType::HISTORY_LOADED, Source<Profile>(profile_));
-        OnHistoryDone();
-      } else {
-        NOTREACHED();
-      }
-      break;
-    }
-
     default:
       NOTREACHED();
       break;
diff --git a/chrome/browser/bookmarks/bookmark_model.h b/chrome/browser/bookmarks/bookmark_model.h
index e406e27..4b987d9 100644
--- a/chrome/browser/bookmarks/bookmark_model.h
+++ b/chrome/browser/bookmarks/bookmark_model.h
@@ -339,24 +339,6 @@ class BookmarkModel : public NotificationObserver, public BookmarkService {
   // This does NOT delete the node.
   void RemoveNode(BookmarkNode* node, std::set<GURL>* removed_urls);
 
-  // Callback from BookmarkStorage that it has finished loading. This method
-  // may be hit twice. In particular, on construction BookmarkModel asks
-  // BookmarkStorage to load the bookmarks. BookmarkStorage invokes this method
-  // with loaded_from_history false and file_exists indicating whether the
-  // bookmarks file exists. If the file doesn't exist, we query history. When
-  // history calls us back (OnHistoryDone) we then ask BookmarkStorage to load
-  // from the migration file. BookmarkStorage again invokes this method, but
-  // with |loaded_from_history| true.
-  void OnBookmarkStorageLoadedBookmarks(bool file_exists,
-                                        bool loaded_from_history);
-
-  // Used for migrating bookmarks from history to standalone file.
-  //
-  // Callback from history that it is done with an empty request. This is used
-  // if there is no bookmarks file. Once done, we attempt to load from the
-  // temporary file creating during migration.
-  void OnHistoryDone();
-
   // Invoked when loading is finished. Sets loaded_ and notifies observers.
   void DoneLoading();
 
@@ -444,11 +426,6 @@ class BookmarkModel : public NotificationObserver, public BookmarkService {
   // Reads/writes bookmarks to disk.
   scoped_refptr<BookmarkStorage> store_;
 
-  // Have we installed a listener on the NotificationService for
-  // NOTIFY_HISTORY_LOADED? A listener is installed if the bookmarks file
-  // doesn't exist and the history service hasn't finished loading.
-  bool waiting_for_history_load_;
-
   base::WaitableEvent loaded_signal_;
 
   DISALLOW_COPY_AND_ASSIGN(BookmarkModel);
diff --git a/chrome/browser/bookmarks/bookmark_storage.cc b/chrome/browser/bookmarks/bookmark_storage.cc
index e529b43..7df18cd 100644
--- a/chrome/browser/bookmarks/bookmark_storage.cc
+++ b/chrome/browser/bookmarks/bookmark_storage.cc
@@ -9,172 +9,240 @@
 #include "base/json_writer.h"
 #include "base/message_loop.h"
 #include "base/thread.h"
+#include "base/time.h"
 #include "chrome/browser/bookmarks/bookmark_codec.h"
 #include "chrome/browser/bookmarks/bookmark_model.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/profile.h"
 #include "chrome/common/chrome_constants.h"
 #include "chrome/common/json_value_serializer.h"
+#include "chrome/common/notification_source.h"
+#include "chrome/common/notification_type.h"
 
 namespace {
 
 // Extension used for backup files (copy of main file created during startup).
 const FilePath::CharType kBackupExtension[] = FILE_PATH_LITERAL("bak");
 
-// Extension for the temporary file. We write to the temp file than move to
-// kBookmarksFileName.
-const FilePath::CharType kTmpExtension[] = FILE_PATH_LITERAL("tmp");
-
 // How often we save.
 const int kSaveDelayMS = 2500;
 
+class BackupTask : public Task {
+ public:
+  explicit BackupTask(const FilePath& path) : path_(path) {
+  }
+
+  virtual void Run() {
+    FilePath backup_path = path_.ReplaceExtension(kBackupExtension);
+    file_util::CopyFile(path_, backup_path);
+  }
+
+ private:
+  const FilePath path_;
+
+  DISALLOW_COPY_AND_ASSIGN(BackupTask);
+};
+
+class FileDeleteTask : public Task {
+ public:
+  explicit FileDeleteTask(const FilePath& path) : path_(path) {
+  }
+
+  virtual void Run() {
+    file_util::Delete(path_, true);
+  }
+
+ private:
+  const FilePath path_;
+
+  DISALLOW_COPY_AND_ASSIGN(FileDeleteTask);
+};
+
 }  // namespace
 
+class BookmarkStorage::LoadTask : public Task {
+ public:
+  LoadTask(const FilePath& path, MessageLoop* loop,
+           BookmarkStorage* storage)
+      : path_(path),
+        loop_(loop),
+        storage_(storage) {
+  }
+
+  virtual void Run() {
+    bool bookmark_file_exists = file_util::PathExists(path_);
+    Value* root = NULL;
+    if (bookmark_file_exists) {
+      JSONFileValueSerializer serializer(path_);
+      root = serializer.Deserialize(NULL);
+    }
+
+    // BookmarkStorage takes ownership of root.
+    if (loop_) {
+      loop_->PostTask(FROM_HERE, NewRunnableMethod(
+          storage_.get(), &BookmarkStorage::OnLoadFinished,
+          bookmark_file_exists, path_, root));
+    } else {
+      storage_->OnLoadFinished(bookmark_file_exists, path_, root);
+    }
+  }
+
+ private:
+  const FilePath path_;
+  MessageLoop* loop_;
+  scoped_refptr<BookmarkStorage> storage_;
+
+  DISALLOW_COPY_AND_ASSIGN(LoadTask);
+};
+
 // BookmarkStorage -------------------------------------------------------------
 
 BookmarkStorage::BookmarkStorage(Profile* profile, BookmarkModel* model)
-    : model_(model),
-      ALLOW_THIS_IN_INITIALIZER_LIST(save_factory_(this)),
-      backend_thread_(g_browser_process->file_thread()) {
-  FilePath path = profile->GetPath().Append(chrome::kBookmarksFileName);
-  FilePath tmp_history_path =
-      profile->GetPath().Append(chrome::kHistoryBookmarksFileName);
-  backend_ = new BookmarkStorageBackend(path, tmp_history_path);
+    : profile_(profile),
+      model_(model),
+      backend_thread_(g_browser_process->file_thread()),
+      writer_(profile->GetPath().Append(chrome::kBookmarksFileName),
+              backend_thread_),
+      tmp_history_path_(
+          profile->GetPath().Append(chrome::kHistoryBookmarksFileName)) {
+  writer_.set_commit_interval(base::TimeDelta::FromMilliseconds(kSaveDelayMS));
+  RunTaskOnBackendThread(new BackupTask(writer_.path()));
+}
+
+BookmarkStorage::~BookmarkStorage() {
+  if (writer_.HasPendingWrite())
+    writer_.DoScheduledWrite();
+}
+
+void BookmarkStorage::LoadBookmarks() {
+  DoLoadBookmarks(writer_.path());
+}
+
+void BookmarkStorage::DoLoadBookmarks(const FilePath& path) {
+  Task* task = new LoadTask(path,
+                            backend_thread() ? MessageLoop::current() : NULL,
+                            this);
+  RunTaskOnBackendThread(task);
 }
 
-void BookmarkStorage::LoadBookmarks(bool load_from_history) {
-  if (!backend_thread()) {
-    backend_->Read(scoped_refptr<BookmarkStorage>(this), NULL,
-                   load_from_history);
+void BookmarkStorage::MigrateFromHistory() {
+  // We need to wait until history has finished loading before reading
+  // from generated bookmarks file.
+  HistoryService* history =
+      profile_->GetHistoryService(Profile::EXPLICIT_ACCESS);
+  if (!history) {
+    // This happens in unit tests.
+    if (model_)
+      model_->DoneLoading();
+    return;
+  }
+  if (!history->backend_loaded()) {
+    // The backend isn't finished loading. Wait for it.
+    notification_registrar_.Add(this, NotificationType::HISTORY_LOADED,
+                                Source<Profile>(profile_));
   } else {
-    backend_thread()->message_loop()->PostTask(
-        FROM_HERE,
-        NewRunnableMethod(backend_.get(), &BookmarkStorageBackend::Read,
-                          scoped_refptr<BookmarkStorage>(this),
-                          MessageLoop::current(), load_from_history));
+    OnHistoryFinishedWriting();
   }
 }
 
+void BookmarkStorage::OnHistoryFinishedWriting() {
+  notification_registrar_.Remove(this, NotificationType::HISTORY_LOADED,
+                                 Source<Profile>(profile_));
+
+  // This is used when migrating bookmarks data from database to file.
+  // History wrote the file for us, and now we want to load data from it.
+  DoLoadBookmarks(tmp_history_path_);
+}
+
 void BookmarkStorage::ScheduleSave() {
-  if (!backend_thread()) {
-    SaveNow();
-  } else if (save_factory_.empty()) {
-    MessageLoop::current()->PostDelayedTask(
-        FROM_HERE, save_factory_.NewRunnableMethod(&BookmarkStorage::SaveNow),
-        kSaveDelayMS);
-  }
+  writer_.ScheduleWrite(this);
 }
 
 void BookmarkStorage::BookmarkModelDeleted() {
-  if (!save_factory_.empty()) {
-    // There's a pending save. We need to save now as otherwise by the time
-    // SaveNow is invoked the model is gone.
-    save_factory_.RevokeAll();
+  // We need to save now as otherwise by the time SaveNow is invoked
+  // the model is gone.
+  if (writer_.HasPendingWrite())
     SaveNow();
-  }
   model_ = NULL;
 }
 
-void BookmarkStorage::LoadedBookmarks(Value* root_value,
-                                      bool bookmark_file_exists,
-                                      bool loaded_from_history) {
+bool BookmarkStorage::SerializeData(std::string* output) {
+  BookmarkCodec codec;
+  scoped_ptr<Value> value(codec.Encode(model_));
+  JSONStringValueSerializer serializer(output);
+  serializer.set_pretty_print(true);
+  return serializer.Serialize(*(value.get()));
+}
+
+void BookmarkStorage::OnLoadFinished(bool file_exists, const FilePath& path,
+                                     Value* root_value) {
   scoped_ptr<Value> value_ref(root_value);
 
-  if (model_) {
-    if (root_value) {
-      BookmarkCodec codec;
-      codec.Decode(model_, *root_value);
-    }
-    model_->OnBookmarkStorageLoadedBookmarks(bookmark_file_exists,
-                                             loaded_from_history);
+  if (path == writer_.path() && !file_exists) {
+    // The file doesn't exist. This means one of two things:
+    // 1. A clean profile.
+    // 2. The user is migrating from an older version where bookmarks were
+    //    saved in history.
+    // We assume step 2. If history had the bookmarks, it will write the
+    // bookmarks to a file for us.
+    MigrateFromHistory();
+    return;
   }
-}
 
-void BookmarkStorage::SaveNow() {
-  if (!model_ || !model_->IsLoaded()) {
-    // We should only get here if we have a valid model and it's finished
-    // loading.
-    NOTREACHED();
+  if (!model_)
     return;
-  }
 
-  BookmarkCodec codec;
-  Value* value = codec.Encode(model_);
-  // The backend deletes value in write.
-  if (!backend_thread()) {
-    backend_->Write(value);
-  } else {
-    backend_thread()->message_loop()->PostTask(
-        FROM_HERE,
-        NewRunnableMethod(backend_.get(), &BookmarkStorageBackend::Write,
-                          value));
+  if (root_value) {
+    BookmarkCodec codec;
+    codec.Decode(model_, *root_value);
   }
-}
 
-// BookmarkStorageBackend ------------------------------------------------------
+  model_->DoneLoading();
+
+  if (path == tmp_history_path_) {
+    // We just finished migration from history. Save now to new file,
+    // after the model is created and done loading.
+    SaveNow();
 
-BookmarkStorageBackend::BookmarkStorageBackend(
-    const FilePath& path,
-    const FilePath& tmp_history_path)
-    : path_(path),
-      tmp_history_path_(tmp_history_path) {
-  // Make a backup of the current file.
-  FilePath backup_path = path.ReplaceExtension(kBackupExtension);
-  file_util::CopyFile(path, backup_path);
+    // Clean up after migration from history.
+    RunTaskOnBackendThread(new FileDeleteTask(tmp_history_path_));
+  }
 }
 
-void BookmarkStorageBackend::Write(Value* value) {
-  DCHECK(value);
-
-  // We own Value.
-  scoped_ptr<Value> value_ref(value);
-
-  std::string content;
-  JSONWriter::Write(value, true, &content);
-
-  // Write to a temp file, then rename.
-  FilePath tmp_file = path_.ReplaceExtension(kTmpExtension);
-
-  int bytes_written = file_util::WriteFile(tmp_file, content.c_str(),
-                                           static_cast<int>(content.length()));
-  if (bytes_written != -1) {
-    if (!file_util::Move(tmp_file, path_)) {
-      // Rename failed. Try again on the off chance someone has locked either
-      // file and hope we're successful the second time through.
-      bool move_result = file_util::Move(tmp_file, path_);
-      DCHECK(move_result);
-      if (!move_result)
-        LOG(WARNING) << " writing bookmarks failed, result=" << move_result;
-      else
-        LOG(INFO) << "wrote bookmarks, file=" << path_.value();
-    } else {
-      LOG(INFO) << "wrote bookmarks, file=" << path_.value();
-    }
-    // Nuke the history file so that we don't attempt to load from it again.
-    file_util::Delete(tmp_history_path_, false);
-  } else {
-    LOG(WARNING) << " writing bookmarks failed, bytes written=" <<
-        bytes_written;
+void BookmarkStorage::Observe(NotificationType type,
+                              const NotificationSource& source,
+                              const NotificationDetails& details) {
+  switch (type.value) {
+    case NotificationType::HISTORY_LOADED:
+      OnHistoryFinishedWriting();
+      break;
+
+    default:
+      NOTREACHED();
+      break;
   }
 }
 
-void BookmarkStorageBackend::Read(scoped_refptr<BookmarkStorage> service,
-                                  MessageLoop* message_loop,
-                                  bool load_from_history) {
-  const FilePath& path = load_from_history ? tmp_history_path_ : path_;
-  bool bookmark_file_exists = file_util::PathExists(path);
-  Value* root = NULL;
-  if (bookmark_file_exists) {
-    JSONFileValueSerializer serializer(path);
-    root = serializer.Deserialize(NULL);
+bool BookmarkStorage::SaveNow() {
+  if (!model_ || !model_->IsLoaded()) {
+    // We should only get here if we have a valid model and it's finished
+    // loading.
+    NOTREACHED();
+    return false;
   }
 
-  // BookmarkStorage takes ownership of root.
-  if (message_loop) {
-    message_loop->PostTask(FROM_HERE, NewRunnableMethod(
-        service.get(), &BookmarkStorage::LoadedBookmarks, root,
-        bookmark_file_exists, load_from_history));
+  std::string data;
+  if (!SerializeData(&data))
+    return false;
+  writer_.WriteNow(data);
+  return true;
+}
+
+void BookmarkStorage::RunTaskOnBackendThread(Task* task) const {
+  if (backend_thread()) {
+    backend_thread()->message_loop()->PostTask(FROM_HERE, task);
   } else {
-    service->LoadedBookmarks(root, bookmark_file_exists, load_from_history);
+    task->Run();
+    delete task;
   }
 }
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_
diff --git a/chrome/common/important_file_writer.cc b/chrome/common/important_file_writer.cc
index 5707744..03e138c 100644
--- a/chrome/common/important_file_writer.cc
+++ b/chrome/common/important_file_writer.cc
@@ -86,22 +86,27 @@ ImportantFileWriter::ImportantFileWriter(const FilePath& path,
                                          const base::Thread* backend_thread)
     : path_(path),
       backend_thread_(backend_thread),
+      serializer_(NULL),
       commit_interval_(TimeDelta::FromMilliseconds(kDefaultCommitIntervalMs)) {
   DCHECK(CalledOnValidThread());
 }
 
 ImportantFileWriter::~ImportantFileWriter() {
+  // We're usually a member variable of some other object, which also tends
+  // to be our serializer. It may not be safe to call back to the parent object
+  // being destructed.
+  DCHECK(!HasPendingWrite());
+}
+
+bool ImportantFileWriter::HasPendingWrite() const {
   DCHECK(CalledOnValidThread());
-  if (timer_.IsRunning()) {
-    timer_.Stop();
-    CommitPendingData();
-  }
+  return timer_.IsRunning();
 }
 
 void ImportantFileWriter::WriteNow(const std::string& data) {
   DCHECK(CalledOnValidThread());
 
-  if (timer_.IsRunning())
+  if (HasPendingWrite())
     timer_.Stop();
 
   Task* task = new WriteToDiskTask(path_, data);
@@ -113,16 +118,32 @@ void ImportantFileWriter::WriteNow(const std::string& data) {
   }
 }
 
-void ImportantFileWriter::ScheduleWrite(const std::string& data) {
+void ImportantFileWriter::ScheduleWrite(DataSerializer* serializer) {
   DCHECK(CalledOnValidThread());
 
-  data_ = data;
+  DCHECK(serializer);
+  serializer_ = serializer;
+
+  if (!MessageLoop::current()) {
+    // Happens in unit tests.
+    DoScheduledWrite();
+    return;
+  }
+
   if (!timer_.IsRunning()) {
     timer_.Start(commit_interval_, this,
-                 &ImportantFileWriter::CommitPendingData);
+                 &ImportantFileWriter::DoScheduledWrite);
   }
 }
 
-void ImportantFileWriter::CommitPendingData() {
-  WriteNow(data_);
+void ImportantFileWriter::DoScheduledWrite() {
+  DCHECK(serializer_);
+  std::string data;
+  if (serializer_->SerializeData(&data)) {
+    WriteNow(data);
+  } else {
+    LOG(WARNING) << "failed to serialize data to be saved in "
+                 << path_.value();
+  }
+  serializer_ = NULL;
 }
diff --git a/chrome/common/important_file_writer.h b/chrome/common/important_file_writer.h
index 6a854bb..623322a 100644
--- a/chrome/common/important_file_writer.h
+++ b/chrome/common/important_file_writer.h
@@ -35,6 +35,18 @@ class Thread;
 // http://valhenson.livejournal.com/37921.html
 class ImportantFileWriter : public NonThreadSafe {
  public:
+  // Used by ScheduleSave to lazily provide the data to be saved. Allows us
+  // to also batch data serializations.
+  class DataSerializer {
+   public:
+    virtual ~DataSerializer() {}
+
+    // Should put serialized string in |data| and return true on successful
+    // serialization. Will be called on the same thread on which
+    // ImportantFileWriter has been created.
+    virtual bool SerializeData(std::string* data) = 0;
+  };
+
   // Initialize the writer.
   // |path| is the name of file to write. Disk operations will be executed on
   // |backend_thread|, or current thread if |backend_thread| is NULL.
@@ -42,20 +54,30 @@ class ImportantFileWriter : public NonThreadSafe {
   // All non-const methods, ctor and dtor must be called on the same thread.
   ImportantFileWriter(const FilePath& path, const base::Thread* backend_thread);
 
-  // On destruction all pending writes are executed on |backend_thread|.
+  // You have to ensure that there are no pending writes at the moment
+  // of destruction.
   ~ImportantFileWriter();
 
   FilePath path() const { return path_; }
 
+  // Returns true if there is a scheduled write pending which has not yet
+  // been started.
+  bool HasPendingWrite() const;
+
   // Save |data| to target filename. Does not block. If there is a pending write
   // scheduled by ScheduleWrite, it is cancelled.
   void WriteNow(const std::string& data);
 
-  // Schedule saving |data| to target filename. Data will be saved to disk after
-  // the commit interval. If an other ScheduleWrite is issued before that, only
-  // one write to disk will happen - with the more recent data. This operation
-  // does not block.
-  void ScheduleWrite(const std::string& data);
+  // Schedule a save to target filename. Data will be serialized and saved
+  // to disk after the commit interval. If another ScheduleWrite is issued
+  // before that, only one serialization and write to disk will happen, and
+  // the most recent |serializer| will be used. This operation does not block.
+  // |serializer| should remain valid through the lifetime of
+  // ImportantFileWriter.
+  void ScheduleWrite(DataSerializer* serializer);
+
+  // Serialize data pending to be saved and execute write on backend thread.
+  void DoScheduledWrite();
 
   base::TimeDelta commit_interval() const {
     return commit_interval_;
@@ -66,9 +88,6 @@ class ImportantFileWriter : public NonThreadSafe {
   }
 
  private:
-  // If there is a data scheduled to write, issue a disk operation.
-  void CommitPendingData();
-
   // Path being written to.
   const FilePath path_;
 
@@ -78,8 +97,8 @@ class ImportantFileWriter : public NonThreadSafe {
   // Timer used to schedule commit after ScheduleWrite.
   base::OneShotTimer<ImportantFileWriter> timer_;
 
-  // Data scheduled to save.
-  std::string data_;
+  // Serializer which will provide the data to be saved.
+  DataSerializer* serializer_;
 
   // Time delta after which scheduled data will be written to disk.
   base::TimeDelta commit_interval_;
diff --git a/chrome/common/important_file_writer_unittest.cc b/chrome/common/important_file_writer_unittest.cc
index 09703a5..e4ce5f0 100644
--- a/chrome/common/important_file_writer_unittest.cc
+++ b/chrome/common/important_file_writer_unittest.cc
@@ -24,6 +24,20 @@ std::string GetFileContent(const FilePath& path) {
   return content;
 }
 
+class DataSerializer : public ImportantFileWriter::DataSerializer {
+ public:
+  explicit DataSerializer(const std::string& data) : data_(data) {
+  }
+
+  virtual bool SerializeData(std::string* output) {
+    output->assign(data_);
+    return true;
+  }
+
+ private:
+  const std::string data_;
+};
+
 }  // namespace
 
 class ImportantFileWriterTest : public testing::Test {
@@ -65,10 +79,26 @@ TEST_F(ImportantFileWriterTest, WithBackendThread) {
 TEST_F(ImportantFileWriterTest, ScheduleWrite) {
   ImportantFileWriter writer(file_, NULL);
   writer.set_commit_interval(base::TimeDelta::FromMilliseconds(25));
-  writer.ScheduleWrite("foo");
+  EXPECT_FALSE(writer.HasPendingWrite());
+  DataSerializer serializer("foo");
+  writer.ScheduleWrite(&serializer);
+  EXPECT_TRUE(writer.HasPendingWrite());
   MessageLoop::current()->PostDelayedTask(FROM_HERE,
                                           new MessageLoop::QuitTask(), 100);
   MessageLoop::current()->Run();
+  EXPECT_FALSE(writer.HasPendingWrite());
+  ASSERT_TRUE(file_util::PathExists(writer.path()));
+  EXPECT_EQ("foo", GetFileContent(writer.path()));
+}
+
+TEST_F(ImportantFileWriterTest, DoScheduledWrite) {
+  ImportantFileWriter writer(file_, NULL);
+  EXPECT_FALSE(writer.HasPendingWrite());
+  DataSerializer serializer("foo");
+  writer.ScheduleWrite(&serializer);
+  EXPECT_TRUE(writer.HasPendingWrite());
+  writer.DoScheduledWrite();
+  EXPECT_FALSE(writer.HasPendingWrite());
   ASSERT_TRUE(file_util::PathExists(writer.path()));
   EXPECT_EQ("foo", GetFileContent(writer.path()));
 }
@@ -76,21 +106,13 @@ TEST_F(ImportantFileWriterTest, ScheduleWrite) {
 TEST_F(ImportantFileWriterTest, BatchingWrites) {
   ImportantFileWriter writer(file_, NULL);
   writer.set_commit_interval(base::TimeDelta::FromMilliseconds(25));
-  writer.ScheduleWrite("foo");
-  writer.ScheduleWrite("bar");
-  writer.ScheduleWrite("baz");
+  DataSerializer foo("foo"), bar("bar"), baz("baz");
+  writer.ScheduleWrite(&foo);
+  writer.ScheduleWrite(&bar);
+  writer.ScheduleWrite(&baz);
   MessageLoop::current()->PostDelayedTask(FROM_HERE,
                                           new MessageLoop::QuitTask(), 100);
   MessageLoop::current()->Run();
   ASSERT_TRUE(file_util::PathExists(writer.path()));
   EXPECT_EQ("baz", GetFileContent(writer.path()));
 }
-
-TEST_F(ImportantFileWriterTest, WriteOnDestruction) {
-  {
-    ImportantFileWriter writer(file_, NULL);
-    writer.ScheduleWrite("foo");
-  }
-  ASSERT_TRUE(file_util::PathExists(file_));
-  EXPECT_EQ("foo", GetFileContent(file_));
-}
diff --git a/chrome/common/pref_service.cc b/chrome/common/pref_service.cc
index 85abb4f..4ceca35 100644
--- a/chrome/common/pref_service.cc
+++ b/chrome/common/pref_service.cc
@@ -84,6 +84,9 @@ PrefService::~PrefService() {
   STLDeleteContainerPairSecondPointers(pref_observers_.begin(),
                                        pref_observers_.end());
   pref_observers_.clear();
+
+  if (writer_.HasPendingWrite())
+    writer_.DoScheduledWrite();
 }
 
 bool PrefService::ReloadPersistentPrefs() {
@@ -111,22 +114,16 @@ bool PrefService::SavePersistentPrefs() {
   DCHECK(CalledOnValidThread());
 
   std::string data;
-  if (!SerializePrefData(&data))
+  if (!SerializeData(&data))
     return false;
 
   writer_.WriteNow(data);
   return true;
 }
 
-bool PrefService::ScheduleSavePersistentPrefs() {
+void PrefService::ScheduleSavePersistentPrefs() {
   DCHECK(CalledOnValidThread());
-
-  std::string data;
-  if (!SerializePrefData(&data))
-    return false;
-
-  writer_.ScheduleWrite(data);
-  return true;
+  writer_.ScheduleWrite(this);
 }
 
 void PrefService::RegisterBooleanPref(const wchar_t* path,
@@ -644,7 +641,7 @@ void PrefService::FireObservers(const wchar_t* path) {
   }
 }
 
-bool PrefService::SerializePrefData(std::string* output) const {
+bool PrefService::SerializeData(std::string* output) {
   // TODO(tc): Do we want to prune webkit preferences that match the default
   // value?
   JSONStringValueSerializer serializer(output);
diff --git a/chrome/common/pref_service.h b/chrome/common/pref_service.h
index b60f497..c4f7ce6 100644
--- a/chrome/common/pref_service.h
+++ b/chrome/common/pref_service.h
@@ -32,7 +32,8 @@ namespace base {
 class Thread;
 }
 
-class PrefService : public NonThreadSafe {
+class PrefService : public NonThreadSafe,
+                    public ImportantFileWriter::DataSerializer {
  public:
 
   // A helper class to store all the information associated with a preference.
@@ -96,7 +97,7 @@ class PrefService : public NonThreadSafe {
   bool SavePersistentPrefs();
 
   // Serializes the data and schedules save using ImportantFileWriter.
-  bool ScheduleSavePersistentPrefs();
+  void ScheduleSavePersistentPrefs();
 
   DictionaryValue* transient() { return transient_.get(); }
 
@@ -191,6 +192,9 @@ class PrefService : public NonThreadSafe {
   // preference is not registered.
   const Preference* FindPreference(const wchar_t* pref_name) const;
 
+  // ImportantFileWriter::DataSerializer
+  virtual bool SerializeData(std::string* output);
+
  private:
   // Add a preference to the PreferenceMap.  If the pref already exists, return
   // false.  This method takes ownership of |pref|.
@@ -208,10 +212,6 @@ class PrefService : public NonThreadSafe {
   void FireObserversIfChanged(const wchar_t* pref_name,
                               const Value* old_value);
 
-  // Serializes stored data to string. |output| is modified only
-  // if serialization was successful. Returns true on success.
-  bool SerializePrefData(std::string* output) const;
-
   scoped_ptr<DictionaryValue> persistent_;
   scoped_ptr<DictionaryValue> transient_;