Update chrome.settingsPrivate to support CrOS-only settings. These needed...

Update chrome.settingsPrivate to support CrOS-only settings. These needed special handling since they are managed by the CrOSSettings rather than by one of the PrefService objects.

I also added a few such settings as used by the users page, to the prefs whitelist, and removed some dead code from the apitest.

BUG=496308

Review URL: https://codereview.chromium.org/1163593005

Cr-Commit-Position: refs/heads/master@{#333180}

chrome/browser/extensions/api/settings_private/prefs_util.h

@@ -17,31 +17,62 @@ class Profile;
 
 namespace extensions {
 
-namespace prefs_util {
+class PrefsUtil {
 
-using TypedPrefMap = std::map<std::string, api::settings_private::PrefType>;
+ public:
+  using TypedPrefMap = std::map<std::string, api::settings_private::PrefType>;
 
-// Gets the list of whitelisted pref keys -- that is, those which correspond to
-// prefs that clients of the settingsPrivate API may retrieve and manipulate.
-const TypedPrefMap& GetWhitelistedKeys();
+  explicit PrefsUtil(Profile* profile);
+  virtual ~PrefsUtil();
 
-// Gets the value of the pref with the given |name|. Returns a pointer to an
-// empty PrefObject if no pref is found for |name|.
-scoped_ptr<api::settings_private::PrefObject> GetPref(Profile* profile,
+  // Gets the list of whitelisted pref keys -- that is, those which correspond
+  // to prefs that clients of the settingsPrivate API may retrieve and
+  // manipulate.
+  const TypedPrefMap& GetWhitelistedKeys();
+
+  // Gets the value of the pref with the given |name|. Returns a pointer to an
+  // empty PrefObject if no pref is found for |name|.
+  virtual scoped_ptr<api::settings_private::PrefObject> GetPref(
       const std::string& name);
 
-// Returns whether |pref_name| corresponds to a pref whose type is URL.
-bool IsPrefTypeURL(const std::string& pref_name);
+  // Sets the pref with the given name and value in the proper PrefService.
+  virtual bool SetPref(const std::string& name, const base::Value* value);
+
+  // Appends the given |value| to the list setting specified by the path in
+  // |setting|.
+  virtual bool AppendToListCrosSetting(const std::string& setting,
+                                       const base::Value& value);
+
+  // Removes the given |value| from the list setting specified by the path in
+  // |setting|.
+  virtual bool RemoveFromListCrosSetting(const std::string& setting,
+                                         const base::Value& value);
+
+  // Returns a pointer to the appropriate PrefService instance for the given
+  // |pref_name|.
+  virtual PrefService* FindServiceForPref(const std::string& pref_name);
+
+  // Returns whether or not the given pref is a CrOS-specific setting.
+  virtual bool IsCrosSetting(const std::string& pref_name);
 
-// Returns whether |pref_name| corresponds to a pref that is user modifiable
-// (i.e., not made restricted by a user or device policy).
-bool IsPrefUserModifiable(Profile* profile, const std::string& pref_name);
+ protected:
+  // Returns whether |pref_name| corresponds to a pref whose type is URL.
+  bool IsPrefTypeURL(const std::string& pref_name);
+
+  // Returns whether |pref_name| corresponds to a pref that is user modifiable
+  // (i.e., not made restricted by a user or device policy).
+  bool IsPrefUserModifiable(const std::string& pref_name);
+
+  api::settings_private::PrefType GetType(const std::string& name,
+                                          base::Value::Type type);
+
+  scoped_ptr<api::settings_private::PrefObject> GetCrosSettingsPref(
+      const std::string& name);
 
-// Returns a pointer to the appropriate PrefService instance for the given
-// |pref_name|.
-PrefService* FindServiceForPref(Profile* profile, const std::string& pref_name);
+  bool SetCrosSettingsPref(const std::string& name, const base::Value* value);
 
-}  // namespace prefs_util
+  Profile* profile_;  // weak
+};
 
 }  // namespace extensions
 

chrome/browser/extensions/api/settings_private/settings_private_apitest.cc

@@ -13,76 +13,24 @@
 #include "content/public/test/test_utils.h"
 #include "extensions/common/switches.h"
 
+#if defined(OS_CHROMEOS)
+#include "chromeos/chromeos_switches.h"
+#endif
+
 namespace extensions {
 
 namespace {
 
-const char kTestPrefName[] = "download.default_directory";
-const char kTestPrefValue[] = "/Downloads";
-
-class TestDelegate : public SettingsPrivateDelegate {
- public:
-  explicit TestDelegate(Profile* profile) : SettingsPrivateDelegate(profile) {}
-
-  bool SetPref(const std::string& name, const base::Value* value) override {
-    // Write to the actual pref service, so that the SettingsPrivateEventRouter
-    // dispatches an onPrefsChanged event.
-    PrefService* pref_service = profile_->GetPrefs();
-    pref_service->Set(name.c_str(), *value);
-    return true;
-  }
-
-  scoped_ptr<base::Value> GetPref(const std::string& name) override {
-    if (name.compare(kTestPrefName) != 0)
-      return base::Value::CreateNullValue();
-
-    return CreateTestPrefObject()->ToValue();
-  }
-
-  scoped_ptr<base::Value> GetAllPrefs() override {
-    base::ListValue* list_value = new base::ListValue();
-    list_value->Append(CreateTestPrefObject()->ToValue().release());
-    return make_scoped_ptr(list_value);
-  }
-
-  ~TestDelegate() override {}
-
- private:
-  scoped_ptr<api::settings_private::PrefObject> CreateTestPrefObject() {
-    scoped_ptr<api::settings_private::PrefObject> pref_object(
-        new api::settings_private::PrefObject());
-    pref_object->key = std::string(kTestPrefName);
-    pref_object->type = api::settings_private::PrefType::PREF_TYPE_STRING;
-    pref_object->value.reset(new base::StringValue(kTestPrefValue));
-    return pref_object.Pass();
-  }
-
-  DISALLOW_COPY_AND_ASSIGN(TestDelegate);
-};
-
 class SettingsPrivateApiTest : public ExtensionApiTest {
  public:
   SettingsPrivateApiTest() {}
   ~SettingsPrivateApiTest() override {}
 
-  static KeyedService* GetSettingsPrivateDelegate(
-      content::BrowserContext* profile) {
-    CHECK(s_test_delegate_);
-    return s_test_delegate_;
-  }
-
   void SetUpCommandLine(base::CommandLine* command_line) override {
     ExtensionApiTest::SetUpCommandLine(command_line);
-  }
-
-  void SetUpOnMainThread() override {
-    ExtensionApiTest::SetUpOnMainThread();
-    if (!s_test_delegate_)
-      s_test_delegate_ = new TestDelegate(profile());
-
-    SettingsPrivateDelegateFactory::GetInstance()->SetTestingFactory(
-        profile(), &SettingsPrivateApiTest::GetSettingsPrivateDelegate);
-    content::RunAllPendingInMessageLoop();
+#if defined(OS_CHROMEOS)
+    command_line->AppendSwitch(chromeos::switches::kStubCrosSettings);
+#endif
   }
 
  protected:
@@ -92,16 +40,10 @@ class SettingsPrivateApiTest : public ExtensionApiTest {
                                kFlagLoadAsComponent);
   }
 
-  // Static pointer to the TestDelegate so that it can be accessed in
-  // GetSettingsPrivateDelegate() passed to SetTestingFactory().
-  static TestDelegate* s_test_delegate_;
-
  private:
   DISALLOW_COPY_AND_ASSIGN(SettingsPrivateApiTest);
 };
 
-// static
-TestDelegate* SettingsPrivateApiTest::s_test_delegate_ = NULL;
 
 }  // namespace
 
@@ -121,4 +63,18 @@ IN_PROC_BROWSER_TEST_F(SettingsPrivateApiTest, OnPrefsChanged) {
   EXPECT_TRUE(RunSettingsSubtest("onPrefsChanged")) << message_;
 }
 
+#if defined(OS_CHROMEOS)
+IN_PROC_BROWSER_TEST_F(SettingsPrivateApiTest, GetPref_CrOSSetting) {
+  EXPECT_TRUE(RunSettingsSubtest("getPref_CrOSSetting")) << message_;
+}
+
+IN_PROC_BROWSER_TEST_F(SettingsPrivateApiTest, SetPref_CrOSSetting) {
+  EXPECT_TRUE(RunSettingsSubtest("setPref_CrOSSetting")) << message_;
+}
+
+IN_PROC_BROWSER_TEST_F(SettingsPrivateApiTest, OnPrefsChanged_CrOSSetting) {
+  EXPECT_TRUE(RunSettingsSubtest("onPrefsChanged_CrOSSetting")) << message_;
+}
+#endif
+
 }  // namespace extensions

chrome/browser/extensions/api/settings_private/settings_private_delegate.cc

@@ -4,14 +4,13 @@
 
 #include "chrome/browser/extensions/api/settings_private/settings_private_delegate.h"
 
-#include "base/json/json_reader.h"
 #include "base/prefs/pref_service.h"
 #include "base/values.h"
 #include "chrome/browser/browser_process.h"
+#include "chrome/browser/chromeos/settings/cros_settings.h"
 #include "chrome/browser/extensions/api/settings_private/prefs_util.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/common/pref_names.h"
-#include "components/url_fixer/url_fixer.h"
 #include "extensions/browser/extension_registry.h"
 #include "extensions/common/extension.h"
 #include "url/gurl.h"
@@ -22,6 +21,7 @@ namespace settings_private = api::settings_private;
 
 SettingsPrivateDelegate::SettingsPrivateDelegate(Profile* profile)
     : profile_(profile) {
+  prefs_util_.reset(new PrefsUtil(profile));
 }
 
 SettingsPrivateDelegate::~SettingsPrivateDelegate() {
@@ -29,13 +29,13 @@ SettingsPrivateDelegate::~SettingsPrivateDelegate() {
 
 scoped_ptr<base::Value> SettingsPrivateDelegate::GetPref(
     const std::string& name) {
-  return prefs_util::GetPref(profile_, name)->ToValue();
+  return prefs_util_->GetPref(name)->ToValue();
 }
 
 scoped_ptr<base::Value> SettingsPrivateDelegate::GetAllPrefs() {
   scoped_ptr<base::ListValue> prefs(new base::ListValue());
 
-  const TypedPrefMap& keys = prefs_util::GetWhitelistedKeys();
+  const TypedPrefMap& keys = prefs_util_->GetWhitelistedKeys();
   for (const auto& it : keys) {
     prefs->Append(GetPref(it.first).release());
   }
@@ -45,69 +45,7 @@ scoped_ptr<base::Value> SettingsPrivateDelegate::GetAllPrefs() {
 
 bool SettingsPrivateDelegate::SetPref(const std::string& pref_name,
                                       const base::Value* value) {
-  PrefService* pref_service =
-      prefs_util::FindServiceForPref(profile_, pref_name);
-
-  if (!prefs_util::IsPrefUserModifiable(profile_, pref_name))
-    return false;
-
-  const PrefService::Preference* pref =
-      pref_service->FindPreference(pref_name.c_str());
-  if (!pref)
-    return false;
-
-  DCHECK_EQ(pref->GetType(), value->GetType());
-
-  scoped_ptr<base::Value> temp_value;
-
-  switch (pref->GetType()) {
-    case base::Value::TYPE_INTEGER: {
-      // In JS all numbers are doubles.
-      double double_value;
-      if (!value->GetAsDouble(&double_value))
-        return false;
-
-      int int_value = static_cast<int>(double_value);
-      temp_value.reset(new base::FundamentalValue(int_value));
-      value = temp_value.get();
-      break;
-    }
-    case base::Value::TYPE_STRING: {
-      std::string original;
-      if (!value->GetAsString(&original))
-        return false;
-
-      if (prefs_util::IsPrefTypeURL(pref_name)) {
-        GURL fixed = url_fixer::FixupURL(original, std::string());
-        temp_value.reset(new base::StringValue(fixed.spec()));
-        value = temp_value.get();
-      }
-      break;
-    }
-    case base::Value::TYPE_LIST: {
-      // In case we have a List pref we got a JSON string.
-      std::string json_string;
-      if (!value->GetAsString(&json_string))
-        return false;
-
-      temp_value.reset(base::JSONReader::DeprecatedRead(json_string));
-      value = temp_value.get();
-      if (!value->IsType(base::Value::TYPE_LIST))
-        return false;
-
-      break;
-    }
-    case base::Value::TYPE_BOOLEAN:
-    case base::Value::TYPE_DOUBLE:
-      break;
-    default:
-      return false;
-  }
-
-  // TODO(orenb): Process setting metrics here (like "ProcessUserMetric" in
-  // CoreOptionsHandler).
-  pref_service->Set(pref_name.c_str(), *value);
-  return true;
+  return prefs_util_->SetPref(pref_name, value);
 }
 
 }  // namespace extensions

chrome/browser/extensions/api/settings_private/settings_private_delegate.h

@@ -11,6 +11,7 @@
 #include "base/callback.h"
 #include "base/macros.h"
 #include "base/memory/scoped_ptr.h"
+#include "chrome/browser/extensions/api/settings_private/prefs_util.h"
 #include "chrome/common/extensions/api/settings_private.h"
 #include "components/keyed_service/core/keyed_service.h"
 #include "extensions/browser/extension_function.h"
@@ -44,6 +45,7 @@ class SettingsPrivateDelegate : public KeyedService {
 
  protected:
   Profile* profile_;  // weak; not owned by us
+  scoped_ptr<PrefsUtil> prefs_util_;
 
  private:
   DISALLOW_COPY_AND_ASSIGN(SettingsPrivateDelegate);

chrome/browser/extensions/api/settings_private/settings_private_event_router.cc

@@ -10,7 +10,6 @@
 #include "base/bind_helpers.h"
 #include "base/prefs/pref_service.h"
 #include "chrome/browser/browser_process.h"
-#include "chrome/browser/extensions/api/settings_private/prefs_util.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/common/extensions/api/settings_private.h"
 #include "content/public/browser/browser_context.h"
@@ -32,6 +31,7 @@ SettingsPrivateEventRouter::SettingsPrivateEventRouter(
   }
 
   Profile* profile = Profile::FromBrowserContext(context_);
+  prefs_util_.reset(new PrefsUtil(profile));
   user_prefs_registrar_.Init(profile->GetPrefs());
   local_state_registrar_.Init(g_browser_process->local_state());
 }
@@ -49,8 +49,10 @@ void SettingsPrivateEventRouter::Shutdown() {
     event_router->UnregisterObserver(this);
 
   if (listening_) {
-    const prefs_util::TypedPrefMap& keys = prefs_util::GetWhitelistedKeys();
+    cros_settings_subscription_map_.clear();
+    const PrefsUtil::TypedPrefMap& keys = prefs_util_->GetWhitelistedKeys();
     for (const auto& it : keys) {
+      if (!prefs_util_->IsCrosSetting(it.first))
         FindRegistrarForPref(it.first)->Remove(it.first);
     }
   }
@@ -73,8 +75,7 @@ void SettingsPrivateEventRouter::OnListenerRemoved(
 PrefChangeRegistrar* SettingsPrivateEventRouter::FindRegistrarForPref(
     const std::string& pref_name) {
   Profile* profile = Profile::FromBrowserContext(context_);
-  if (prefs_util::FindServiceForPref(profile, pref_name) ==
-      profile->GetPrefs()) {
+  if (prefs_util_->FindServiceForPref(pref_name) == profile->GetPrefs()) {
     return &user_prefs_registrar_;
   }
   return &local_state_registrar_;
@@ -86,16 +87,34 @@ void SettingsPrivateEventRouter::StartOrStopListeningForPrefsChanges() {
       api::settings_private::OnPrefsChanged::kEventName);
 
   if (should_listen && !listening_) {
-    const prefs_util::TypedPrefMap& keys = prefs_util::GetWhitelistedKeys();
+    const PrefsUtil::TypedPrefMap& keys = prefs_util_->GetWhitelistedKeys();
     for (const auto& it : keys) {
-      FindRegistrarForPref(it.first)->Add(
-          it.first,
+      std::string pref_name = it.first;
+      if (prefs_util_->IsCrosSetting(pref_name)) {
+#if defined(OS_CHROMEOS)
+        scoped_ptr<chromeos::CrosSettings::ObserverSubscription> observer =
+            chromeos::CrosSettings::Get()->AddSettingsObserver(
+                pref_name.c_str(),
                 base::Bind(&SettingsPrivateEventRouter::OnPreferenceChanged,
-                     base::Unretained(this), user_prefs_registrar_.prefs()));
+                           base::Unretained(this), pref_name));
+        linked_ptr<chromeos::CrosSettings::ObserverSubscription> subscription(
+            observer.release());
+        cros_settings_subscription_map_.insert(
+            make_pair(pref_name, subscription));
+#endif
+      } else {
+        FindRegistrarForPref(it.first)
+            ->Add(pref_name,
+                  base::Bind(&SettingsPrivateEventRouter::OnPreferenceChanged,
+                             base::Unretained(this)));
+      }
     }
   } else if (!should_listen && listening_) {
-    const prefs_util::TypedPrefMap& keys = prefs_util::GetWhitelistedKeys();
+    const PrefsUtil::TypedPrefMap& keys = prefs_util_->GetWhitelistedKeys();
     for (const auto& it : keys) {
+      if (prefs_util_->IsCrosSetting(it.first))
+        cros_settings_subscription_map_.erase(it.first);
+      else
         FindRegistrarForPref(it.first)->Remove(it.first);
     }
   }
@@ -103,7 +122,6 @@ void SettingsPrivateEventRouter::StartOrStopListeningForPrefsChanges() {
 }
 
 void SettingsPrivateEventRouter::OnPreferenceChanged(
-    PrefService* service,
     const std::string& pref_name) {
   EventRouter* event_router = EventRouter::Get(context_);
   if (!event_router->HasEventListener(
@@ -111,9 +129,8 @@ void SettingsPrivateEventRouter::OnPreferenceChanged(
     return;
   }
 
-  Profile* profile = Profile::FromBrowserContext(context_);
   api::settings_private::PrefObject* pref_object =
-      prefs_util::GetPref(profile, pref_name).release();
+      prefs_util_->GetPref(pref_name).release();
 
   std::vector<linked_ptr<api::settings_private::PrefObject>> prefs;
   prefs.push_back(linked_ptr<api::settings_private::PrefObject>(pref_object));

chrome/browser/extensions/api/settings_private/settings_private_event_router.h

@@ -6,6 +6,8 @@
 #define CHROME_BROWSER_EXTENSIONS_API_SETTINGS_PRIVATE_SETTINGS_PRIVATE_EVENT_ROUTER_H_
 
 #include "base/prefs/pref_change_registrar.h"
+#include "chrome/browser/chromeos/settings/cros_settings.h"
+#include "chrome/browser/extensions/api/settings_private/prefs_util.h"
 #include "components/keyed_service/core/keyed_service.h"
 #include "extensions/browser/event_router.h"
 
@@ -28,7 +30,6 @@ class SettingsPrivateEventRouter : public KeyedService,
   ~SettingsPrivateEventRouter() override;
 
  protected:
-  SettingsPrivateEventRouter() {}
   explicit SettingsPrivateEventRouter(content::BrowserContext* context);
 
   // KeyedService overrides:
@@ -51,13 +52,20 @@ class SettingsPrivateEventRouter : public KeyedService,
   // Otherwise, we want to unregister and not be listening for pref changes.
   void StartOrStopListeningForPrefsChanges();
 
-  void OnPreferenceChanged(PrefService* service, const std::string& pref_name);
+  void OnPreferenceChanged(const std::string& pref_name);
 
   PrefChangeRegistrar* FindRegistrarForPref(const std::string& pref_name);
 
+  typedef std::map<std::string,
+                   linked_ptr<chromeos::CrosSettings::ObserverSubscription>>
+      SubscriptionMap;
+  SubscriptionMap cros_settings_subscription_map_;
+
   content::BrowserContext* context_;
   bool listening_;
 
+  scoped_ptr<PrefsUtil> prefs_util_;
+
   DISALLOW_COPY_AND_ASSIGN(SettingsPrivateEventRouter);
 };
 

chrome/test/data/extensions/api_test/settings_private/test.js

@@ -28,6 +28,16 @@ var availableTests = [
           chrome.test.succeed();
         });
   },
+  function setPref_CrOSSetting() {
+    chrome.settingsPrivate.setPref(
+        'cros.accounts.allowBWSI',
+        false,
+        kTestPageId,
+        function(success) {
+          callbackResult(success);
+          chrome.test.succeed();
+        });
+  },
   function getPref() {
     chrome.settingsPrivate.getPref(
         kTestPrefName,
@@ -37,6 +47,15 @@ var availableTests = [
           chrome.test.succeed();
         });
   },
+  function getPref_CrOSSetting() {
+    chrome.settingsPrivate.getPref(
+        'cros.accounts.allowBWSI',
+        function(value) {
+          chrome.test.assertTrue(value !== null);
+          callbackResult(true);
+          chrome.test.succeed();
+        });
+  },
   function getAllPrefs() {
     chrome.settingsPrivate.getAllPrefs(
         function(prefs) {
@@ -60,6 +79,21 @@ var availableTests = [
         kTestPageId,
         function() {});
   },
+  function onPrefsChanged_CrOSSetting() {
+    chrome.settingsPrivate.onPrefsChanged.addListener(function(prefs) {
+      chrome.test.assertTrue(prefs.length > 0);
+      chrome.test.assertEq('cros.accounts.allowBWSI', prefs[0].key);
+      chrome.test.assertEq(false, prefs[0].value);
+      callbackResult(true);
+      chrome.test.succeed();
+    });
+
+    chrome.settingsPrivate.setPref(
+        'cros.accounts.allowBWSI',
+        false,
+        kTestPageId,
+        function() {});
+  },
 ];
 
 var testToRun = window.location.search.substring(1);