Support API aliases

Support API aliases

Introduce API feature properties to specify an API source and alias.

|source| is set on API alias, and is used during API registration
- if an API feature has source set, the bindings installed under API
name will be the bindings of |source| API.

|alias| is used on source API, and is used when checking API
availability - when checking availability of an API that has alias
set, the API will be deemed available if either one of the API and
the API alias are available in the context.
Note that this logic should not be used when determining API
availability for binding registration - in this case alias
availability should be ignored in order to avoid registering
source API bindings when only alias bindings are needed.

BUG=647415
Committed: https://crrev.com/feb4b052b4c86b9047f8c220e3ec3983e5483cdf
Cr-Commit-Position: refs/heads/master@{#435034}

[tbarzic, 2016-11-11 23:51:47]

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

[commit-bot: I haz the power, 2016-11-11 23:52:24]

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-11-11 23:55:27]

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

[commit-bot: I haz the power, 2016-11-11 23:55:28]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[tbarzic, 2016-11-12 00:02:55]

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

[commit-bot: I haz the power, 2016-11-12 00:03:42]

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

[tbarzic, 2016-11-12 00:06:05]

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

[commit-bot: I haz the power, 2016-11-12 00:48:12]

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

[commit-bot: I haz the power, 2016-11-12 00:48:13]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Devlin, 2016-11-14 18:03:56]

https://codereview.chromium.org/2494653005/diff/60001/chrome/common/extension...
File chrome/common/extensions/api/_features.md (right):

https://codereview.chromium.org/2494653005/diff/60001/chrome/common/extension...
chrome/common/extensions/api/_features.md:125: API features that have an alias.
The value should be the name of the alias API
We should describe what an alias is here. :)

https://codereview.chromium.org/2494653005/diff/60001/chrome/common/extension...
chrome/common/extensions/api/_features.md:278: ### source
I think having an example here would probably be useful.

https://codereview.chromium.org/2494653005/diff/60001/extensions/common/exten...
File extensions/common/extension_api.cc (right):

https://codereview.chromium.org/2494653005/diff/60001/extensions/common/exten...
extensions/common/extension_api.cc:202: if (!alias)
Can this happen?

https://codereview.chromium.org/2494653005/diff/60001/extensions/common/exten...
File extensions/common/extension_api.h (right):

https://codereview.chromium.org/2494653005/diff/60001/extensions/common/exten...
extensions/common/extension_api.h:99: bool allow_alias);
I think this would be more clear with an enum, ALLOW_ALIASES/DONT_ALLOW_ALIASES.
 Anonymous bools in permission checks are something I'd prefer to avoid.

https://codereview.chromium.org/2494653005/diff/60001/extensions/common/exten...
extensions/common/extension_api.h:111: bool allow_alias);
ditto

https://codereview.chromium.org/2494653005/diff/60001/extensions/common/featu...
File extensions/common/features/feature.h (right):

https://codereview.chromium.org/2494653005/diff/60001/extensions/common/featu...
extensions/common/features/feature.h:149: virtual std::string GetSource() const
= 0;
We document that complex features only support a single alias/source - with
that, we should be able to put alias and source on Feature itself and avoid
having it be virtual.

https://codereview.chromium.org/2494653005/diff/60001/tools/json_schema_compi...
File tools/json_schema_compiler/feature_compiler.py (right):

https://codereview.chromium.org/2494653005/diff/60001/tools/json_schema_compi...
tools/json_schema_compiler/feature_compiler.py:105: 'alias': {
We need to test these.  We also need to throw errors if:
- The alias doesn't exist
- The source doesn't exist
- There are more than one alias/sources specified on complex features

[tbarzic, 2016-11-15 03:56:15]

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

[commit-bot: I haz the power, 2016-11-15 03:56:54]

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

[tbarzic, 2016-11-15 04:44:48]

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

[tbarzic, 2016-11-15 04:45:07]

https://codereview.chromium.org/2494653005/diff/60001/chrome/common/extension...
File chrome/common/extensions/api/_features.md (right):

https://codereview.chromium.org/2494653005/diff/60001/chrome/common/extension...
chrome/common/extensions/api/_features.md:125: API features that have an alias.
The value should be the name of the alias API
On 2016/11/14 18:03:56, Devlin wrote:
> We should describe what an alias is here. :)

Done.

