This moves the ONC parsing code into chromeos/network/onc

This moves the ONC parsing code into chromeos/network/onc
so that it can be used there without violating dependency
rules.

(Using the "refactoring" OWNERS TBR rule...)

BUG=none
TEST=ran unit tests
TBR=jhawkins@chromium.org


Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=172446

[Greg Spencer (Chromium), 2012-11-28 21:57:53]

This should have no functional changes, just moving things.

[Greg Spencer (Chromium), 2012-11-28 22:14:53]

Actually, not QUITE no functional changes: I did add a util function for getting
the test data directory.  That was necessary because we can't load the test data
out of chrome/test/data anymore.

[pneubeck (no reviews), 2012-11-28 22:20:41]

Should the test data be again put into a subfolder like
chromeos/test/data/network ?

https://codereview.chromium.org/11299236/diff/1/chromeos/network/onc_mapper.h
File chromeos/network/onc_mapper.h (right):

https://codereview.chromium.org/11299236/diff/1/chromeos/network/onc_mapper.h...
chromeos/network/onc_mapper.h:37: class CHROMEOS_EXPORT Mapper {
do we have to export base classes? otherwise Mapper isn't used so far outside of
this folder...

https://codereview.chromium.org/11299236/diff/1/chromeos/network/onc_signature.h
File chromeos/network/onc_signature.h (right):

https://codereview.chromium.org/11299236/diff/1/chromeos/network/onc_signatur...
chromeos/network/onc_signature.h:18: struct CHROMEOS_EXPORT OncFieldSignature {
There shouldn't be a user of this outside of this folder, yet.

https://codereview.chromium.org/11299236/diff/1/chromeos/network/onc_signatur...
chromeos/network/onc_signature.h:30: CHROMEOS_EXPORT const OncFieldSignature*
GetFieldSignature(
the same here.

[Greg Spencer (Chromium), 2012-11-28 23:09:14]

I moved the data into a "network" subdir and modified the helper function to
take a "component" dir.

https://codereview.chromium.org/11299236/diff/1/chromeos/network/onc_mapper.h
File chromeos/network/onc_mapper.h (right):

https://codereview.chromium.org/11299236/diff/1/chromeos/network/onc_mapper.h...
chromeos/network/onc_mapper.h:37: class CHROMEOS_EXPORT Mapper {
On 2012/11/28 22:20:41, pneubeck wrote:
> do we have to export base classes? otherwise Mapper isn't used so far outside
of
> this folder...

No, we don't.  Done.

https://codereview.chromium.org/11299236/diff/1/chromeos/network/onc_signature.h
File chromeos/network/onc_signature.h (right):

https://codereview.chromium.org/11299236/diff/1/chromeos/network/onc_signatur...
chromeos/network/onc_signature.h:18: struct CHROMEOS_EXPORT OncFieldSignature {
On 2012/11/28 22:20:41, pneubeck wrote:
> There shouldn't be a user of this outside of this folder, yet.

Done.

https://codereview.chromium.org/11299236/diff/1/chromeos/network/onc_signatur...
chromeos/network/onc_signature.h:30: CHROMEOS_EXPORT const OncFieldSignature*
GetFieldSignature(
On 2012/11/28 22:20:41, pneubeck wrote:
> the same here.

Done.

[stevenjb, 2012-11-28 23:20:05]

So, I hate to suggest this, but, we have a lot of onc_ files. Does it make sense
to put them into a separate directory, i.e. chromeos/network/onc?

[Greg Spencer (Chromium), 2012-11-28 23:27:03]

On 2012/11/28 23:20:05, stevenjb (chromium) wrote:
> So, I hate to suggest this, but, we have a lot of onc_ files. Does it make
sense
> to put them into a separate directory, i.e. chromeos/network/onc?

I thought about that too, and was on the fence, but if you think the same, then
I'll do it.

[Greg Spencer (Chromium), 2012-11-28 23:29:07]

On 2012/11/28 23:27:03, Greg Spencer (Chromium) wrote:
> On 2012/11/28 23:20:05, stevenjb (chromium) wrote:
> > So, I hate to suggest this, but, we have a lot of onc_ files. Does it make
> sense
> > to put them into a separate directory, i.e. chromeos/network/onc?
> 
> I thought about that too, and was on the fence, but if you think the same,
then
> I'll do it.

But does that mean I need to move the test data to
chromeos/test/data/network/onc?  Seems like a lot of empty directories...

[stevenjb, 2012-11-28 23:35:11]

Nah, I don't think we need to move the test data, at least not yet :)

[Greg Spencer (Chromium), 2012-11-29 00:09:30]

On 2012/11/28 23:35:11, stevenjb (chromium) wrote:
> Nah, I don't think we need to move the test data, at least not yet :)

OK, moved all the sources.  PTAL.

[stevenjb, 2012-11-29 00:16:23]

lgtm

[pneubeck (no reviews), 2012-11-29 08:23:52]

lgtm

[commit-bot: I haz the power, 2012-11-29 17:35:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gspencer@chromium.org/11299236/1165

[commit-bot: I haz the power, 2012-11-29 17:35:58]

Failed to apply patch for chrome/browser/chromeos/cros/onc_network_parser.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/browser/chromeos/cros/onc_network_parser.cc
  Hunk #2 FAILED at 17.
  1 out of 2 hunks FAILED -- saving rejects to file
chrome/browser/chromeos/cros/onc_network_parser.cc.rej

Patch:       chrome/browser/chromeos/cros/onc_network_parser.cc
Index: chrome/browser/chromeos/cros/onc_network_parser.cc
diff --git a/chrome/browser/chromeos/cros/onc_network_parser.cc
b/chrome/browser/chromeos/cros/onc_network_parser.cc
index
9bb533ee306a32b13519e637bc6a8681a578192d..7a4abd359ba7b8a1e68dac7cf706b4c34e728a2f
100644
--- a/chrome/browser/chromeos/cros/onc_network_parser.cc
+++ b/chrome/browser/chromeos/cros/onc_network_parser.cc
@@ -9,7 +9,6 @@
 
 #include "base/base64.h"
 #include "base/json/json_string_value_serializer.h"
-#include "chrome/browser/chromeos/login/user_manager.h"
 #include "base/json/json_writer.h"  // for debug output only.
 #include "base/stringprintf.h"
 #include "base/values.h"
@@ -18,12 +17,13 @@
 #include "chrome/browser/chromeos/cros/native_network_constants.h"
 #include "chrome/browser/chromeos/cros/native_network_parser.h"
 #include "chrome/browser/chromeos/cros/network_library.h"
-#include "chrome/browser/chromeos/cros/onc_constants.h"
-#include "chrome/browser/chromeos/network_settings/onc_signature.h"
-#include "chrome/browser/chromeos/network_settings/onc_validator.h"
+#include "chrome/browser/chromeos/login/user_manager.h"
 #include "chrome/browser/chromeos/proxy_config_service_impl.h"
 #include "chrome/browser/prefs/proxy_config_dictionary.h"
 #include "chrome/common/net/x509_certificate_model.h"
+#include "chromeos/network/onc/onc_constants.h"
+#include "chromeos/network/onc/onc_signature.h"
+#include "chromeos/network/onc/onc_validator.h"
 #include "content/public/browser/browser_thread.h"
 #include "crypto/encryptor.h"
 #include "crypto/hmac.h"

[Greg Spencer (Chromium), 2012-12-01 01:06:42]

OK, this just got real. :-)

I tried to merge in Phillipp's latest change, and because the certificate
importer has strings, I had to add resources to chromeos.

I tried going the other route: generating error codes from the certificate
importer, and translating the error codes in the chrome code that uses it, but
at a certain level we get strings (hardcoded, in English) from the JSON importer
that give us syntax error information that is pretty useful information, and if
I don't return "human readable" strings from the certificate importer, then that
information gets lost.  I could have added all the JSON codes to the result
codes for the importer, perhaps, but that seems wrong because then they have to
be kept in sync.  So I went with adding string resources.

So, this change as it is has string resources in it, but there are dependency
violations (because checkdeps has no rules for this lib, I think) to i10n_util
and crypto still.

Any ideas on the best approach to take here?

[pneubeck (no reviews), 2012-12-03 14:58:24]

On 2012/12/01 01:06:42, Greg Spencer (Chromium) wrote:
> OK, this just got real. :-)
> 
> I tried to merge in Phillipp's latest change, and because the certificate
> importer has strings, I had to add resources to chromeos.

Not only the certificate importer has translated error strings but also the
decryption function and the onc validation (CL already published).

> I tried going the other route: generating error codes from the certificate
> importer, and translating the error codes in the chrome code that uses it, but
> at a certain level we get strings (hardcoded, in English) from the JSON
importer
> that give us syntax error information that is pretty useful information, and
if
> I don't return "human readable" strings from the certificate importer, then
that
> information gets lost.  I could have added all the JSON codes to the result
> codes for the importer, perhaps, but that seems wrong because then they have
to
> be kept in sync.  So I went with adding string resources.
> 
> So, this change as it is has string resources in it, but there are dependency
> violations (because checkdeps has no rules for this lib, I think) to i10n_util
> and crypto still.
> 
> Any ideas on the best approach to take here?

Until now I also thought that returning internationalized strings would be nice
for the UI and easy to implement. Also was it the way the certificate importer
and decrypter (from OncNetworkParser) already did it.

However, because of JsonReader returning only English errors (and only these
contains the location as line/column number) and the not-so-nice fact that we
would have to add grit resources to src/chromeos made me rethink that direction.

The important question is, what detail of error description do we need in the
UI?

The end-user shouldn't be interested in Json and detailed ONC errors. These can
only occur if CPanel or Chrome are buggy or if a wrong ONC was imported (and
that feature is unsupported by Google). Thus, a high-level error like "Not a
valid json dictionary" or "Invalid ONC" should be sufficient for the user to get
an idea of what broke.

For developers, a highlevel error is enough of a hint to search for the detailed
error in the log files. Sure, it isn't as convenient as a detailed error in
about:policy but is this convenience worth the effort?

[stevenjb, 2012-12-03 17:37:32]

1. I do not think we should have translated strings in src/chromeos. In addition
to added complexity, it puts UI code in a component that I envision as being
lower level than that.

2. I think that error strings are better than codes, but they should be kept
simple, e.g. "Certificate GUID missing" is good, but "Encrypted ONC file uses an
unsupported encryption scheme" is too long and should probably be shortened to
"unsupported encryption scheme," assuming the calling code has enough context to
provide "in encrypted ONC file" if we wish to present it in a human readable
form.

3. I've spoken to Zel about this in the past, and the rule of thumb is generally
that if a string can be viewed in the browser, even in a debugging page (e.g. in
chrome://network), we should translate it. Only log messages do not need to be
translated. (chrome://system toes that line, but essentially that is displaying
log messages and we don't translate them).
3b. If any of these errors are user facing outside of a log file, we should
translate the error strings there, e.g. "Certificate GUID missing" should be
defined as kErrorCertificateGUIDMissing somewhere and we should map that to
IDS_NETWORK_ONC_ERROR_CERT_GUID_MISSING (which should probably be more user
friendly) in the UI code.

Does that sound reasonable?


On 2012/12/03 14:58:24, pneubeck wrote:
> On 2012/12/01 01:06:42, Greg Spencer (Chromium) wrote:
> > OK, this just got real. :-)
> > 
> > I tried to merge in Phillipp's latest change, and because the certificate
> > importer has strings, I had to add resources to chromeos.
> 
> Not only the certificate importer has translated error strings but also the
> decryption function and the onc validation (CL already published).
> 
> > I tried going the other route: generating error codes from the certificate
> > importer, and translating the error codes in the chrome code that uses it,
but
> > at a certain level we get strings (hardcoded, in English) from the JSON
> importer
> > that give us syntax error information that is pretty useful information, and
> if
> > I don't return "human readable" strings from the certificate importer, then
> that
> > information gets lost.  I could have added all the JSON codes to the result
> > codes for the importer, perhaps, but that seems wrong because then they have
> to
> > be kept in sync.  So I went with adding string resources.
> > 
> > So, this change as it is has string resources in it, but there are
dependency
> > violations (because checkdeps has no rules for this lib, I think) to
i10n_util
> > and crypto still.
> > 
> > Any ideas on the best approach to take here?
> 
> Until now I also thought that returning internationalized strings would be
nice
> for the UI and easy to implement. Also was it the way the certificate importer
> and decrypter (from OncNetworkParser) already did it.
> 
> However, because of JsonReader returning only English errors (and only these
> contains the location as line/column number) and the not-so-nice fact that we
> would have to add grit resources to src/chromeos made me rethink that
direction.
> 
> The important question is, what detail of error description do we need in the
> UI?
> 
> The end-user shouldn't be interested in Json and detailed ONC errors. These
can
> only occur if CPanel or Chrome are buggy or if a wrong ONC was imported (and
> that feature is unsupported by Google). Thus, a high-level error like "Not a
> valid json dictionary" or "Invalid ONC" should be sufficient for the user to
get
> an idea of what broke.
> 
> For developers, a highlevel error is enough of a hint to search for the
detailed
> error in the log files. Sure, it isn't as convenient as a detailed error in
> about:policy but is this convenience worth the effort?

[stevenjb, 2012-12-03 17:39:53]

Also, I agree with the statement "a highlevel error is enough of a hint to
search for the detailed error in the log files". Logging the specific English
error and providing a translated "There was an error parsing the ONC file" makes
a lot of sense to me.

On 2012/12/03 17:37:32, stevenjb (chromium) wrote:
> 1. I do not think we should have translated strings in src/chromeos. In
addition
> to added complexity, it puts UI code in a component that I envision as being
> lower level than that.
> 
> 2. I think that error strings are better than codes, but they should be kept
> simple, e.g. "Certificate GUID missing" is good, but "Encrypted ONC file uses
an
> unsupported encryption scheme" is too long and should probably be shortened to
> "unsupported encryption scheme," assuming the calling code has enough context
to
> provide "in encrypted ONC file" if we wish to present it in a human readable
> form.
> 
> 3. I've spoken to Zel about this in the past, and the rule of thumb is
generally
> that if a string can be viewed in the browser, even in a debugging page (e.g.
in
> chrome://network), we should translate it. Only log messages do not need to be
> translated. (chrome://system toes that line, but essentially that is
displaying
> log messages and we don't translate them).
> 3b. If any of these errors are user facing outside of a log file, we should
> translate the error strings there, e.g. "Certificate GUID missing" should be
> defined as kErrorCertificateGUIDMissing somewhere and we should map that to
> IDS_NETWORK_ONC_ERROR_CERT_GUID_MISSING (which should probably be more user
> friendly) in the UI code.
> 
> Does that sound reasonable?
> 
> 
> On 2012/12/03 14:58:24, pneubeck wrote:
> > On 2012/12/01 01:06:42, Greg Spencer (Chromium) wrote:
> > > OK, this just got real. :-)
> > > 
> > > I tried to merge in Phillipp's latest change, and because the certificate
> > > importer has strings, I had to add resources to chromeos.
> > 
> > Not only the certificate importer has translated error strings but also the
> > decryption function and the onc validation (CL already published).
> > 
> > > I tried going the other route: generating error codes from the certificate
> > > importer, and translating the error codes in the chrome code that uses it,
> but
> > > at a certain level we get strings (hardcoded, in English) from the JSON
> > importer
> > > that give us syntax error information that is pretty useful information,
and
> > if
> > > I don't return "human readable" strings from the certificate importer,
then
> > that
> > > information gets lost.  I could have added all the JSON codes to the
result
> > > codes for the importer, perhaps, but that seems wrong because then they
have
> > to
> > > be kept in sync.  So I went with adding string resources.
> > > 
> > > So, this change as it is has string resources in it, but there are
> dependency
> > > violations (because checkdeps has no rules for this lib, I think) to
> i10n_util
> > > and crypto still.
> > > 
> > > Any ideas on the best approach to take here?
> > 
> > Until now I also thought that returning internationalized strings would be
> nice
> > for the UI and easy to implement. Also was it the way the certificate
importer
> > and decrypter (from OncNetworkParser) already did it.
> > 
> > However, because of JsonReader returning only English errors (and only these
> > contains the location as line/column number) and the not-so-nice fact that
we
> > would have to add grit resources to src/chromeos made me rethink that
> direction.
> > 
> > The important question is, what detail of error description do we need in
the
> > UI?
> > 
> > The end-user shouldn't be interested in Json and detailed ONC errors. These
> can
> > only occur if CPanel or Chrome are buggy or if a wrong ONC was imported (and
> > that feature is unsupported by Google). Thus, a high-level error like "Not a
> > valid json dictionary" or "Invalid ONC" should be sufficient for the user to
> get
> > an idea of what broke.
> > 
> > For developers, a highlevel error is enough of a hint to search for the
> detailed
> > error in the log files. Sure, it isn't as convenient as a detailed error in
> > about:policy but is this convenience worth the effort?

[pneubeck (no reviews), 2012-12-03 17:44:53]

Consider also my comment on the other CL
(https://codereview.chromium.org/11414101/), that the
ManagedNetworkConfigurationHandler should not be in src/chromeos but
src/chrome/browser:

On 2012/12/03 14:57:35, pneubeck wrote:
> Shouldn't the ManagedNetworkConfigurationHandler  be located in
> src/chrome/browser/... ?
> 
> I see several potential reasons:
> 
> The policy class are located in src/chrome/browser/policy.
> 
> For the proxy configuration we have to access proxy classes in
> src/chrome/browser/chromeos as well as the preferences.
> 
> Functions like ConnectTo... will have to call UI functions to gather further
> user input.



On 2012/12/03 17:39:53, stevenjb (chromium) wrote:
> Also, I agree with the statement "a highlevel error is enough of a hint to
> search for the detailed error in the log files". Logging the specific English
> error and providing a translated "There was an error parsing the ONC file"
makes
> a lot of sense to me.
> 
> On 2012/12/03 17:37:32, stevenjb (chromium) wrote:
> > 1. I do not think we should have translated strings in src/chromeos. In
> addition
> > to added complexity, it puts UI code in a component that I envision as being
> > lower level than that.
> > 
> > 2. I think that error strings are better than codes, but they should be kept
> > simple, e.g. "Certificate GUID missing" is good, but "Encrypted ONC file
uses
> an
> > unsupported encryption scheme" is too long and should probably be shortened
to
> > "unsupported encryption scheme," assuming the calling code has enough
context
> to
> > provide "in encrypted ONC file" if we wish to present it in a human readable
> > form.
> > 
> > 3. I've spoken to Zel about this in the past, and the rule of thumb is
> generally
> > that if a string can be viewed in the browser, even in a debugging page
(e.g.
> in
> > chrome://network), we should translate it. Only log messages do not need to
be
> > translated. (chrome://system toes that line, but essentially that is
> displaying
> > log messages and we don't translate them).
> > 3b. If any of these errors are user facing outside of a log file, we should
> > translate the error strings there, e.g. "Certificate GUID missing" should be
> > defined as kErrorCertificateGUIDMissing somewhere and we should map that to
> > IDS_NETWORK_ONC_ERROR_CERT_GUID_MISSING (which should probably be more user
> > friendly) in the UI code.
> > 
> > Does that sound reasonable?
> > 
> > 
> > On 2012/12/03 14:58:24, pneubeck wrote:
> > > On 2012/12/01 01:06:42, Greg Spencer (Chromium) wrote:
> > > > OK, this just got real. :-)
> > > > 
> > > > I tried to merge in Phillipp's latest change, and because the
certificate
> > > > importer has strings, I had to add resources to chromeos.
> > > 
> > > Not only the certificate importer has translated error strings but also
the
> > > decryption function and the onc validation (CL already published).
> > > 
> > > > I tried going the other route: generating error codes from the
certificate
> > > > importer, and translating the error codes in the chrome code that uses
it,
> > but
> > > > at a certain level we get strings (hardcoded, in English) from the JSON
> > > importer
> > > > that give us syntax error information that is pretty useful information,
> and
> > > if
> > > > I don't return "human readable" strings from the certificate importer,
> then
> > > that
> > > > information gets lost.  I could have added all the JSON codes to the
> result
> > > > codes for the importer, perhaps, but that seems wrong because then they
> have
> > > to
> > > > be kept in sync.  So I went with adding string resources.
> > > > 
> > > > So, this change as it is has string resources in it, but there are
> > dependency
> > > > violations (because checkdeps has no rules for this lib, I think) to
> > i10n_util
> > > > and crypto still.
> > > > 
> > > > Any ideas on the best approach to take here?
> > > 
> > > Until now I also thought that returning internationalized strings would be
> > nice
> > > for the UI and easy to implement. Also was it the way the certificate
> importer
> > > and decrypter (from OncNetworkParser) already did it.
> > > 
> > > However, because of JsonReader returning only English errors (and only
these
> > > contains the location as line/column number) and the not-so-nice fact that
> we
> > > would have to add grit resources to src/chromeos made me rethink that
> > direction.
> > > 
> > > The important question is, what detail of error description do we need in
> the
> > > UI?
> > > 
> > > The end-user shouldn't be interested in Json and detailed ONC errors.
These
> > can
> > > only occur if CPanel or Chrome are buggy or if a wrong ONC was imported
(and
> > > that feature is unsupported by Google). Thus, a high-level error like "Not
a
> > > valid json dictionary" or "Invalid ONC" should be sufficient for the user
to
> > get
> > > an idea of what broke.
> > > 
> > > For developers, a highlevel error is enough of a hint to search for the
> > detailed
> > > error in the log files. Sure, it isn't as convenient as a detailed error
in
> > > about:policy but is this convenience worth the effort?

[pneubeck (no reviews), 2012-12-03 17:53:50]

Steven, do you propose to
- return english string constants (and no error code) from low level classes
- then in the calling function to compare the returned error string to the
available constants 
- and then map to the right translatable message ID
?

It think, if we are already at it, then we should use error codes instead,
because:
1) Lower runtime/memory (strings are copied all the time and are expensive to
compare).
2) Typesafety because of Enums.
3) We don't need the exact error code in many cases. Thus carrying around a
string is a bit too much. E.g. All critical ONC errors may be mapped to a code,
all warnings may be mapped to another code, ...

On 2012/12/03 17:37:32, stevenjb (chromium) wrote:
> 1. I do not think we should have translated strings in src/chromeos. In
addition
> to added complexity, it puts UI code in a component that I envision as being
> lower level than that.
> 
> 2. I think that error strings are better than codes, but they should be kept
> simple, e.g. "Certificate GUID missing" is good, but "Encrypted ONC file uses
an
> unsupported encryption scheme" is too long and should probably be shortened to
> "unsupported encryption scheme," assuming the calling code has enough context
to
> provide "in encrypted ONC file" if we wish to present it in a human readable
> form.
> 
> 3. I've spoken to Zel about this in the past, and the rule of thumb is
generally
> that if a string can be viewed in the browser, even in a debugging page (e.g.
in
> chrome://network), we should translate it. Only log messages do not need to be
> translated. (chrome://system toes that line, but essentially that is
displaying
> log messages and we don't translate them).
> 3b. If any of these errors are user facing outside of a log file, we should
> translate the error strings there, e.g. "Certificate GUID missing" should be
> defined as kErrorCertificateGUIDMissing somewhere and we should map that to
> IDS_NETWORK_ONC_ERROR_CERT_GUID_MISSING (which should probably be more user
> friendly) in the UI code.
> 
> Does that sound reasonable?
> 
> 
> On 2012/12/03 14:58:24, pneubeck wrote:
> > On 2012/12/01 01:06:42, Greg Spencer (Chromium) wrote:
> > > OK, this just got real. :-)
> > > 
> > > I tried to merge in Phillipp's latest change, and because the certificate
> > > importer has strings, I had to add resources to chromeos.
> > 
> > Not only the certificate importer has translated error strings but also the
> > decryption function and the onc validation (CL already published).
> > 
> > > I tried going the other route: generating error codes from the certificate
> > > importer, and translating the error codes in the chrome code that uses it,
> but
> > > at a certain level we get strings (hardcoded, in English) from the JSON
> > importer
> > > that give us syntax error information that is pretty useful information,
and
> > if
> > > I don't return "human readable" strings from the certificate importer,
then
> > that
> > > information gets lost.  I could have added all the JSON codes to the
result
> > > codes for the importer, perhaps, but that seems wrong because then they
have
> > to
> > > be kept in sync.  So I went with adding string resources.
> > > 
> > > So, this change as it is has string resources in it, but there are
> dependency
> > > violations (because checkdeps has no rules for this lib, I think) to
> i10n_util
> > > and crypto still.
> > > 
> > > Any ideas on the best approach to take here?
> > 
> > Until now I also thought that returning internationalized strings would be
> nice
> > for the UI and easy to implement. Also was it the way the certificate
importer
> > and decrypter (from OncNetworkParser) already did it.
> > 
> > However, because of JsonReader returning only English errors (and only these
> > contains the location as line/column number) and the not-so-nice fact that
we
> > would have to add grit resources to src/chromeos made me rethink that
> direction.
> > 
> > The important question is, what detail of error description do we need in
the
> > UI?
> > 
> > The end-user shouldn't be interested in Json and detailed ONC errors. These
> can
> > only occur if CPanel or Chrome are buggy or if a wrong ONC was imported (and
> > that feature is unsupported by Google). Thus, a high-level error like "Not a
> > valid json dictionary" or "Invalid ONC" should be sufficient for the user to
> get
> > an idea of what broke.
> > 
> > For developers, a highlevel error is enough of a hint to search for the
> detailed
> > error in the log files. Sure, it isn't as convenient as a detailed error in
> > about:policy but is this convenience worth the effort?

[Greg Spencer (Chromium), 2012-12-03 19:26:37]

OK, so here's what I see my options are:

1) Move onc_utils and onc_certificate_importer back under chrome/ and just leave
the strings as they are.  Neither the Decrypt from the utils nor the importer
are referenced from the other onc_* code, so this is fine: it's only ever
referenced from Chrome code anyhow.  I was hoping to move anything onc_* into
chromeos, but it's not strictly necessary, I guess.

2) Combine the JSON errors and the ONC cert errors into one return code and
eliminate the localized strings.  I like this because I prefer enums like
Phillipp does, but I can see that it's kind of a franken-code: combining result
codes from two different parts of the code.

3) Convert from localized strings to hardcoded English (non-user-facing)
strings.  This has the advantage that I don't really need to change the
interface, and I don't need to combine the codes.  I still think I might want
more than just a bool for a result though, but I can try it and see if it seems
necessary.

I'm leaning towards #3 at the moment (which I think is basically what Steven
proposed).

[pneubeck (no reviews), 2012-12-03 20:15:34]

Since, I extracted these classes from OncNetworkParser and I already had the CL
ready that added error messages to the ONC validator (which I have to revert and
adapt), I'd volunteer to write a CL for something like 3).

What do you think?

On 2012/12/03 19:26:37, Greg Spencer (Chromium) wrote:
> OK, so here's what I see my options are:
> 
> 1) Move onc_utils and onc_certificate_importer back under chrome/ and just
leave
> the strings as they are.  Neither the Decrypt from the utils nor the importer
> are referenced from the other onc_* code, so this is fine: it's only ever
> referenced from Chrome code anyhow.  I was hoping to move anything onc_* into
> chromeos, but it's not strictly necessary, I guess.
> 
> 2) Combine the JSON errors and the ONC cert errors into one return code and
> eliminate the localized strings.  I like this because I prefer enums like
> Phillipp does, but I can see that it's kind of a franken-code: combining
result
> codes from two different parts of the code.
> 
> 3) Convert from localized strings to hardcoded English (non-user-facing)
> strings.  This has the advantage that I don't really need to change the
> interface, and I don't need to combine the codes.  I still think I might want
> more than just a bool for a result though, but I can try it and see if it
seems
> necessary.
> 
> I'm leaning towards #3 at the moment (which I think is basically what Steven
> proposed).

