Consistently decorate pref values sent to the settings UI code

Consistently decorate pref values sent to the settings UI code

Pref values are provided to the settings UI code by three different
classes. Each decorates the values slightly differently, adding
different metadata fields or none at all. This CL makes the classes
use a consistent decoration with identically named metadata fields,
simplifying the handling of pref values on the JS side.

BUG=104955
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=150527

[bartfab (slow), 2012-08-01 17:17:19]

Hi everyone, could you please have a look?

I believe you should be the most appropriate owners for these parts of the CL:

* jhawkins: options2, proxy_cros_settings_parser.cc
* nkostylev: options2/chromeos
* darin: testing_automation_provider_chromeos.cc

[darin (slow to review), 2012-08-02 06:19:38]

rubber stamp LGTM for automation

[Nikita (slow), 2012-08-02 07:46:11]

lgtm

[James Hawkins, 2012-08-02 20:52:07]

Per-offline, waiting for the next patch which should contain tests.

[bartfab (slow), 2012-08-07 17:51:29]

Hi James

I see now why there was no test for this stuff: It took the better part of a
week to come up with a comprehensive browser test. Please have a look at the
updated patch.

[James Hawkins, 2012-08-07 17:59:04]

https://chromiumcodereview.appspot.com/10834109/diff/8001/chrome/browser/auto...
File chrome/browser/automation/testing_automation_provider_chromeos.cc (right):

https://chromiumcodereview.appspot.com/10834109/diff/8001/chrome/browser/auto...
chrome/browser/automation/testing_automation_provider_chromeos.cc:96: delete
setting;
nit: Wrap the returned value in a scoped_ptr instead of manually deleting.

https://chromiumcodereview.appspot.com/10834109/diff/8001/chrome/browser/ui/w...
File chrome/browser/ui/webui/options2/preferences_browsertest.cc (right):