https://codereview.chromium.org/2494653005/diff/60001/chrome/common/extension...
chrome/common/extensions/api/_features.md:278: ### source
On 2016/11/14 18:03:56, Devlin wrote:
> I think having an example here would probably be useful.

Done.

https://codereview.chromium.org/2494653005/diff/60001/extensions/common/exten...
File extensions/common/extension_api.cc (right):

https://codereview.chromium.org/2494653005/diff/60001/extensions/common/exten...
extensions/common/extension_api.cc:202: if (!alias)
On 2016/11/14 18:03:56, Devlin wrote:
> Can this happen?

It should not anymore

https://codereview.chromium.org/2494653005/diff/60001/extensions/common/exten...
File extensions/common/extension_api.h (right):

https://codereview.chromium.org/2494653005/diff/60001/extensions/common/exten...
extensions/common/extension_api.h:99: bool allow_alias);
On 2016/11/14 18:03:56, Devlin wrote:
> I think this would be more clear with an enum,
ALLOW_ALIASES/DONT_ALLOW_ALIASES.
>  Anonymous bools in permission checks are something I'd prefer to avoid.

Done.

https://codereview.chromium.org/2494653005/diff/60001/extensions/common/exten...
extensions/common/extension_api.h:111: bool allow_alias);
On 2016/11/14 18:03:56, Devlin wrote:
> ditto

Done.

https://codereview.chromium.org/2494653005/diff/60001/extensions/common/featu...
File extensions/common/features/feature.h (right):

https://codereview.chromium.org/2494653005/diff/60001/extensions/common/featu...
extensions/common/features/feature.h:149: virtual std::string GetSource() const
= 0;
On 2016/11/14 18:03:56, Devlin wrote:
> We document that complex features only support a single alias/source - with
> that, we should be able to put alias and source on Feature itself and avoid
> having it be virtual.

Done.

https://codereview.chromium.org/2494653005/diff/60001/tools/json_schema_compi...
File tools/json_schema_compiler/feature_compiler.py (right):

https://codereview.chromium.org/2494653005/diff/60001/tools/json_schema_compi...
tools/json_schema_compiler/feature_compiler.py:105: 'alias': {
On 2016/11/14 18:03:56, Devlin wrote:
> We need to test these.  We also need to throw errors if:
> - The alias doesn't exist
> - The source doesn't exist
> - There are more than one alias/sources specified on complex features

Done.

[commit-bot: I haz the power, 2016-11-15 04:45:12]

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-11-15 06:09:03]

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

[commit-bot: I haz the power, 2016-11-15 06:09:03]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Devlin, 2016-11-17 01:11:16]

This mostly looks good, but it looks like a lot of complexity in the feature
compiler that I think we might be able to avoid.  I'll look into it a bit
tomorrow and see if we can find something a little simpler.

https://codereview.chromium.org/2494653005/diff/160001/chrome/common/extensio...
File chrome/common/extensions/api/_features.md (right):

https://codereview.chromium.org/2494653005/diff/160001/chrome/common/extensio...
chrome/common/extensions/api/_features.md:125: Alias feature is a feature that
provides the same functionality as it's source
An alias feature
or
Alias features are

https://codereview.chromium.org/2494653005/diff/160001/chrome/common/extensio...
chrome/common/extensions/api/_features.md:127: provides bindings for source API
under a different name. If one wanted to
for _the_ source API

https://codereview.chromium.org/2494653005/diff/160001/chrome/common/extensio...
chrome/common/extensions/api/_features.md:156: The alias API will inherit schema
from the source API, but it will not respect
inherit _the_ schema

https://codereview.chromium.org/2494653005/diff/160001/chrome/common/extensio...
chrome/common/extensions/api/_features.md:158: children, identical child
features should be added for the alias API.
If this becomes common, we should address it.  Hopefully, it won't be (do we
know how many private child features there are?).

https://codereview.chromium.org/2494653005/diff/160001/chrome/common/extensio...
File chrome/common/extensions/api/common_extension_api_unittest.cc (right):