[Greg Spencer (Chromium), 2012-12-04 01:32:44]

OK, PTAL.  This now sends back untranslated strings from the ONC certificate
parsing code and the onc utils, and returns a bool for success or failure.

I removed the error output that printed the string to the UI, and replaced it
with a single string that says that ONC loading failed.  The logs should contain
any errors that occur.  Before, we were printing the errors even if they were
untranslated strings from the JSON parser.  At least this is more correct, and
we can compare to the constants to see which error has occurred.

[stevenjb (google-dont-use), 2012-12-04 09:05:06]

I think that if we eventually end up with something between 2) and 3) that
would be OK, i.e. we return an enum result code that only contains cases
that the code cares about (e.g. success, json parse error, onc validation
error, cert error, encryption error), and we set a more descriptive
(non-localized) error string for logging (and possibly
more descriptive localized UI messages).



On Mon, Dec 3, 2012 at 11:26 AM, <gspencer@chromium.org> wrote:

> OK, so here's what I see my options are:
>
> 1) Move onc_utils and onc_certificate_importer back under chrome/ and just
> leave
> the strings as they are.  Neither the Decrypt from the utils nor the
> importer
> are referenced from the other onc_* code, so this is fine: it's only ever
> referenced from Chrome code anyhow.  I was hoping to move anything onc_*
> into
> chromeos, but it's not strictly necessary, I guess.
>
> 2) Combine the JSON errors and the ONC cert errors into one return code and
> eliminate the localized strings.  I like this because I prefer enums like
> Phillipp does, but I can see that it's kind of a franken-code: combining
> result
> codes from two different parts of the code.
>
> 3) Convert from localized strings to hardcoded English (non-user-facing)
> strings.  This has the advantage that I don't really need to change the
> interface, and I don't need to combine the codes.  I still think I might
> want
> more than just a bool for a result though, but I can try it and see if it
> seems
> necessary.
>
> I'm leaning towards #3 at the moment (which I think is basically what
> Steven
> proposed).
>
>
https://codereview.chromium.**org/11299236/<https://codereview.chromium.org/1...
>

