Re-enable preferences service access for browser+ash

chrome/browser/chromeos/preferences_chromeos_browsertest.cc

Index: chrome/browser/chromeos/preferences_chromeos_browsertest.cc
diff --git a/chrome/browser/chromeos/preferences_chromeos_browsertest.cc b/chrome/browser/chromeos/preferences_chromeos_browsertest.cc
index 93812b0559b4ab1794188d9861f11a55fa15f11d..86712c64fa506a965d7cd669f4df28f4692bb62f 100644
--- a/chrome/browser/chromeos/preferences_chromeos_browsertest.cc
+++ b/chrome/browser/chromeos/preferences_chromeos_browsertest.cc
@@ -5,10 +5,16 @@
 #include <stddef.h>
 #include <sys/types.h>
 
+#include <set>
+#include <string>
+
 #include "ash/shell.h"
 #include "base/command_line.h"
 #include "base/compiler_specific.h"
 #include "base/macros.h"
+#include "base/memory/ref_counted.h"
+#include "base/run_loop.h"
+#include "base/values.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/chromeos/input_method/input_method_manager_impl.h"
 #include "chrome/browser/chromeos/login/login_manager_test.h"
@@ -20,12 +26,20 @@
 #include "chrome/browser/chromeos/settings/stub_cros_settings_provider.h"
 #include "chrome/browser/chromeos/system/fake_input_device_settings.h"
 #include "chrome/browser/ui/ash/multi_user/multi_user_window_manager_chromeos.h"
+#include "chrome/browser/ui/browser.h"
 #include "chrome/common/pref_names.h"
+#include "chrome/test/base/in_process_browser_test.h"
 #include "chromeos/chromeos_switches.h"
 #include "components/feedback/tracing_manager.h"
 #include "components/prefs/pref_service.h"
+#include "components/prefs/pref_store.h"
+#include "components/prefs/writeable_pref_store.h"
 #include "components/user_manager/user_manager.h"
+#include "content/public/common/service_manager_connection.h"
 #include "content/public/test/test_utils.h"
+#include "services/preferences/public/cpp/pref_client_store.h"
+#include "services/preferences/public/interfaces/preferences.mojom.h"
+#include "services/service_manager/public/cpp/connector.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "ui/base/ime/chromeos/fake_ime_keyboard.h"
 #include "ui/events/event_utils.h"
@@ -141,6 +155,60 @@ class PreferencesTest : public LoginManagerTest {
   DISALLOW_COPY_AND_ASSIGN(PreferencesTest);
 };
 
