Show parse errors in the UI when loading ONC files.

Show parse errors in the UI when loading ONC files. 
Resubmitting reverted CL with fix
BUG=chromium-os:23757
TEST=manual
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=114558

[achuithb, 2011-12-10 01:18:11]

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

http://codereview.chromium.org/8883046/diff/8001/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros/network_library.h:1794: virtual bool
LoadOncNetworks(const std::string& onc_blob,
I don't think anybody cares about the return value any more...

http://codereview.chromium.org/8883046/diff/8001/chrome/browser/resources/net...
File chrome/browser/resources/net_internals/chromeos_view.js (right):

http://codereview.chromium.org/8883046/diff/8001/chrome/browser/resources/net...
chrome/browser/resources/net_internals/chromeos_view.js:76: "ONC file parse
failed: " + error : "ONC file successfully parsed";
This is going to be a bit weird since error is localized text and you're going
to mix English with other languages when you do this. Can you just do:
parseStatus.textContent = error;
and if there's no error, pass an i18n error string that says "ONC file
successfully parsed"?

[kmixter1, 2011-12-10 15:13:09]

Some early feedback.  Looking forward to having these better diagnostics.

http://codereview.chromium.org/8883046/diff/8001/chrome/browser/chromeos/cros...
File chrome/browser/chromeos/cros/network_library.cc (right):

http://codereview.chromium.org/8883046/diff/8001/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros/network_library.cc:2883: if
(parser.GetNetworkConfigsSize() == 0) {
rebase...

http://codereview.chromium.org/8883046/diff/8001/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros/network_library.cc:3719: if (error !=
NETWORK_METHOD_ERROR_NONE) {
This is an important error that we really should be reporting through UI though
it's harder to report.

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

http://codereview.chromium.org/8883046/diff/8001/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros/network_library.h:1794: virtual bool
LoadOncNetworks(const std::string& onc_blob,
On 2011/12/10 01:18:11, achuith.bhandarkar wrote:
> I don't think anybody cares about the return value any more...

True, though it's inflexible to have errors returned as localized strings.  I
feel like that's going to be insufficient at some point.  For instance, if we
ever want to start aggregating parse/network errors server side or doing UMA
stats, we'll want to be using enumerators.  (As a side benefit, it would also
simplify unit testing of error handling.)  I'd suggest creating the enum now,
returning that, and creating a OncNetworkParser::GetErrorString(OncErrorCode)
function.

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

http://codereview.chromium.org/8883046/diff/8001/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros/onc_network_parser.cc:239: parse_error_ =
l10n_util::GetStringUTF8(
Currently almost all of the unit tests correctly parse, but we should adds ones
that purposefully don't and test parse_error_.

http://codereview.chromium.org/8883046/diff/8001/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros/onc_network_parser.cc:300:
IDS_NETWORK_CONFIG_ERROR_NETWORK_PROP_DICT_MALFORMED);
Please use JSON and ONC terminology in error messages (dictionary should be
object, network should be NetworkConfiguration).

http://codereview.chromium.org/8883046/diff/8001/chrome/browser/resources/net...
File chrome/browser/resources/net_internals/chromeos_view.js (right):

http://codereview.chromium.org/8883046/diff/8001/chrome/browser/resources/net...
chrome/browser/resources/net_internals/chromeos_view.js:23: setParseStatus_("ONC
file parse failed: file not found");
Aren't there many possible causes for this, such as the file has the wrong
permissions (I've hit that one).  Also, i18n?

[Charlie Lee, 2011-12-14 17:02:20]

http://codereview.chromium.org/8883046/diff/8001/chrome/browser/chromeos/cros...
File chrome/browser/chromeos/cros/network_library.cc (right):

http://codereview.chromium.org/8883046/diff/8001/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros/network_library.cc:2883: if
(parser.GetNetworkConfigsSize() == 0) {
On 2011/12/10 15:13:09, kmixter1 wrote:
> rebase...

Done.

http://codereview.chromium.org/8883046/diff/8001/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros/network_library.cc:3719: if (error !=
NETWORK_METHOD_ERROR_NONE) {
On 2011/12/10 15:13:09, kmixter1 wrote:
> This is an important error that we really should be reporting through UI
though
> it's harder to report.

I agree. This will be for R18.

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

http://codereview.chromium.org/8883046/diff/8001/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros/network_library.h:1794: virtual bool
LoadOncNetworks(const std::string& onc_blob,
On 2011/12/10 01:18:11, achuith.bhandarkar wrote:
> I don't think anybody cares about the return value any more...

We still use the return value in some cases.

http://codereview.chromium.org/8883046/diff/8001/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros/network_library.h:1794: virtual bool
LoadOncNetworks(const std::string& onc_blob,
On 2011/12/10 15:13:09, kmixter1 wrote:
> On 2011/12/10 01:18:11, achuith.bhandarkar wrote:
> > I don't think anybody cares about the return value any more...
> 
> True, though it's inflexible to have errors returned as localized strings.  I
> feel like that's going to be insufficient at some point.  For instance, if we
> ever want to start aggregating parse/network errors server side or doing UMA
> stats, we'll want to be using enumerators.  (As a side benefit, it would also
> simplify unit testing of error handling.)  I'd suggest creating the enum now,
> returning that, and creating a OncNetworkParser::GetErrorString(OncErrorCode)
> function.

I'd like to keep this for now and fix this for R18.
Would returning the grd constant make sense here?

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

http://codereview.chromium.org/8883046/diff/8001/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros/onc_network_parser.cc:300:
IDS_NETWORK_CONFIG_ERROR_NETWORK_PROP_DICT_MALFORMED);
On 2011/12/10 15:13:09, kmixter1 wrote:
> Please use JSON and ONC terminology in error messages (dictionary should be
> object, network should be NetworkConfiguration).

I can't fix this for R17. Will remember to fix it for R18.

http://codereview.chromium.org/8883046/diff/8001/chrome/browser/resources/net...
File chrome/browser/resources/net_internals/chromeos_view.js (right):

http://codereview.chromium.org/8883046/diff/8001/chrome/browser/resources/net...
chrome/browser/resources/net_internals/chromeos_view.js:23: setParseStatus_("ONC
file parse failed: file not found");
On 2011/12/10 15:13:09, kmixter1 wrote:
> Aren't there many possible causes for this, such as the file has the wrong
> permissions (I've hit that one).  Also, i18n?

Ok, I made it more general. Also, since net-internals is english only, this is
ok for now.

http://codereview.chromium.org/8883046/diff/8001/chrome/browser/resources/net...
chrome/browser/resources/net_internals/chromeos_view.js:76: "ONC file parse
failed: " + error : "ONC file successfully parsed";
On 2011/12/10 01:18:11, achuith.bhandarkar wrote:
> This is going to be a bit weird since error is localized text and you're going
> to mix English with other languages when you do this. Can you just do:
> parseStatus.textContent = error;
> and if there's no error, pass an i18n error string that says "ONC file
> successfully parsed"?

Let's fix this for R18. For now, we'll just be ok with mixing languages.

[kmixter1, 2011-12-14 19:46:38]

lgtm

nits

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

http://codereview.chromium.org/8883046/diff/8001/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros/network_library.h:1794: virtual bool
LoadOncNetworks(const std::string& onc_blob,
On 2011/12/14 17:02:20, Charlie Lee wrote:
> On 2011/12/10 15:13:09, kmixter1 wrote:
> > On 2011/12/10 01:18:11, achuith.bhandarkar wrote:
> > > I don't think anybody cares about the return value any more...
> > 
> > True, though it's inflexible to have errors returned as localized strings. 
I
> > feel like that's going to be insufficient at some point.  For instance, if
we
> > ever want to start aggregating parse/network errors server side or doing UMA
> > stats, we'll want to be using enumerators.  (As a side benefit, it would
also
> > simplify unit testing of error handling.)  I'd suggest creating the enum
now,
> > returning that, and creating a
OncNetworkParser::GetErrorString(OncErrorCode)
> > function.
> 
> I'd like to keep this for now and fix this for R18.
> Would returning the grd constant make sense here?
> 

Fair enough.  I think the grd constant makes sense.  I would also then suggest
that we return a "imported successfully" constant on success and thus always set
|error|.

http://codereview.chromium.org/8883046/diff/15001/chrome/browser/chromeos/cro...
File chrome/browser/chromeos/cros/network_library.cc (right):

http://codereview.chromium.org/8883046/diff/15001/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros/network_library.cc:2853: if (error)
There are cases where false is returned but error is unset which is inconsistent
with the function comment.  Should document when the function sets error (or
always set it at the top).

[kmixter1, 2011-12-14 19:48:36]

http://codereview.chromium.org/8883046/diff/15001/chrome/browser/chromeos/cro...
File chrome/browser/chromeos/cros/network_library.cc (right):

http://codereview.chromium.org/8883046/diff/15001/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros/network_library.cc:2853: if (error)
On 2011/12/14 19:46:38, kmixter1 wrote:
> There are cases where false is returned but error is unset which is
inconsistent
> with the function comment.  Should document when the function sets error (or
> always set it at the top).

Sorry, misread the code.  I think error is always set.

[achuithb, 2011-12-14 20:08:03]

lgtm

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

http://codereview.chromium.org/8883046/diff/8001/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros/network_library.h:1794: virtual bool
LoadOncNetworks(const std::string& onc_blob,
On 2011/12/14 19:46:38, kmixter1 wrote:
> On 2011/12/14 17:02:20, Charlie Lee wrote:
> > On 2011/12/10 15:13:09, kmixter1 wrote:
> > > On 2011/12/10 01:18:11, achuith.bhandarkar wrote:
> > > > I don't think anybody cares about the return value any more...
> > > 
> > > True, though it's inflexible to have errors returned as localized strings.

> I
> > > feel like that's going to be insufficient at some point.  For instance, if
> we
> > > ever want to start aggregating parse/network errors server side or doing
UMA
> > > stats, we'll want to be using enumerators.  (As a side benefit, it would
> also
> > > simplify unit testing of error handling.)  I'd suggest creating the enum
> now,
> > > returning that, and creating a
> OncNetworkParser::GetErrorString(OncErrorCode)
> > > function.
> > 
> > I'd like to keep this for now and fix this for R18.
> > Would returning the grd constant make sense here?
> > 
> 
> Fair enough.  I think the grd constant makes sense.  I would also then suggest
> that we return a "imported successfully" constant on success and thus always
set
> |error|.

+1. Before you return true, perhaps set *error. If it's not possible to do this
for M17 because of grd string limitations, maybe try to do this for M18?

[Charlie Lee, 2011-12-14 22:34:59]

Mattias, please review chrome/browser/policy
Matt, please review net_internals changes

[mmenke, 2011-12-14 22:42:41]

LGTM

http://codereview.chromium.org/8883046/diff/16013/chrome/browser/resources/ne...
File chrome/browser/resources/net_internals/chromeos_view.js (right):

http://codereview.chromium.org/8883046/diff/16013/chrome/browser/resources/ne...
chrome/browser/resources/net_internals/chromeos_view.js:23: setParseStatus_("ONC
file parse failed: cannot read file");
nit:  Javascript should use single quotes, as it's compatible with quoting of
HTML strings.  This applies below, too, just missed it before.

[Mattias Nissler (ping if slow), 2011-12-14 22:45:12]

Policy LGTM. And I'll have to rebase my CL since I added another parameter.
Sigh.

http://codereview.chromium.org/8883046/diff/16013/chrome/browser/policy/netwo...
File chrome/browser/policy/network_configuration_updater.cc (right):

http://codereview.chromium.org/8883046/diff/16013/chrome/browser/policy/netwo...
chrome/browser/policy/network_configuration_updater.cc:60: if
(!network_library_->LoadOncNetworks(new_network_config, "", &error))
you need curlies here.

[Charlie Lee, 2011-12-14 22:52:02]

Thanks.

http://codereview.chromium.org/8883046/diff/16013/chrome/browser/policy/netwo...
File chrome/browser/policy/network_configuration_updater.cc (right):

http://codereview.chromium.org/8883046/diff/16013/chrome/browser/policy/netwo...
chrome/browser/policy/network_configuration_updater.cc:60: if
(!network_library_->LoadOncNetworks(new_network_config, "", &error))
On 2011/12/14 22:45:12, Mattias Nissler wrote:
> you need curlies here.

Done.

http://codereview.chromium.org/8883046/diff/16013/chrome/browser/resources/ne...
File chrome/browser/resources/net_internals/chromeos_view.js (right):

http://codereview.chromium.org/8883046/diff/16013/chrome/browser/resources/ne...
chrome/browser/resources/net_internals/chromeos_view.js:23: setParseStatus_("ONC
file parse failed: cannot read file");
On 2011/12/14 22:42:41, Matt Menke wrote:
> nit:  Javascript should use single quotes, as it's compatible with quoting of
> HTML strings.  This applies below, too, just missed it before.

Done.