[stevenjb (google-dont-use), 2012-12-04 09:10:34]

On Mon, Dec 3, 2012 at 9:53 AM, <pneubeck@chromium.org> wrote:

> Steven, do you propose to
> - return english string constants (and no error code) from low level
> classes
>
I propose using strings as error codes. If convenient error codes already
exist, we can go ahead and use them, but we shouldn't be defining enums
where a string will be easier to read and debug.


> - then in the calling function to compare the returned error string to the
> available constants
>
Where we need to do more than just log the error code, yes. Mostly I
suspect the function will return a bool and just use the error code for
logging.


> - and then map to the right translatable message ID
>
Just in places where we want to display a user facing message in Chrome.



> It think, if we are already at it, then we should use error codes instead,
> because:
> 1) Lower runtime/memory (strings are copied all the time and are expensive
> to
> compare).
>
I don't think this is a significant overhead and strings are easier to read
and debug.


> 2) Typesafety because of Enums.
>
This doesn't seem particularly important, unless we rely heavily on the
specific error instead of just failed/succeeded. If the error result is
more than just informative then I could be could be convinced to use an
enum.

3) We don't need the exact error code in many cases. Thus carrying around a
> string is a bit too much. E.g. All critical ONC errors may be mapped to a
> code,
> all warnings may be mapped to another code, ...

