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

5 files changed, 109 insertions, 50 deletions

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_;