sync: Refactor TwoClientPreferencesSyncTest

chrome/browser/sync/test/integration/preferences_helper.cc

Index: chrome/browser/sync/test/integration/preferences_helper.cc
diff --git a/chrome/browser/sync/test/integration/preferences_helper.cc b/chrome/browser/sync/test/integration/preferences_helper.cc
index d29714c2ecb829fcfe7b7ae6e56fe4d36e9c2c81..f9f69528e2704396e178443122eeb7699d5ca734 100644
--- a/chrome/browser/sync/test/integration/preferences_helper.cc
+++ b/chrome/browser/sync/test/integration/preferences_helper.cc
@@ -4,9 +4,10 @@
 
 #include "chrome/browser/sync/test/integration/preferences_helper.h"
 
+#include "base/prefs/pref_change_registrar.h"
 #include "base/prefs/pref_service.h"
 #include "base/prefs/scoped_user_pref_update.h"
-#include "base/values.h"
+#include "base/strings/stringprintf.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/sync/test/integration/multi_client_status_change_checker.h"
 #include "chrome/browser/sync/test/integration/profile_sync_service_harness.h"
@@ -107,8 +108,8 @@ bool BooleanPrefMatches(const char* pref_name) {
   }
   for (int i = 0; i < test()->num_clients(); ++i) {
     if (reference_value != GetPrefs(i)->GetBoolean(pref_name)) {
-      LOG(ERROR) << "Boolean preference " << pref_name << " mismatched in"
-                 << " profile " << i << ".";
+      DVLOG(1) << "Boolean preference " << pref_name << " mismatched in"
+               << " profile " << i << ".";
       return false;
     }
   }
@@ -124,8 +125,8 @@ bool IntegerPrefMatches(const char* pref_name) {
   }
   for (int i = 0; i < test()->num_clients(); ++i) {
     if (reference_value != GetPrefs(i)->GetInteger(pref_name)) {
-      LOG(ERROR) << "Integer preference " << pref_name << " mismatched in"
-                 << " profile " << i << ".";
+      DVLOG(1) << "Integer preference " << pref_name << " mismatched in"
+               << " profile " << i << ".";
       return false;
     }
   }
@@ -141,8 +142,8 @@ bool Int64PrefMatches(const char* pref_name) {
   }
   for (int i = 0; i < test()->num_clients(); ++i) {
     if (reference_value != GetPrefs(i)->GetInt64(pref_name)) {
-      LOG(ERROR) << "Integer preference " << pref_name << " mismatched in"
-                 << " profile " << i << ".";
+      DVLOG(1) << "Integer preference " << pref_name << " mismatched in"
+               << " profile " << i << ".";
       return false;
     }
   }
@@ -158,8 +159,8 @@ bool DoublePrefMatches(const char* pref_name) {
   }
   for (int i = 0; i < test()->num_clients(); ++i) {
     if (reference_value != GetPrefs(i)->GetDouble(pref_name)) {
-      LOG(ERROR) << "Double preference " << pref_name << " mismatched in"
-                 << " profile " << i << ".";
+      DVLOG(1) << "Double preference " << pref_name << " mismatched in"
+               << " profile " << i << ".";
       return false;
     }
   }
@@ -175,8 +176,8 @@ bool StringPrefMatches(const char* pref_name) {
   }
   for (int i = 0; i < test()->num_clients(); ++i) {
     if (reference_value != GetPrefs(i)->GetString(pref_name)) {
-      LOG(ERROR) << "String preference " << pref_name << " mismatched in"
-                 << " profile " << i << ".";
+      DVLOG(1) << "String preference " << pref_name << " mismatched in"
+               << " profile " << i << ".";
       return false;
     }
   }
@@ -192,8 +193,8 @@ bool FilePathPrefMatches(const char* pref_name) {
   }
   for (int i = 0; i < test()->num_clients(); ++i) {
     if (reference_value != GetPrefs(i)->GetFilePath(pref_name)) {
-      LOG(ERROR) << "base::FilePath preference " << pref_name
-                 << " mismatched in" << " profile " << i << ".";
+      DVLOG(1) << "base::FilePath preference " << pref_name << " mismatched in"
+               << " profile " << i << ".";
       return false;
     }
   }