Again I don't see this as being significant overhead.

I won't push too strongly against using error codes, but if we are just
mapping an error -> an enum -> a log string, I think we are better off just
passing around the log string.

-Steven



>
>
> On 2012/12/03 17:37:32, stevenjb (chromium) wrote:
>
>> 1. I do not think we should have translated strings in src/chromeos. In
>>
> addition
>
>> to added complexity, it puts UI code in a component that I envision as
>> being
>> lower level than that.
>>
>
>  2. I think that error strings are better than codes, but they should be
>> kept
>> simple, e.g. "Certificate GUID missing" is good, but "Encrypted ONC file
>> uses
>>
> an
>
>> unsupported encryption scheme" is too long and should probably be
>> shortened to
>> "unsupported encryption scheme," assuming the calling code has enough
>> context
>>
> to
>
>> provide "in encrypted ONC file" if we wish to present it in a human
>> readable
>> form.
>>
>
>  3. I've spoken to Zel about this in the past, and the rule of thumb is
>>
> generally
>
>> that if a string can be viewed in the browser, even in a debugging page
>> (e.g.
>>
> in
>
>> chrome://network), we should translate it. Only log messages do not need
>> to be
>> translated. (chrome://system toes that line, but essentially that is
>>
> displaying
>
>> log messages and we don't translate them).
>> 3b. If any of these errors are user facing outside of a log file, we
>> should
>> translate the error strings there, e.g. "Certificate GUID missing" should
>> be
>> defined as kErrorCertificateGUIDMissing somewhere and we should map that
>> to
>> IDS_NETWORK_ONC_ERROR_CERT_**GUID_MISSING (which should probably be more
>> user
>> friendly) in the UI code.
>>
>
>  Does that sound reasonable?
>>
>
>
>  On 2012/12/03 14:58:24, pneubeck wrote:
>> > On 2012/12/01 01:06:42, Greg Spencer (Chromium) wrote:
>> > > OK, this just got real. :-)
>> > >
>> > > I tried to merge in Phillipp's latest change, and because the
>> certificate
>> > > importer has strings, I had to add resources to chromeos.
>> >
>> > Not only the certificate importer has translated error strings but also
>> the
>> > decryption function and the onc validation (CL already published).
>> >
>> > > I tried going the other route: generating error codes from the
>> certificate
>> > > importer, and translating the error codes in the chrome code that
>> uses it,
>> but
>> > > at a certain level we get strings (hardcoded, in English) from the
>> JSON
>> > importer
>> > > that give us syntax error information that is pretty useful
>> information,
>>
> and
>
>> > if
>> > > I don't return "human readable" strings from the certificate importer,
>>
> then
>
>> > that
>> > > information gets lost.  I could have added all the JSON codes to the
>>
> result
>
>> > > codes for the importer, perhaps, but that seems wrong because then
>> they
>>
> have
>
>> > to
>> > > be kept in sync.  So I went with adding string resources.
>> > >
>> > > So, this change as it is has string resources in it, but there are
>> dependency
>> > > violations (because checkdeps has no rules for this lib, I think) to
>> i10n_util
>> > > and crypto still.
>> > >
>> > > Any ideas on the best approach to take here?
>> >
>> > Until now I also thought that returning internationalized strings would
>> be
>> nice
>> > for the UI and easy to implement. Also was it the way the certificate
>>
> importer
>
>> > and decrypter (from OncNetworkParser) already did it.
>> >
>> > However, because of JsonReader returning only English errors (and only
>> these
>> > contains the location as line/column number) and the not-so-nice fact
>> that
>>
> we
>
>> > would have to add grit resources to src/chromeos made me rethink that
>> direction.
>> >
>> > The important question is, what detail of error description do we need
>> in
>>
> the
>
>> > UI?
>> >
>> > The end-user shouldn't be interested in Json and detailed ONC errors.
>> These
>> can
>> > only occur if CPanel or Chrome are buggy or if a wrong ONC was imported
>> (and
>> > that feature is unsupported by Google). Thus, a high-level error like
>> "Not a
>> > valid json dictionary" or "Invalid ONC" should be sufficient for the
>> user to
>> get
>> > an idea of what broke.
>> >
>> > For developers, a highlevel error is enough of a hint to search for the
>> detailed
>> > error in the log files. Sure, it isn't as convenient as a detailed
>> error in
>> > about:policy but is this convenience worth the effort?
>>
>
>
https://codereview.chromium.**org/11299236/<https://codereview.chromium.org/1...
>

[pneubeck (no reviews), 2012-12-04 10:43:55]

I'm rather unhappy with the resulting code. I think we indeed ended up with
franken-code. The error strings are never used as "error codes" (we don't even
see easily which constants are potentially returned), aren't translated, don't
need to be logged.

Perhaps we lost sight of the high-level goal/requirements.

1) net-internals is an internal page and doesn't need any i18n.

2) about:policy (or in general all user visible UI) has to be translated.

In both cases, we only need the bare minimum of information:
 - All successful without any problems
 - Something was odd. Imported partially, or at warnings occurred.
 - Fatal problems occurred. Nothing worked.

This should be enough information to file a bug (in case of a end-user) or to
search for the warning/error in the log files (in case of a developer).


Thus my proposal is:
- In general, don't forward errors.
  Reason: This forwarding makes filtering of errors at the call-site difficult.
Most of the times it's unneeded because of the logging.

- Reduce the number of different errors to what the callers need.
  Reason: If we have many different error codes (or string constants), filtering
of errors is to painful to be worth the effort. As we want to distinguish
certain kinds of failures, we have to reduce the number of constants.


In the case of ParseAndStoreCertificates it is sufficient to return 
 enum Result { SUCCESSFUL_IMPORT, PARTIAL_IMPORT, IMPORT_ABORTED }
instead of the bool and string.
Accordingly most internal errors could be removed.

In the case of Decrypt a boolean is sufficient.

https://codereview.chromium.org/11299236/diff/5085/chrome/browser/chromeos/cr...
File chrome/browser/chromeos/cros/network_library_impl_base.cc (right):

https://codereview.chromium.org/11299236/diff/5085/chrome/browser/chromeos/cr...
chrome/browser/chromeos/cros/network_library_impl_base.cc:1065: *error =
decrypt_error;
We shouldn't forward constants here. They are/should not be used for logging.
See my high-level comment.

https://codereview.chromium.org/11299236/diff/5085/chrome/browser/chromeos/cr...
chrome/browser/chromeos/cros/network_library_impl_base.cc:1067: << " with error:
" << decrypt_error;
No need to log decrypt_error, as it is already logged by the decrypt function.

