Add MoveToThread method to PrefMember to make it safe to read pref values from other threads.

BUG=73385
TEST=PrefMemberTest.MoveToThread

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

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

4 files changed, 237 insertions, 142 deletions

diff --git a/chrome/browser/prefs/pref_member.cc b/chrome/browser/prefs/pref_member.cc
index 5ff0f0d..db17123 100644
--- a/chrome/browser/prefs/pref_member.cc
+++ b/chrome/browser/prefs/pref_member.cc
@@ -5,6 +5,8 @@
 #include "chrome/browser/prefs/pref_member.h"
 
 #include "base/logging.h"
+#include "base/sys_string_conversions.h"
+#include "base/utf_string_conversions.h"
 #include "chrome/browser/prefs/pref_service.h"
 #include "chrome/common/notification_type.h"
 
@@ -13,7 +15,6 @@ namespace subtle {
 PrefMemberBase::PrefMemberBase()
     : observer_(NULL),
       prefs_(NULL),
-      is_synced_(false),
       setting_value_(false) {
 }
 
@@ -33,7 +34,7 @@ void PrefMemberBase::Init(const char* pref_name, PrefService* prefs,
   pref_name_ = pref_name;
   DCHECK(!pref_name_.empty());
 
-  // Add ourself as a pref observer so we can keep our local value in sync.
+  // Add ourselves as a pref observer so we can keep our local value in sync.
   prefs_->AddPrefObserver(pref_name, this);
 }
 
@@ -45,19 +46,26 @@ void PrefMemberBase::Destroy() {
 }
 
 bool PrefMemberBase::IsManaged() const {
-  DCHECK(!pref_name_.empty());
+  VerifyValuePrefName();
   const PrefService::Preference* pref =
       prefs_->FindPreference(pref_name_.c_str());
   return pref && pref->IsManaged();
 }
 
+void PrefMemberBase::MoveToThread(BrowserThread::ID thread_id) {
+  VerifyValuePrefName();
+  // Load the value from preferences if it hasn't been loaded so far.
+  if (!internal())
+    UpdateValueFromPref();
+  internal()->MoveToThread(thread_id);
+}
+
 void PrefMemberBase::Observe(NotificationType type,
                              const NotificationSource& source,
                              const NotificationDetails& details) {
-  DCHECK(!pref_name_.empty());
+  VerifyValuePrefName();
   DCHECK(NotificationType::PREF_CHANGED == type);
   UpdateValueFromPref();
-  is_synced_ = true;
   if (!setting_value_ && observer_)
     observer_->Observe(type, source, details);
 }
@@ -66,74 +74,98 @@ void PrefMemberBase::VerifyValuePrefName() const {
   DCHECK(!pref_name_.empty());
 }
 
-}  // namespace subtle
-
-BooleanPrefMember::BooleanPrefMember() : PrefMember<bool>() {
+void PrefMemberBase::UpdateValueFromPref() const {
+  VerifyValuePrefName();
+  const PrefService::Preference* pref =
+      prefs_->FindPreference(pref_name_.c_str());
+  DCHECK(pref);
+  if (!internal())
+    CreateInternal();
+  internal()->UpdateValue(pref->GetValue()->DeepCopy());
+}
+
+PrefMemberBase::Internal::Internal() : thread_id_(BrowserThread::UI) { }
+PrefMemberBase::Internal::~Internal() { }
+
+bool PrefMemberBase::Internal::IsOnCorrectThread() const {
+  // In unit tests, there may not be a UI thread.
+  return (BrowserThread::CurrentlyOn(thread_id_) ||
+          (thread_id_ == BrowserThread::UI &&
+           !BrowserThread::IsMessageLoopValid(BrowserThread::UI)));
+}
+
+void PrefMemberBase::Internal::UpdateValue(Value* v) const {
+  scoped_ptr<Value> value(v);
+  if (IsOnCorrectThread()) {
+    bool rv = UpdateValueInternal(*value);
+    DCHECK(rv);
+  } else {
+    bool rv = BrowserThread::PostTask(
+        thread_id_, FROM_HERE,
+        NewRunnableMethod(this,
+                          &PrefMemberBase::Internal::UpdateValue,
+                          value.release()));
+    DCHECK(rv);
+  }
 }
 
-BooleanPrefMember::~BooleanPrefMember() {
+void PrefMemberBase::Internal::MoveToThread(BrowserThread::ID thread_id) {
+  CheckOnCorrectThread();
+  thread_id_ = thread_id;
 }
 
-void BooleanPrefMember::UpdateValueFromPref() const {
-  value_ = prefs()->GetBoolean(pref_name().c_str());
-}
+}  // namespace subtle
 
-void BooleanPrefMember::UpdatePref(const bool& value) {
+template <>
+void PrefMember<bool>::UpdatePref(const bool& value) {
   prefs()->SetBoolean(pref_name().c_str(), value);
 }
 
-IntegerPrefMember::IntegerPrefMember() : PrefMember<int>() {
-}
-
-IntegerPrefMember::~IntegerPrefMember() {
+template <>
+bool PrefMember<bool>::Internal::UpdateValueInternal(const Value& value) const {
+  return value.GetAsBoolean(&value_);
 }
 
-void IntegerPrefMember::UpdateValueFromPref() const {
-  value_ = prefs()->GetInteger(pref_name().c_str());
-}
-
-void IntegerPrefMember::UpdatePref(const int& value) {
+template <>
+void PrefMember<int>::UpdatePref(const int& value) {
   prefs()->SetInteger(pref_name().c_str(), value);
 }
 
-DoublePrefMember::DoublePrefMember() : PrefMember<double>() {
-}
-
-DoublePrefMember::~DoublePrefMember() {
+template <>
+bool PrefMember<int>::Internal::UpdateValueInternal(const Value& value) const {
+  return value.GetAsInteger(&value_);
 }
 
-void DoublePrefMember::UpdateValueFromPref() const {
-  value_ = prefs()->GetDouble(pref_name().c_str());
-}
-
-void DoublePrefMember::UpdatePref(const double& value) {
+template <>
+void PrefMember<double>::UpdatePref(const double& value) {
   prefs()->SetDouble(pref_name().c_str(), value);
 }
 
-StringPrefMember::StringPrefMember() : PrefMember<std::string>() {
+template <>
+bool PrefMember<double>::Internal::UpdateValueInternal(const Value& value)
+    const {
+  return value.GetAsDouble(&value_);
 }
 
-StringPrefMember::~StringPrefMember() {
-}
-
-void StringPrefMember::UpdateValueFromPref() const {
-  value_ = prefs()->GetString(pref_name().c_str());
-}
-
-void StringPrefMember::UpdatePref(const std::string& value) {
+template <>
+void PrefMember<std::string>::UpdatePref(const std::string& value) {
   prefs()->SetString(pref_name().c_str(), value);
 }
 
-FilePathPrefMember::FilePathPrefMember() : PrefMember<FilePath>() {
+template <>
+bool PrefMember<std::string>::Internal::UpdateValueInternal(const Value& value)
+    const {
+  return value.GetAsString(&value_);
 }
 
-FilePathPrefMember::~FilePathPrefMember() {
+template <>
+void PrefMember<FilePath>::UpdatePref(const FilePath& value) {
+  prefs()->SetFilePath(pref_name().c_str(), value);
 }
 
-void FilePathPrefMember::UpdateValueFromPref() const {
-  value_ = prefs()->GetFilePath(pref_name().c_str());
+template <>
+bool PrefMember<FilePath>::Internal::UpdateValueInternal(const Value& value)
+    const {
+  return value.GetAsFilePath(&value_);
 }
 
-void FilePathPrefMember::UpdatePref(const FilePath& value) {
-  prefs()->SetFilePath(pref_name().c_str(), value);
-}
diff --git a/chrome/browser/prefs/pref_member.h b/chrome/browser/prefs/pref_member.h
index facaa1b..0dc0e71 100644
--- a/chrome/browser/prefs/pref_member.h
+++ b/chrome/browser/prefs/pref_member.h
@@ -29,6 +29,9 @@
 
 #include "base/basictypes.h"
 #include "base/file_path.h"
+#include "base/ref_counted.h"
+#include "base/values.h"
+#include "chrome/browser/browser_thread.h"
 #include "chrome/common/notification_observer.h"
 
 class PrefService;
@@ -37,6 +40,37 @@ namespace subtle {
 
 class PrefMemberBase : public NotificationObserver {
  protected:
+  class Internal : public base::RefCountedThreadSafe<Internal> {
+   public:
+    Internal();
+
+    // Update the value, either by calling |UpdateValueInternal| directly
+    // or by dispatching to the right thread.
+    // Takes ownership of |value|.
+    virtual void UpdateValue(Value* value) const;
+
+    void MoveToThread(BrowserThread::ID thread_id);
+
+   protected:
+    friend class base::RefCountedThreadSafe<Internal>;
+    virtual ~Internal();
+
+    void CheckOnCorrectThread() const {
+      DCHECK(IsOnCorrectThread());
+    }
+
+   private:
+    // This method actually updates the value. It should only be called from
+    // the thread the PrefMember is on.
+    virtual bool UpdateValueInternal(const Value& value) const = 0;
+
+    bool IsOnCorrectThread() const;
+
+    BrowserThread::ID thread_id_;
+
+    DISALLOW_COPY_AND_ASSIGN(Internal);
+  };
+
   PrefMemberBase();
   virtual ~PrefMemberBase();
 
@@ -44,12 +78,16 @@ class PrefMemberBase : public NotificationObserver {
   void Init(const char* pref_name, PrefService* prefs,
             NotificationObserver* observer);
 
+  virtual void CreateInternal() const = 0;
+
   // See PrefMember<> for description.
   void Destroy();
 
   // See PrefMember<> for description.
   bool IsManaged() const;
 
+  void MoveToThread(BrowserThread::ID thread_id);
+
   // NotificationObserver
   virtual void Observe(NotificationType type,
                        const NotificationSource& source,
@@ -57,15 +95,17 @@ class PrefMemberBase : public NotificationObserver {
 
   void VerifyValuePrefName() const;
 
-  // This methods is used to do the actual sync with pref of the specified type.
-  // Note: this method is logically const, because it doesn't modify the state
+  // This method is used to do the actual sync with the preference.
+  // Note: it is logically const, because it doesn't modify the state
   // seen by the outside world. It is just doing a lazy load behind the scenes.
-  virtual void UpdateValueFromPref() const = 0;
+  virtual void UpdateValueFromPref() const;
 
   const std::string& pref_name() const { return pref_name_; }
   PrefService* prefs() { return prefs_; }
   const PrefService* prefs() const { return prefs_; }
 
+  virtual Internal* internal() const = 0;
+
  // Ordered the members to compact the class instance.
  private:
   std::string pref_name_;
@@ -73,23 +113,22 @@ class PrefMemberBase : public NotificationObserver {
   PrefService* prefs_;
 
  protected:
-  mutable bool is_synced_;
   bool setting_value_;
 };
 
 }  // namespace subtle
 
-
 template <typename ValueType>
 class PrefMember : public subtle::PrefMemberBase {
  public:
   // Defer initialization to an Init method so it's easy to make this class be
   // a member variable.
-  PrefMember() : value_(ValueType()) {}
+  PrefMember() {}
   virtual ~PrefMember() {}
 
   // Do the actual initialization of the class.  |observer| may be null if you
   // don't want any notifications of changes.
+  // This method should only be called on the UI thread.
   void Init(const char* pref_name, PrefService* prefs,
             NotificationObserver* observer) {
     subtle::PrefMemberBase::Init(pref_name, prefs, observer);
@@ -97,27 +136,38 @@ class PrefMember : public subtle::PrefMemberBase {
 
   // Unsubscribes the PrefMember from the PrefService. After calling this
   // function, the PrefMember may not be used any more.
+  // This method should only be called on the UI thread.
   void Destroy() {
     subtle::PrefMemberBase::Destroy();
   }
 
+  // Moves the PrefMember to another thread, allowing read accesses from there.
+  // Changes from the PrefService will be propagated asynchronously
+  // via PostTask.
+  // This method should only be used from the thread the PrefMember is currently
+  // on, which is the UI thread by default.
+  void MoveToThread(BrowserThread::ID thread_id) {
+    subtle::PrefMemberBase::MoveToThread(thread_id);
+  }
+
   // Check whether the pref is managed, i.e. controlled externally through
   // enterprise configuration management (e.g. windows group policy). Returns
   // false for unknown prefs.
+  // This method should only be called on the UI thread.
   bool IsManaged() const {
     return subtle::PrefMemberBase::IsManaged();
   }
 
   // Retrieve the value of the member variable.
+  // This method should only be used from the thread the PrefMember is currently
+  // on, which is the UI thread unless changed by |MoveToThread|.
   ValueType GetValue() const {
     VerifyValuePrefName();
     // We lazily fetch the value from the pref service the first time GetValue
     // is called.
-    if (!is_synced_) {
+    if (!internal_.get())
       UpdateValueFromPref();
-      is_synced_ = true;
-    }
-    return value_;
+    return internal_->value();
   }
 
   // Provided as a convenience.
@@ -126,6 +176,7 @@ class PrefMember : public subtle::PrefMemberBase {
   }
 
   // Set the value of the member variable.
+  // This method should only be called on the UI thread.
   void SetValue(const ValueType& value) {
     VerifyValuePrefName();
     setting_value_ = true;
@@ -134,87 +185,52 @@ class PrefMember : public subtle::PrefMemberBase {
   }
 
   // Set the value of the member variable if it is not managed.
+  // This method should only be called on the UI thread.
   void SetValueIfNotManaged(const ValueType& value) {
     if (!IsManaged()) {
       SetValue(value);
     }
   }
 
- protected:
-  // This methods is used to do the actual sync with pref of the specified type.
-  virtual void UpdatePref(const ValueType& value) = 0;
-
-  // We cache the value of the pref so we don't have to keep walking the pref
-  // tree.
-  mutable ValueType value_;
-};
-
-///////////////////////////////////////////////////////////////////////////////
-// Implementations of Boolean, Integer, Real, and String PrefMember below.
-
-class BooleanPrefMember : public PrefMember<bool> {
- public:
-  BooleanPrefMember();
-  virtual ~BooleanPrefMember();
-
- protected:
-  virtual void UpdateValueFromPref() const;
-  virtual void UpdatePref(const bool& value);
-
  private:
-  DISALLOW_COPY_AND_ASSIGN(BooleanPrefMember);
-};
+  class Internal : public subtle::PrefMemberBase::Internal {
+   public:
+    Internal() : value_(ValueType()) {}
 
-class IntegerPrefMember : public PrefMember<int> {
- public:
-  IntegerPrefMember();
-  virtual ~IntegerPrefMember();
+    ValueType value() {
+      CheckOnCorrectThread();
+      return value_;
+    }
 
- protected:
-  virtual void UpdateValueFromPref() const;
-  virtual void UpdatePref(const int& value);
+   protected:
+    virtual ~Internal() {}
 
- private:
-  DISALLOW_COPY_AND_ASSIGN(IntegerPrefMember);
-};
+    virtual bool UpdateValueInternal(const Value& value) const;
 
-class DoublePrefMember : public PrefMember<double> {
- public:
-  DoublePrefMember();
-  virtual ~DoublePrefMember();
+    // We cache the value of the pref so we don't have to keep walking the pref
+    // tree.
+    mutable ValueType value_;
 
- protected:
-  virtual void UpdateValueFromPref() const;
-  virtual void UpdatePref(const double& value);
+    DISALLOW_COPY_AND_ASSIGN(Internal);
+  };
 
- private:
-  DISALLOW_COPY_AND_ASSIGN(DoublePrefMember);
-};
+  virtual Internal* internal() const { return internal_; }
+  virtual void CreateInternal() const {
+    internal_ = new Internal();
+  }
 
-class StringPrefMember : public PrefMember<std::string> {
- public:
-  StringPrefMember();
-  virtual ~StringPrefMember();
+  // This method is used to do the actual sync with pref of the specified type.
+  virtual void UpdatePref(const ValueType& value);
 
- protected:
-  virtual void UpdateValueFromPref() const;
-  virtual void UpdatePref(const std::string& value);
+  mutable scoped_refptr<Internal> internal_;
 
- private:
-  DISALLOW_COPY_AND_ASSIGN(StringPrefMember);
+  DISALLOW_COPY_AND_ASSIGN(Internal);
 };
 
-class FilePathPrefMember : public PrefMember<FilePath> {
- public:
-  FilePathPrefMember();
-  virtual ~FilePathPrefMember();
-
- protected:
-  virtual void UpdateValueFromPref() const;
-  virtual void UpdatePref(const FilePath& value);
-
- private:
-  DISALLOW_COPY_AND_ASSIGN(FilePathPrefMember);
-};
+typedef PrefMember<bool> BooleanPrefMember;
+typedef PrefMember<int> IntegerPrefMember;
+typedef PrefMember<double> DoublePrefMember;
+typedef PrefMember<std::string> StringPrefMember;
+typedef PrefMember<FilePath> FilePathPrefMember;
 
 #endif  // CHROME_BROWSER_PREFS_PREF_MEMBER_H_
diff --git a/chrome/browser/prefs/pref_member_unittest.cc b/chrome/browser/prefs/pref_member_unittest.cc
index f0bf181..10da62d 100644
--- a/chrome/browser/prefs/pref_member_unittest.cc
+++ b/chrome/browser/prefs/pref_member_unittest.cc
@@ -3,6 +3,9 @@
 // found in the LICENSE file.
 
 #include "chrome/browser/prefs/pref_member.h"
+
+#include "base/message_loop.h"
+#include "chrome/browser/browser_thread.h"
 #include "chrome/browser/prefs/pref_value_store.h"
 #include "chrome/common/notification_details.h"
 #include "chrome/common/notification_source.h"
@@ -12,10 +15,10 @@
 
 namespace {
 
-static const char kBoolPref[] = "bool";
-static const char kIntPref[] = "int";
-static const char kDoublePref[] = "double";
-static const char kStringPref[] = "string";
+const char kBoolPref[] = "bool";
+const char kIntPref[] = "int";
+const char kDoublePref[] = "double";
+const char kStringPref[] = "string";
 
 void RegisterTestPrefs(PrefService* prefs) {
   prefs->RegisterBooleanPref(kBoolPref, false);
@@ -24,6 +27,43 @@ void RegisterTestPrefs(PrefService* prefs) {
   prefs->RegisterStringPref(kStringPref, "default");
 }
 
+class GetPrefValueCallback
+    : public base::RefCountedThreadSafe<GetPrefValueCallback> {
+ public:
+  GetPrefValueCallback() : value_(false) {}
+
+  void Init(const char* pref_name, PrefService* prefs) {
+    pref_.Init(pref_name, prefs, NULL);
+    pref_.MoveToThread(BrowserThread::IO);
+  }
+
+  bool FetchValue() {
+    if (!BrowserThread::PostTask(
+        BrowserThread::IO, FROM_HERE,
+        NewRunnableMethod(this,
+                          &GetPrefValueCallback::GetPrefValueOnIOThread))) {
+      return false;
+    }
+    MessageLoop::current()->Run();
+    return true;
+  }
+
+  bool value() { return value_; }
+
+ private:
+  friend class base::RefCountedThreadSafe<GetPrefValueCallback>;
+  ~GetPrefValueCallback() {}
+
+  void GetPrefValueOnIOThread() {
+    value_ = pref_.GetValue();
+    BrowserThread::PostTask(BrowserThread::UI, FROM_HERE,
+                            new MessageLoop::QuitTask());
+  }
+
+  BooleanPrefMember pref_;
+  bool value_;
+};
+
 class PrefMemberTestClass : public NotificationObserver {
  public:
   explicit PrefMemberTestClass(PrefService* prefs)
@@ -193,3 +233,23 @@ TEST(PrefMemberTest, NoInit) {
   // Make sure not calling Init on a PrefMember doesn't cause problems.
   IntegerPrefMember pref;
 }
+
+TEST(PrefMemberTest, MoveToThread) {
+  MessageLoop message_loop;
+  BrowserThread ui_thread(BrowserThread::UI, &message_loop);
+  BrowserThread io_thread(BrowserThread::IO);
+  ASSERT_TRUE(io_thread.Start());
+  TestingPrefService prefs;
+  RegisterTestPrefs(&prefs);
+  scoped_refptr<GetPrefValueCallback> callback =
+      make_scoped_refptr(new GetPrefValueCallback());
+  callback->Init(kBoolPref, &prefs);
+
+  ASSERT_TRUE(callback->FetchValue());
+  EXPECT_EQ(false, callback->value());
+
+  prefs.SetBoolean(kBoolPref, true);
+
+  ASSERT_TRUE(callback->FetchValue());
+  EXPECT_EQ(true, callback->value());
+}
diff --git a/chrome/browser/prefs/pref_service.cc b/chrome/browser/prefs/pref_service.cc
index 1df38ee..0e55f31 100644
--- a/chrome/browser/prefs/pref_service.cc
+++ b/chrome/browser/prefs/pref_service.cc
@@ -355,20 +355,16 @@ std::string PrefService::GetString(const char* path) const {
 FilePath PrefService::GetFilePath(const char* path) const {
   DCHECK(CalledOnValidThread());
 
-  FilePath::StringType result;
+  FilePath result;
 
   const Preference* pref = FindPreference(path);
   if (!pref) {
     NOTREACHED() << "Trying to read an unregistered pref: " << path;
     return FilePath(result);
   }
-  bool rv = pref->GetValue()->GetAsString(&result);
+  bool rv = pref->GetValue()->GetAsFilePath(&result);
   DCHECK(rv);
-#if defined(OS_POSIX)
-  // We store filepaths as UTF8, so convert it back to the system type.
-  result = base::SysWideToNativeMB(UTF8ToWide(result));
-#endif
-  return FilePath(result);
+  return result;
 }
 
 bool PrefService::HasPrefPath(const char* path) const {
@@ -522,16 +518,7 @@ void PrefService::SetString(const char* path, const std::string& value) {
 }
 
 void PrefService::SetFilePath(const char* path, const FilePath& value) {
-#if defined(OS_POSIX)
-  // Value::SetString only knows about UTF8 strings, so convert the path from
-  // the system native value to UTF8.
-  std::string path_utf8 = WideToUTF8(base::SysNativeMBToWide(value.value()));
-  Value* new_value = Value::CreateStringValue(path_utf8);
-#else
-  Value* new_value = Value::CreateStringValue(value.value());
-#endif
-
-  SetUserPrefValue(path, new_value);
+  SetUserPrefValue(path, Value::CreateFilePathValue(value));
 }
 
 void PrefService::SetInt64(const char* path, int64 value) {