@@ -209,8 +210,8 @@ bool ListPrefMatches(const char* pref_name) {
   }
   for (int i = 0; i < test()->num_clients(); ++i) {
     if (!reference_value->Equals(GetPrefs(i)->GetList(pref_name))) {
-      LOG(ERROR) << "List preference " << pref_name << " mismatched in"
-                 << " profile " << i << ".";
+      DVLOG(1) << "List preference " << pref_name << " mismatched in"
+               << " profile " << i << ".";
       return false;
     }
   }
@@ -220,37 +221,166 @@ bool ListPrefMatches(const char* pref_name) {
 
 namespace {
 
-// Helper class used in the implementation of AwaitListPrefMatches.
-class ListPrefMatchStatusChecker : public MultiClientStatusChangeChecker {
+class PrefMatchChecker : public StatusChangeChecker {
  public:
-  explicit ListPrefMatchStatusChecker(const char* pref_name);
-  virtual ~ListPrefMatchStatusChecker();
+  PrefMatchChecker(const char* path);
+  virtual ~PrefMatchChecker();
 
-  virtual bool IsExitConditionSatisfied() OVERRIDE;
+  // StatusChangeChecker implementation.
+  virtual bool IsExitConditionSatisfied() = 0;
   virtual std::string GetDebugMessage() const OVERRIDE;
+
+  // Wait for condition to become true.
+  void Wait();
+
+ protected:
+  const char* GetPath() const;
+
  private:
-  const char* pref_name_;
+  void RegisterPrefListener(PrefService* pref_service);
+
+  ScopedVector<PrefChangeRegistrar> pref_change_registrars_;
+  const char* path_;
 };
 
-ListPrefMatchStatusChecker::ListPrefMatchStatusChecker(const char* pref_name)
-    : MultiClientStatusChangeChecker(
-        sync_datatype_helper::test()->GetSyncServices()),
-      pref_name_(pref_name) {}
+PrefMatchChecker::PrefMatchChecker(const char* path) : path_(path) {
+}
 
-ListPrefMatchStatusChecker::~ListPrefMatchStatusChecker() {}
+PrefMatchChecker::~PrefMatchChecker() {
+}
+
+std::string PrefMatchChecker::GetDebugMessage() const {
+  return base::StringPrintf("Waiting for pref '%s' to match", GetPath());
+}
+
+void PrefMatchChecker::Wait() {
+  if (test()->use_verifier()) {
+    RegisterPrefListener(GetVerifierPrefs());
+  }
+
+  for (int i = 0; i < test()->num_clients(); ++i) {
+    RegisterPrefListener(GetPrefs(i));
+  }
+}
+
+const char* PrefMatchChecker::GetPath() const {
+  return path_;
+}
+
+void PrefMatchChecker::RegisterPrefListener(PrefService* pref_service) {
+  scoped_ptr<PrefChangeRegistrar> registrar(new PrefChangeRegistrar());
+  registrar->Init(pref_service);
+  registrar->Add(path_,
+                 base::Bind(&PrefMatchChecker::CheckExitCondition,
+                            base::Unretained(this)));
+  pref_change_registrars_.push_back(registrar.release());
+}

[pval...(no longer on Chromium), 2014/06/04 18:06:31]

add blank line after this one

[rlarocque, 2014/06/04 18:24:40]

Done.

+// Helper class used in the implementation of AwaitListPrefMatches.
+class ListPrefMatchChecker : public PrefMatchChecker {
+ public:
+  explicit ListPrefMatchChecker(const char* path);
+  virtual ~ListPrefMatchChecker();
+
+  // Implementation of PrefMatchChecker.
+  virtual bool IsExitConditionSatisfied() OVERRIDE;
+};
+
+ListPrefMatchChecker::ListPrefMatchChecker(const char* path)
+    : PrefMatchChecker(path) {
+}
+
+ListPrefMatchChecker::~ListPrefMatchChecker() {
+}
+
+bool ListPrefMatchChecker::IsExitConditionSatisfied() {
+  return ListPrefMatches(GetPath());
+}
+
+// Helper class used in the implementation of AwaitBooleanPrefMatches.
+class BooleanPrefMatchChecker : public PrefMatchChecker {
+ public:
+  BooleanPrefMatchChecker(const char* path);
+  virtual ~BooleanPrefMatchChecker();
+
+  // Implementation of PrefMatchChecker.
+  virtual bool IsExitConditionSatisfied() OVERRIDE;
+};
 
-bool ListPrefMatchStatusChecker::IsExitConditionSatisfied() {
-  return ListPrefMatches(pref_name_);
+BooleanPrefMatchChecker::BooleanPrefMatchChecker(const char* path)
+    : PrefMatchChecker(path) {
 }
 
-std::string ListPrefMatchStatusChecker::GetDebugMessage() const {
-  return "Waiting for matching preferences";
+BooleanPrefMatchChecker::~BooleanPrefMatchChecker() {
+}
+
+bool BooleanPrefMatchChecker::IsExitConditionSatisfied() {
+  return BooleanPrefMatches(GetPath());
+}
+
+// Helper class used in the implementation of AwaitIntegerPrefMatches.
+class IntegerPrefMatchChecker : public PrefMatchChecker {
+ public:
+  explicit IntegerPrefMatchChecker(const char* path);
+  virtual ~IntegerPrefMatchChecker();
+
+  // Implementation of PrefMatchChecker.
+  virtual bool IsExitConditionSatisfied() OVERRIDE;
+};
+
+IntegerPrefMatchChecker::IntegerPrefMatchChecker(const char* path)
+    : PrefMatchChecker(path) {
+}
+
+IntegerPrefMatchChecker::~IntegerPrefMatchChecker() {
+}
+
+bool IntegerPrefMatchChecker::IsExitConditionSatisfied() {
+  return IntegerPrefMatches(GetPath());
+}
+
+// Helper class used in the implementation of AwaitStringPrefMatches.
+class StringPrefMatchChecker : public PrefMatchChecker {
+ public:
+  explicit StringPrefMatchChecker(const char* path);
+  virtual ~StringPrefMatchChecker();
+
+  // Implementation of PrefMatchChecker.
+  virtual bool IsExitConditionSatisfied() OVERRIDE;
+};
+
+StringPrefMatchChecker::StringPrefMatchChecker(const char* path)
+    : PrefMatchChecker(path) {
+}
+
+StringPrefMatchChecker::~StringPrefMatchChecker() {
+}
+
+bool StringPrefMatchChecker::IsExitConditionSatisfied() {
+  return StringPrefMatches(GetPath());
 }
 
 }  //  namespace
 
 bool AwaitListPrefMatches(const char* pref_name) {
-  ListPrefMatchStatusChecker checker(pref_name);
+  ListPrefMatchChecker checker(pref_name);
+  checker.Wait();
+  return !checker.TimedOut();
+}
+
+bool AwaitBooleanPrefMatches(const char* pref_name) {

[pval...(no longer on Chromium), 2014/06/04 18:06:31]

It's unfortunate that the PrefService design forces us to have all these
per-type verification methods, but I don't see any other way to do this.

Any crazy ideas of alternatives?

[rlarocque, 2014/06/04 18:24:40]

Templates: Too complicated.  Often requires placing lots of code in the header
file.  The original PrefService would have had even greater reason to use
templates than we do here.  They wisely chose not to.

Delegate classes:

class PrefChecker {
  PrefChecker();
  virtual CheckEqual(const char* pref_name, PrefService* service1, PrefService*
service2) = 0;
}

then subclass for each type and inject them.  This gets ugly fast.  Defining
each of these classes still requires lots of boilerplate and would confuse the
implementation of the StatusChangeChecker.  Also, these would need to be passed
around by reference or pointer in order to take advantage of the vptr, so
lifetimes may become an issue.  Where exactly would we allocate these things?

In short, this is a bad idea, too.

[pval...(no longer on Chromium), 2014/06/04 23:00:41]

Sounds reasonable; 3 more methods isn't too bad.

+  BooleanPrefMatchChecker checker(pref_name);
+  checker.Wait();
+  return !checker.TimedOut();
+}
+
+bool AwaitIntegerPrefMatches(const char* pref_name) {
+  IntegerPrefMatchChecker checker(pref_name);
+  checker.Wait();
+  return !checker.TimedOut();
+}
+
+bool AwaitStringPrefMatches(const char* pref_name) {
+  StringPrefMatchChecker checker(pref_name);
   checker.Wait();
   return !checker.TimedOut();
 }