Extending the translation from ONC to Shill.

Extending the translation from ONC to Shill.

Added string expansion and UIData creation.

BUG=162040, 147624
TBR=jochen@chromium.org (for gypi changes)


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

[pneubeck (no reviews), 2012-12-21 15:00:48]

This adds all missing parts necessary to get from ONC policies to Shill
dictionaries.

I had to separate the functionality and put parts in to chrome/ (dependency on
Proxy classes and UserManager).

Certificate matching is not needed in this translation. Considering our recent
discussions with Ryan Sleevi, it should make more sense to put certificate
matching and certificate observation in to a separate asynchronous service.

[pneubeck (no reviews), 2012-12-21 15:03:35]

Ah, nearly forgot:
This is more like a preview. I will do another cleanup pass over this code and
add missing comments.

Highlevel comments are welcome!

[Greg Spencer (Chromium), 2012-12-21 18:50:54]

LGTM with the changes below.

https://codereview.chromium.org/11664005/diff/42/chromeos/network/onc/onc_cer...
File chromeos/network/onc/onc_certificate_importer.cc (right):

https://codereview.chromium.org/11664005/diff/42/chromeos/network/onc/onc_cer...
chromeos/network/onc/onc_certificate_importer.cc:172: // TODO(jamescook):
implement me.
Should probably have "NOTIMPLEMENTED()"

https://codereview.chromium.org/11664005/diff/42/chromeos/network/onc/onc_cer...
chromeos/network/onc/onc_certificate_importer.cc:188: return std::string();
Might want to log an error here if we find more than one cert with that GUID.

[stevenjb, 2012-12-26 21:37:00]

lgtm w/ nits

https://codereview.chromium.org/11664005/diff/42/chrome/browser/chromeos/net/...
File chrome/browser/chromeos/net/onc_utils.cc (right):

https://codereview.chromium.org/11664005/diff/42/chrome/browser/chromeos/net/...
chrome/browser/chromeos/net/onc_utils.cc:29:
onc_proxy_location.GetInteger(onc::proxy::kPort, &port);
If onc_proxy_location.GetInteger() fails is using a port of 0 OK/expected, or do
we need to handle that differently?

https://codereview.chromium.org/11664005/diff/42/chrome/browser/chromeos/net/...
chrome/browser/chromeos/net/onc_utils.cc:140: NOTREACHED();
This should be LOG(ERROR) if the input might be data driven; NOTREACHED will
crash on Debug builds.

https://codereview.chromium.org/11664005/diff/42/chrome/browser/chromeos/net/...
chrome/browser/chromeos/net/onc_utils.cc:150: DCHECK(success);
Also LOG(ERROR) if data driven.

https://codereview.chromium.org/11664005/diff/42/chromeos/network/onc/onc_cer...
File chromeos/network/onc/onc_certificate_importer.cc (right):

https://codereview.chromium.org/11664005/diff/42/chromeos/network/onc/onc_cer...
chromeos/network/onc/onc_certificate_importer.cc:172: // TODO(jamescook):
implement me.
On 2012/12/21 18:50:55, Greg Spencer (Chromium) wrote:
> Should probably have "NOTIMPLEMENTED()"
+1

[pneubeck (no reviews), 2013-01-08 13:39:43]

Addressed the comments.

@Steven, I'm unsure about the LOG(ERROR) vs. NOTREACHED() again. See answers
below.

https://codereview.chromium.org/11664005/diff/42/chrome/browser/chromeos/net/...
File chrome/browser/chromeos/net/onc_utils.cc (right):

https://codereview.chromium.org/11664005/diff/42/chrome/browser/chromeos/net/...
chrome/browser/chromeos/net/onc_utils.cc:29:
onc_proxy_location.GetInteger(onc::proxy::kPort, &port);
On 2012/12/26 21:37:00, stevenjb (chromium) wrote:
> If onc_proxy_location.GetInteger() fails is using a port of 0 OK/expected, or
do
> we need to handle that differently?

The port field is required by the ONC spec and the validator ensures that the
field exists.

https://codereview.chromium.org/11664005/diff/42/chrome/browser/chromeos/net/...
chrome/browser/chromeos/net/onc_utils.cc:140: NOTREACHED();
On 2012/12/26 21:37:00, stevenjb (chromium) wrote:
> This should be LOG(ERROR) if the input might be data driven; NOTREACHED will
> crash on Debug builds.
> 

The else part should logically never occur as the validation ensures that the
field has a valid value.

I'd liked to remove the "type = ...NONE;" assignment but release compilation
requires that we initialize type in every case.

https://codereview.chromium.org/11664005/diff/42/chrome/browser/chromeos/net/...
chrome/browser/chromeos/net/onc_utils.cc:150: DCHECK(success);
On 2012/12/26 21:37:00, stevenjb (chromium) wrote:
> Also LOG(ERROR) if data driven.

Same as above.

You mentioned here https://codereview.chromium.org/11614035/#msg8 that we should
use LOG(ERROR) for easily recoverable errors.

However, in the cases here, our internal representation (valid ONC) would
already be broken and I don't see how we can recover anymore.

