Surface error messages from ONC parsing in about:policy.

Surface error messages from ONC parsing in about:policy.

BUG=chromium-os:19411
TEST=Configure network configuration policy that is broken and check about:policy


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

[Mattias Nissler (ping if slow), 2011-11-04 16:53:04]

Joao, here is some error-surfacing. Please review!

[Joao da Silva, 2011-11-04 17:44:24]

Looks good but some tests might need updates.

http://codereview.chromium.org/8480003/diff/1/chrome/browser/policy/configura...
File chrome/browser/policy/configuration_policy_handler_chromeos.cc (right):

http://codereview.chromium.org/8480003/diff/1/chrome/browser/policy/configura...
chrome/browser/policy/configuration_policy_handler_chromeos.cc:25: if
(!TypeCheckingPolicyHandler::CheckAndGetValue(policies, errors, &value))
Nit: invoking this method doesn't require the super class name

http://codereview.chromium.org/8480003/diff/1/chrome/browser/policy/configura...
File chrome/browser/policy/configuration_policy_handler_chromeos_unittest.cc
(right):

http://codereview.chromium.org/8480003/diff/1/chrome/browser/policy/configura...
chrome/browser/policy/configuration_policy_handler_chromeos_unittest.cc:65: }
Nit: // namespace policy

http://codereview.chromium.org/8480003/diff/1/chrome/chrome_tests.gypi
File chrome/chrome_tests.gypi (right):

http://codereview.chromium.org/8480003/diff/1/chrome/chrome_tests.gypi#newcod...
chrome/chrome_tests.gypi:2046: ['exclude',
'^browser/policy/network_configuration_updater_unittest.cc' ],
Exclude the test when chromeos == 0?

http://codereview.chromium.org/8480003/diff/2001/chrome/browser/chromeos/cros...
File chrome/browser/chromeos/cros/onc_network_parser.h (right):

http://codereview.chromium.org/8480003/diff/2001/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros/onc_network_parser.h:55: const std::string&
parse_error() { return parse_error_; }
Nit: const method

http://codereview.chromium.org/8480003/diff/2001/chrome/tools/build/generate_...
File chrome/tools/build/generate_policy_source.py (right):