https://chromiumcodereview.appspot.com/10834109/diff/8001/chrome/browser/ui/w...
chrome/browser/ui/webui/options2/preferences_browsertest.cc:79: base::Value*
expected) {
nit: Document that this method takes ownership of |expected|.

https://chromiumcodereview.appspot.com/10834109/diff/8001/chrome/browser/ui/w...
chrome/browser/ui/webui/options2/preferences_browsertest.cc:80:
ASSERT_TRUE(dict);
These are internal methods so you shouldn't be asserting on the ptr parameters.

https://chromiumcodereview.appspot.com/10834109/diff/8001/chrome/browser/ui/w...
chrome/browser/ui/webui/options2/preferences_browsertest.cc:83:
ASSERT_TRUE(dict->Get(key, &actual)) << "Was checking key: " << key;
These two should be EXPECT_TRUE instead of ASSERT_TRUE.

https://chromiumcodereview.appspot.com/10834109/diff/8001/chrome/browser/ui/w...
File chrome/browser/ui/webui/options2/preferences_browsertest.h (right):

https://chromiumcodereview.appspot.com/10834109/diff/8001/chrome/browser/ui/w...
chrome/browser/ui/webui/options2/preferences_browsertest.h:17: class
DictionaryValue;
nit: Forward declarations inside namespaces should not be indented.

https://chromiumcodereview.appspot.com/10834109/diff/8001/chrome/browser/ui/w...
chrome/browser/ui/webui/options2/preferences_browsertest.h:25: class
PreferencesBrowserTest : public InProcessBrowserTest {
nit: Document class.

https://chromiumcodereview.appspot.com/10834109/diff/8001/chrome/browser/ui/w...
chrome/browser/ui/webui/options2/preferences_browsertest.h:28: 
nit: 

// InProcessBrowserTest implementation.

https://chromiumcodereview.appspot.com/10834109/diff/8001/chrome/browser/ui/w...
chrome/browser/ui/webui/options2/preferences_browsertest.h:33: void
VerifyValue(const base::DictionaryValue* dict,
nit: Document this block of similar methods.

https://chromiumcodereview.appspot.com/10834109/diff/8001/chrome/browser/ui/w...
chrome/browser/ui/webui/options2/preferences_browsertest.h:55: void
FetchPrefs(const std::vector<std::string>& names,
nit: Document method.

https://chromiumcodereview.appspot.com/10834109/diff/8001/chrome/browser/ui/w...
chrome/browser/ui/webui/options2/preferences_browsertest.h:58: void
SetUserPolicies(const std::vector<std::string>& names,
nit: Document method.

https://chromiumcodereview.appspot.com/10834109/diff/8001/chrome/browser/ui/w...
chrome/browser/ui/webui/options2/preferences_browsertest.h:62: void
DeleteValues(std::vector<base::Value*>& values);
nit: Document method.

https://chromiumcodereview.appspot.com/10834109/diff/8001/chrome/browser/ui/w...
chrome/browser/ui/webui/options2/preferences_browsertest.h:64:
content::RenderViewHost* render_view_host_;
nit: Document member variable.

https://chromiumcodereview.appspot.com/10834109/diff/8001/chrome/browser/ui/w...
chrome/browser/ui/webui/options2/preferences_browsertest.h:66:
policy::MockConfigurationPolicyProvider policy_provider_;
nit: Document member variable.

https://chromiumcodereview.appspot.com/10834109/diff/8001/chrome/browser/ui/w...
chrome/browser/ui/webui/options2/preferences_browsertest.h:67: };
DISALLOW_COPY_AND_ASSIGN?

[bartfab (slow), 2012-08-07 18:27:04]

https://chromiumcodereview.appspot.com/10834109/diff/8001/chrome/browser/auto...
File chrome/browser/automation/testing_automation_provider_chromeos.cc (right):

https://chromiumcodereview.appspot.com/10834109/diff/8001/chrome/browser/auto...
chrome/browser/automation/testing_automation_provider_chromeos.cc:96: delete
setting;
On 2012/08/07 17:59:05, James Hawkins wrote:
> nit: Wrap the returned value in a scoped_ptr instead of manually deleting.

Done.

https://chromiumcodereview.appspot.com/10834109/diff/8001/chrome/browser/ui/w...
File chrome/browser/ui/webui/options2/preferences_browsertest.cc (right):

https://chromiumcodereview.appspot.com/10834109/diff/8001/chrome/browser/ui/w...
chrome/browser/ui/webui/options2/preferences_browsertest.cc:79: base::Value*
expected) {
On 2012/08/07 17:59:05, James Hawkins wrote:
> nit: Document that this method takes ownership of |expected|.

Done.

https://chromiumcodereview.appspot.com/10834109/diff/8001/chrome/browser/ui/w...
chrome/browser/ui/webui/options2/preferences_browsertest.cc:80:
ASSERT_TRUE(dict);
On 2012/08/07 17:59:05, James Hawkins wrote:
> These are internal methods so you shouldn't be asserting on the ptr
parameters.

Done.

https://chromiumcodereview.appspot.com/10834109/diff/8001/chrome/browser/ui/w...
chrome/browser/ui/webui/options2/preferences_browsertest.cc:83:
ASSERT_TRUE(dict->Get(key, &actual)) << "Was checking key: " << key;
On 2012/08/07 17:59:05, James Hawkins wrote:
> These two should be EXPECT_TRUE instead of ASSERT_TRUE.

Done.

https://chromiumcodereview.appspot.com/10834109/diff/8001/chrome/browser/ui/w...
File chrome/browser/ui/webui/options2/preferences_browsertest.h (right):

https://chromiumcodereview.appspot.com/10834109/diff/8001/chrome/browser/ui/w...
chrome/browser/ui/webui/options2/preferences_browsertest.h:17: class
DictionaryValue;
On 2012/08/07 17:59:05, James Hawkins wrote:
> nit: Forward declarations inside namespaces should not be indented.

Done.

https://chromiumcodereview.appspot.com/10834109/diff/8001/chrome/browser/ui/w...
chrome/browser/ui/webui/options2/preferences_browsertest.h:25: class
PreferencesBrowserTest : public InProcessBrowserTest {
On 2012/08/07 17:59:05, James Hawkins wrote:
> nit: Document class.

Done.

https://chromiumcodereview.appspot.com/10834109/diff/8001/chrome/browser/ui/w...
chrome/browser/ui/webui/options2/preferences_browsertest.h:28: 
On 2012/08/07 17:59:05, James Hawkins wrote:
> nit: 
> 
> // InProcessBrowserTest implementation.

Done.

https://chromiumcodereview.appspot.com/10834109/diff/8001/chrome/browser/ui/w...
chrome/browser/ui/webui/options2/preferences_browsertest.h:33: void
VerifyValue(const base::DictionaryValue* dict,
On 2012/08/07 17:59:05, James Hawkins wrote:
> nit: Document this block of similar methods.

Done.

https://chromiumcodereview.appspot.com/10834109/diff/8001/chrome/browser/ui/w...
chrome/browser/ui/webui/options2/preferences_browsertest.h:55: void
FetchPrefs(const std::vector<std::string>& names,
On 2012/08/07 17:59:05, James Hawkins wrote:
> nit: Document method.

Done.

https://chromiumcodereview.appspot.com/10834109/diff/8001/chrome/browser/ui/w...
chrome/browser/ui/webui/options2/preferences_browsertest.h:58: void
SetUserPolicies(const std::vector<std::string>& names,
On 2012/08/07 17:59:05, James Hawkins wrote:
> nit: Document method.

Done.

https://chromiumcodereview.appspot.com/10834109/diff/8001/chrome/browser/ui/w...
chrome/browser/ui/webui/options2/preferences_browsertest.h:62: void
DeleteValues(std::vector<base::Value*>& values);
On 2012/08/07 17:59:05, James Hawkins wrote:
> nit: Document method.

Done.

https://chromiumcodereview.appspot.com/10834109/diff/8001/chrome/browser/ui/w...
chrome/browser/ui/webui/options2/preferences_browsertest.h:64:
content::RenderViewHost* render_view_host_;
On 2012/08/07 17:59:05, James Hawkins wrote:
> nit: Document member variable.

Done.

https://chromiumcodereview.appspot.com/10834109/diff/8001/chrome/browser/ui/w...
chrome/browser/ui/webui/options2/preferences_browsertest.h:66:
policy::MockConfigurationPolicyProvider policy_provider_;
On 2012/08/07 17:59:05, James Hawkins wrote:
> nit: Document member variable.

Done.

https://chromiumcodereview.appspot.com/10834109/diff/8001/chrome/browser/ui/w...
chrome/browser/ui/webui/options2/preferences_browsertest.h:67: };
On 2012/08/07 17:59:05, James Hawkins wrote:
> DISALLOW_COPY_AND_ASSIGN?

Done.

[James Hawkins, 2012-08-07 18:29:50]

LGTM with nits.

https://chromiumcodereview.appspot.com/10834109/diff/10/chrome/browser/ui/web...
File chrome/browser/ui/webui/options2/preferences_browsertest.cc (right):

https://chromiumcodereview.appspot.com/10834109/diff/10/chrome/browser/ui/web...
chrome/browser/ui/webui/options2/preferences_browsertest.cc:121:
ASSERT_TRUE(prefs);
nit: Don't assert here.

https://chromiumcodereview.appspot.com/10834109/diff/10/chrome/browser/ui/web...
chrome/browser/ui/webui/options2/preferences_browsertest.cc:156: delete
set_value;
nit: Use a scoped_ptr parameter instead of manually deleting.

[bartfab (slow), 2012-08-07 18:35:55]

One nit address, question on the other.

https://chromiumcodereview.appspot.com/10834109/diff/10/chrome/browser/ui/web...
File chrome/browser/ui/webui/options2/preferences_browsertest.cc (right):

https://chromiumcodereview.appspot.com/10834109/diff/10/chrome/browser/ui/web...
chrome/browser/ui/webui/options2/preferences_browsertest.cc:121:
ASSERT_TRUE(prefs);
There is a reason for this assert: VerifyPrefs() is fed the output of
FetchPrefs(). FetchPrefs() could have failed, producing a NULL prefs pointer. If
you do not like the assert here, I can guard each call to VerifyPrefs() with an
if(prefs) instead.

https://chromiumcodereview.appspot.com/10834109/diff/10/chrome/browser/ui/web...
chrome/browser/ui/webui/options2/preferences_browsertest.cc:156: delete
set_value;
On 2012/08/07 18:29:50, James Hawkins wrote:
> nit: Use a scoped_ptr parameter instead of manually deleting.

Done.

[James Hawkins, 2012-08-07 18:37:38]

SLGTM

https://chromiumcodereview.appspot.com/10834109/diff/10/chrome/browser/ui/web...
File chrome/browser/ui/webui/options2/preferences_browsertest.cc (right):

https://chromiumcodereview.appspot.com/10834109/diff/10/chrome/browser/ui/web...
chrome/browser/ui/webui/options2/preferences_browsertest.cc:121:
ASSERT_TRUE(prefs);
On 2012/08/07 18:35:56, bartfab wrote:
> There is a reason for this assert: VerifyPrefs() is fed the output of
> FetchPrefs(). FetchPrefs() could have failed, producing a NULL prefs pointer.
If
> you do not like the assert here, I can guard each call to VerifyPrefs() with
an
> if(prefs) instead.

I'm not going to nit it to death, but yes, I would do whatever it takes to
remove this ASSERT, since this is an internal method.

[bartfab (slow), 2012-08-07 18:41:21]

https://chromiumcodereview.appspot.com/10834109/diff/10/chrome/browser/ui/web...
File chrome/browser/ui/webui/options2/preferences_browsertest.cc (right):

https://chromiumcodereview.appspot.com/10834109/diff/10/chrome/browser/ui/web...
chrome/browser/ui/webui/options2/preferences_browsertest.cc:121:
ASSERT_TRUE(prefs);
No problem, I replaced the ASSERT in the method with an if-check before each
call to it. This makes it clearer what is being checked and why as well.

[James Hawkins, 2012-08-07 18:42:55]

SLGTM ship it!

[commit-bot: I haz the power, 2012-08-07 18:44:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bartfab@chromium.org/10834109/8002

[commit-bot: I haz the power, 2012-08-07 19:17:17]

Try job failure for 10834109-8002 (retry) on linux_chromeos for step "compile"
(clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2012-08-08 08:07:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bartfab@chromium.org/10834109/7003

[commit-bot: I haz the power, 2012-08-08 09:35:50]

Change committed as 150527