+class PreferencesServiceBrowserTest : public InProcessBrowserTest {
+ public:
+  PreferencesServiceBrowserTest() {}
+
+ protected:
+  static service_manager::Connector* connector() {
+    return content::ServiceManagerConnection::GetForProcess()->GetConnector();
+  }
+
+  void WaitForPrefChange(PrefStore* store, const std::string& key) {
+    base::RunLoop run_loop;
+    TestPrefObserver observer(key, run_loop.QuitClosure());
+    store->AddObserver(&observer);
+    run_loop.Run();
+    store->RemoveObserver(&observer);
+  }
+
+  int GetIntegerPrefValue(PrefStore* store, const std::string& key) {
+    int int_value = 0;
+    const base::Value* value = nullptr;
+    EXPECT_TRUE(store->GetValue(key, &value));

[sky, 2017/02/17 17:44:22]

If this returns false, won't the next line crash? Maybe make this function
return a bool and take an int* so that callers can do:

ASSERT(GetIntegerPrefValue(store, key, &value));
ASSERT_EQ(..., value)
?

[Ken Rockot(use gerrit already), 2017/02/17 18:58:22]

Done

+    EXPECT_TRUE(value->GetAsInteger(&int_value));
+    return int_value;
+  }
+
+ private:
+  class TestPrefObserver : public PrefStore::Observer {
+   public:
+    TestPrefObserver(const std::string& pref_name,
+                     const base::Closure& callback)
+        : pref_name_(pref_name), callback_(callback) {}
+
+    ~TestPrefObserver() override {}
+
+    // PrefStore::Observer:
+    void OnPrefValueChanged(const std::string& key) override {
+      if (key == pref_name_)

[jonross, 2017/02/16 21:53:35]

Have this be an EXPECT_EQ? That way we fail explicitly vs never exiting the run
loop

[Ken Rockot(use gerrit already), 2017/02/17 00:00:31]

I think the test should tolerate other prefs being set. See other inline
comments.

[jonross, 2017/02/17 02:15:46]

SGTM

+        callback_.Run();
+    }
+
+    void OnInitializationCompleted(bool success) override {
+      ASSERT_TRUE(success);
+    }
+
+   private:
+    const std::string pref_name_;
+    const base::Closure callback_;
+
+    DISALLOW_COPY_AND_ASSIGN(TestPrefObserver);
+  };
+
+  DISALLOW_COPY_AND_ASSIGN(PreferencesServiceBrowserTest);
+};
+
 IN_PROC_BROWSER_TEST_F(PreferencesTest, PRE_MultiProfiles) {
   RegisterUser(test_users_[0].GetUserEmail());
   RegisterUser(test_users_[1].GetUserEmail());
@@ -214,4 +282,39 @@ IN_PROC_BROWSER_TEST_F(PreferencesTest, MultiProfiles) {
   CheckLocalStateCorrespondsToPrefs(prefs1);
 }
 
+IN_PROC_BROWSER_TEST_F(PreferencesServiceBrowserTest, Basic) {
+  prefs::mojom::PreferencesServiceFactoryPtr factory_a;
+  connector()->BindInterface(prefs::mojom::kServiceName, &factory_a);
+  scoped_refptr<preferences::PrefClientStore> pref_store_a =
+      new preferences::PrefClientStore(std::move(factory_a));
+
+  prefs::mojom::PreferencesServiceFactoryPtr factory_b;
+  connector()->BindInterface(prefs::mojom::kServiceName, &factory_b);
+  scoped_refptr<preferences::PrefClientStore> pref_store_b =
+      new preferences::PrefClientStore(std::move(factory_b));
+
+  constexpr int kInitialValue = 1;
+  PrefService* pref_service = browser()->profile()->GetPrefs();

[jonross, 2017/02/16 21:53:35]

This exposes the test to other preferences being updated by the browser. Would
it make sense to use a TestingProfileManager here to control a TestingProfile
directly?

see: preferences_service_unittest.cc

[Ken Rockot(use gerrit already), 2017/02/17 00:00:31]

I started going down this route just to see what it would look like.

My conclusion is that I don't think it makes sense to use a
TestingProfileManager in a browser test (in fact there is virtually zero
precedent for this).

The purpose of this test is to act as a browser integration test, so the more
mocking that we do, the less useful the test is. I guess it would still test
that PreferencesConnectionManager is reachable via a service connector, but it
would have to create a whole lot of scaffolding just to prove that point and it
would provide strictly less meaningful test coverage.

Generally those sorts of tests seem to be fragile and expensive to maintain. At
least, whenever I find myself having to maintain such mock-heavy tests, it makes
me very sad.

I think the only real risk here is that someone introduces a change which causes
the standard Chrome OS browser test environment to modify the kMouseSensitivity
preference arbitrarily while this test is running. That seems rather unlikely.

We *could* introduce some new test-only prefs to be registed in normal Profiles
within the test environment if we want to be extra safe, but I stopped shy of
that here to keep this change as small as possible.

[jonross, 2017/02/17 02:15:46]

Thanks for the clarification. I'm fine with treating this as a full integration
test.

If something does introduce changes to the chosen pref for the test, that will
be caught at submission time. So pretty early on, and we can adjust as
necessary.

This SGTM.

+  pref_service->SetInteger(prefs::kMouseSensitivity, kInitialValue);
+
+  pref_store_a->Subscribe({prefs::kMouseSensitivity});
+  WaitForPrefChange(pref_store_a.get(), prefs::kMouseSensitivity);
+  EXPECT_EQ(kInitialValue,
+            GetIntegerPrefValue(pref_store_a.get(), prefs::kMouseSensitivity));
+
+  pref_store_b->Subscribe({prefs::kMouseSensitivity});
+  WaitForPrefChange(pref_store_b.get(), prefs::kMouseSensitivity);
+  EXPECT_EQ(kInitialValue,
+            GetIntegerPrefValue(pref_store_b.get(), prefs::kMouseSensitivity));
+
+  const int kTestValue = 42;
+  pref_store_a->SetValue(prefs::kMouseSensitivity,
+                         base::MakeUnique<base::FundamentalValue>(kTestValue),
+                         WriteablePrefStore::DEFAULT_PREF_WRITE_FLAGS);
+
+  WaitForPrefChange(pref_store_b.get(), prefs::kMouseSensitivity);
+  EXPECT_EQ(kTestValue,
+            GetIntegerPrefValue(pref_store_b.get(), prefs::kMouseSensitivity));
+}
+
 }  // namespace chromeos