[Extensions] Add a SimpleFeature::Validate function

[Extensions] Add a SimpleFeature::Validate function

Currently, most feature validation happens in Parse(). Add a Validate()
function that can be called after Parse() so that if a feature is
generated, validation can still be performed (independent of whether or
not the feature is created from a JSON value).  In the future, these
validations should be performed as part of the compile process, but this
is a starting point.

Also update tests that parsed features and used JSONFeatureProvider only
to test this validation. This way, once we move away from
JSON-based features and JSONFeatureProvider, these tests are still
active.

BUG=280286
Committed: https://crrev.com/23f765ccc6be80523d9d09358e249fc749cf5331
Cr-Commit-Position: refs/heads/master@{#406076}

[Devlin, 2016-07-15 01:15:04]

The CQ bit was checked by rdevlin.cronin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-15 01:15:46]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-15 02:38:49]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-15 02:38:50]

Dry run: This issue passed the CQ dry run.

[Devlin, 2016-07-15 22:11:09]

Description was changed from

==========
[Extensions] Add a SimpleFeature::Validate function

Currently, most feature validation happens in Parse(). Add a Validate() function
that can be called after Parse() so that if a feature is generated, validation
can still be performed (independent of whether or not the feature is created
from a JSON value).  In the future, these validations should be performed as
part of the compile process, but this is a starting point.

BUG=280286
==========

to

==========
[Extensions] Add a SimpleFeature::Validate function

Currently, most feature validation happens in Parse(). Add a Validate() 
function that can be called after Parse() so that if a feature is
generated, validation can still be performed (independent of whether or
not the feature is created from a JSON value).  In the future, these
validations should be performed as part of the compile process, but this
is a starting point.

Also update tests that parsed features and used JSONFeatureProvider only
to test this validation. This way, once we move away from
JSON-based features and JSONFeatureProvider, these tests are still
active.

BUG=280286
==========

[Devlin, 2016-07-15 22:11:28]

rdevlin.cronin@chromium.org changed reviewers:
+ lazyboy@chromium.org

[Devlin, 2016-07-15 22:11:28]

Istiaque, mind taking a look?

[lazyboy, 2016-07-15 22:53:40]

https://codereview.chromium.org/2150193002/diff/20001/extensions/common/featu...
File extensions/common/features/simple_feature.cc (right):

https://codereview.chromium.org/2150193002/diff/20001/extensions/common/featu...
extensions/common/features/simple_feature.cc:314: internal_(false) {}
This is exposing "internal" field to all features from APIFeatures. Not sure if
that's a good idea, couldn't we make Parse virtual and override it in
APIFeatures? That way only APIFeatures::Parse would look into "internal" field.

On the flip side, SimpleFeature::IsInternal exists, so maybe a non-issue.

https://codereview.chromium.org/2150193002/diff/20001/extensions/common/featu...
File extensions/common/features/simple_feature.h (right):

https://codereview.chromium.org/2150193002/diff/20001/extensions/common/featu...
extensions/common/features/simple_feature.h:51: // us to implement inheritance
by parsing one value after another. Returns
This comment needs to be updated.

Also add a comment for Validate below. Note how Parse doesn't call validate
anymore?

https://codereview.chromium.org/2150193002/diff/20001/extensions/common/featu...
File extensions/common/features/simple_feature_unittest.cc (right):

https://codereview.chromium.org/2150193002/diff/20001/extensions/common/featu...
extensions/common/features/simple_feature_unittest.cc:941:
std::unique_ptr<SimpleFeature> feature1(CreateFeature<SimpleFeature>());
Retain the comment: "Rule: "extension", channel trunk." I think these make stuff
easier to read/follow.

https://codereview.chromium.org/2150193002/diff/20001/extensions/common/featu...
extensions/common/features/simple_feature_unittest.cc:946:
feature2->channel_.reset(
Same for this one.

[Devlin, 2016-07-18 17:51:58]

https://codereview.chromium.org/2150193002/diff/20001/extensions/common/featu...
File extensions/common/features/simple_feature.cc (right):

https://codereview.chromium.org/2150193002/diff/20001/extensions/common/featu...
extensions/common/features/simple_feature.cc:314: internal_(false) {}
On 2016/07/15 22:53:40, lazyboy wrote:
> This is exposing "internal" field to all features from APIFeatures. Not sure
if
> that's a good idea, couldn't we make Parse virtual and override it in
> APIFeatures? That way only APIFeatures::Parse would look into "internal"
field.
> 
> On the flip side, SimpleFeature::IsInternal exists, so maybe a non-issue.

It is, but there's nothing to say that only api features can be internal (other
features could easily be, too).  And since we already have the IsInternal()
function exposed, I don't see any harm.  We could restrict it to only api
features, but it'll be a little messy right now (once we have these compiled,
it'd probably be a bit cleaner).

https://codereview.chromium.org/2150193002/diff/20001/extensions/common/featu...
File extensions/common/features/simple_feature.h (right):

https://codereview.chromium.org/2150193002/diff/20001/extensions/common/featu...
extensions/common/features/simple_feature.h:51: // us to implement inheritance
by parsing one value after another. Returns
On 2016/07/15 22:53:40, lazyboy wrote:
> This comment needs to be updated.
> 
> Also add a comment for Validate below. Note how Parse doesn't call validate
> anymore?