https://codereview.chromium.org/2494653005/diff/160001/chrome/common/extensio...
chrome/common/extensions/api/common_extension_api_unittest.cc:228:
ASSERT_FALSE(api.IsAvailable("alias_api_source", NULL,
nullptr in new code

https://codereview.chromium.org/2494653005/diff/160001/extensions/common/exte...
File extensions/common/extension_api.cc (right):

https://codereview.chromium.org/2494653005/diff/160001/extensions/common/exte...
extensions/common/extension_api.cc:332: Feature* api,
s/api/feature

https://codereview.chromium.org/2494653005/diff/160001/extensions/common/exte...
extensions/common/extension_api.cc:343: // Check if there is a child feature
associated with full name for alias API.
Can you give an example of when this would be the case (and add it to the
comment here)?

https://codereview.chromium.org/2494653005/diff/160001/extensions/renderer/sc...
File extensions/renderer/script_context.h (right):

https://codereview.chromium.org/2494653005/diff/160001/extensions/renderer/sc...
extensions/renderer/script_context.h:131: // |alias_status| Whether API that has
an alias that is available should be
I think |alias_status| is self-explanatory without this comment.

https://codereview.chromium.org/2494653005/diff/160001/extensions/renderer/sc...
extensions/renderer/script_context.h:134: CheckAliasStatus alias_status);
maybe s/alias_status/check_alias?

[tbarzic, 2016-11-17 18:12:13]

https://codereview.chromium.org/2494653005/diff/160001/chrome/common/extensio...
File chrome/common/extensions/api/_features.md (right):

https://codereview.chromium.org/2494653005/diff/160001/chrome/common/extensio...
chrome/common/extensions/api/_features.md:125: Alias feature is a feature that
provides the same functionality as it's source
On 2016/11/17 at 01:11:15, Devlin wrote:
> An alias feature
> or
> Alias features are

Done.

https://codereview.chromium.org/2494653005/diff/160001/chrome/common/extensio...
chrome/common/extensions/api/_features.md:127: provides bindings for source API
under a different name. If one wanted to
On 2016/11/17 at 01:11:15, Devlin wrote:
> for _the_ source API

Done.

https://codereview.chromium.org/2494653005/diff/160001/chrome/common/extensio...
chrome/common/extensions/api/_features.md:156: The alias API will inherit schema
from the source API, but it will not respect
On 2016/11/17 at 01:11:15, Devlin wrote:
> inherit _the_ schema

Done.

https://codereview.chromium.org/2494653005/diff/160001/chrome/common/extensio...
chrome/common/extensions/api/_features.md:158: children, identical child
features should be added for the alias API.
On 2016/11/17 at 01:11:15, Devlin wrote:
> If this becomes common, we should address it.  Hopefully, it won't be (do we
know how many private child features there are?).

I don't expect it to become common; but, I think it is worth a comment.
Though, addressing this for a general case might be tricky (if we want to do it
cleanly).
One problematic case I can think of is the child feature foo.x depending on
permission foo.write (or anything foo specific).

https://codereview.chromium.org/2494653005/diff/160001/chrome/common/extensio...
File chrome/common/extensions/api/common_extension_api_unittest.cc (right):

