Moved JsonPrefStore to use SequencedWorkerPool

chrome/common/json_pref_store.h

Index: chrome/common/json_pref_store.h
diff --git a/chrome/common/json_pref_store.h b/chrome/common/json_pref_store.h
index 7ceb7dad2ac0c472e51a1405e58861e6fd6ad93a..a9a61be7cc23c49f54fe2e31c61706d6d008cfdd 100644
--- a/chrome/common/json_pref_store.h
+++ b/chrome/common/json_pref_store.h
@@ -18,7 +18,8 @@
 
 namespace base {
 class DictionaryValue;
-class MessageLoopProxy;
+class SequencedWorkerPool;
+class TaskRunner;
 class Value;
 }
 
@@ -28,10 +29,15 @@ class FilePath;
 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,
@@ -63,13 +69,16 @@ class JsonPrefStore : public PersistentPrefStore,
   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_;