Adding a separate ONC validation binary.

Adding a separate ONC validation binary.

BUG=chromium-os:37238
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=179142

[pneubeck (no reviews), 2013-01-15 20:17:16]

Hi David,
this is a first working version of the separate binary mapping directly on the
OncValidator.
Open for discussion.

[pneubeck (no reviews), 2013-01-15 20:34:20]

@Steven, does this look like the right place for the code? Are the gyp changes
ok?

@Greg FYI

[pneubeck (no reviews), 2013-01-23 10:05:46]

On 2013/01/15 20:34:20, pneubeck wrote:
> @Steven, does this look like the right place for the code? Are the gyp changes
> ok?
> 
> @Greg FYI

ping.

Btw. the binary is already used by David and it works fine.

[stevenjb, 2013-01-23 17:22:00]

This lgtm. I'm not a gyp expert, but I don't see any problems. 

+oshima@ to look at the gyp files (or recommend someone)

https://codereview.chromium.org/11885046/diff/6001/chromeos/tools/onc_validat...
File chromeos/tools/onc_validator/onc_validator.cc (right):

https://codereview.chromium.org/11885046/diff/6001/chromeos/tools/onc_validat...
chromeos/tools/onc_validator/onc_validator.cc:26: const char* kSwitches[] = {
kSwitchErrorOnUnknownField,
nit: first switch on newline is more common and matches other formatting (e.g.
enum)

https://codereview.chromium.org/11885046/diff/6001/chromeos/tools/onc_validat...
chromeos/tools/onc_validator/onc_validator.cc:32: kSwitchUserImport };
} on separate line

https://codereview.chromium.org/11885046/diff/6001/chromeos/tools/onc_validat...
chromeos/tools/onc_validator/onc_validator.cc:48: kCertificate };
Same nits here

https://codereview.chromium.org/11885046/diff/6001/chromeos/tools/onc_validat...
chromeos/tools/onc_validator/onc_validator.cc:67: "Exist status is one of:\n"
Exit?

[oshima, 2013-01-23 19:24:37]

https://codereview.chromium.org/11885046/diff/6001/chromeos/chromeos_tools.gypi
File chromeos/chromeos_tools.gypi (right):

https://codereview.chromium.org/11885046/diff/6001/chromeos/chromeos_tools.gy...
chromeos/chromeos_tools.gypi:1: # Copyright (c) 2009 The Chromium Authors. All
rights reserved.
2013

https://codereview.chromium.org/11885046/diff/6001/chromeos/chromeos_tools.gy...
chromeos/chromeos_tools.gypi:8: },
do you need this?

[pneubeck (no reviews), 2013-01-23 21:24:47]

https://codereview.chromium.org/11885046/diff/6001/chromeos/chromeos_tools.gypi
File chromeos/chromeos_tools.gypi (right):

https://codereview.chromium.org/11885046/diff/6001/chromeos/chromeos_tools.gy...
chromeos/chromeos_tools.gypi:8: },
On 2013/01/23 19:24:38, oshima wrote:
> do you need this?

At first I copied this from another gypi and then checked the documentation in
build/common.gypi and guessed that it should be right to set it. 

Looking at it again, shouldn't the variable be set per target?
How can I correctly decide if that flag is needed?

[oshima, 2013-01-23 23:16:17]

https://codereview.chromium.org/11885046/diff/6001/chromeos/chromeos_tools.gypi
File chromeos/chromeos_tools.gypi (right):

https://codereview.chromium.org/11885046/diff/6001/chromeos/chromeos_tools.gy...
chromeos/chromeos_tools.gypi:8: },
On 2013/01/23 21:24:47, pneubeck wrote:
> On 2013/01/23 19:24:38, oshima wrote:
> > do you need this?
> 
> At first I copied this from another gypi and then checked the documentation in
> build/common.gypi and guessed that it should be right to set it. 
> 
> Looking at it again, shouldn't the variable be set per target?
> How can I correctly decide if that flag is needed?

This is already set in chromeos.gyp, you probably don't need this, unless you
have specific reason to do so.

[pneubeck (no reviews), 2013-01-24 13:35:02]

Addressed Steven's and Oshima's comments.

https://codereview.chromium.org/11885046/diff/6001/chromeos/chromeos_tools.gypi
File chromeos/chromeos_tools.gypi (right):

https://codereview.chromium.org/11885046/diff/6001/chromeos/chromeos_tools.gy...
chromeos/chromeos_tools.gypi:1: # Copyright (c) 2009 The Chromium Authors. All
rights reserved.
On 2013/01/23 19:24:38, oshima wrote:
> 2013

Done.

https://codereview.chromium.org/11885046/diff/6001/chromeos/chromeos_tools.gy...
chromeos/chromeos_tools.gypi:8: },
On 2013/01/23 23:16:17, oshima wrote:
> On 2013/01/23 21:24:47, pneubeck wrote:
> > On 2013/01/23 19:24:38, oshima wrote:
> > > do you need this?
> > 
> > At first I copied this from another gypi and then checked the documentation
in
> > build/common.gypi and guessed that it should be right to set it. 
> > 
> > Looking at it again, shouldn't the variable be set per target?
> > How can I correctly decide if that flag is needed?
> 
> This is already set in chromeos.gyp, you probably don't need this, unless you
> have specific reason to do so.

Done.

https://codereview.chromium.org/11885046/diff/6001/chromeos/tools/onc_validat...
File chromeos/tools/onc_validator/onc_validator.cc (right):

https://codereview.chromium.org/11885046/diff/6001/chromeos/tools/onc_validat...
chromeos/tools/onc_validator/onc_validator.cc:26: const char* kSwitches[] = {
kSwitchErrorOnUnknownField,
On 2013/01/23 17:22:00, stevenjb (chromium) wrote:
> nit: first switch on newline is more common and matches other formatting (e.g.
> enum)

Done.

https://codereview.chromium.org/11885046/diff/6001/chromeos/tools/onc_validat...
chromeos/tools/onc_validator/onc_validator.cc:32: kSwitchUserImport };
On 2013/01/23 17:22:00, stevenjb (chromium) wrote:
> } on separate line

Done.

https://codereview.chromium.org/11885046/diff/6001/chromeos/tools/onc_validat...
chromeos/tools/onc_validator/onc_validator.cc:48: kCertificate };
On 2013/01/23 17:22:00, stevenjb (chromium) wrote:
> Same nits here

Done.

https://codereview.chromium.org/11885046/diff/6001/chromeos/tools/onc_validat...
chromeos/tools/onc_validator/onc_validator.cc:67: "Exist status is one of:\n"
On 2013/01/23 17:22:00, stevenjb (chromium) wrote:
> Exit?

Done.

[oshima, 2013-01-25 23:12:56]

lgtm

[commit-bot: I haz the power, 2013-01-28 11:52:39]

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

[commit-bot: I haz the power, 2013-01-28 14:27:19]

Change committed as 179142