Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1910323002:
Allow whitelisted prefs to be displayed in ChromeOS in chrome://local-state. (Closed)
DescriptionAllow whitelisted prefs to be displayed in ChromeOS in chrome://local-state.
BUG=506331
Committed: https://crrev.com/dff82fa93471550370c3ce574a8ae31e911f7759
Cr-Commit-Position: refs/heads/master@{#389871}
Patch Set 1 #Patch Set 2 : #Patch Set 3 : #Patch Set 4 : #Patch Set 5 : #
Total comments: 2
Patch Set 6 : #Patch Set 7 : #
Total comments: 12
Patch Set 8 : #
Total comments: 2
Patch Set 9 : #
Total comments: 13
Patch Set 10 : #
Total comments: 6
Patch Set 11 : #
Total comments: 2
Patch Set 12 : #Patch Set 13 : #
Messages
Total messages: 41 (17 generated)
hamelphi@chromium.org changed reviewers: + asvitkine@chromium.org, bauerb@chromium.org
Let's see what Bernhard says about the new API. We should also have some unit tests for this, although maybe let's wait to see if Bernhard prefers a different structure for this.
https://codereview.chromium.org/1910323002/diff/80001/chrome/browser/ui/webui... File chrome/browser/ui/webui/local_state/local_state_ui.cc (right): https://codereview.chromium.org/1910323002/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/local_state/local_state_ui.cc:60: ->GetWhitelistedPreferenceValuesOmitDefaults(whitelisted_prefixes)); Couldn't we just do the whitelisting here? We would end up making a couple of unnecessary copies of the values outside of the whitelist, but it's just for a debugging page, and we wouldn't be blocking the UI thread for longer than we would for full GetPreferenceValuesOmitDefaults() anyway.
https://codereview.chromium.org/1910323002/diff/80001/chrome/browser/ui/webui... File chrome/browser/ui/webui/local_state/local_state_ui.cc (right): https://codereview.chromium.org/1910323002/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/local_state/local_state_ui.cc:60: ->GetWhitelistedPreferenceValuesOmitDefaults(whitelisted_prefixes)); On 2016/04/22 18:24:38, Bernhard Bauer wrote: > Couldn't we just do the whitelisting here? We would end up making a couple of > unnecessary copies of the values outside of the whitelist, but it's just for a > debugging page, and we wouldn't be blocking the UI thread for longer than we > would for full GetPreferenceValuesOmitDefaults() anyway. I was on the fence between doing the filtering here or in pref_service. I don't feel strongly one way or another. I moved the filtering here instead.
Can you add a unit test? https://codereview.chromium.org/1910323002/diff/120001/chrome/browser/ui/webu... File chrome/browser/ui/webui/local_state/local_state_ui.cc (right): https://codereview.chromium.org/1910323002/diff/120001/chrome/browser/ui/webu... chrome/browser/ui/webui/local_state/local_state_ui.cc:54: bool HasValidPrefix(const std::string& pref_name, These should be in the anon namespace or they can be static so you can test them. https://codereview.chromium.org/1910323002/diff/120001/chrome/browser/ui/webu... chrome/browser/ui/webui/local_state/local_state_ui.cc:63: void FilterPrefs(const std::vector<std::string> valid_prefixes, Nit: const ref https://codereview.chromium.org/1910323002/diff/120001/chrome/browser/ui/webu... chrome/browser/ui/webui/local_state/local_state_ui.cc:71: for (const std::string pref_to_remove : prefs_to_remove) { const ref please.
https://codereview.chromium.org/1910323002/diff/120001/chrome/browser/ui/webu... File chrome/browser/ui/webui/local_state/local_state_ui.cc (right): https://codereview.chromium.org/1910323002/diff/120001/chrome/browser/ui/webu... chrome/browser/ui/webui/local_state/local_state_ui.cc:73: DCHECK(prefs->Remove(pref_to_remove, &removed_value)); 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); https://codereview.chromium.org/1910323002/diff/120001/chrome/browser/ui/webu... chrome/browser/ui/webui/local_state/local_state_ui.cc:76: #endif // !defined(OS_CHROMEOS) Remove the exclamation mark.
https://codereview.chromium.org/1910323002/diff/120001/chrome/browser/ui/webu... File chrome/browser/ui/webui/local_state/local_state_ui.cc (right): https://codereview.chromium.org/1910323002/diff/120001/chrome/browser/ui/webu... chrome/browser/ui/webui/local_state/local_state_ui.cc:54: bool HasValidPrefix(const std::string& pref_name, On 2016/04/22 20:35:51, Alexei Svitkine wrote: > These should be in the anon namespace or they can be static so you can test > them. 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. https://codereview.chromium.org/1910323002/diff/120001/chrome/browser/ui/webu... chrome/browser/ui/webui/local_state/local_state_ui.cc:63: void FilterPrefs(const std::vector<std::string> valid_prefixes, On 2016/04/22 20:35:51, Alexei Svitkine wrote: > Nit: const ref Done. https://codereview.chromium.org/1910323002/diff/120001/chrome/browser/ui/webu... chrome/browser/ui/webui/local_state/local_state_ui.cc:71: for (const std::string pref_to_remove : prefs_to_remove) { On 2016/04/22 20:35:51, Alexei Svitkine wrote: > const ref please. Done. https://codereview.chromium.org/1910323002/diff/120001/chrome/browser/ui/webu... chrome/browser/ui/webui/local_state/local_state_ui.cc:73: DCHECK(prefs->Remove(pref_to_remove, &removed_value)); On 2016/04/25 09:16:28, Bernhard Bauer wrote: > 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); Oh yeah right. Thanks for catching that. Done. https://codereview.chromium.org/1910323002/diff/120001/chrome/browser/ui/webu... chrome/browser/ui/webui/local_state/local_state_ui.cc:76: #endif // !defined(OS_CHROMEOS) On 2016/04/25 09:16:28, Bernhard Bauer wrote: > Remove the exclamation mark. Done.
lgtm https://codereview.chromium.org/1910323002/diff/140001/chrome/browser/ui/webu... File chrome/browser/ui/webui/local_state/local_state_ui.cc (right): https://codereview.chromium.org/1910323002/diff/140001/chrome/browser/ui/webu... chrome/browser/ui/webui/local_state/local_state_ui.cc:86: #endif // !defined(OS_CHROMEOS) Remove the exclamation mark here as well.
https://codereview.chromium.org/1910323002/diff/120001/chrome/browser/ui/webu... File chrome/browser/ui/webui/local_state/local_state_ui.cc (right): https://codereview.chromium.org/1910323002/diff/120001/chrome/browser/ui/webu... chrome/browser/ui/webui/local_state/local_state_ui.cc:54: bool HasValidPrefix(const std::string& pref_name, On 2016/04/25 15:58:01, hamelphi wrote: > On 2016/04/22 20:35:51, Alexei Svitkine wrote: > > These should be in the anon namespace or they can be static so you can test > > them. > > 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. 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.
https://codereview.chromium.org/1910323002/diff/120001/chrome/browser/ui/webu... File chrome/browser/ui/webui/local_state/local_state_ui.cc (right): https://codereview.chromium.org/1910323002/diff/120001/chrome/browser/ui/webu... chrome/browser/ui/webui/local_state/local_state_ui.cc:54: bool HasValidPrefix(const std::string& pref_name, On 2016/04/25 16:24:55, Alexei Svitkine wrote: > On 2016/04/25 15:58:01, hamelphi wrote: > > On 2016/04/22 20:35:51, Alexei Svitkine wrote: > > > These should be in the anon namespace or they can be static so you can test > > > them. > > > > 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. > > 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. Done. https://codereview.chromium.org/1910323002/diff/140001/chrome/browser/ui/webu... File chrome/browser/ui/webui/local_state/local_state_ui.cc (right): https://codereview.chromium.org/1910323002/diff/140001/chrome/browser/ui/webu... chrome/browser/ui/webui/local_state/local_state_ui.cc:86: #endif // !defined(OS_CHROMEOS) On 2016/04/25 16:17:58, Bernhard Bauer wrote: > Remove the exclamation mark here as well. Done.
Looks good, just small style things. https://codereview.chromium.org/1910323002/diff/160001/chrome/browser/ui/webu... File chrome/browser/ui/webui/local_state/local_state_ui.cc (right): https://codereview.chromium.org/1910323002/diff/160001/chrome/browser/ui/webu... chrome/browser/ui/webui/local_state/local_state_ui.cc:20: namespace internal { Move the internal namespace below the anon namespace. Then, HasValidPrefix() can be in the normal anon namespace. https://codereview.chromium.org/1910323002/diff/160001/chrome/browser/ui/webu... chrome/browser/ui/webui/local_state/local_state_ui.cc:22: namespace { Nit: Add an empty line below this above above the closing. https://codereview.chromium.org/1910323002/diff/160001/chrome/browser/ui/webu... chrome/browser/ui/webui/local_state/local_state_ui.cc:23: bool HasValidPrefix(const std::string& pref_name, Nit: Add a short comment above this function. https://codereview.chromium.org/1910323002/diff/160001/chrome/browser/ui/webu... File chrome/browser/ui/webui/local_state/local_state_ui.h (right): https://codereview.chromium.org/1910323002/diff/160001/chrome/browser/ui/webu... chrome/browser/ui/webui/local_state/local_state_ui.h:12: #include "base/values.h" Nit: You can forward declare base::DictionaryValue instead. https://codereview.chromium.org/1910323002/diff/160001/chrome/browser/ui/webu... chrome/browser/ui/webui/local_state/local_state_ui.h:15: namespace internal { Nit: Add a short comment about the purpose of this namespace (i.e. for testing). https://codereview.chromium.org/1910323002/diff/160001/chrome/browser/ui/webu... chrome/browser/ui/webui/local_state/local_state_ui.h:16: void FilterPrefs(const std::vector<std::string>& valid_prefixes, Nit: Add a short comment about what this function does.
https://codereview.chromium.org/1910323002/diff/160001/chrome/browser/ui/webu... File chrome/browser/ui/webui/local_state/local_state_ui.cc (right): https://codereview.chromium.org/1910323002/diff/160001/chrome/browser/ui/webu... chrome/browser/ui/webui/local_state/local_state_ui.cc:20: namespace internal { On 2016/04/25 21:30:13, Alexei Svitkine wrote: > Move the internal namespace below the anon namespace. Then, HasValidPrefix() can > be in the normal anon namespace. Done. https://codereview.chromium.org/1910323002/diff/160001/chrome/browser/ui/webu... chrome/browser/ui/webui/local_state/local_state_ui.cc:22: namespace { On 2016/04/25 21:30:13, Alexei Svitkine wrote: > Nit: Add an empty line below this above above the closing. Done. https://codereview.chromium.org/1910323002/diff/160001/chrome/browser/ui/webu... chrome/browser/ui/webui/local_state/local_state_ui.cc:23: bool HasValidPrefix(const std::string& pref_name, On 2016/04/25 21:30:13, Alexei Svitkine wrote: > Nit: Add a short comment above this function. Done. https://codereview.chromium.org/1910323002/diff/160001/chrome/browser/ui/webu... File chrome/browser/ui/webui/local_state/local_state_ui.h (right): https://codereview.chromium.org/1910323002/diff/160001/chrome/browser/ui/webu... chrome/browser/ui/webui/local_state/local_state_ui.h:12: #include "base/values.h" On 2016/04/25 21:30:13, Alexei Svitkine wrote: > Nit: You can forward declare base::DictionaryValue instead. Done. However, can you explain why this is better in this case? In the Google Style Guide, forward declarations are frowned upon, since it can potentially cause more trouble down the road (go/fwd_decl_harmful). https://codereview.chromium.org/1910323002/diff/160001/chrome/browser/ui/webu... chrome/browser/ui/webui/local_state/local_state_ui.h:15: namespace internal { On 2016/04/25 21:30:13, Alexei Svitkine wrote: > Nit: Add a short comment about the purpose of this namespace (i.e. for testing). Done. https://codereview.chromium.org/1910323002/diff/160001/chrome/browser/ui/webu... chrome/browser/ui/webui/local_state/local_state_ui.h:16: void FilterPrefs(const std::vector<std::string>& valid_prefixes, On 2016/04/25 21:30:13, Alexei Svitkine wrote: > Nit: Add a short comment about what this function does. Done.
Thanks! LGTM % remaining comments. https://codereview.chromium.org/1910323002/diff/160001/chrome/browser/ui/webu... File chrome/browser/ui/webui/local_state/local_state_ui.h (right): https://codereview.chromium.org/1910323002/diff/160001/chrome/browser/ui/webu... chrome/browser/ui/webui/local_state/local_state_ui.h:12: #include "base/values.h" On 2016/04/26 15:25:28, hamelphi wrote: > On 2016/04/25 21:30:13, Alexei Svitkine wrote: > > Nit: You can forward declare base::DictionaryValue instead. > > Done. > > However, can you explain why this is better in this case? In the Google Style > Guide, forward declarations are frowned upon, since it can potentially cause > more trouble down the road (go/fwd_decl_harmful). Chromium diverges from Google Style here - as the build time improvements are more relevant for Chromium since most builds are done locally. See: https://www.chromium.org/developers/coding-style#TOC-Forward-declarations-vs.... https://codereview.chromium.org/1910323002/diff/180001/chrome/browser/ui/webu... File chrome/browser/ui/webui/local_state/local_state_ui.cc (right): https://codereview.chromium.org/1910323002/diff/180001/chrome/browser/ui/webu... chrome/browser/ui/webui/local_state/local_state_ui.cc:23: #if defined(OS_CHROMEOS) Nit: Add a comment above this block. e.g.: // On ChromeOS, the local state file contains some information about other // user accounts which we don't want to expose to other users. Use a whitelist // to only show variations and UMA related fields which don't contain PII. https://codereview.chromium.org/1910323002/diff/180001/chrome/browser/ui/webu... File chrome/browser/ui/webui/local_state/local_state_ui.h (right): https://codereview.chromium.org/1910323002/diff/180001/chrome/browser/ui/webu... chrome/browser/ui/webui/local_state/local_state_ui.h:18: } // namespace base Nit: No need for this end comment for a forward decl. However, please add one for the internal namespace where you declare a function below. Same comment about the empty lines around namespace - no need for the fwd decl, but add them below. https://codereview.chromium.org/1910323002/diff/180001/chrome/browser/ui/webu... chrome/browser/ui/webui/local_state/local_state_ui.h:20: // Namespace for exposing the method for unittests. Nit: unittests -> unit tests
hamelphi@google.com changed reviewers: + hamelphi@google.com
https://codereview.chromium.org/1910323002/diff/180001/chrome/browser/ui/webu... File chrome/browser/ui/webui/local_state/local_state_ui.cc (right): https://codereview.chromium.org/1910323002/diff/180001/chrome/browser/ui/webu... chrome/browser/ui/webui/local_state/local_state_ui.cc:23: #if defined(OS_CHROMEOS) On 2016/04/26 15:33:35, Alexei Svitkine wrote: > Nit: Add a comment above this block. > > e.g.: > > // On ChromeOS, the local state file contains some information about other > // user accounts which we don't want to expose to other users. Use a whitelist > // to only show variations and UMA related fields which don't contain PII. Done. https://codereview.chromium.org/1910323002/diff/180001/chrome/browser/ui/webu... File chrome/browser/ui/webui/local_state/local_state_ui.h (right): https://codereview.chromium.org/1910323002/diff/180001/chrome/browser/ui/webu... chrome/browser/ui/webui/local_state/local_state_ui.h:18: } // namespace base On 2016/04/26 15:33:35, Alexei Svitkine wrote: > Nit: No need for this end comment for a forward decl. However, please add one > for the internal namespace where you declare a function below. > > Same comment about the empty lines around namespace - no need for the fwd decl, > but add them below. Done. https://codereview.chromium.org/1910323002/diff/180001/chrome/browser/ui/webu... chrome/browser/ui/webui/local_state/local_state_ui.h:20: // Namespace for exposing the method for unittests. On 2016/04/26 15:33:35, Alexei Svitkine wrote: > Nit: unittests -> unit tests Done.
Description was changed from ========== Allow whitelisted prefs to be displayed in ChromeOS in chrome://local-state. BUG=506331 ========== to ========== Allow whitelisted prefs to be displayed in ChromeOS in chrome://local-state. BUG=506331 ==========
hamelphi@google.com changed reviewers: - hamelphi@google.com
The CQ bit was checked by hamelphi@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org, asvitkine@chromium.org Link to the patchset: https://codereview.chromium.org/1910323002/#ps200001 (title: " ")
The CQ bit was unchecked by hamelphi@google.com
hamelphi@chromium.org changed reviewers: - hamelphi@google.com
The CQ bit was checked by hamelphi@chromium.org
Sorry, one last thing. Otherwise, all good - thanks! LGTM. https://codereview.chromium.org/1910323002/diff/200001/chrome/browser/ui/webu... File chrome/browser/ui/webui/local_state/local_state_ui.cc (right): https://codereview.chromium.org/1910323002/diff/200001/chrome/browser/ui/webu... chrome/browser/ui/webui/local_state/local_state_ui.cc:84: if (pref_name.compare(0, prefix.size(), prefix) == 0) Oops, missed this earlier. Can you change this to StartsWith() from: https://code.google.com/p/chromium/codesearch#chromium/src/base/strings/strin...
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1910323002/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1910323002/200001
The CQ bit was unchecked by asvitkine@chromium.org
https://codereview.chromium.org/1910323002/diff/200001/chrome/browser/ui/webu... File chrome/browser/ui/webui/local_state/local_state_ui.cc (right): https://codereview.chromium.org/1910323002/diff/200001/chrome/browser/ui/webu... chrome/browser/ui/webui/local_state/local_state_ui.cc:84: if (pref_name.compare(0, prefix.size(), prefix) == 0) On 2016/04/26 17:46:35, Alexei Svitkine wrote: > Oops, missed this earlier. Can you change this to StartsWith() from: > > https://code.google.com/p/chromium/codesearch#chromium/src/base/strings/strin... Done.
The CQ bit was checked by hamelphi@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org, asvitkine@chromium.org Link to the patchset: https://codereview.chromium.org/1910323002/#ps220001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1910323002/220001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1910323002/220001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_chromium_gn_compile_rel on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_chro...) linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by hamelphi@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org, asvitkine@chromium.org Link to the patchset: https://codereview.chromium.org/1910323002/#ps240001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1910323002/240001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1910323002/240001
Message was sent while issue was closed.
Description was changed from ========== Allow whitelisted prefs to be displayed in ChromeOS in chrome://local-state. BUG=506331 ========== to ========== Allow whitelisted prefs to be displayed in ChromeOS in chrome://local-state. BUG=506331 ==========
Message was sent while issue was closed.
Committed patchset #13 (id:240001)
Message was sent while issue was closed.
Description was changed from ========== Allow whitelisted prefs to be displayed in ChromeOS in chrome://local-state. BUG=506331 ========== to ========== Allow whitelisted prefs to be displayed in ChromeOS in chrome://local-state. BUG=506331 Committed: https://crrev.com/dff82fa93471550370c3ce574a8ae31e911f7759 Cr-Commit-Position: refs/heads/master@{#389871} ==========
Message was sent while issue was closed.
Patchset 13 (id:??) landed as https://crrev.com/dff82fa93471550370c3ce574a8ae31e911f7759 Cr-Commit-Position: refs/heads/master@{#389871} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
