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

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

Index: chrome/browser/ui/webui/local_state/local_state_ui.cc
diff --git a/chrome/browser/ui/webui/local_state/local_state_ui.cc b/chrome/browser/ui/webui/local_state/local_state_ui.cc
index 22740b170aed2da1de510627ecdb5e800432cc78..c91b6e9d4e71d0a72f1b30f7783c40785a7296db 100644
--- a/chrome/browser/ui/webui/local_state/local_state_ui.cc
+++ b/chrome/browser/ui/webui/local_state/local_state_ui.cc
@@ -50,11 +50,39 @@ void LocalStateUIHandler::RegisterMessages() {
                                 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);
@@ -64,7 +92,6 @@ void LocalStateUIHandler::HandleRequestJson(const base::ListValue* args) {
 
   web_ui()->CallJavascriptFunction("localState.setLocalState",
                                    base::StringValue(json));
-#endif  // !defined(OS_CHROMEOS)
 }
 
 }  // namespace