https://codereview.chromium.org/11299236/diff/5085/chrome/browser/chromeos/cr...
chrome/browser/chromeos/cros/network_library_impl_base.cc:1108: if
(!cert_importer.ParseAndStoreCertificates(*certificates, error)) {
same here, we shouldn't forward the error strings.

https://codereview.chromium.org/11299236/diff/5085/chrome/browser/chromeos/cr...
chrome/browser/chromeos/cros/network_library_impl_base.cc:1110: << "source " <<
source << " with error: " << *error;
We don't have to log this error as it is logged already by the importer.

https://codereview.chromium.org/11299236/diff/5085/chrome/browser/policy/netw...
File chrome/browser/policy/network_configuration_updater.cc (right):

https://codereview.chromium.org/11299236/diff/5085/chrome/browser/policy/netw...
chrome/browser/policy/network_configuration_updater.cc:102:
network_library_->LoadOncNetworks(new_network_config, "", onc_source,
LoadOncNetworks doesn't LOG(ERROR).
Please do that here, as you did with in net_internals.cc.

Alternatively, change LOG(WARNING) to LOG(ERROR) in LoadOncNetworks.

https://codereview.chromium.org/11299236/diff/5085/chrome/browser/ui/webui/ne...
File chrome/browser/ui/webui/net_internals/net_internals_ui.cc (right):

https://codereview.chromium.org/11299236/diff/5085/chrome/browser/ui/webui/ne...
chrome/browser/ui/webui/net_internals/net_internals_ui.cc:1496: if
(!cros_network->LoadOncNetworks(onc_blob, passcode,
We should also forwards warnings in the case of LoadOncNetworks returning true.

See also my (abandoned) change in
https://codereview.chromium.org/11415148/patch/6035/8035

https://codereview.chromium.org/11299236/diff/5085/chrome/browser/ui/webui/ne...
chrome/browser/ui/webui/net_internals/net_internals_ui.cc:1500: LOG(ERROR) <<
"Unable to load ONC blob: " << log_error;
no need to log |log_error|...

https://codereview.chromium.org/11299236/diff/5085/chrome/browser/ui/webui/ne...
chrome/browser/ui/webui/net_internals/net_internals_ui.cc:1502:
IDS_NETWORK_CONFIG_ERROR_UNABLE_TO_LOAD_ONC);
Actually, l10n isn't needed for net-internals. None of it is translated and it's
an "internal" page.

https://codereview.chromium.org/11299236/diff/5085/chromeos/network/onc/onc_c...
File chromeos/network/onc/onc_certificate_importer.cc (right):

https://codereview.chromium.org/11299236/diff/5085/chromeos/network/onc/onc_c...
chromeos/network/onc/onc_certificate_importer.cc:389: LOG(WARNING) << "ONC File:
PKCS12 data contains no importable certificates";
It would be helpful for feedback and debugging to propagate to the UI if a
warning occurred.
We could set |result| to a warning code/string.

The return value still indicates if the import was successful or that an error
occurred.

https://codereview.chromium.org/11299236/diff/5085/chromeos/network/onc/onc_c...
chromeos/network/onc/onc_certificate_importer.cc:410: LOG(WARNING) << "ONC File:
Unable to find private key for cert";
same here: set |result| to a warning

https://codereview.chromium.org/11299236/diff/5085/chromeos/network/onc/onc_c...
File chromeos/network/onc/onc_certificate_importer.h (right):

https://codereview.chromium.org/11299236/diff/5085/chromeos/network/onc/onc_c...
chromeos/network/onc/onc_certificate_importer.h:30: CHROMEOS_EXPORT extern const
char kErrorCertDataMalformed[];
If we want to use the strings as replacements for error codes, then it should be
clear which strings can be returned. If we share the strings between different
classes/functions, then it's impossible to figure that out except by looking at
the implementation.

Therefore, I would consider putting the strings (except standard codes like
kSuccess) into the class definition and in case of the Decrypt function, into a
namespace.

https://codereview.chromium.org/11299236/diff/5085/chromeos/network/onc/onc_c...
chromeos/network/onc/onc_certificate_importer.h:59: bool
ParseAndStoreCertificates(
You don't document the return value anymore.

I would write "...sets |result|..." instead of "Returns the result" to
distinguish |result| and return value.

Please, also refer to the error constants above.

PARSE_OK?

https://codereview.chromium.org/11299236/diff/5085/chromeos/network/onc/onc_c...
chromeos/network/onc/onc_certificate_importer.h:85: bool ParseClientCertificate(
nit: add an empty line

https://codereview.chromium.org/11299236/diff/5085/chromeos/network/onc/onc_u...
File chromeos/network/onc/onc_utils.cc (right):

https://codereview.chromium.org/11299236/diff/5085/chromeos/network/onc/onc_u...
chromeos/network/onc/onc_utils.cc:158: *error =
kErrorPropertyDictionaryMalformed;
If we go on using prefixes for the constants, than this constant should also
start with "kErrorEncryptedOnc..."

https://codereview.chromium.org/11299236/diff/5085/chromeos/network/onc/onc_u...
chromeos/network/onc/onc_utils.cc:162: *error = kResultSuccess;
That overwrites the json error, as there is no return in the last if clause,
instead the new_root pointer is NULL.

https://codereview.chromium.org/11299236/diff/5085/chromeos/network/onc/onc_u...
File chromeos/network/onc/onc_utils.h (right):

https://codereview.chromium.org/11299236/diff/5085/chromeos/network/onc/onc_u...
chromeos/network/onc/onc_utils.h:31: // doesn't modify |error|. Otherwise
returns NULL and sets |error| to a
|error| -> |result|

https://codereview.chromium.org/11299236/diff/5085/chromeos/network/onc/onc_u...
chromeos/network/onc/onc_utils.h:39: // |error| is not modified. If an error
occurs, returns NULL and if additionally
|error| is always modified if not-null.

https://codereview.chromium.org/11299236/diff/5085/chromeos/network/onc/onc_u...
chromeos/network/onc/onc_utils.h:40: // |error| is not NULL, |error| is set to a
non-user-facing (untranslated) error
|error| -> |result|

[Greg Spencer (Chromium), 2012-12-06 18:18:15]

On 2012/12/04 10:43:55, pneubeck wrote:
> I'm rather unhappy with the resulting code. I think we indeed ended up with
> franken-code. The error strings are never used as "error codes" (we don't even
> see easily which constants are potentially returned), aren't translated, don't
> need to be logged.

The string constants are available in the headers, but I know what you mean:
we're not using them, so why have them?  I would prefer an enum, I think.

> 1) net-internals is an internal page and doesn't need any i18n.
> 
> 2) about:policy (or in general all user visible UI) has to be translated.
> 
> In both cases, we only need the bare minimum of information:
>  - All successful without any problems
>  - Something was odd. Imported partially, or at warnings occurred.
>  - Fatal problems occurred. Nothing worked.

Actually, I think that on net-internals, it is nice to see more detailed
information, especially since it doesn't need translation.  It helps ONC writers
to be able to attempt import of their files and see what went wrong (especially
JSON errors that have line numbers) without having to know about and dig through
long logs. 

In the case of about:policy, I agree with you that we don't need a lot of
information there.

> Thus my proposal is:
> - In general, don't forward errors.

If we're going to display detailed information anywhere, we would need to
forward the errors.

>   Reason: This forwarding makes filtering of errors at the call-site
difficult.
> Most of the times it's unneeded because of the logging.

I agree that logging of the same error happens at multiple levels with the
current code, and I don't really like that.  If it weren't for losing the line
number information in the JSON parser, I would  like the idea of forming a big
enum with all the possible errors, and just do logging at the lower levels.

So, the questions are: 1) how important is the detailed information on
net-internals. and 2) If that's important, then how important is the line number
information from the JSON parser.

You are thinking #1 isn't important.  I disagree, but I'm not sure how strongly
I feel about it.  I can try just doing a three-value enum and see how annoying
it is.  Certainly the API is simpler that way.  I'll upload that and address
your other comments and see how it looks.

[pneubeck (no reviews), 2012-12-06 18:26:00]

On 2012/12/06 18:18:15, Greg Spencer (Chromium) wrote:
> On 2012/12/04 10:43:55, pneubeck wrote:
> > I'm rather unhappy with the resulting code. I think we indeed ended up with
> > franken-code. The error strings are never used as "error codes" (we don't
even
> > see easily which constants are potentially returned), aren't translated,
don't
> > need to be logged.
> 
> The string constants are available in the headers, but I know what you mean:
> we're not using them, so why have them?  I would prefer an enum, I think.
> 
> > 1) net-internals is an internal page and doesn't need any i18n.
> > 
> > 2) about:policy (or in general all user visible UI) has to be translated.
> > 
> > In both cases, we only need the bare minimum of information:
> >  - All successful without any problems
> >  - Something was odd. Imported partially, or at warnings occurred.
> >  - Fatal problems occurred. Nothing worked.
> 
> Actually, I think that on net-internals, it is nice to see more detailed
> information, especially since it doesn't need translation.  It helps ONC
writers
> to be able to attempt import of their files and see what went wrong
(especially
> JSON errors that have line numbers) without having to know about and dig
through
> long logs. 

I think, there is a more generic solution to that problem:
There is already a "Logs" page on net-internals. It's current state is rather
simplistic. But if it would support simple filtering by URL arguments, we could
put a link in the ONC import message that to a Logs page that shows only ONC
errors.

That way, we wouldn't have to pollute the code with message forwarding and still
would ease the debugging for developers.

[stevenjb (google-dont-use), 2012-12-06 18:27:27]

First off a caveat: I haven't really looked at the details here, just
providing some birds-eye-view feedback. So, keep that in mind wrt any of my
feedback :)

If detailed error information in net-internals is the only place where you
want to forward the detailed error strings, you might want to look
at network_event_log.h and do something similar. I added this so that we
can generate logging events for debugging in the chromeos/network code,
then display them as a string in the UI, without spamming the stdio log.
You could either do something similar for ONC parsing, or just use the same
log. Alternatively, it might make more sense to have a GetLastError()
method in the parser.

One last comment on enums: If we pay attention to them in the code, using
them is fine, but I would strongly suggest against defining a bunch of
result codes that are only used to inform the user; strings work better for
that.





On Thu, Dec 6, 2012 at 10:18 AM, <gspencer@chromium.org> wrote:

