Moved JsonPrefStore to use SequencedWorkerPool

chrome/common/json_pref_store.h

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #ifndef CHROME_COMMON_JSON_PREF_STORE_H_
 #define CHROME_COMMON_JSON_PREF_STORE_H_
 
 #include <set>
 #include <string>
 
 #include "base/basictypes.h"
 #include "base/compiler_specific.h"
 #include "base/file_path.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/observer_list.h"
 #include "chrome/common/important_file_writer.h"
 #include "chrome/common/persistent_pref_store.h"
 
 namespace base {
 class DictionaryValue;
-class MessageLoopProxy;
+class SequencedWorkerPool;
+class TaskRunner;
 class Value;
 }
 
 class FilePath;
 
 // A writable PrefStore implementation that is used for user preferences.
 class JsonPrefStore : public PersistentPrefStore,
                       public ImportantFileWriter::DataSerializer {
  public:
-  // |file_message_loop_proxy| is the MessageLoopProxy for a thread on which
-  // file I/O can be done.
-  JsonPrefStore(const FilePath& pref_filename,
-                base::MessageLoopProxy* file_message_loop_proxy);
+  // |worker_pool| is the SequencedWorkerPool on which file I/O can be done.
+  // The worker pool will also ensure that write requests for the same file are
+  // sequenced properly and that such request block shutdown.
+  static JsonPrefStore* Create(const FilePath& pref_filename,

[willchan no longer on Chromium, 2012/10/05 22:38:34]

Do we have many other places that use the 2nd Create() instead of this one? If
not, why not just provide this one?

The thing is, if you provide the second one, then people can pass in a
sequenced_worker_pool->GetTaskRunnerWithShutdownBehavior(value !=
BLOCK_SHUTDOWN). If you want to make sure it's BLOCK_SHUTDOWN, then you should
only provide this one. If you need the second one, then it may make sense to get
rid of this one and have people that want the BLOCK_SHUTDOWN behavior to simply
call sequenced_worker_pool->GetTaskRunnerWithShutdownBehavior(BLOCK_SHUTDOWN)
themselves.

Overloading functions is disallowed by the style guide if behaviors differ among
the overloaded functions, which arguably seems to be the case here (although
you're using the default shutdown behavior, so maybe it's obvious).

[zel, 2012/10/10 01:27:50]

Yes, FILE thread flavor was still used from ServiceProcess. I've modified that
path (+few unit tests), added blocking pool there too. It's removed now, only
one Create() here.

+                               base::SequencedWorkerPool* worker_pool);
+
+  // |task_runner| is the TaskRunner for a thread on which file I/O can be done.
+  static JsonPrefStore* Create(const FilePath& pref_filename,
+                               base::TaskRunner* task_runner);

[willchan no longer on Chromium, 2012/10/05 22:38:34]

I think you want a SequencedTaskRunner. TaskRunner allows multiple tasks to run
at the same time, which I suspect would be buggy here.

[zel, 2012/10/10 01:27:50]

Done.

 
   // PrefStore overrides:
   virtual ReadResult GetValue(const std::string& key,
                               const base::Value** result) const OVERRIDE;
   virtual void AddObserver(PrefStore::Observer* observer) OVERRIDE;
   virtual void RemoveObserver(PrefStore::Observer* observer) OVERRIDE;
   virtual size_t NumberOfObservers() const OVERRIDE;
   virtual bool IsInitializationComplete() const OVERRIDE;
 
   // PersistentPrefStore overrides:
@@ skipping 11 matching lines @@
   virtual void CommitPendingWrite() OVERRIDE;
   virtual void ReportValueChanged(const std::string& key) OVERRIDE;
 
   // This method is called after JSON file has been read. Method takes
   // ownership of the |value| pointer. Note, this method is used with
   // asynchronous file reading, so class exposes it only for the internal needs.
   // (read: do not call it manually).
   void OnFileRead(base::Value* value_owned, PrefReadError error, bool no_dir);
 
  private:
+  // |task_runner| is the TaskRunner for a thread on which file I/O can be done.
+  JsonPrefStore(const FilePath& pref_filename,
+                base::TaskRunner* task_runner);
   virtual ~JsonPrefStore();
 
   // ImportantFileWriter::DataSerializer overrides:
   virtual bool SerializeData(std::string* output) OVERRIDE;
 
   FilePath path_;
-  scoped_refptr<base::MessageLoopProxy> file_message_loop_proxy_;
+  scoped_refptr<base::TaskRunner> task_runner_;

[willchan no longer on Chromium, 2012/10/05 22:38:34]

SequencedTaskRunner.

[zel, 2012/10/10 01:27:50]

Done.

 
   scoped_ptr<base::DictionaryValue> prefs_;
 
   bool read_only_;
 
   // Helper for safely writing pref data.
   ImportantFileWriter writer_;
 
   ObserverList<PrefStore::Observer, true> observers_;
 
   scoped_ptr<ReadErrorDelegate> error_delegate_;
 
   bool initialized_;
   PrefReadError read_error_;
 
   std::set<std::string> keys_need_empty_value_;
 
   DISALLOW_COPY_AND_ASSIGN(JsonPrefStore);
 };
 
 #endif  // CHROME_COMMON_JSON_PREF_STORE_H_