Allow whitelisted prefs to be displayed in ChromeOS in chrome://local-state.

chrome/browser/ui/webui/local_state/local_state_ui.cc

 // Copyright 2014 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/ui/webui/local_state/local_state_ui.h"
 
 #include <string>
 
 #include "base/json/json_string_value_serializer.h"
 #include "base/macros.h"
@@ skipping 32 matching lines @@
 
 LocalStateUIHandler::~LocalStateUIHandler() {
 }
 
 void LocalStateUIHandler::RegisterMessages() {
   web_ui()->RegisterMessageCallback(
       "requestJson", base::Bind(&LocalStateUIHandler::HandleRequestJson,
                                 base::Unretained(this)));
 }
 
+#if defined(OS_CHROMEOS)
+bool HasValidPrefix(const std::string& pref_name,

[Alexei Svitkine (slow), 2016/04/22 20:35:51]

These should be in the anon namespace or they can be static so you can test
them.

[hamelphi, 2016/04/25 15:58:01]

They are already under anon namespace. The whole LocalStateUIHandler class is.
If you insist on unit tests, maybe these functions would make more sense to make
them public as static functions of pref_service (like they were before). I don't
think it would be easy to test these functions as standalone functions since
they won't be compiled unless OS_CHROMEOS is defined. If I don't put them under
the if statement, the compiler complains about unused methods when OS_CHROMEOS
is not defined.

Let me know what you prefer.

[Alexei Svitkine (slow), 2016/04/25 16:24:55]

Ah right. I forgot that LocalStateUIHandler is in the anon namespace.

Anyway, keeping them in the anon namespace is fine, but place them either above
or below all the LocalStateUIHandler code. Right now, they're in the middle with
methods of that class being defined above or below. Also, please add 1-line
comments for them.

As far as unit testing, You can make it so the code is compiled on all
platforms, but only used on ChromeOS.

e.g.

#if defined(OS_CHROMEOS)
#define ENABLE_FILTERING true
#else
#define ENABLE_FILTERING false
#endif

if (ENABLE_FILTERING) {
   std::vector<std::string> whitelisted_prefixes = {...};
   FilterPrefs(whitelisted_prefixes, local_state_values.get());
}

In terms of how to expose these for testing, one way is to put them in an
internal namespace ("namespace internal") instead of the anon one and expose
their signature from headers, so that unit tests can test FilterPrefs function.

[hamelphi, 2016/04/25 21:05:14]

Done.

+                    const std::vector<std::string> valid_prefixes) {
+  for (const std::string& prefix : valid_prefixes) {
+    if (pref_name.compare(0, prefix.size(), prefix) == 0)
+      return true;
+  }
+  return false;
+}
+
+void FilterPrefs(const std::vector<std::string> valid_prefixes,

[Alexei Svitkine (slow), 2016/04/22 20:35:51]

Nit: const ref

[hamelphi, 2016/04/25 15:58:01]

Done.

+                 base::DictionaryValue* prefs) {
+  std::vector<std::string> prefs_to_remove;
+  for (base::DictionaryValue::Iterator it(*prefs); !it.IsAtEnd();
+       it.Advance()) {
+    if (!HasValidPrefix(it.key(), valid_prefixes))
+      prefs_to_remove.push_back(it.key());
+  }
+  for (const std::string pref_to_remove : prefs_to_remove) {

[Alexei Svitkine (slow), 2016/04/22 20:35:51]

const ref please.

[hamelphi, 2016/04/25 15:58:01]

Done.

+    std::unique_ptr<base::Value> removed_value;
+    DCHECK(prefs->Remove(pref_to_remove, &removed_value));

[Bernhard Bauer, 2016/04/25 09:16:28]

In a Release build, this will be compiled out, so the prefs won't actually be
removed. What you want to do instead is this:

  const bool success = prefs->Remove(...);
  DCHECK(success);

[hamelphi, 2016/04/25 15:58:01]

Oh yeah right. Thanks for catching that. Done.

+  }
+}
+#endif  // !defined(OS_CHROMEOS)

[Bernhard Bauer, 2016/04/25 09:16:28]

Remove the exclamation mark.

[hamelphi, 2016/04/25 15:58:01]

Done.

+
 void LocalStateUIHandler::HandleRequestJson(const base::ListValue* args) {
-#if !defined(OS_CHROMEOS)
   std::unique_ptr<base::DictionaryValue> local_state_values(
       g_browser_process->local_state()->GetPreferenceValuesOmitDefaults());
-
+#if defined(OS_CHROMEOS)
+  std::vector<std::string> whitelisted_prefixes = {
+      "variations", "user_experience_metrics", "uninstall_metrics"};
+  FilterPrefs(whitelisted_prefixes, local_state_values.get());
+#endif  // !defined(OS_CHROMEOS)
   std::string json;
   JSONStringValueSerializer serializer(&json);
   serializer.set_pretty_print(true);
   bool result = serializer.Serialize(*local_state_values);
   if (!result)
     json = "Error loading Local State file.";
 
   web_ui()->CallJavascriptFunction("localState.setLocalState",
                                    base::StringValue(json));
-#endif  // !defined(OS_CHROMEOS)
 }
 
 }  // namespace
 
 LocalStateUI::LocalStateUI(content::WebUI* web_ui) : WebUIController(web_ui) {
   // Set up the chrome://local-state source.
   content::WebUIDataSource* html_source =
       content::WebUIDataSource::Create(chrome::kChromeUILocalStateHost);
   html_source->SetDefaultResource(IDR_LOCAL_STATE_HTML);
   html_source->AddResourcePath("local_state.js", IDR_LOCAL_STATE_JS);
   content::WebUIDataSource::Add(Profile::FromWebUI(web_ui), html_source);
 
   // AddMessageHandler takes ownership of LocalStateUIHandler.
   web_ui->AddMessageHandler(new LocalStateUIHandler);
 }
 
 LocalStateUI::~LocalStateUI() {
 }