> On 2012/12/04 10:43:55, pneubeck wrote:
>
>> I'm rather unhappy with the resulting code. I think we indeed ended up
>> with
>> franken-code. The error strings are never used as "error codes" (we don't
>> even
>> see easily which constants are potentially returned), aren't translated,
>> don't
>> need to be logged.
>>
>
> The string constants are available in the headers, but I know what you
> mean:
> we're not using them, so why have them?  I would prefer an enum, I think.
>
>
>  1) net-internals is an internal page and doesn't need any i18n.
>>
>
>  2) about:policy (or in general all user visible UI) has to be translated.
>>
>
>  In both cases, we only need the bare minimum of information:
>>   - All successful without any problems
>>   - Something was odd. Imported partially, or at warnings occurred.
>>   - Fatal problems occurred. Nothing worked.
>>
>
> Actually, I think that on net-internals, it is nice to see more detailed
> information, especially since it doesn't need translation.  It helps ONC
> writers
> to be able to attempt import of their files and see what went wrong
> (especially
> JSON errors that have line numbers) without having to know about and dig
> through
> long logs.
>
> In the case of about:policy, I agree with you that we don't need a lot of
> information there.
>
>
>  Thus my proposal is:
>> - In general, don't forward errors.
>>
>
> If we're going to display detailed information anywhere, we would need to
> forward the errors.
>
>
>     Reason: This forwarding makes filtering of errors at the call-site
>>
> difficult.
>
>> Most of the times it's unneeded because of the logging.
>>
>
> I agree that logging of the same error happens at multiple levels with the
> current code, and I don't really like that.  If it weren't for losing the
> line
> number information in the JSON parser, I would  like the idea of forming a
> big
> enum with all the possible errors, and just do logging at the lower levels.
>
> So, the questions are: 1) how important is the detailed information on
> net-internals. and 2) If that's important, then how important is the line
> number
> information from the JSON parser.
>
> You are thinking #1 isn't important.  I disagree, but I'm not sure how
> strongly
> I feel about it.  I can try just doing a three-value enum and see how
> annoying
> it is.  Certainly the API is simpler that way.  I'll upload that and
> address
> your other comments and see how it looks.
>
>
https://codereview.chromium.**org/11299236/<https://codereview.chromium.org/1...
>

[Greg Spencer (Chromium), 2012-12-06 18:30:16]

On 2012/12/06 18:27:27, stevenjb1 wrote:
> If detailed error information in net-internals is the only place where you
> want to forward the detailed error strings, you might want to look
> at network_event_log.h and do something similar. I added this so that we
> can generate logging events for debugging in the chromeos/network code,
> then display them as a string in the UI, without spamming the stdio log.
> You could either do something similar for ONC parsing, or just use the same
> log. Alternatively, it might make more sense to have a GetLastError()
> method in the parser.

OK, I'll take a look at using network_event_log: Phillipp had the same
suggestion.  I didn't know that existed.

[Greg Spencer (Chromium), 2012-12-06 18:33:10]

On 2012/12/06 18:30:16, Greg Spencer (Chromium) wrote:
> On 2012/12/06 18:27:27, stevenjb1 wrote:
> > If detailed error information in net-internals is the only place where you
> > want to forward the detailed error strings, you might want to look
> > at network_event_log.h and do something similar. I added this so that we
> > can generate logging events for debugging in the chromeos/network code,
> > then display them as a string in the UI, without spamming the stdio log.
> > You could either do something similar for ONC parsing, or just use the same
> > log. Alternatively, it might make more sense to have a GetLastError()
> > method in the parser.
> 
> OK, I'll take a look at using network_event_log: Phillipp had the same
> suggestion.  I didn't know that existed.

I just looked at the header: THAT network event log.  Yes, I did know about
that, and I should have thought of using it sooner.  I guess I was thinking of
it more as something specifically for networking events like connection problems
and such.

[pneubeck (no reviews), 2012-12-06 18:40:37]

On 2012/12/06 18:33:10, Greg Spencer (Chromium) wrote:
> On 2012/12/06 18:30:16, Greg Spencer (Chromium) wrote:
> > On 2012/12/06 18:27:27, stevenjb1 wrote:
> > > If detailed error information in net-internals is the only place where you
> > > want to forward the detailed error strings, you might want to look
> > > at network_event_log.h and do something similar. I added this so that we
> > > can generate logging events for debugging in the chromeos/network code,
> > > then display them as a string in the UI, without spamming the stdio log.
> > > You could either do something similar for ONC parsing, or just use the
same
> > > log. Alternatively, it might make more sense to have a GetLastError()
> > > method in the parser.
> > 
> > OK, I'll take a look at using network_event_log: Phillipp had the same
> > suggestion.  I didn't know that existed.
> 
> I just looked at the header: THAT network event log.  Yes, I did know about
> that, and I should have thought of using it sooner.  I guess I was thinking of
> it more as something specifically for networking events like connection
problems
> and such.

I like the idea a lot to use a more capable logging system to simplify our
problem. However in its current state, network_event_log.h doesn't replace
logging.h because it doesn't provide macros to output the source-code location
(file/line number) and logging to both logs doesn't make sense to me.
There should also be a command-line flag to forward the network_event_logs to
the chrome.log so that it is part of the user feedback.

My suggestion is to post-pone that decision and for now go with the most simple
solution: logging.h

[stevenjb (google-dont-use), 2012-12-06 18:56:26]

Do we really care about source code line numbers here? If we do, we could
certainly optionally add that to network_event_log, or just define a macro
that formats the 'module' argument with line number info in the places that
we care about using __FILE__ and __LINE__.

network_event_log uses VLOG(2) to log to stdout with
--vmodule=network_event_log.cc. If there are places where it makes sense to
both log the event for convenient viewing and log an ERROR or WARNING I
think doing both (perhaps in a wrapper) is perfectly reasonable.
See ShillErrorCallbackFunction for an example.



On Thu, Dec 6, 2012 at 10:40 AM, <pneubeck@chromium.org> wrote:

> On 2012/12/06 18:33:10, Greg Spencer (Chromium) wrote:
>
>> On 2012/12/06 18:30:16, Greg Spencer (Chromium) wrote:
>> > On 2012/12/06 18:27:27, stevenjb1 wrote:
>> > > If detailed error information in net-internals is the only place
>> where you
>> > > want to forward the detailed error strings, you might want to look
>> > > at network_event_log.h and do something similar. I added this so that
>> we
>> > > can generate logging events for debugging in the chromeos/network
>> code,
>> > > then display them as a string in the UI, without spamming the stdio
>> log.
>> > > You could either do something similar for ONC parsing, or just use the
>>
> same
>
>> > > log. Alternatively, it might make more sense to have a GetLastError()
>> > > method in the parser.
>> >
>> > OK, I'll take a look at using network_event_log: Phillipp had the same
>> > suggestion.  I didn't know that existed.
>>
>
>  I just looked at the header: THAT network event log.  Yes, I did know
>> about
>> that, and I should have thought of using it sooner.  I guess I was
>> thinking of
>> it more as something specifically for networking events like connection
>>
> problems
>
>> and such.
>>
>
> I like the idea a lot to use a more capable logging system to simplify our
> problem. However in its current state, network_event_log.h doesn't replace
> logging.h because it doesn't provide macros to output the source-code
> location
> (file/line number) and logging to both logs doesn't make sense to me.
> There should also be a command-line flag to forward the network_event_logs
> to
> the chrome.log so that it is part of the user feedback.
>
> My suggestion is to post-pone that decision and for now go with the most
> simple
> solution: logging.h
>
>
https://codereview.chromium.**org/11299236/<https://codereview.chromium.org/1...
>

[Greg Spencer (Chromium), 2012-12-06 19:14:35]

On Thu, Dec 6, 2012 at 10:56 AM, Steven Bennetts <stevenjb@google.com> wrote:
> Do we really care about source code line numbers here? If we do, we
> could certainly optionally add that to network_event_log, or just
> define a macro that formats the 'module' argument with line number
> info in the places that we care about using __FILE__ and __LINE__.

Yes, before I even saw this email, I had already defined a macro at
the top of the file that did __FILE__(__func__):__LINE__ as the
"module" field.  It was pretty easy.
 
> network_event_log uses VLOG(2) to log to stdout with
> --vmodule=network_event_log.cc. If there are places where it makes
> sense to both log the event for convenient viewing and log an ERROR
> or WARNING I think doing both (perhaps in a wrapper) is perfectly
> reasonable. See ShillErrorCallbackFunction for an example.

To do this, we could use a ERROR or WARNING prefix to the "event"
field, and that could be in the macro too.  I'd rather fix the network
logging to include what we need than revert to using logging.h logging.

One thing I'd like to see in the header for the network logging: a
good, concrete description of what "module" and "event" fields should
contain.  Otherwise they're going to contain all kinds of different
things and will be hard to format/filter.  For instance, should module
contain file/line information like above, or should it be a simple
identifier that groups the log output (e.g. "ONC").  Or should there
be yet another input string to the logging...

-Greg.

[pneubeck (no reviews), 2012-12-06 19:25:58]

On 2012/12/06 18:27:27, stevenjb1 wrote:
> you might want to look
> at network_event_log.h and do something similar. I added this so that we
> can generate logging events for debugging in the chromeos/network code,
> then display them as a string in the UI, without spamming the stdio log.

In general I'm fine with the network_event_log, but still think that it's a bit
too much...
Please remember: Spamming the stdio shouldn't occur with the functions/classes
we're talking about. The relevant message's are only produced in case of
corrupted configuration or a broken certificate database.

Looks very different compared to the current usage of network_event_logger.

[stevenjb (google-dont-use), 2012-12-06 19:26:33]

Great, please, by all means, expand and improve network_event_logging.

My thinking behind "module" and "event" was that "module" would generally
be a class or file name, and event a short description of what occurred
like "Connection Requested" with "details" containing information like the
name of the network connected to. Please feel free to add comments to the
header.

So, if we're defining a macro, I would suggest __FILE__ as the module and
__func__:__LINE__ as the event. The intention was that if we want to
display only certain types of events we could just filter by module.