https://codereview.chromium.org/2494653005/diff/160001/chrome/common/extensio...
chrome/common/extensions/api/common_extension_api_unittest.cc:228:
ASSERT_FALSE(api.IsAvailable("alias_api_source", NULL,
On 2016/11/17 at 01:11:15, Devlin wrote:
> nullptr in new code

it's not really new, more of a product of git cl format :)

fixed, though

https://codereview.chromium.org/2494653005/diff/160001/extensions/common/exte...
File extensions/common/extension_api.cc (right):

https://codereview.chromium.org/2494653005/diff/160001/extensions/common/exte...
extensions/common/extension_api.cc:332: Feature* api,
On 2016/11/17 at 01:11:15, Devlin wrote:
> s/api/feature

Done.

https://codereview.chromium.org/2494653005/diff/160001/extensions/common/exte...
extensions/common/extension_api.cc:343: // Check if there is a child feature
associated with full name for alias API.
On 2016/11/17 at 01:11:15, Devlin wrote:
> Can you give an example of when this would be the case (and add it to the
comment here)?


Done.

https://codereview.chromium.org/2494653005/diff/160001/extensions/renderer/sc...
File extensions/renderer/script_context.h (right):

https://codereview.chromium.org/2494653005/diff/160001/extensions/renderer/sc...
extensions/renderer/script_context.h:131: // |alias_status| Whether API that has
an alias that is available should be
On 2016/11/17 at 01:11:15, Devlin wrote:
> I think |alias_status| is self-explanatory without this comment.

I'm not that certain it is. Without the context, it seems unclear what "checking
alias" means.

https://codereview.chromium.org/2494653005/diff/160001/extensions/renderer/sc...
extensions/renderer/script_context.h:134: CheckAliasStatus alias_status);
On 2016/11/17 at 01:11:15, Devlin wrote:
> maybe s/alias_status/check_alias?

Done.

[Devlin, 2016-11-21 16:55:53]

https://codereview.chromium.org/2494653005/diff/200001/tools/json_schema_comp...
File tools/json_schema_compiler/feature_compiler.py (right):

https://codereview.chromium.org/2494653005/diff/200001/tools/json_schema_comp...
tools/json_schema_compiler/feature_compiler.py:105: #   'feature_reference':
Only applicable for strings. An object that, if
This feature reference code looks like it adds a lot of complication.  I wonder
if a simpler (and potentially more flexible) way of doing it would be to have a
final validation that happens once we parse all our features and is called
similar to validation.  Then, we could do something like:


def IsFeatureCrossReference(first_key, second_key, feature, all_features):
  val = feature.GetValue(first_key)
  if not val:
    return True
  if not val in all_features:
    return False
  xref_feature = all_features[val]
  xref_val = xref_feature.GetValue(second_key)
  if not xref_val or xref_val is not feature.name:
    return False
  return True

FINAL_VALIDATION = ([
  'APIFeature': [
    (partial(IsFeatureCrossReference, 'alias', 'source'),
     '<error message>')
  ]
])

(continued below)

This would allow us to later add any other validations that require knowledge of
other features more easily.

https://codereview.chromium.org/2494653005/diff/200001/tools/json_schema_comp...
tools/json_schema_compiler/feature_compiler.py:343: feature_reference: Object
set if the value should represent a feature
(With the above, we can remove this and most "feature_reference" code so that we
don't need to have knowledge of it everywhere)

https://codereview.chromium.org/2494653005/diff/200001/tools/json_schema_comp...
tools/json_schema_compiler/feature_compiler.py:547: def __init__(self, name,
shared_values):
self.shared_values should start empty, so no need to pass in

https://codereview.chromium.org/2494653005/diff/200001/tools/json_schema_comp...
tools/json_schema_compiler/feature_compiler.py:669:
feature.SetSharedValues(shared_values)
Ideally, this should be handled as part of feature.SetParent

https://codereview.chromium.org/2494653005/diff/200001/tools/json_schema_comp...
tools/json_schema_compiler/feature_compiler.py:700: # Validate values that
reference feature
Then here

def _FinalValidation(self):
  for name, feature in self._features.items():
     for validator, error in (FINAL_VALIDATION[self._feature_class] +
FINAL_VALIDATION['all']):
       if not validator(self._features, feature):
         self._AddError(error)

def Compile(self):
  ....
  self._FinalValidation()

https://codereview.chromium.org/2494653005/diff/200001/tools/json_schema_comp...
File tools/json_schema_compiler/feature_compiler_test.py (right):

https://codereview.chromium.org/2494653005/diff/200001/tools/json_schema_comp...
tools/json_schema_compiler/feature_compiler_test.py:185:
compiler._CompileFeature('feature_alpha', [{
I think instead of this approach, it would be a bit cleaner to move the Load()
call out of Compile(), and then just set _json here.  e.g.

compiler._json = ({
    'feature_alpha': [{
      ...
    }]})
compiler.Compile()

[tbarzic, 2016-11-21 21:53:18]

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

[commit-bot: I haz the power, 2016-11-21 21:54:13]

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-11-21 21:57:14]

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

[commit-bot: I haz the power, 2016-11-21 21:57:15]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[tbarzic, 2016-11-21 23:44:03]

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

[commit-bot: I haz the power, 2016-11-21 23:44:56]

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

[tbarzic, 2016-11-21 23:49:48]

https://codereview.chromium.org/2494653005/diff/200001/tools/json_schema_comp...
File tools/json_schema_compiler/feature_compiler.py (right):

https://codereview.chromium.org/2494653005/diff/200001/tools/json_schema_comp...
tools/json_schema_compiler/feature_compiler.py:105: #   'feature_reference':
Only applicable for strings. An object that, if
On 2016/11/21 at 16:55:53, Devlin wrote:
> This feature reference code looks like it adds a lot of complication.  I
wonder if a simpler (and potentially more flexible) way of doing it would be to
have a final validation that happens once we parse all our features and is
called similar to validation.  Then, we could do something like:
> 
> 
> def IsFeatureCrossReference(first_key, second_key, feature, all_features):
>   val = feature.GetValue(first_key)
>   if not val:
>     return True
>   if not val in all_features:
>     return False
>   xref_feature = all_features[val]
>   xref_val = xref_feature.GetValue(second_key)
>   if not xref_val or xref_val is not feature.name:
>     return False
>   return True
> 
> FINAL_VALIDATION = ([
>   'APIFeature': [
>     (partial(IsFeatureCrossReference, 'alias', 'source'),
>      '<error message>')
>   ]
> ])
> 
> (continued below)
> 
> This would allow us to later add any other validations that require knowledge
of other features more easily.

Done (though the error message returned using this approach is slightly less
precise :/).

https://codereview.chromium.org/2494653005/diff/200001/tools/json_schema_comp...
tools/json_schema_compiler/feature_compiler.py:343: feature_reference: Object
set if the value should represent a feature
On 2016/11/21 at 16:55:53, Devlin wrote:
> (With the above, we can remove this and most "feature_reference" code so that
we don't need to have knowledge of it everywhere)

Done.

https://codereview.chromium.org/2494653005/diff/200001/tools/json_schema_comp...
tools/json_schema_compiler/feature_compiler.py:547: def __init__(self, name,
shared_values):
On 2016/11/21 at 16:55:53, Devlin wrote:
> self.shared_values should start empty, so no need to pass in

Done.

https://codereview.chromium.org/2494653005/diff/200001/tools/json_schema_comp...
tools/json_schema_compiler/feature_compiler.py:669:
feature.SetSharedValues(shared_values)
On 2016/11/21 at 16:55:53, Devlin wrote:
> Ideally, this should be handled as part of feature.SetParent

parent might not be set

https://codereview.chromium.org/2494653005/diff/200001/tools/json_schema_comp...
tools/json_schema_compiler/feature_compiler.py:700: # Validate values that
reference feature
On 2016/11/21 at 16:55:53, Devlin wrote:
> Then here
> 
> def _FinalValidation(self):
>   for name, feature in self._features.items():
>      for validator, error in (FINAL_VALIDATION[self._feature_class] +
FINAL_VALIDATION['all']):
>        if not validator(self._features, feature):
>          self._AddError(error)
> 
> def Compile(self):
>   ....
>   self._FinalValidation()

Done.

https://codereview.chromium.org/2494653005/diff/200001/tools/json_schema_comp...
File tools/json_schema_compiler/feature_compiler_test.py (right):

https://codereview.chromium.org/2494653005/diff/200001/tools/json_schema_comp...
tools/json_schema_compiler/feature_compiler_test.py:185:
compiler._CompileFeature('feature_alpha', [{
On 2016/11/21 at 16:55:53, Devlin wrote:
> I think instead of this approach, it would be a bit cleaner to move the Load()
call out of Compile(), and then just set _json here.  e.g.
> 
> compiler._json = ({
>     'feature_alpha': [{
>       ...
>     }]})
> compiler.Compile()

Done.

[Devlin, 2016-11-23 15:56:14]

https://codereview.chromium.org/2494653005/diff/260001/tools/json_schema_comp...
File tools/json_schema_compiler/feature_compiler.py (right):

https://codereview.chromium.org/2494653005/diff/260001/tools/json_schema_comp...
tools/json_schema_compiler/feature_compiler.py:232: feature_values =
feature.GetAllFeatureValues()
This would be a lot more efficient as a GetValue() implementation to avoid
copying all the entries.

https://codereview.chromium.org/2494653005/diff/260001/tools/json_schema_comp...
tools/json_schema_compiler/feature_compiler.py:491: def
_GetCodeForFeatureValues(self, feature_values):
This should be either a static function or a function separate from the feature
class.

https://codereview.chromium.org/2494653005/diff/260001/tools/json_schema_comp...
tools/json_schema_compiler/feature_compiler.py:505:
c.Concat(self._GetCodeForFeatureValues(self.GetAllFeatureValues()))
As this is written now, won't this result in each simple feature of a complex
feature setting the shared properties?  (I think this will be fixed with the
suggestion for shared_values below)

https://codereview.chromium.org/2494653005/diff/260001/tools/json_schema_comp...
tools/json_schema_compiler/feature_compiler.py:561: if not errors:
When is this the case?

https://codereview.chromium.org/2494653005/diff/260001/tools/json_schema_comp...
tools/json_schema_compiler/feature_compiler.py:647:
feature.SetSharedValues(shared_values)
We shouldn't have to call this each time.  Instead, let's pass the shared_values
object that we use into Parse().  So for complex features, we'd pass the complex
feature's shared values in, and otherwise we'd pass the simple feature's shared
values in.

Does that make sense?

https://codereview.chromium.org/2494653005/diff/260001/tools/json_schema_comp...
File tools/json_schema_compiler/feature_compiler_test.py (right):

https://codereview.chromium.org/2494653005/diff/260001/tools/json_schema_comp...
tools/json_schema_compiler/feature_compiler_test.py:57: 'source': 'feature',
It doesn't make sense to have both an alias and a source, right?  I think we
should have a compile error for that.

https://codereview.chromium.org/2494653005/diff/260001/tools/json_schema_comp...
tools/json_schema_compiler/feature_compiler_test.py:186: 'contexts':
['blessed_extension'],
indentation

https://codereview.chromium.org/2494653005/diff/260001/tools/json_schema_comp...
tools/json_schema_compiler/feature_compiler_test.py:255: 'contexts':
['blessed_extension'],
indentation

https://codereview.chromium.org/2494653005/diff/260001/tools/json_schema_comp...
File tools/json_schema_compiler/test/features_test.json (right):

https://codereview.chromium.org/2494653005/diff/260001/tools/json_schema_comp...
tools/json_schema_compiler/test/features_test.json:112: "source":
"parent_source"
What would this mean, for a child feature to be a source for its parent?  I
wonder if that should be a compiler error?  At the very least, it shouldn't be a
part of a test.

[tbarzic, 2016-11-23 20:13:03]

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

[commit-bot: I haz the power, 2016-11-23 20:13:36]

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

[tbarzic, 2016-11-23 20:16:41]

https://codereview.chromium.org/2494653005/diff/260001/tools/json_schema_comp...
File tools/json_schema_compiler/feature_compiler.py (right):

https://codereview.chromium.org/2494653005/diff/260001/tools/json_schema_comp...
tools/json_schema_compiler/feature_compiler.py:232: feature_values =
feature.GetAllFeatureValues()
On 2016/11/23 at 15:56:13, Devlin wrote:
> This would be a lot more efficient as a GetValue() implementation to avoid
copying all the entries.

Done.

https://codereview.chromium.org/2494653005/diff/260001/tools/json_schema_comp...
tools/json_schema_compiler/feature_compiler.py:491: def
_GetCodeForFeatureValues(self, feature_values):
On 2016/11/23 at 15:56:13, Devlin wrote:
> This should be either a static function or a function separate from the
feature class.

Done.

https://codereview.chromium.org/2494653005/diff/260001/tools/json_schema_comp...
tools/json_schema_compiler/feature_compiler.py:561: if not errors:
On 2016/11/23 at 15:56:13, Devlin wrote:
> When is this the case?

when error is [].
I initially wrote this assuming errors is None by default, and forgot to remove
it once I realized that's not the case.

https://codereview.chromium.org/2494653005/diff/260001/tools/json_schema_comp...
tools/json_schema_compiler/feature_compiler.py:647:
feature.SetSharedValues(shared_values)
On 2016/11/23 at 15:56:13, Devlin wrote:
> We shouldn't have to call this each time.  Instead, let's pass the
shared_values object that we use into Parse().  So for complex features, we'd
pass the complex feature's shared values in, and otherwise we'd pass the simple
feature's shared values in.
> 
> Does that make sense?

Sure.

https://codereview.chromium.org/2494653005/diff/260001/tools/json_schema_comp...
File tools/json_schema_compiler/feature_compiler_test.py (right):

https://codereview.chromium.org/2494653005/diff/260001/tools/json_schema_comp...
tools/json_schema_compiler/feature_compiler_test.py:57: 'source': 'feature',
On 2016/11/23 at 15:56:14, Devlin wrote:
> It doesn't make sense to have both an alias and a source, right?  I think we
should have a compile error for that.

I wouldn't say it necessarily doesn't make sense, it's just not supported right
now (and I can't see us supporting transitive aliases in foreseeable future) -
added compile error.

https://codereview.chromium.org/2494653005/diff/260001/tools/json_schema_comp...
tools/json_schema_compiler/feature_compiler_test.py:186: 'contexts':
['blessed_extension'],
On 2016/11/23 at 15:56:13, Devlin wrote:
> indentation

Done

https://codereview.chromium.org/2494653005/diff/260001/tools/json_schema_comp...
tools/json_schema_compiler/feature_compiler_test.py:255: 'contexts':
['blessed_extension'],
On 2016/11/23 at 15:56:14, Devlin wrote:
> indentation

Done

https://codereview.chromium.org/2494653005/diff/260001/tools/json_schema_comp...
File tools/json_schema_compiler/test/features_test.json (right):

https://codereview.chromium.org/2494653005/diff/260001/tools/json_schema_comp...
tools/json_schema_compiler/test/features_test.json:112: "source":
"parent_source"
On 2016/11/23 at 15:56:14, Devlin wrote:
> What would this mean, for a child feature to be a source for its parent?  I
wonder if that should be a compiler error?  At the very least, it shouldn't be a
part of a test.

OK, I guess I was testing too much things using the same feature, updated the
tests to avoid this situation.

[commit-bot: I haz the power, 2016-11-23 22:34:05]

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

[commit-bot: I haz the power, 2016-11-23 22:34:06]

Dry run: This issue passed the CQ dry run.

[tbarzic, 2016-11-28 20:10:51]

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

[commit-bot: I haz the power, 2016-11-28 20:12:37]

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

[tbarzic, 2016-11-28 20:16:23]

ping?

(note that I updated the cl this morning - patchset 16 - in order to fix a bug
where child feature were not able to override shared values set by parent)

[Devlin, 2016-11-28 20:42:22]

Nice!  Just a few last small comments, but otherwise this looks good!

https://codereview.chromium.org/2494653005/diff/300001/tools/json_schema_comp...
File tools/json_schema_compiler/feature_compiler.py (right):

https://codereview.chromium.org/2494653005/diff/300001/tools/json_schema_comp...
tools/json_schema_compiler/feature_compiler.py:241: if not parsed_value:
Will this ever fail if we only call it for features that we expect to have a
mapping?  It seems like those should always be unicode.

https://codereview.chromium.org/2494653005/diff/300001/tools/json_schema_comp...
tools/json_schema_compiler/feature_compiler.py:500:
feature_values.update(shared_values)
Doesn't GetAllFeatureValues already add shared values?

https://codereview.chromium.org/2494653005/diff/300001/tools/json_schema_comp...
tools/json_schema_compiler/feature_compiler.py:540: self.shared_values = {}
shared_values should be init'd as part of the feature init, right?

https://codereview.chromium.org/2494653005/diff/300001/tools/json_schema_comp...
tools/json_schema_compiler/feature_compiler.py:546: for f in self.feature_list:
As a sanity check, could we here assert that f.shared_values is empty?

https://codereview.chromium.org/2494653005/diff/300001/tools/json_schema_comp...
tools/json_schema_compiler/feature_compiler.py:553:
c.Concat(GetCodeForFeatureValues(self.GetAllFeatureValues()))
I think it's more confusing to pass in GetCodeForFeatureValues here rather than
just self.shared_values

https://codereview.chromium.org/2494653005/diff/300001/tools/json_schema_comp...
File tools/json_schema_compiler/test/features_test.json (right):

https://codereview.chromium.org/2494653005/diff/300001/tools/json_schema_comp...
tools/json_schema_compiler/test/features_test.json:113:
"parent_source.child_source" : {
Wild.  I'm not entirely sure we want to use this, but it's cool that it works.
:)

[tbarzic, 2016-11-28 21:43:29]

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

[tbarzic, 2016-11-28 21:43:52]

https://codereview.chromium.org/2494653005/diff/300001/tools/json_schema_comp...
File tools/json_schema_compiler/feature_compiler.py (right):

https://codereview.chromium.org/2494653005/diff/300001/tools/json_schema_comp...
tools/json_schema_compiler/feature_compiler.py:241: if not parsed_value:
On 2016/11/28 20:42:21, Devlin wrote:
> Will this ever fail if we only call it for features that we expect to have a
> mapping?  It seems like those should always be unicode.

It shouldn't, but it might be misused - the question is how we want it to fail
(since returning false here will cause assertion).

Changed this to explicit assertion.

https://codereview.chromium.org/2494653005/diff/300001/tools/json_schema_comp...
tools/json_schema_compiler/feature_compiler.py:500:
feature_values.update(shared_values)
On 2016/11/28 20:42:21, Devlin wrote:
> Doesn't GetAllFeatureValues already add shared values?

yes, but at this point self.shared_values are not yet set (and if this is a
simple feature that's part of a complex feature, self.shared_values will always
be empty). 

I guess having feature_values = copy(self.feature_values) might make thing a bit
clearer.

https://codereview.chromium.org/2494653005/diff/300001/tools/json_schema_comp...
tools/json_schema_compiler/feature_compiler.py:540: self.shared_values = {}
On 2016/11/28 20:42:21, Devlin wrote:
> shared_values should be init'd as part of the feature init, right?

Yes, removed.

https://codereview.chromium.org/2494653005/diff/300001/tools/json_schema_comp...
tools/json_schema_compiler/feature_compiler.py:546: for f in self.feature_list:
On 2016/11/28 20:42:21, Devlin wrote:
> As a sanity check, could we here assert that f.shared_values is empty?

Done.

https://codereview.chromium.org/2494653005/diff/300001/tools/json_schema_comp...
tools/json_schema_compiler/feature_compiler.py:553:
c.Concat(GetCodeForFeatureValues(self.GetAllFeatureValues()))
On 2016/11/28 20:42:21, Devlin wrote:
> I think it's more confusing to pass in GetCodeForFeatureValues here rather
than
> just self.shared_values

Done.

https://codereview.chromium.org/2494653005/diff/300001/tools/json_schema_comp...
File tools/json_schema_compiler/test/features_test.json (right):

https://codereview.chromium.org/2494653005/diff/300001/tools/json_schema_comp...
tools/json_schema_compiler/test/features_test.json:113:
"parent_source.child_source" : {
On 2016/11/28 20:42:22, Devlin wrote:
> Wild.  I'm not entirely sure we want to use this, but it's cool that it works.
> :)

Yeah, I was mostly concerned that this might be needed for some future shared
values.

[commit-bot: I haz the power, 2016-11-28 21:44:19]

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-11-29 00:04:58]

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

[commit-bot: I haz the power, 2016-11-29 00:05:00]

Dry run: This issue passed the CQ dry run.

[Devlin, 2016-11-29 16:10:30]

lgtm

[tbarzic, 2016-11-29 18:11:37]

The CQ bit was checked by tbarzic@chromium.org

[commit-bot: I haz the power, 2016-11-29 18:12:07]

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-11-29 18:23:16]

CQ is committing da patch.

Bot data: {"patchset_id": 340001, "attempt_start_ts": 1480443097939080,
"parent_rev": "220d7d016fcc6d7f55365a0298f6dd9927535c8d", "commit_rev":
"6ad326202d6e33d457d3989643d4fb00fbd13e10"}

[commit-bot: I haz the power, 2016-11-29 18:23:47]

Committed patchset #18 (id:340001)

[commit-bot: I haz the power, 2016-11-29 18:26:01]

Description was changed from

==========
Support API aliases

Introduce API feature properties to specify an API source and alias.

|source| is set on API alias, and is used during API registration
- if an API feature has source set, the bindings installed under API
name will be the bindings of |source| API.

|alias| is used on source API, and is used when checking API
availability - when checking availability of an API that has alias
set, the API will be deemed available if either one of the API and
the API alias are available in the context.
Note that this logic should not be used when determining API
availability for binding registration - in this case alias
availability should be ignored in order to avoid registering
source API bindings when only alias bindings are needed.

BUG=647415
==========

to

==========
Support API aliases

Introduce API feature properties to specify an API source and alias.

|source| is set on API alias, and is used during API registration
- if an API feature has source set, the bindings installed under API
name will be the bindings of |source| API.

|alias| is used on source API, and is used when checking API
availability - when checking availability of an API that has alias
set, the API will be deemed available if either one of the API and
the API alias are available in the context.
Note that this logic should not be used when determining API
availability for binding registration - in this case alias
availability should be ignored in order to avoid registering
source API bindings when only alias bindings are needed.

BUG=647415

Committed: https://crrev.com/feb4b052b4c86b9047f8c220e3ec3983e5483cdf
Cr-Commit-Position: refs/heads/master@{#435034}
==========

[commit-bot: I haz the power, 2016-11-29 18:26:02]

Patchset 18 (id:??) landed as
https://crrev.com/feb4b052b4c86b9047f8c220e3ec3983e5483cdf
Cr-Commit-Position: refs/heads/master@{#435034}