https://codereview.chromium.org/11664005/diff/42/chromeos/network/onc/onc_cer...
File chromeos/network/onc/onc_certificate_importer.cc (right):

https://codereview.chromium.org/11664005/diff/42/chromeos/network/onc/onc_cer...
chromeos/network/onc/onc_certificate_importer.cc:172: // TODO(jamescook):
implement me.
On 2012/12/26 21:37:00, stevenjb (chromium) wrote:
> On 2012/12/21 18:50:55, Greg Spencer (Chromium) wrote:
> > Should probably have "NOTIMPLEMENTED()"
> +1

Done.

https://codereview.chromium.org/11664005/diff/42/chromeos/network/onc/onc_cer...
chromeos/network/onc/onc_certificate_importer.cc:172: // TODO(jamescook):
implement me.
On 2012/12/21 18:50:55, Greg Spencer (Chromium) wrote:
> Should probably have "NOTIMPLEMENTED()"

Done.

https://codereview.chromium.org/11664005/diff/42/chromeos/network/onc/onc_cer...
chromeos/network/onc/onc_certificate_importer.cc:188: return std::string();
On 2012/12/21 18:50:55, Greg Spencer (Chromium) wrote:
> Might want to log an error here if we find more than one cert with that GUID.

Done.

[pneubeck (no reviews), 2013-01-08 13:53:11]

@Steven, did you see the change to chromeos/chromeos.gyp?
I changed that so that the tests in
chrome/browser/chromeos/net/onc_utils_unittest.cc
can access the test files in chromeos/ using  test_utils::ReadTestDictionary.

On 2012/12/26 21:37:00, stevenjb (chromium) wrote:
> lgtm w/ nits
> 
>
https://codereview.chromium.org/11664005/diff/42/chrome/browser/chromeos/net/...
> File chrome/browser/chromeos/net/onc_utils.cc (right):
> 
>
https://codereview.chromium.org/11664005/diff/42/chrome/browser/chromeos/net/...
> chrome/browser/chromeos/net/onc_utils.cc:29:
> onc_proxy_location.GetInteger(onc::proxy::kPort, &port);
> If onc_proxy_location.GetInteger() fails is using a port of 0 OK/expected, or
do
> we need to handle that differently?
> 
>
https://codereview.chromium.org/11664005/diff/42/chrome/browser/chromeos/net/...
> chrome/browser/chromeos/net/onc_utils.cc:140: NOTREACHED();
> This should be LOG(ERROR) if the input might be data driven; NOTREACHED will
> crash on Debug builds.
> 
>
https://codereview.chromium.org/11664005/diff/42/chrome/browser/chromeos/net/...
> chrome/browser/chromeos/net/onc_utils.cc:150: DCHECK(success);
> Also LOG(ERROR) if data driven.
> 
>
https://codereview.chromium.org/11664005/diff/42/chromeos/network/onc/onc_cer...
> File chromeos/network/onc/onc_certificate_importer.cc (right):
> 
>
https://codereview.chromium.org/11664005/diff/42/chromeos/network/onc/onc_cer...
> chromeos/network/onc/onc_certificate_importer.cc:172: // TODO(jamescook):
> implement me.
> On 2012/12/21 18:50:55, Greg Spencer (Chromium) wrote:
> > Should probably have "NOTIMPLEMENTED()"
> +1

[stevenjb, 2013-01-08 18:38:22]

https://codereview.chromium.org/11664005/diff/42/chrome/browser/chromeos/net/...
File chrome/browser/chromeos/net/onc_utils.cc (right):

https://codereview.chromium.org/11664005/diff/42/chrome/browser/chromeos/net/...
chrome/browser/chromeos/net/onc_utils.cc:140: NOTREACHED();
On 2013/01/08 13:39:44, pneubeck wrote:
> On 2012/12/26 21:37:00, stevenjb (chromium) wrote:
> > This should be LOG(ERROR) if the input might be data driven; NOTREACHED will
> > crash on Debug builds.
> > 
> 
> The else part should logically never occur as the validation ensures that the
> field has a valid value.
> 
> I'd liked to remove the "type = ...NONE;" assignment but release compilation
> requires that we initialize type in every case.

If it logically shouldn't happen, then NOTREACHED is correct (I should have
phrased my comment better)

[pneubeck (no reviews), 2013-01-10 21:38:20]

@Craig, could you please take a look at the pyauto changes
(chrome/test/functional and chrome/test/pyautolib).
These changes together with the moving of the files shouldn't change the
behavior of the pyauto tests.

@Steven, @Greg, sorry that the CL got that long, but I added/adapted a lot of
tests. You may take a final look at network_library_impl_base.cc, the rest
should be uncritical.

[commit-bot: I haz the power, 2013-01-11 08:44:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pneubeck@chromium.org/11664005/37002

[commit-bot: I haz the power, 2013-01-11 08:45:08]

Presubmit check for 11664005-37002 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome

Presubmit checks took 2.5s to calculate.

Was the presubmit check useful? Please send feedback & hate mail to
maruel@chromium.org!

[commit-bot: I haz the power, 2013-01-11 08:47:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pneubeck@chromium.org/11664005/37002

[commit-bot: I haz the power, 2013-01-11 11:21:25]

Change committed as 176312