-Steven


On Thu, Dec 6, 2012 at 11:14 AM, <gspencer@chromium.org> wrote:

> On Thu, Dec 6, 2012 at 10:56 AM, Steven Bennetts <stevenjb@google.com>
> wrote:
>
>> Do we really care about source code line numbers here? If we do, we
>> could certainly optionally add that to network_event_log, or just
>> define a macro that formats the 'module' argument with line number
>> info in the places that we care about using __FILE__ and __LINE__.
>>
>
> Yes, before I even saw this email, I had already defined a macro at
> the top of the file that did __FILE__(__func__):__LINE__ as the
> "module" field.  It was pretty easy.
>
>
>  network_event_log uses VLOG(2) to log to stdout with
>> --vmodule=network_event_log.**cc. If there are places where it makes
>> sense to both log the event for convenient viewing and log an ERROR
>> or WARNING I think doing both (perhaps in a wrapper) is perfectly
>> reasonable. See ShillErrorCallbackFunction for an example.
>>
>
> To do this, we could use a ERROR or WARNING prefix to the "event"
> field, and that could be in the macro too.  I'd rather fix the network
> logging to include what we need than revert to using logging.h logging.
>
> One thing I'd like to see in the header for the network logging: a
> good, concrete description of what "module" and "event" fields should
> contain.  Otherwise they're going to contain all kinds of different
> things and will be hard to format/filter.  For instance, should module
> contain file/line information like above, or should it be a simple
> identifier that groups the log output (e.g. "ONC").  Or should there
> be yet another input string to the logging...
>
> -Greg.
>
>
https://codereview.chromium.**org/11299236/<https://codereview.chromium.org/1...
>

[Greg Spencer (Chromium), 2012-12-07 18:12:27]

OK, I've removed the error text forwarding, and instead log detailed errors to
the net_event_log, and just show success or failure in net-internals.

https://codereview.chromium.org/11299236/diff/5085/chrome/browser/chromeos/cr...
File chrome/browser/chromeos/cros/network_library_impl_base.cc (right):

https://codereview.chromium.org/11299236/diff/5085/chrome/browser/chromeos/cr...
chrome/browser/chromeos/cros/network_library_impl_base.cc:1065: *error =
decrypt_error;
On 2012/12/04 10:43:56, pneubeck wrote:
> We shouldn't forward constants here. They are/should not be used for logging.
> See my high-level comment.

No longer forwarding.

https://codereview.chromium.org/11299236/diff/5085/chrome/browser/chromeos/cr...
chrome/browser/chromeos/cros/network_library_impl_base.cc:1067: << " with error:
" << decrypt_error;
On 2012/12/04 10:43:56, pneubeck wrote:
> No need to log decrypt_error, as it is already logged by the decrypt function.

Done.