http://codereview.chromium.org/8480003/diff/2001/chrome/tools/build/generate_...
chrome/tools/build/generate_policy_source.py:108: return [(name, platforms,
vtype) for (name, vtype, platforms, is_device_only)
Since |is_device_only| is never used, it could be removed from the tuple
returned from _GetPolicyDetails.

http://codereview.chromium.org/8480003/diff/2001/chrome/tools/build/generate_...
chrome/tools/build/generate_policy_source.py:236: _OutputGeneratedWarningForC(f,
args[1])
args[2]?

http://codereview.chromium.org/8480003/diff/2001/chrome/tools/build/generate_...
chrome/tools/build/generate_policy_source.py:306: _OutputGeneratedWarningForC(f,
args[1])
args[2]?

http://codereview.chromium.org/8480003/diff/2001/chrome/tools/build/generate_...
chrome/tools/build/generate_policy_source.py:428: _OutputGeneratedWarningForC(f,
args[1])
args[2]?

[Mattias Nissler (ping if slow), 2011-11-07 10:48:44]

New version with comments addressed is up, PTAL! Thanks.

http://codereview.chromium.org/8480003/diff/2001/chrome/browser/chromeos/cros...
File chrome/browser/chromeos/cros/onc_network_parser.h (right):

http://codereview.chromium.org/8480003/diff/2001/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros/onc_network_parser.h:55: const std::string&
parse_error() { return parse_error_; }
On 2011/11/04 17:44:24, Joao da Silva wrote:
> Nit: const method

Done.

http://codereview.chromium.org/8480003/diff/2001/chrome/tools/build/generate_...
File chrome/tools/build/generate_policy_source.py (right):

http://codereview.chromium.org/8480003/diff/2001/chrome/tools/build/generate_...
chrome/tools/build/generate_policy_source.py:108: return [(name, platforms,
vtype) for (name, vtype, platforms, is_device_only)
On 2011/11/04 17:44:24, Joao da Silva wrote:
> Since |is_device_only| is never used, it could be removed from the tuple
> returned from _GetPolicyDetails.

Done.

http://codereview.chromium.org/8480003/diff/2001/chrome/tools/build/generate_...
chrome/tools/build/generate_policy_source.py:236: _OutputGeneratedWarningForC(f,
args[1])
On 2011/11/04 17:44:24, Joao da Silva wrote:
> args[2]?

Done.

http://codereview.chromium.org/8480003/diff/2001/chrome/tools/build/generate_...
chrome/tools/build/generate_policy_source.py:306: _OutputGeneratedWarningForC(f,
args[1])
On 2011/11/04 17:44:24, Joao da Silva wrote:
> args[2]?

Done.

http://codereview.chromium.org/8480003/diff/2001/chrome/tools/build/generate_...
chrome/tools/build/generate_policy_source.py:428: _OutputGeneratedWarningForC(f,
args[1])
On 2011/11/04 17:44:24, Joao da Silva wrote:
> args[2]?

Done.

[Joao da Silva, 2011-11-07 13:20:45]

LGTM with nits

http://codereview.chromium.org/8480003/diff/8001/chrome/app/generated_resourc...
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/8480003/diff/8001/chrome/app/generated_resourc...
chrome/app/generated_resources.grd:4463: Network configuration failed to be
parsed: <ph name="PARSE_ERROR_MESSAGE">$1<ex>Line: 1, column: 1, Root value must
be an array or object.</ex></ph>
The string might be translated, but $1 won't. I don't mind this though.

http://codereview.chromium.org/8480003/diff/8001/chrome/browser/chromeos/cros...
File chrome/browser/chromeos/cros/onc_network_parser.h (right):

http://codereview.chromium.org/8480003/diff/8001/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros/onc_network_parser.h:52: const std::string&
parse_error() { return parse_error_; }
Nit: const method?

http://codereview.chromium.org/8480003/diff/8001/chrome/browser/policy/config...
File chrome/browser/policy/configuration_policy_handler_chromeos.cc (right):

http://codereview.chromium.org/8480003/diff/8001/chrome/browser/policy/config...
chrome/browser/policy/configuration_policy_handler_chromeos.cc:25: if
(!TypeCheckingPolicyHandler::CheckAndGetValue(policies, errors, &value))
Nit: invoked this method doesn't require the superclass name

http://codereview.chromium.org/8480003/diff/8001/chrome/browser/policy/config...
File chrome/browser/policy/configuration_policy_handler_chromeos_unittest.cc
(right):

http://codereview.chromium.org/8480003/diff/8001/chrome/browser/policy/config...
chrome/browser/policy/configuration_policy_handler_chromeos_unittest.cc:65: }
Nit: // namespace policy

[Mattias Nissler (ping if slow), 2011-11-11 14:57:02]

OK, ready for another round.

Adding Steven for the OncNetworkParser change and Ken FYI.

http://codereview.chromium.org/8480003/diff/8001/chrome/app/generated_resourc...
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/8480003/diff/8001/chrome/app/generated_resourc...
chrome/app/generated_resources.grd:4463: Network configuration failed to be
parsed: <ph name="PARSE_ERROR_MESSAGE">$1<ex>Line: 1, column: 1, Root value must
be an array or object.</ex></ph>
On 2011/11/07 13:20:45, Joao da Silva wrote:
> The string might be translated, but $1 won't. I don't mind this though.

Yes, I know. Hard to fix though, so I thought it's good enough for now and an
English message is better than no message.

http://codereview.chromium.org/8480003/diff/8001/chrome/browser/chromeos/cros...
File chrome/browser/chromeos/cros/onc_network_parser.h (right):

http://codereview.chromium.org/8480003/diff/8001/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros/onc_network_parser.h:52: const std::string&
parse_error() { return parse_error_; }
On 2011/11/07 13:20:45, Joao da Silva wrote:
> Nit: const method?

Done.

http://codereview.chromium.org/8480003/diff/8001/chrome/browser/policy/config...
File chrome/browser/policy/configuration_policy_handler_chromeos.cc (right):

http://codereview.chromium.org/8480003/diff/8001/chrome/browser/policy/config...
chrome/browser/policy/configuration_policy_handler_chromeos.cc:25: if
(!TypeCheckingPolicyHandler::CheckAndGetValue(policies, errors, &value))
On 2011/11/07 13:20:45, Joao da Silva wrote:
> Nit: invoked this method doesn't require the superclass name

Done.

http://codereview.chromium.org/8480003/diff/8001/chrome/browser/policy/config...
File chrome/browser/policy/configuration_policy_handler_chromeos_unittest.cc
(right):

http://codereview.chromium.org/8480003/diff/8001/chrome/browser/policy/config...
chrome/browser/policy/configuration_policy_handler_chromeos_unittest.cc:65: }
On 2011/11/07 13:20:45, Joao da Silva wrote:
> Nit: // namespace policy

Done.

[Joao da Silva, 2011-11-11 16:20:54]

LGTM.

Notice that the win build linker failed missing some JSON symbols.

http://codereview.chromium.org/8480003/diff/12001/chrome/browser/policy/confi...
File chrome/browser/policy/configuration_policy_reader.cc (right):

http://codereview.chromium.org/8480003/diff/12001/chrome/browser/policy/confi...
chrome/browser/policy/configuration_policy_reader.cc:82:
handler_list->PrepareForDisplaying(&policies);
How was this missing :-)

[stevenjb, 2011-11-11 19:13:02]

chromeos changes LGTM

[commit-bot: I haz the power, 2011-11-14 09:10:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mnissler@chromium.org/8480003/12001

[commit-bot: I haz the power, 2011-11-14 10:24:41]

Change committed as 109857