Done.

https://codereview.chromium.org/2150193002/diff/20001/extensions/common/featu...
File extensions/common/features/simple_feature_unittest.cc (right):

https://codereview.chromium.org/2150193002/diff/20001/extensions/common/featu...
extensions/common/features/simple_feature_unittest.cc:941:
std::unique_ptr<SimpleFeature> feature1(CreateFeature<SimpleFeature>());
On 2016/07/15 22:53:40, lazyboy wrote:
> Retain the comment: "Rule: "extension", channel trunk." I think these make
stuff
> easier to read/follow.

Done.

https://codereview.chromium.org/2150193002/diff/20001/extensions/common/featu...
extensions/common/features/simple_feature_unittest.cc:946:
feature2->channel_.reset(
On 2016/07/15 22:53:40, lazyboy wrote:
> Same for this one.

Done.

[lazyboy, 2016-07-18 17:57:43]

lgtm

https://codereview.chromium.org/2150193002/diff/20001/extensions/common/featu...
File extensions/common/features/simple_feature.cc (right):

https://codereview.chromium.org/2150193002/diff/20001/extensions/common/featu...
extensions/common/features/simple_feature.cc:314: internal_(false) {}
On 2016/07/18 17:51:58, Devlin wrote:
> On 2016/07/15 22:53:40, lazyboy wrote:
> > This is exposing "internal" field to all features from APIFeatures. Not sure
> if
> > that's a good idea, couldn't we make Parse virtual and override it in
> > APIFeatures? That way only APIFeatures::Parse would look into "internal"
> field.
> > 
> > On the flip side, SimpleFeature::IsInternal exists, so maybe a non-issue.
> 
> It is, but there's nothing to say that only api features can be internal
(other
> features could easily be, too).  And since we already have the IsInternal()
> function exposed, I don't see any harm.  We could restrict it to only api
> features, but it'll be a little messy right now (once we have these compiled,
> it'd probably be a bit cleaner).

Acknowledged.

[Devlin, 2016-07-18 19:19:59]

The CQ bit was checked by rdevlin.cronin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-18 19:20:58]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Devlin, 2016-07-18 20:03:15]

The CQ bit was unchecked by rdevlin.cronin@chromium.org

[Devlin, 2016-07-18 20:03:16]

The CQ bit was checked by rdevlin.cronin@chromium.org

[commit-bot: I haz the power, 2016-07-18 20:03:39]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-18 20:08:57]

Description was changed from

==========
[Extensions] Add a SimpleFeature::Validate function

Currently, most feature validation happens in Parse(). Add a Validate() 
function that can be called after Parse() so that if a feature is
generated, validation can still be performed (independent of whether or
not the feature is created from a JSON value).  In the future, these
validations should be performed as part of the compile process, but this
is a starting point.

Also update tests that parsed features and used JSONFeatureProvider only
to test this validation. This way, once we move away from
JSON-based features and JSONFeatureProvider, these tests are still
active.

BUG=280286
==========

to

==========
[Extensions] Add a SimpleFeature::Validate function

Currently, most feature validation happens in Parse(). Add a Validate() 
function that can be called after Parse() so that if a feature is
generated, validation can still be performed (independent of whether or
not the feature is created from a JSON value).  In the future, these
validations should be performed as part of the compile process, but this
is a starting point.

Also update tests that parsed features and used JSONFeatureProvider only
to test this validation. This way, once we move away from
JSON-based features and JSONFeatureProvider, these tests are still
active.

BUG=280286
==========

[commit-bot: I haz the power, 2016-07-18 20:08:58]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-07-18 20:09:09]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-07-18 20:13:01]

Description was changed from

==========
[Extensions] Add a SimpleFeature::Validate function

Currently, most feature validation happens in Parse(). Add a Validate() 
function that can be called after Parse() so that if a feature is
generated, validation can still be performed (independent of whether or
not the feature is created from a JSON value).  In the future, these
validations should be performed as part of the compile process, but this
is a starting point.

Also update tests that parsed features and used JSONFeatureProvider only
to test this validation. This way, once we move away from
JSON-based features and JSONFeatureProvider, these tests are still
active.

BUG=280286
==========

to

==========
[Extensions] Add a SimpleFeature::Validate function

Currently, most feature validation happens in Parse(). Add a Validate()
function that can be called after Parse() so that if a feature is
generated, validation can still be performed (independent of whether or
not the feature is created from a JSON value).  In the future, these
validations should be performed as part of the compile process, but this
is a starting point.

Also update tests that parsed features and used JSONFeatureProvider only
to test this validation. This way, once we move away from
JSON-based features and JSONFeatureProvider, these tests are still
active.

BUG=280286

Committed: https://crrev.com/23f765ccc6be80523d9d09358e249fc749cf5331
Cr-Commit-Position: refs/heads/master@{#406076}
==========

[commit-bot: I haz the power, 2016-07-18 20:13:03]

Patchset 3 (id:??) landed as
https://crrev.com/23f765ccc6be80523d9d09358e249fc749cf5331
Cr-Commit-Position: refs/heads/master@{#406076}