https://codereview.chromium.org/11299236/diff/5085/chrome/browser/chromeos/cr...
chrome/browser/chromeos/cros/network_library_impl_base.cc:1108: if
(!cert_importer.ParseAndStoreCertificates(*certificates, error)) {
On 2012/12/04 10:43:56, pneubeck wrote:
> same here, we shouldn't forward the error strings.

Done.

https://codereview.chromium.org/11299236/diff/5085/chrome/browser/chromeos/cr...
chrome/browser/chromeos/cros/network_library_impl_base.cc:1110: << "source " <<
source << " with error: " << *error;
On 2012/12/04 10:43:56, pneubeck wrote:
> We don't have to log this error as it is logged already by the importer.

Done.

https://codereview.chromium.org/11299236/diff/5085/chrome/browser/policy/netw...
File chrome/browser/policy/network_configuration_updater.cc (right):

https://codereview.chromium.org/11299236/diff/5085/chrome/browser/policy/netw...
chrome/browser/policy/network_configuration_updater.cc:102:
network_library_->LoadOncNetworks(new_network_config, "", onc_source,
On 2012/12/04 10:43:56, pneubeck wrote:
> LoadOncNetworks doesn't LOG(ERROR).
> Please do that here, as you did with in net_internals.cc.
> 
> Alternatively, change LOG(WARNING) to LOG(ERROR) in LoadOncNetworks.

|error| went away.

https://codereview.chromium.org/11299236/diff/5085/chrome/browser/ui/webui/ne...
File chrome/browser/ui/webui/net_internals/net_internals_ui.cc (right):

https://codereview.chromium.org/11299236/diff/5085/chrome/browser/ui/webui/ne...
chrome/browser/ui/webui/net_internals/net_internals_ui.cc:1496: if
(!cros_network->LoadOncNetworks(onc_blob, passcode,
On 2012/12/04 10:43:56, pneubeck wrote:
> We should also forwards warnings in the case of LoadOncNetworks returning
true.
> 
> See also my (abandoned) change in
> https://codereview.chromium.org/11415148/patch/6035/8035

Logging now goes to network logging interface, including warnings.

https://codereview.chromium.org/11299236/diff/5085/chrome/browser/ui/webui/ne...
chrome/browser/ui/webui/net_internals/net_internals_ui.cc:1500: LOG(ERROR) <<
"Unable to load ONC blob: " << log_error;
On 2012/12/04 10:43:56, pneubeck wrote:
> no need to log |log_error|...

Done.

https://codereview.chromium.org/11299236/diff/5085/chrome/browser/ui/webui/ne...
chrome/browser/ui/webui/net_internals/net_internals_ui.cc:1502:
IDS_NETWORK_CONFIG_ERROR_UNABLE_TO_LOAD_ONC);
On 2012/12/04 10:43:56, pneubeck wrote:
> Actually, l10n isn't needed for net-internals. None of it is translated and
it's
> an "internal" page.

Done.

https://codereview.chromium.org/11299236/diff/5085/chromeos/network/onc/onc_c...
File chromeos/network/onc/onc_certificate_importer.cc (right):

https://codereview.chromium.org/11299236/diff/5085/chromeos/network/onc/onc_c...
chromeos/network/onc/onc_certificate_importer.cc:389: LOG(WARNING) << "ONC File:
PKCS12 data contains no importable certificates";
On 2012/12/04 10:43:56, pneubeck wrote:
> It would be helpful for feedback and debugging to propagate to the UI if a
> warning occurred.
> We could set |result| to a warning code/string.
> 
> The return value still indicates if the import was successful or that an error
> occurred.

We log warnings to the network log now.

https://codereview.chromium.org/11299236/diff/5085/chromeos/network/onc/onc_c...
chromeos/network/onc/onc_certificate_importer.cc:410: LOG(WARNING) << "ONC File:
Unable to find private key for cert";
On 2012/12/04 10:43:56, pneubeck wrote:
> same here: set |result| to a warning

result no longer exists.

https://codereview.chromium.org/11299236/diff/5085/chromeos/network/onc/onc_c...
File chromeos/network/onc/onc_certificate_importer.h (right):

https://codereview.chromium.org/11299236/diff/5085/chromeos/network/onc/onc_c...
chromeos/network/onc/onc_certificate_importer.h:30: CHROMEOS_EXPORT extern const
char kErrorCertDataMalformed[];
On 2012/12/04 10:43:56, pneubeck wrote:
> If we want to use the strings as replacements for error codes, then it should
be
> clear which strings can be returned. If we share the strings between different
> classes/functions, then it's impossible to figure that out except by looking
at
> the implementation.
> 
> Therefore, I would consider putting the strings (except standard codes like
> kSuccess) into the class definition and in case of the Decrypt function, into
a
> namespace.

We're not returning these anymore.

https://codereview.chromium.org/11299236/diff/5085/chromeos/network/onc/onc_c...
chromeos/network/onc/onc_certificate_importer.h:59: bool
ParseAndStoreCertificates(
On 2012/12/04 10:43:56, pneubeck wrote:
> You don't document the return value anymore.
> 
> I would write "...sets |result|..." instead of "Returns the result" to
> distinguish |result| and return value.
> 
> Please, also refer to the error constants above.
> 
> PARSE_OK? 

Fixed this: it now returns an error code, and documents the error code.

https://codereview.chromium.org/11299236/diff/5085/chromeos/network/onc/onc_c...
chromeos/network/onc/onc_certificate_importer.h:85: bool ParseClientCertificate(
On 2012/12/04 10:43:56, pneubeck wrote:
> nit: add an empty line

Done.

https://codereview.chromium.org/11299236/diff/5085/chromeos/network/onc/onc_u...
File chromeos/network/onc/onc_utils.cc (right):

https://codereview.chromium.org/11299236/diff/5085/chromeos/network/onc/onc_u...
chromeos/network/onc/onc_utils.cc:158: *error =
kErrorPropertyDictionaryMalformed;
On 2012/12/04 10:43:56, pneubeck wrote:
> If we go on using prefixes for the constants, than this constant should also
> start with "kErrorEncryptedOnc..."

Constant went away.

https://codereview.chromium.org/11299236/diff/5085/chromeos/network/onc/onc_u...
chromeos/network/onc/onc_utils.cc:162: *error = kResultSuccess;
On 2012/12/04 10:43:56, pneubeck wrote:
> That overwrites the json error, as there is no return in the last if clause,
> instead the new_root pointer is NULL.

good catch.  Added return from last if.

https://codereview.chromium.org/11299236/diff/5085/chromeos/network/onc/onc_u...
File chromeos/network/onc/onc_utils.h (right):

https://codereview.chromium.org/11299236/diff/5085/chromeos/network/onc/onc_u...
chromeos/network/onc/onc_utils.h:31: // doesn't modify |error|. Otherwise
returns NULL and sets |error| to a
On 2012/12/04 10:43:56, pneubeck wrote:
> |error| -> |result|

Removed error.

https://codereview.chromium.org/11299236/diff/5085/chromeos/network/onc/onc_u...
chromeos/network/onc/onc_utils.h:39: // |error| is not modified. If an error
occurs, returns NULL and if additionally
On 2012/12/04 10:43:56, pneubeck wrote:
> |error| is always modified if not-null.

removed error

https://codereview.chromium.org/11299236/diff/5085/chromeos/network/onc/onc_u...
chromeos/network/onc/onc_utils.h:40: // |error| is not NULL, |error| is set to a
non-user-facing (untranslated) error
On 2012/12/04 10:43:56, pneubeck wrote:
> |error| -> |result|

removed error

[pneubeck (no reviews), 2012-12-10 09:33:10]

lgtm

looks really good to me now :-)

https://codereview.chromium.org/11299236/diff/20005/chrome/browser/chromeos/c...
File chrome/browser/chromeos/cros/mock_network_library.h (right):

https://codereview.chromium.org/11299236/diff/20005/chrome/browser/chromeos/c...
chrome/browser/chromeos/cros/mock_network_library.h:11: #include
"testing/gmock/include/gmock/gmock.h"
include onc_constants

https://codereview.chromium.org/11299236/diff/20005/chrome/browser/chromeos/c...
File chrome/browser/chromeos/cros/network_library_impl_base.cc (right):

https://codereview.chromium.org/11299236/diff/20005/chrome/browser/chromeos/c...
chrome/browser/chromeos/cros/network_library_impl_base.cc:52: std::string
GetSourceAsString(onc::ONCSource source) {
move to onc_utils.*

https://codereview.chromium.org/11299236/diff/20005/chrome/browser/chromeos/c...
File chrome/browser/chromeos/cros/network_ui_data_unittest.cc (right):

https://codereview.chromium.org/11299236/diff/20005/chrome/browser/chromeos/c...
chrome/browser/chromeos/cros/network_ui_data_unittest.cc:38:
EXPECT_EQ(onc::ONC_SOURCE_USER_IMPORT,
nit: no linebreak

https://codereview.chromium.org/11299236/diff/20005/chrome/browser/ui/webui/n...
File chrome/browser/ui/webui/net_internals/net_internals_ui.cc (right):

https://codereview.chromium.org/11299236/diff/20005/chrome/browser/ui/webui/n...
chrome/browser/ui/webui/net_internals/net_internals_ui.cc:74: #include
"ui/base/l10n/l10n_util.h"
unused?

https://codereview.chromium.org/11299236/diff/20005/chromeos/network/network_...
File chromeos/network/network_event_log.h (right):

https://codereview.chromium.org/11299236/diff/20005/chromeos/network/network_...
chromeos/network/network_event_log.h:52: std::string(__FILE__) + ":" +
::base::StringPrintf("%d",__LINE__),   \
that seems rather fine-grained. maybe better:
module: __FILE__
event: __LINE__ + __FUNC__

StringPrintf instead of concatentation: "%s %d", FUNC, LINE

https://codereview.chromium.org/11299236/diff/20005/chromeos/network/onc/onc_...
File chromeos/network/onc/onc_certificate_importer.cc (right):

https://codereview.chromium.org/11299236/diff/20005/chromeos/network/onc/onc_...
chromeos/network/onc/onc_certificate_importer.cc:76: NET_LOG_WARNING("Unable to
delete certificate");
to match the "return false", this should perhaps be an ERROR?

https://codereview.chromium.org/11299236/diff/20005/chromeos/network/onc/onc_...
chromeos/network/onc/onc_certificate_importer.cc:189: LOG(WARNING) << "Web trust
not granted for certificate: " << guid;
NET_LOG_

https://codereview.chromium.org/11299236/diff/20005/chromeos/network/onc/onc_...
File chromeos/network/onc/onc_certificate_importer_unittest.cc (right):

https://codereview.chromium.org/11299236/diff/20005/chromeos/network/onc/onc_...
chromeos/network/onc/onc_certificate_importer_unittest.cc:29: net::CertType
GetCertType(net::X509Certificate::OSCertHandle cert) {
please add a comment where this code comes from.
maybe we can merge it again, later.

https://codereview.chromium.org/11299236/diff/20005/chromeos/network/onc/onc_...
File chromeos/network/onc/onc_mapper.h (right):

https://codereview.chromium.org/11299236/diff/20005/chromeos/network/onc/onc_...
chromeos/network/onc/onc_mapper.h:11: #include "chromeos/chromeos_export.h"
unused

https://codereview.chromium.org/11299236/diff/20005/chromeos/network/onc/onc_...
File chromeos/network/onc/onc_test_utils.cc (right):

https://codereview.chromium.org/11299236/diff/20005/chromeos/network/onc/onc_...
chromeos/network/onc/onc_test_utils.cc:32: NOTREACHED() << "Unable to get test
dictionary path for "
better log a message at the DCHECK in GetTestDataPath
and just CHECK here.

[stevenjb, 2012-12-10 20:46:51]

lgtm

https://codereview.chromium.org/11299236/diff/20005/chromeos/network/network_...
File chromeos/network/network_event_log.h (right):

https://codereview.chromium.org/11299236/diff/20005/chromeos/network/network_...
chromeos/network/network_event_log.h:52: std::string(__FILE__) + ":" +
::base::StringPrintf("%d",__LINE__),   \
On 2012/12/10 09:33:10, pneubeck wrote:
> that seems rather fine-grained. maybe better:
> module: __FILE__
> event: __LINE__ + __FUNC__
> 
> StringPrintf instead of concatentation: "%s %d", FUNC, LINE
Agreed (although I'd do __FUNC__:__LINE__). I expect we'll want to add
per-module filtering to network_event_log::GetString; filtering by file seems
more useful than by file + line number.

[Greg Spencer (Chromium), 2012-12-10 23:54:03]

https://codereview.chromium.org/11299236/diff/20005/chrome/browser/chromeos/c...
File chrome/browser/chromeos/cros/mock_network_library.h (right):

https://codereview.chromium.org/11299236/diff/20005/chrome/browser/chromeos/c...
chrome/browser/chromeos/cros/mock_network_library.h:11: #include
"testing/gmock/include/gmock/gmock.h"
On 2012/12/10 09:33:10, pneubeck wrote:
> include onc_constants

Done.

https://codereview.chromium.org/11299236/diff/20005/chrome/browser/chromeos/c...
File chrome/browser/chromeos/cros/network_library_impl_base.cc (right):

https://codereview.chromium.org/11299236/diff/20005/chrome/browser/chromeos/c...
chrome/browser/chromeos/cros/network_library_impl_base.cc:52: std::string
GetSourceAsString(onc::ONCSource source) {
On 2012/12/10 09:33:10, pneubeck wrote:
> move to onc_utils.*

Done.

https://codereview.chromium.org/11299236/diff/20005/chrome/browser/chromeos/c...
File chrome/browser/chromeos/cros/network_ui_data_unittest.cc (right):

https://codereview.chromium.org/11299236/diff/20005/chrome/browser/chromeos/c...
chrome/browser/chromeos/cros/network_ui_data_unittest.cc:38:
EXPECT_EQ(onc::ONC_SOURCE_USER_IMPORT,
On 2012/12/10 09:33:10, pneubeck wrote:
> nit: no linebreak

Done.

https://codereview.chromium.org/11299236/diff/20005/chrome/browser/ui/webui/n...
File chrome/browser/ui/webui/net_internals/net_internals_ui.cc (right):

https://codereview.chromium.org/11299236/diff/20005/chrome/browser/ui/webui/n...
chrome/browser/ui/webui/net_internals/net_internals_ui.cc:74: #include
"ui/base/l10n/l10n_util.h"
On 2012/12/10 09:33:10, pneubeck wrote:
> unused?

Yes.  Removed.

https://codereview.chromium.org/11299236/diff/20005/chromeos/network/network_...
File chromeos/network/network_event_log.h (right):

https://codereview.chromium.org/11299236/diff/20005/chromeos/network/network_...
chromeos/network/network_event_log.h:52: std::string(__FILE__) + ":" +
::base::StringPrintf("%d",__LINE__),   \
On 2012/12/10 20:46:51, stevenjb (chromium) wrote:
> On 2012/12/10 09:33:10, pneubeck wrote:
> > that seems rather fine-grained. maybe better:
> > module: __FILE__
> > event: __LINE__ + __FUNC__
> > 
> > StringPrintf instead of concatentation: "%s %d", FUNC, LINE
> Agreed (although I'd do __FUNC__:__LINE__). I expect we'll want to add
> per-module filtering to network_event_log::GetString; filtering by file seems
> more useful than by file + line number.

What about if I just combine all of them for the "event", (something like file +
":" + line_number + " (" + function + ")") and pass in a "module" so that we can
use "ONC" for the module, and the event includes all the specifics about which
file, etc.  I'm thinking about both of your comments about showing
warnings/errors during import by just showing the log info.

If we had a way to filter all the output from a particular system (like "ONC"),
then it would be easier to display the log entries for it.  Otherwise, we'll
have to keep updating a list of files that need to be displayed on the ONC
import page.

[commit-bot: I haz the power, 2012-12-10 23:55:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gspencer@chromium.org/11299236/44001

[commit-bot: I haz the power, 2012-12-11 00:40:00]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_chromeos.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2012-12-11 17:39:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gspencer@chromium.org/11299236/54001

[commit-bot: I haz the power, 2012-12-11 18:11:56]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_chromeos.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2012-12-11 18:56:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gspencer@chromium.org/11299236/56006

[commit-bot: I haz the power, 2012-12-11 19:41:30]

Retried try job too often on linux_chromeos for step(s) chromeos_unittests

[commit-bot: I haz the power, 2012-12-11 19:59:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gspencer@chromium.org/11299236/56130

[commit-bot: I haz the power, 2012-12-11 23:02:58]

Change committed as 172446