sync: Refactor TwoClientPreferencesSyncTest

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

 // Copyright (c) 2011 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.
 
 #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"
 #include "chrome/browser/sync/test/integration/sync_datatype_helper.h"
 #include "chrome/browser/sync/test/integration/sync_test.h"
 
 using sync_datatype_helper::test;
 
 namespace preferences_helper {
 
@@ skipping 80 matching lines @@
 
 bool BooleanPrefMatches(const char* pref_name) {
   bool reference_value;
   if (test()->use_verifier()) {
     reference_value = GetVerifierPrefs()->GetBoolean(pref_name);
   } else {
     reference_value = GetPrefs(0)->GetBoolean(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;
     }
   }
   return true;
 }
 
 bool IntegerPrefMatches(const char* pref_name) {
   int reference_value;
   if (test()->use_verifier()) {
     reference_value = GetVerifierPrefs()->GetInteger(pref_name);
   } else {
     reference_value = GetPrefs(0)->GetInteger(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;
     }
   }
   return true;
 }
 
 bool Int64PrefMatches(const char* pref_name) {
   int64 reference_value;
   if (test()->use_verifier()) {
     reference_value = GetVerifierPrefs()->GetInt64(pref_name);
   } else {
     reference_value = GetPrefs(0)->GetInt64(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;
     }
   }
   return true;
 }
 
 bool DoublePrefMatches(const char* pref_name) {
   double reference_value;
   if (test()->use_verifier()) {
     reference_value = GetVerifierPrefs()->GetDouble(pref_name);
   } else {
     reference_value = GetPrefs(0)->GetDouble(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;
     }
   }
   return true;
 }
 
 bool StringPrefMatches(const char* pref_name) {
   std::string reference_value;
   if (test()->use_verifier()) {
     reference_value = GetVerifierPrefs()->GetString(pref_name);
   } else {
     reference_value = GetPrefs(0)->GetString(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;
     }
   }
   return true;
 }
 
 bool FilePathPrefMatches(const char* pref_name) {
   base::FilePath reference_value;
   if (test()->use_verifier()) {
     reference_value = GetVerifierPrefs()->GetFilePath(pref_name);
   } else {
     reference_value = GetPrefs(0)->GetFilePath(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;
     }
   }
   return true;
 }
 
 bool ListPrefMatches(const char* pref_name) {
   const base::ListValue* reference_value;
   if (test()->use_verifier()) {
     reference_value = GetVerifierPrefs()->GetList(pref_name);
   } else {
     reference_value = GetPrefs(0)->GetList(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;
     }
   }
   return true;
 }
 
 
 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) {}
-
-ListPrefMatchStatusChecker::~ListPrefMatchStatusChecker() {}
-
-bool ListPrefMatchStatusChecker::IsExitConditionSatisfied() {
-  return ListPrefMatches(pref_name_);
+PrefMatchChecker::PrefMatchChecker(const char* path) : path_(path) {
 }
 
-std::string ListPrefMatchStatusChecker::GetDebugMessage() const {
-  return "Waiting for matching preferences";
+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;
+};
+
+BooleanPrefMatchChecker::BooleanPrefMatchChecker(const char* path)
+    : PrefMatchChecker(path) {
+}
+
+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();
+}
 
 }  // namespace preferences_helper