Extract permission alias info from PermissionsProvider

Extract permission alias info from PermissionsProvider

Move the methods for getting lists of extension permission aliases
to standalone methods extensions::GetChromePermissionAliases and
extensions::GetExtensionsPermissionAliases (depending on whether
aliases refer to Chrome permissions, or code extensions
permissions).

This is done in preparation of adding suppport of API aliases, in
order to be able to declare sets of aliases (API and permission
aliases) together.

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

[tbarzic, 2016-09-29 01:34:56]

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

[commit-bot: I haz the power, 2016-09-29 01:35:32]

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

[tbarzic, 2016-09-29 16:31:04]

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

[tbarzic, 2016-09-29 17:33:03]

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

[commit-bot: I haz the power, 2016-09-29 17:33:17]

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-09-29 18:31:17]

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

[commit-bot: I haz the power, 2016-09-29 18:31:18]

Dry run: This issue passed the CQ dry run.

[Devlin, 2016-09-29 22:03:43]

https://codereview.chromium.org/2379763003/diff/40001/extensions/common/alias...
File extensions/common/alias_provider.h (right):

https://codereview.chromium.org/2379763003/diff/40001/extensions/common/alias...
extensions/common/alias_provider.h:31: virtual std::vector<Alias>
GetApiAliases() const = 0;
Until we use ApiAliases, I'd like to hold off on adding this functionality.

https://codereview.chromium.org/2379763003/diff/40001/extensions/common/api_a...
File extensions/common/api_alias_info.h (right):

https://codereview.chromium.org/2379763003/diff/40001/extensions/common/api_a...
extensions/common/api_alias_info.h:19: class ApiAliasInfo {
Similarly here, it doesn't look like we use this class at all (beyond
registering providers), so I'd prefer we hold off on its addition.

[tbarzic, 2016-09-29 22:27:13]

https://codereview.chromium.org/2379763003/diff/40001/extensions/common/api_a...
File extensions/common/api_alias_info.h (right):

https://codereview.chromium.org/2379763003/diff/40001/extensions/common/api_a...
extensions/common/api_alias_info.h:19: class ApiAliasInfo {
On 2016/09/29 22:03:43, Devlin (catching up) wrote:
> Similarly here, it doesn't look like we use this class at all (beyond
> registering providers), so I'd prefer we hold off on its addition.

I'm working on adding code using this fairly soon (in order of days).

[Devlin, 2016-09-29 22:30:41]

https://codereview.chromium.org/2379763003/diff/40001/extensions/common/api_a...
File extensions/common/api_alias_info.h (right):

https://codereview.chromium.org/2379763003/diff/40001/extensions/common/api_a...
extensions/common/api_alias_info.h:19: class ApiAliasInfo {
On 2016/09/29 22:27:13, tbarzic wrote:
> On 2016/09/29 22:03:43, Devlin (catching up) wrote:
> > Similarly here, it doesn't look like we use this class at all (beyond
> > registering providers), so I'd prefer we hold off on its addition.
> 
> I'm working on adding code using this fairly soon (in order of days).

Then let's add it in that CL :)  It's difficult to review code, especially from
an API stand point, when there aren't any uses of it.

[tbarzic, 2016-09-29 22:54:22]

Description was changed from

==========
Introduce extension::AliasProvider

Interface for definining aliases for extension APIs and permissions.

Each alias is defined by it's name, the real name of the feature
it is aliasing, and optionaly, upper bound on Chrome version on which
it is valid. The aliases bounded by the version will be ignored
past upper version bound - this was added to encourage cleanup of
aliases that are meant to be temporary.

This adds implementations for chrome specific and core extension
aliases, and move handling of existing permission aliases from
extensions::PermissionProvider implementations to
newly created extension::AliasProviders.

Add extensions::ApiAliasInfo, that will be used to lookup information
about existing API aliases (none are yet defined).

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

to

==========
Introduce extension::AliasProvider

Interface for definining aliases for extension permissions (planned
to be used for extension API aliases as well).

Each alias is defined by it's name, the real name of the feature
it is aliasing, and optionaly, upper bound on Chrome version on which
it is valid. The aliases bounded by the version will be ignored
past upper version bound - this was added to encourage cleanup of
aliases that are meant to be temporary.

This adds implementations for chrome specific and core extension
aliases, and move handling of existing permission aliases from
extensions::PermissionProvider implementations to
newly created extension::AliasProviders.


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

[tbarzic, 2016-09-29 22:57:31]

https://codereview.chromium.org/2379763003/diff/40001/extensions/common/api_a...
File extensions/common/api_alias_info.h (right):

https://codereview.chromium.org/2379763003/diff/40001/extensions/common/api_a...
extensions/common/api_alias_info.h:19: class ApiAliasInfo {
On 2016/09/29 22:30:41, Devlin (catching up) wrote:
> On 2016/09/29 22:27:13, tbarzic wrote:
> > On 2016/09/29 22:03:43, Devlin (catching up) wrote:
> > > Similarly here, it doesn't look like we use this class at all (beyond
> > > registering providers), so I'd prefer we hold off on its addition.
> > 
> > I'm working on adding code using this fairly soon (in order of days).
> 
> Then let's add it in that CL :)  It's difficult to review code, especially
from
> an API stand point, when there aren't any uses of it.

OK, my plan was to reduce the size of that CL, but I think even with moving this
particular part there it should be manageable.

[tbarzic, 2016-09-29 22:57:37]

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

[commit-bot: I haz the power, 2016-09-29 22:58:11]

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

[Devlin, 2016-10-06 00:38:06]

On 2016/09/29 22:57:31, tbarzic wrote:
>
https://codereview.chromium.org/2379763003/diff/40001/extensions/common/api_a...
> File extensions/common/api_alias_info.h (right):
> 
>
https://codereview.chromium.org/2379763003/diff/40001/extensions/common/api_a...
> extensions/common/api_alias_info.h:19: class ApiAliasInfo {
> On 2016/09/29 22:30:41, Devlin (catching up) wrote:
> > On 2016/09/29 22:27:13, tbarzic wrote:
> > > On 2016/09/29 22:03:43, Devlin (catching up) wrote:
> > > > Similarly here, it doesn't look like we use this class at all (beyond
> > > > registering providers), so I'd prefer we hold off on its addition.
> > > 
> > > I'm working on adding code using this fairly soon (in order of days).
> > 
> > Then let's add it in that CL :)  It's difficult to review code, especially
> from
> > an API stand point, when there aren't any uses of it.
> 
> OK, my plan was to reduce the size of that CL, but I think even with moving
this
> particular part there it should be manageable.

Sorry, I didn't realize this was updated.  I'll be sure to get to it tomorrow.

[Devlin, 2016-10-06 15:00:11]

Taking a step back here, why is having an AliasProvider et al significantly
better than just having a std::map of these?  It seems like we might be
over-architecting a bit here.  Is there a greater advantage that I'm missing?

Also, I'm a little torn on limiting aliases to certain versions.  I can
understand the motivation and it's kind of nifty, but the result is that we're
forcing more work to be done when we dynamically realize that code should be
removed - which is kind of weird.  Overall, I think we'd probably be better off
by putting an expiration date/mstone in a TODO next to the aliases, and then
cleaning them up afterwards.  It would result in quite a bit less complexity. 
If all we needed was a reminder, we could set up an email or calendar entry for
that date. :)

[tbarzic, 2016-10-06 17:34:41]

On 2016/10/06 15:00:11, Devlin wrote:
> Taking a step back here, why is having an AliasProvider et al significantly
> better than just having a std::map of these?  It seems like we might be
> over-architecting a bit here.  Is there a greater advantage that I'm missing?
> 
> Also, I'm a little torn on limiting aliases to certain versions.  I can
> understand the motivation and it's kind of nifty, but the result is that we're
> forcing more work to be done when we dynamically realize that code should be
> removed - which is kind of weird.  Overall, I think we'd probably be better
off
> by putting an expiration date/mstone in a TODO next to the aliases, and then
> cleaning them up afterwards.  It would result in quite a bit less complexity. 
> If all we needed was a reminder, we could set up an email or calendar entry
for
> that date. :)

I'm fine with nuking support for limiting aliases to certain versions. I was on
the fence about usefulness of adding this, too.

In this cl, I'm essentially splitting permissions aliases from permissions
provider to another provider for aliases (which would make more sense with
support for both permissions and aliases).

Main problem with just having a map of those is that the set of available
aliases is different based on whether one is building core extensions or chrome
extensions (i.e. set of aliases will be different for chrome and app shell).

Arguably, we could just define a map of aliases at the place where AliasProvider
is currently used, but I don't like that approach mainly because core extension
aliases are registered at different places - ChromeExtensionClient,
ShellExtensionClient, TestExtensionClient - so the core extension alias map
would have to be duplicated few times.

[Devlin, 2016-10-06 19:05:58]

On 2016/10/06 17:34:41, tbarzic wrote:
> On 2016/10/06 15:00:11, Devlin wrote:
> > Taking a step back here, why is having an AliasProvider et al significantly
> > better than just having a std::map of these?  It seems like we might be
> > over-architecting a bit here.  Is there a greater advantage that I'm
missing?
> > 
> > Also, I'm a little torn on limiting aliases to certain versions.  I can
> > understand the motivation and it's kind of nifty, but the result is that
we're
> > forcing more work to be done when we dynamically realize that code should be
> > removed - which is kind of weird.  Overall, I think we'd probably be better
> off
> > by putting an expiration date/mstone in a TODO next to the aliases, and then
> > cleaning them up afterwards.  It would result in quite a bit less
complexity. 
> > If all we needed was a reminder, we could set up an email or calendar entry
> for
> > that date. :)
> 
> I'm fine with nuking support for limiting aliases to certain versions. I was
on
> the fence about usefulness of adding this, too.

Let's nuke it until we're sure we need it. :)

> In this cl, I'm essentially splitting permissions aliases from permissions
> provider to another provider for aliases (which would make more sense with
> support for both permissions and aliases).
> 
> Main problem with just having a map of those is that the set of available
> aliases is different based on whether one is building core extensions or
chrome
> extensions (i.e. set of aliases will be different for chrome and app shell).
> 
> Arguably, we could just define a map of aliases at the place where
AliasProvider
> is currently used, but I don't like that approach mainly because core
extension
> aliases are registered at different places - ChromeExtensionClient,
> ShellExtensionClient, TestExtensionClient - so the core extension alias map
> would have to be duplicated few times.

Basically, I'm just questioning why we need an encapsulating class (especially
if we remove version support, and thus the concept of validity).  I'm fine with
having separate sections for the different targets, but we could just reduce it
to a single

std::map GetFooAliases() {
  return std::map({
    {alias...}
  };
}

in each section.

(And then in the various clients, we do AddAliases(GetFooAliases());
AddAliases(GetBarAliases()); etc).

I think that overall, something like that would just be easier to grok for
readers, since they don't have to dive down into a hierarchical provider
structure that really just wraps around a container.  WDYT?

[tbarzic, 2016-10-06 20:21:00]

On 2016/10/06 19:05:58, Devlin wrote:
> On 2016/10/06 17:34:41, tbarzic wrote:
> > On 2016/10/06 15:00:11, Devlin wrote:
> > > Taking a step back here, why is having an AliasProvider et al
significantly
> > > better than just having a std::map of these?  It seems like we might be
> > > over-architecting a bit here.  Is there a greater advantage that I'm
> missing?
> > > 
> > > Also, I'm a little torn on limiting aliases to certain versions.  I can
> > > understand the motivation and it's kind of nifty, but the result is that
> we're
> > > forcing more work to be done when we dynamically realize that code should
be
> > > removed - which is kind of weird.  Overall, I think we'd probably be
better
> > off
> > > by putting an expiration date/mstone in a TODO next to the aliases, and
then
> > > cleaning them up afterwards.  It would result in quite a bit less
> complexity. 
> > > If all we needed was a reminder, we could set up an email or calendar
entry
> > for
> > > that date. :)
> > 
> > I'm fine with nuking support for limiting aliases to certain versions. I was
> on
> > the fence about usefulness of adding this, too.
> 
> Let's nuke it until we're sure we need it. :)
> 
> > In this cl, I'm essentially splitting permissions aliases from permissions
> > provider to another provider for aliases (which would make more sense with
> > support for both permissions and aliases).
> > 
> > Main problem with just having a map of those is that the set of available
> > aliases is different based on whether one is building core extensions or
> chrome
> > extensions (i.e. set of aliases will be different for chrome and app shell).
> > 
> > Arguably, we could just define a map of aliases at the place where
> AliasProvider
> > is currently used, but I don't like that approach mainly because core
> extension
> > aliases are registered at different places - ChromeExtensionClient,
> > ShellExtensionClient, TestExtensionClient - so the core extension alias map
> > would have to be duplicated few times.
> 
> Basically, I'm just questioning why we need an encapsulating class (especially
> if we remove version support, and thus the concept of validity).  I'm fine
with
> having separate sections for the different targets, but we could just reduce
it
> to a single
> 
> std::map GetFooAliases() {
>   return std::map({
>     {alias...}
>   };
> }
> 
> in each section.
> 
> (And then in the various clients, we do AddAliases(GetFooAliases());
> AddAliases(GetBarAliases()); etc).
> 
> I think that overall, something like that would just be easier to grok for
> readers, since they don't have to dive down into a hierarchical provider
> structure that really just wraps around a container.  WDYT?

I'm not completely clear on what you mean by sections. Are you proposing having
a single file with something like:

namespace extensions {
namespace alias {
GetChromeAliases();
GetCoreExtensionAliases();
}
}

Wouldn't that break dependencies - this would have to be in extensions/common/ -
and extensions/ knowing about APIs in Chrome seems wrong.

Or do you refer to encapsulating class around an alias (i.e. using Alias()
instead of pair<string, string>). I prefer having full class - I find it much
more readable (foo.name/real_name is more readable than foo.first/second). Also,
it leaves more room for extending functionality in the future.


For now, I removed support for bounding aliases to versions, and that made
things cleaner - for one, AliasProvider is now pure interface.

[Devlin, 2016-10-06 23:35:57]

On 2016/10/06 20:21:00, tbarzic wrote:
> On 2016/10/06 19:05:58, Devlin wrote:
> > On 2016/10/06 17:34:41, tbarzic wrote:
> > > On 2016/10/06 15:00:11, Devlin wrote:
> > > > Taking a step back here, why is having an AliasProvider et al
> significantly
> > > > better than just having a std::map of these?  It seems like we might be
> > > > over-architecting a bit here.  Is there a greater advantage that I'm
> > missing?
> > > > 
> > > > Also, I'm a little torn on limiting aliases to certain versions.  I can
> > > > understand the motivation and it's kind of nifty, but the result is that
> > we're
> > > > forcing more work to be done when we dynamically realize that code
should
> be
> > > > removed - which is kind of weird.  Overall, I think we'd probably be
> better
> > > off
> > > > by putting an expiration date/mstone in a TODO next to the aliases, and
> then
> > > > cleaning them up afterwards.  It would result in quite a bit less
> > complexity. 
> > > > If all we needed was a reminder, we could set up an email or calendar
> entry
> > > for
> > > > that date. :)
> > > 
> > > I'm fine with nuking support for limiting aliases to certain versions. I
was
> > on
> > > the fence about usefulness of adding this, too.
> > 
> > Let's nuke it until we're sure we need it. :)
> > 
> > > In this cl, I'm essentially splitting permissions aliases from permissions
> > > provider to another provider for aliases (which would make more sense with
> > > support for both permissions and aliases).
> > > 
> > > Main problem with just having a map of those is that the set of available
> > > aliases is different based on whether one is building core extensions or
> > chrome
> > > extensions (i.e. set of aliases will be different for chrome and app
shell).
> > > 
> > > Arguably, we could just define a map of aliases at the place where
> > AliasProvider
> > > is currently used, but I don't like that approach mainly because core
> > extension
> > > aliases are registered at different places - ChromeExtensionClient,
> > > ShellExtensionClient, TestExtensionClient - so the core extension alias
map
> > > would have to be duplicated few times.
> > 
> > Basically, I'm just questioning why we need an encapsulating class
(especially
> > if we remove version support, and thus the concept of validity).  I'm fine
> with
> > having separate sections for the different targets, but we could just reduce
> it
> > to a single
> > 
> > std::map GetFooAliases() {
> >   return std::map({
> >     {alias...}
> >   };
> > }
> > 
> > in each section.
> > 
> > (And then in the various clients, we do AddAliases(GetFooAliases());
> > AddAliases(GetBarAliases()); etc).
> > 
> > I think that overall, something like that would just be easier to grok for
> > readers, since they don't have to dive down into a hierarchical provider
> > structure that really just wraps around a container.  WDYT?
> 
> I'm not completely clear on what you mean by sections. Are you proposing
having
> a single file with something like:
> 
> namespace extensions {
> namespace alias {
> GetChromeAliases();
> GetCoreExtensionAliases();
> }
> }
> 
> Wouldn't that break dependencies - this would have to be in extensions/common/
-
> and extensions/ knowing about APIs in Chrome seems wrong.
> 
> Or do you refer to encapsulating class around an alias (i.e. using Alias()
> instead of pair<string, string>). I prefer having full class - I find it much
> more readable (foo.name/real_name is more readable than foo.first/second).
Also,
> it leaves more room for extending functionality in the future.
> 
> 
> For now, I removed support for bounding aliases to versions, and that made
> things cleaner - for one, AliasProvider is now pure interface.

I mean:

c/b/e/chrome_aliases.h

std::vector<Alias> GetChromeAliases();

c/c/e/chrome_aliases.cc

std::vector<Alias> GetChromeAliases() {
  return {
    {...},
  };
}

e/c/extension_aliases.h

std::vector<Alias> GetExtensionAliases();

e/c/extension_aliases.cc

std::vector<Alias> GetExtensionAliases() {
  return {
    {...},
  };
}

chrome_extensions_client.cc

...
AddAliases(GetChromeExtensionAliases());
AddAliases(GetExtensionAliases());
...

etc

Basically, I'm just asking if we need an encapsulating class in order to return
a constant set of items.

[tbarzic, 2016-10-07 17:06:26]

On 2016/10/06 23:35:57, Devlin wrote:
> On 2016/10/06 20:21:00, tbarzic wrote:
> > On 2016/10/06 19:05:58, Devlin wrote:
> > > On 2016/10/06 17:34:41, tbarzic wrote:
> > > > On 2016/10/06 15:00:11, Devlin wrote:
> > > > > Taking a step back here, why is having an AliasProvider et al
> > significantly
> > > > > better than just having a std::map of these?  It seems like we might
be
> > > > > over-architecting a bit here.  Is there a greater advantage that I'm
> > > missing?
> > > > > 
> > > > > Also, I'm a little torn on limiting aliases to certain versions.  I
can
> > > > > understand the motivation and it's kind of nifty, but the result is
that
> > > we're
> > > > > forcing more work to be done when we dynamically realize that code
> should
> > be
> > > > > removed - which is kind of weird.  Overall, I think we'd probably be
> > better
> > > > off
> > > > > by putting an expiration date/mstone in a TODO next to the aliases,
and
> > then
> > > > > cleaning them up afterwards.  It would result in quite a bit less
> > > complexity. 
> > > > > If all we needed was a reminder, we could set up an email or calendar
> > entry
> > > > for
> > > > > that date. :)
> > > > 
> > > > I'm fine with nuking support for limiting aliases to certain versions. I
> was
> > > on
> > > > the fence about usefulness of adding this, too.
> > > 
> > > Let's nuke it until we're sure we need it. :)
> > > 
> > > > In this cl, I'm essentially splitting permissions aliases from
permissions
> > > > provider to another provider for aliases (which would make more sense
with
> > > > support for both permissions and aliases).
> > > > 
> > > > Main problem with just having a map of those is that the set of
available
> > > > aliases is different based on whether one is building core extensions or
> > > chrome
> > > > extensions (i.e. set of aliases will be different for chrome and app
> shell).
> > > > 
> > > > Arguably, we could just define a map of aliases at the place where
> > > AliasProvider
> > > > is currently used, but I don't like that approach mainly because core
> > > extension
> > > > aliases are registered at different places - ChromeExtensionClient,
> > > > ShellExtensionClient, TestExtensionClient - so the core extension alias
> map
> > > > would have to be duplicated few times.
> > > 
> > > Basically, I'm just questioning why we need an encapsulating class
> (especially
> > > if we remove version support, and thus the concept of validity).  I'm fine
> > with
> > > having separate sections for the different targets, but we could just
reduce
> > it
> > > to a single
> > > 
> > > std::map GetFooAliases() {
> > >   return std::map({
> > >     {alias...}
> > >   };
> > > }
> > > 
> > > in each section.
> > > 
> > > (And then in the various clients, we do AddAliases(GetFooAliases());
> > > AddAliases(GetBarAliases()); etc).
> > > 
> > > I think that overall, something like that would just be easier to grok for
> > > readers, since they don't have to dive down into a hierarchical provider
> > > structure that really just wraps around a container.  WDYT?
> > 
> > I'm not completely clear on what you mean by sections. Are you proposing
> having
> > a single file with something like:
> > 
> > namespace extensions {
> > namespace alias {
> > GetChromeAliases();
> > GetCoreExtensionAliases();
> > }
> > }
> > 
> > Wouldn't that break dependencies - this would have to be in
extensions/common/
> -
> > and extensions/ knowing about APIs in Chrome seems wrong.
> > 
> > Or do you refer to encapsulating class around an alias (i.e. using Alias()
> > instead of pair<string, string>). I prefer having full class - I find it
much
> > more readable (foo.name/real_name is more readable than foo.first/second).
> Also,
> > it leaves more room for extending functionality in the future.
> > 
> > 
> > For now, I removed support for bounding aliases to versions, and that made
> > things cleaner - for one, AliasProvider is now pure interface.
> 
> I mean:
> 
> c/b/e/chrome_aliases.h
> 
> std::vector<Alias> GetChromeAliases();
> 
> c/c/e/chrome_aliases.cc
> 
> std::vector<Alias> GetChromeAliases() {
>   return {
>     {...},
>   };
> }
> 
> e/c/extension_aliases.h
> 
> std::vector<Alias> GetExtensionAliases();
> 
> e/c/extension_aliases.cc
> 
> std::vector<Alias> GetExtensionAliases() {
>   return {
>     {...},
>   };
> }
> 
> chrome_extensions_client.cc
> 
> ...
> AddAliases(GetChromeExtensionAliases());
> AddAliases(GetExtensionAliases());
> ...
> 
> etc
> 
> Basically, I'm just asking if we need an encapsulating class in order to
return
> a constant set of items.

OK, done.

One downside of using bare functions is that it's easier to pass in wrong type
of aliases into permissions_info (e.g. one could easily do
permissions_info->AddProvider(permissions, GetApiAliases()); passing in a
provider class would at least ensured only permission aliases are taken into
account), but honestly, I don't have *strong* preference either way.

[Devlin, 2016-10-07 18:24:31]

On 2016/10/07 17:06:26, tbarzic wrote:
> On 2016/10/06 23:35:57, Devlin wrote:
> > On 2016/10/06 20:21:00, tbarzic wrote:
> > > On 2016/10/06 19:05:58, Devlin wrote:
> > > > On 2016/10/06 17:34:41, tbarzic wrote:
> > > > > On 2016/10/06 15:00:11, Devlin wrote:
> > > > > > Taking a step back here, why is having an AliasProvider et al
> > > significantly
> > > > > > better than just having a std::map of these?  It seems like we might
> be
> > > > > > over-architecting a bit here.  Is there a greater advantage that I'm
> > > > missing?
> > > > > > 
> > > > > > Also, I'm a little torn on limiting aliases to certain versions.  I
> can
> > > > > > understand the motivation and it's kind of nifty, but the result is
> that
> > > > we're
> > > > > > forcing more work to be done when we dynamically realize that code
> > should
> > > be
> > > > > > removed - which is kind of weird.  Overall, I think we'd probably be
> > > better
> > > > > off
> > > > > > by putting an expiration date/mstone in a TODO next to the aliases,
> and
> > > then
> > > > > > cleaning them up afterwards.  It would result in quite a bit less
> > > > complexity. 
> > > > > > If all we needed was a reminder, we could set up an email or
calendar
> > > entry
> > > > > for
> > > > > > that date. :)
> > > > > 
> > > > > I'm fine with nuking support for limiting aliases to certain versions.
I
> > was
> > > > on
> > > > > the fence about usefulness of adding this, too.
> > > > 
> > > > Let's nuke it until we're sure we need it. :)
> > > > 
> > > > > In this cl, I'm essentially splitting permissions aliases from
> permissions
> > > > > provider to another provider for aliases (which would make more sense
> with
> > > > > support for both permissions and aliases).
> > > > > 
> > > > > Main problem with just having a map of those is that the set of
> available
> > > > > aliases is different based on whether one is building core extensions
or
> > > > chrome
> > > > > extensions (i.e. set of aliases will be different for chrome and app
> > shell).
> > > > > 
> > > > > Arguably, we could just define a map of aliases at the place where
> > > > AliasProvider
> > > > > is currently used, but I don't like that approach mainly because core
> > > > extension
> > > > > aliases are registered at different places - ChromeExtensionClient,
> > > > > ShellExtensionClient, TestExtensionClient - so the core extension
alias
> > map
> > > > > would have to be duplicated few times.
> > > > 
> > > > Basically, I'm just questioning why we need an encapsulating class
> > (especially
> > > > if we remove version support, and thus the concept of validity).  I'm
fine
> > > with
> > > > having separate sections for the different targets, but we could just
> reduce
> > > it
> > > > to a single
> > > > 
> > > > std::map GetFooAliases() {
> > > >   return std::map({
> > > >     {alias...}
> > > >   };
> > > > }
> > > > 
> > > > in each section.
> > > > 
> > > > (And then in the various clients, we do AddAliases(GetFooAliases());
> > > > AddAliases(GetBarAliases()); etc).
> > > > 
> > > > I think that overall, something like that would just be easier to grok
for
> > > > readers, since they don't have to dive down into a hierarchical provider
> > > > structure that really just wraps around a container.  WDYT?
> > > 
> > > I'm not completely clear on what you mean by sections. Are you proposing
> > having
> > > a single file with something like:
> > > 
> > > namespace extensions {
> > > namespace alias {
> > > GetChromeAliases();
> > > GetCoreExtensionAliases();
> > > }
> > > }
> > > 
> > > Wouldn't that break dependencies - this would have to be in
> extensions/common/
> > -
> > > and extensions/ knowing about APIs in Chrome seems wrong.
> > > 
> > > Or do you refer to encapsulating class around an alias (i.e. using Alias()
> > > instead of pair<string, string>). I prefer having full class - I find it
> much
> > > more readable (foo.name/real_name is more readable than foo.first/second).
> > Also,
> > > it leaves more room for extending functionality in the future.
> > > 
> > > 
> > > For now, I removed support for bounding aliases to versions, and that made
> > > things cleaner - for one, AliasProvider is now pure interface.
> > 
> > I mean:
> > 
> > c/b/e/chrome_aliases.h
> > 
> > std::vector<Alias> GetChromeAliases();
> > 
> > c/c/e/chrome_aliases.cc
> > 
> > std::vector<Alias> GetChromeAliases() {
> >   return {
> >     {...},
> >   };
> > }
> > 
> > e/c/extension_aliases.h
> > 
> > std::vector<Alias> GetExtensionAliases();
> > 
> > e/c/extension_aliases.cc
> > 
> > std::vector<Alias> GetExtensionAliases() {
> >   return {
> >     {...},
> >   };
> > }
> > 
> > chrome_extensions_client.cc
> > 
> > ...
> > AddAliases(GetChromeExtensionAliases());
> > AddAliases(GetExtensionAliases());
> > ...
> > 
> > etc
> > 
> > Basically, I'm just asking if we need an encapsulating class in order to
> return
> > a constant set of items.
> 
> OK, done.
> 
> One downside of using bare functions is that it's easier to pass in wrong type
> of aliases into permissions_info (e.g. one could easily do
> permissions_info->AddProvider(permissions, GetApiAliases()); passing in a
> provider class would at least ensured only permission aliases are taken into
> account), but honestly, I don't have *strong* preference either way.

Two notes: with what you had before, it just took an AliasProvider, so there
wasn't that guarantee anyway (you could easily pass in
ExtensionAPIAliasProvider).  But more importantly, that's why we have code
reviews and tests - either one of which would hopefully catch that mistake. :)

[tbarzic, 2016-10-07 18:32:01]

On 2016/10/07 18:24:31, Devlin wrote:
> On 2016/10/07 17:06:26, tbarzic wrote:
> > On 2016/10/06 23:35:57, Devlin wrote:
> > > On 2016/10/06 20:21:00, tbarzic wrote:
> > > > On 2016/10/06 19:05:58, Devlin wrote:
> > > > > On 2016/10/06 17:34:41, tbarzic wrote:
> > > > > > On 2016/10/06 15:00:11, Devlin wrote:
> > > > > > > Taking a step back here, why is having an AliasProvider et al
> > > > significantly
> > > > > > > better than just having a std::map of these?  It seems like we
might
> > be
> > > > > > > over-architecting a bit here.  Is there a greater advantage that
I'm
> > > > > missing?
> > > > > > > 
> > > > > > > Also, I'm a little torn on limiting aliases to certain versions. 
I
> > can
> > > > > > > understand the motivation and it's kind of nifty, but the result
is
> > that
> > > > > we're
> > > > > > > forcing more work to be done when we dynamically realize that code
> > > should
> > > > be
> > > > > > > removed - which is kind of weird.  Overall, I think we'd probably
be
> > > > better
> > > > > > off
> > > > > > > by putting an expiration date/mstone in a TODO next to the
aliases,
> > and
> > > > then
> > > > > > > cleaning them up afterwards.  It would result in quite a bit less
> > > > > complexity. 
> > > > > > > If all we needed was a reminder, we could set up an email or
> calendar
> > > > entry
> > > > > > for
> > > > > > > that date. :)
> > > > > > 
> > > > > > I'm fine with nuking support for limiting aliases to certain
versions.
> I
> > > was
> > > > > on
> > > > > > the fence about usefulness of adding this, too.
> > > > > 
> > > > > Let's nuke it until we're sure we need it. :)
> > > > > 
> > > > > > In this cl, I'm essentially splitting permissions aliases from
> > permissions
> > > > > > provider to another provider for aliases (which would make more
sense
> > with
> > > > > > support for both permissions and aliases).
> > > > > > 
> > > > > > Main problem with just having a map of those is that the set of
> > available
> > > > > > aliases is different based on whether one is building core
extensions
> or
> > > > > chrome
> > > > > > extensions (i.e. set of aliases will be different for chrome and app
> > > shell).
> > > > > > 
> > > > > > Arguably, we could just define a map of aliases at the place where
> > > > > AliasProvider
> > > > > > is currently used, but I don't like that approach mainly because
core
> > > > > extension
> > > > > > aliases are registered at different places - ChromeExtensionClient,
> > > > > > ShellExtensionClient, TestExtensionClient - so the core extension
> alias
> > > map
> > > > > > would have to be duplicated few times.
> > > > > 
> > > > > Basically, I'm just questioning why we need an encapsulating class
> > > (especially
> > > > > if we remove version support, and thus the concept of validity).  I'm
> fine
> > > > with
> > > > > having separate sections for the different targets, but we could just
> > reduce
> > > > it
> > > > > to a single
> > > > > 
> > > > > std::map GetFooAliases() {
> > > > >   return std::map({
> > > > >     {alias...}
> > > > >   };
> > > > > }
> > > > > 
> > > > > in each section.
> > > > > 
> > > > > (And then in the various clients, we do AddAliases(GetFooAliases());
> > > > > AddAliases(GetBarAliases()); etc).
> > > > > 
> > > > > I think that overall, something like that would just be easier to grok
> for
> > > > > readers, since they don't have to dive down into a hierarchical
provider
> > > > > structure that really just wraps around a container.  WDYT?
> > > > 
> > > > I'm not completely clear on what you mean by sections. Are you proposing
> > > having
> > > > a single file with something like:
> > > > 
> > > > namespace extensions {
> > > > namespace alias {
> > > > GetChromeAliases();
> > > > GetCoreExtensionAliases();
> > > > }
> > > > }
> > > > 
> > > > Wouldn't that break dependencies - this would have to be in
> > extensions/common/
> > > -
> > > > and extensions/ knowing about APIs in Chrome seems wrong.
> > > > 
> > > > Or do you refer to encapsulating class around an alias (i.e. using
Alias()
> > > > instead of pair<string, string>). I prefer having full class - I find it
> > much
> > > > more readable (foo.name/real_name is more readable than
foo.first/second).
> > > Also,
> > > > it leaves more room for extending functionality in the future.
> > > > 
> > > > 
> > > > For now, I removed support for bounding aliases to versions, and that
made
> > > > things cleaner - for one, AliasProvider is now pure interface.
> > > 
> > > I mean:
> > > 
> > > c/b/e/chrome_aliases.h
> > > 
> > > std::vector<Alias> GetChromeAliases();
> > > 
> > > c/c/e/chrome_aliases.cc
> > > 
> > > std::vector<Alias> GetChromeAliases() {
> > >   return {
> > >     {...},
> > >   };
> > > }
> > > 
> > > e/c/extension_aliases.h
> > > 
> > > std::vector<Alias> GetExtensionAliases();
> > > 
> > > e/c/extension_aliases.cc
> > > 
> > > std::vector<Alias> GetExtensionAliases() {
> > >   return {
> > >     {...},
> > >   };
> > > }
> > > 
> > > chrome_extensions_client.cc
> > > 
> > > ...
> > > AddAliases(GetChromeExtensionAliases());
> > > AddAliases(GetExtensionAliases());
> > > ...
> > > 
> > > etc
> > > 
> > > Basically, I'm just asking if we need an encapsulating class in order to
> > return
> > > a constant set of items.
> > 
> > OK, done.
> > 
> > One downside of using bare functions is that it's easier to pass in wrong
type
> > of aliases into permissions_info (e.g. one could easily do
> > permissions_info->AddProvider(permissions, GetApiAliases()); passing in a
> > provider class would at least ensured only permission aliases are taken into
> > account), but honestly, I don't have *strong* preference either way.
> 
> Two notes: with what you had before, it just took an AliasProvider, so there
> wasn't that guarantee anyway (you could easily pass in
> ExtensionAPIAliasProvider).  But more importantly, that's why we have code
> reviews and tests - either one of which would hopefully catch that mistake. :)

I know, that's why I don't mind dropping AliasProvider :)

[Devlin, 2016-10-07 18:36:21]

Much simpler; thanks!

https://codereview.chromium.org/2379763003/diff/140001/chrome/common/extensio...
File chrome/common/extensions/chrome_aliases.cc (right):

https://codereview.chromium.org/2379763003/diff/140001/chrome/common/extensio...
chrome/common/extensions/chrome_aliases.cc:7: #include "base/macros.h"
needed?

https://codereview.chromium.org/2379763003/diff/140001/chrome/common/extensio...
chrome/common/extensions/chrome_aliases.cc:25: std::vector<Alias> result;
Why not
return {
  Alias("windows", "tabs"),
};
?

https://codereview.chromium.org/2379763003/diff/140001/chrome/common/extensio...
File chrome/common/extensions/chrome_extensions_client.cc (right):

https://codereview.chromium.org/2379763003/diff/140001/chrome/common/extensio...
chrome/common/extensions/chrome_extensions_client.cc:31: #include
"extensions/common/alias.h"
needed?

https://codereview.chromium.org/2379763003/diff/140001/extensions/common/alias.h
File extensions/common/alias.h (right):

https://codereview.chromium.org/2379763003/diff/140001/extensions/common/alia...
extensions/common/alias.h:12: // Information about a specific alias.
What is an Alias?  Why do we need them/where do we use them?  (Describe in the
comments)

https://codereview.chromium.org/2379763003/diff/140001/extensions/common/alia...
extensions/common/alias.h:26: std::string name_;
nit: \n

https://codereview.chromium.org/2379763003/diff/140001/extensions/common/exte...
File extensions/common/extensions_aliases.cc (right):

https://codereview.chromium.org/2379763003/diff/140001/extensions/common/exte...
extensions/common/extensions_aliases.cc:27: std::vector<Alias> result;
ditto re simpler body

[tbarzic, 2016-10-07 19:22:48]

Description was changed from

==========
Introduce extension::AliasProvider

Interface for definining aliases for extension permissions (planned
to be used for extension API aliases as well).

Each alias is defined by it's name, the real name of the feature
it is aliasing, and optionaly, upper bound on Chrome version on which
it is valid. The aliases bounded by the version will be ignored
past upper version bound - this was added to encourage cleanup of
aliases that are meant to be temporary.

This adds implementations for chrome specific and core extension
aliases, and move handling of existing permission aliases from
extensions::PermissionProvider implementations to
newly created extension::AliasProviders.


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

to

==========
Extract permission alias info from PermissionsProvider

Move the methods for getting lists of extension permission aliases
to standalone methods extensions::GetChromePermissionAliases and
extensions::GetExtensionsPermissionAliases (depending on whether
aliases refer to Chrome permissions, or code extensions
permissions).

This is done in preparation of adding suppport of API aliases, in
order to be able to declare sets of aliases (API and permission
aliases) together.

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

[tbarzic, 2016-10-07 20:40:59]

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

[tbarzic, 2016-10-07 20:41:18]

https://codereview.chromium.org/2379763003/diff/140001/chrome/common/extensio...
File chrome/common/extensions/chrome_aliases.cc (right):

https://codereview.chromium.org/2379763003/diff/140001/chrome/common/extensio...
chrome/common/extensions/chrome_aliases.cc:7: #include "base/macros.h"
On 2016/10/07 18:36:21, Devlin wrote:
> needed?

yes, for arraysize.

https://codereview.chromium.org/2379763003/diff/140001/chrome/common/extensio...
chrome/common/extensions/chrome_aliases.cc:25: std::vector<Alias> result;
On 2016/10/07 18:36:21, Devlin wrote:
> Why not
> return {
>   Alias("windows", "tabs"),
> };
> ?

Main reason: it's clearer which string is alias name, and which one is real name
when parsing this file.
with return {Alias("window", "tabs")} one would have to look up alias.h to
figure out which one is alias and which one is not.

And I find writing Alias("window" /* alias_name */, "tab" /* real_name */);
for each added alias clunkier (more evident in the other file).

Though, I don't feel that strongly about it.

https://codereview.chromium.org/2379763003/diff/140001/chrome/common/extensio...
File chrome/common/extensions/chrome_extensions_client.cc (right):

https://codereview.chromium.org/2379763003/diff/140001/chrome/common/extensio...
chrome/common/extensions/chrome_extensions_client.cc:31: #include
"extensions/common/alias.h"
On 2016/10/07 18:36:21, Devlin wrote:
> needed?

Done.

https://codereview.chromium.org/2379763003/diff/140001/extensions/common/alias.h
File extensions/common/alias.h (right):

https://codereview.chromium.org/2379763003/diff/140001/extensions/common/alia...
extensions/common/alias.h:12: // Information about a specific alias.
On 2016/10/07 18:36:21, Devlin wrote:
> What is an Alias?  Why do we need them/where do we use them?  (Describe in the
> comments)

Done.

https://codereview.chromium.org/2379763003/diff/140001/extensions/common/alia...
extensions/common/alias.h:26: std::string name_;
On 2016/10/07 18:36:21, Devlin wrote:
> nit: \n

Done.

[commit-bot: I haz the power, 2016-10-07 20:41:32]

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

[Devlin, 2016-10-07 21:05:59]

lgtm with nit

https://codereview.chromium.org/2379763003/diff/140001/chrome/common/extensio...
File chrome/common/extensions/chrome_aliases.cc (right):

https://codereview.chromium.org/2379763003/diff/140001/chrome/common/extensio...
chrome/common/extensions/chrome_aliases.cc:25: std::vector<Alias> result;
On 2016/10/07 20:41:17, tbarzic wrote:
> On 2016/10/07 18:36:21, Devlin wrote:
> > Why not
> > return {
> >   Alias("windows", "tabs"),
> > };
> > ?
> 
> Main reason: it's clearer which string is alias name, and which one is real
name
> when parsing this file.
> with return {Alias("window", "tabs")} one would have to look up alias.h to
> figure out which one is alias and which one is not.
> 
> And I find writing Alias("window" /* alias_name */, "tab" /* real_name */);
> for each added alias clunkier (more evident in the other file).
> 
> Though, I don't feel that strongly about it.

But it's only clearer because you basically re-define the class as a struct in
AliasInfo - which isn't great.  To avoid the "window" /* alias name */ et al you
could just put one top-level comment above the return value:
// First value is the alias name; second value is the real name. See also
alias.h.

https://codereview.chromium.org/2379763003/diff/180001/extensions/common/exte...
File extensions/common/extensions_aliases.cc (right):

https://codereview.chromium.org/2379763003/diff/180001/extensions/common/exte...
extensions/common/extensions_aliases.cc:11: Alias("alwaysOnTopWindows" /* alias
*/,
nit (from other comment)
please do just add
// First value is the alias name; second value is the real name. See also
alias.h.
It's a bit more readable that way, IMO.

[tbarzic, 2016-10-07 21:28:39]

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

[commit-bot: I haz the power, 2016-10-07 21:29:13]

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

[tbarzic, 2016-10-07 21:29:32]

https://codereview.chromium.org/2379763003/diff/180001/extensions/common/exte...
File extensions/common/extensions_aliases.cc (right):

https://codereview.chromium.org/2379763003/diff/180001/extensions/common/exte...
extensions/common/extensions_aliases.cc:11: Alias("alwaysOnTopWindows" /* alias
*/,
On 2016/10/07 21:05:59, Devlin wrote:
> nit (from other comment)
> please do just add
> // First value is the alias name; second value is the real name. See also
> alias.h.
> It's a bit more readable that way, IMO.

Done.

[commit-bot: I haz the power, 2016-10-07 22:40:02]

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

[commit-bot: I haz the power, 2016-10-07 22:40:03]

Dry run: This issue passed the CQ dry run.

[tbarzic, 2016-10-07 23:47:27]

The CQ bit was checked by tbarzic@chromium.org

[tbarzic, 2016-10-07 23:47:28]

The patchset sent to the CQ was uploaded after l-g-t-m from
rdevlin.cronin@chromium.org
Link to the patchset: https://codereview.chromium.org/2379763003/#ps200001
(title: ".")

[commit-bot: I haz the power, 2016-10-07 23:47:55]

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-10-07 23:54:08]

Description was changed from

==========
Extract permission alias info from PermissionsProvider

Move the methods for getting lists of extension permission aliases
to standalone methods extensions::GetChromePermissionAliases and
extensions::GetExtensionsPermissionAliases (depending on whether
aliases refer to Chrome permissions, or code extensions
permissions).

This is done in preparation of adding suppport of API aliases, in
order to be able to declare sets of aliases (API and permission
aliases) together.

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

to

==========
Extract permission alias info from PermissionsProvider

Move the methods for getting lists of extension permission aliases
to standalone methods extensions::GetChromePermissionAliases and
extensions::GetExtensionsPermissionAliases (depending on whether
aliases refer to Chrome permissions, or code extensions
permissions).

This is done in preparation of adding suppport of API aliases, in
order to be able to declare sets of aliases (API and permission
aliases) together.

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

[commit-bot: I haz the power, 2016-10-07 23:54:12]

Committed patchset #11 (id:200001)

[commit-bot: I haz the power, 2016-10-07 23:55:59]

Description was changed from

==========
Extract permission alias info from PermissionsProvider

Move the methods for getting lists of extension permission aliases
to standalone methods extensions::GetChromePermissionAliases and
extensions::GetExtensionsPermissionAliases (depending on whether
aliases refer to Chrome permissions, or code extensions
permissions).

This is done in preparation of adding suppport of API aliases, in
order to be able to declare sets of aliases (API and permission
aliases) together.

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

to

==========
Extract permission alias info from PermissionsProvider

Move the methods for getting lists of extension permission aliases
to standalone methods extensions::GetChromePermissionAliases and
extensions::GetExtensionsPermissionAliases (depending on whether
aliases refer to Chrome permissions, or code extensions
permissions).

This is done in preparation of adding suppport of API aliases, in
order to be able to declare sets of aliases (API and permission
aliases) together.

BUG=647415

Committed: https://crrev.com/6844aad556802f24338385f23a8c1aa0cf19bf3e
Cr-Commit-Position: refs/heads/master@{#424018}
==========

[commit-bot: I haz the power, 2016-10-07 23:56:00]

Patchset 11 (id:??) landed as
https://crrev.com/6844aad556802f24338385f23a8c1aa0cf19bf3e
Cr-Commit-Position: refs/heads/master@{#424018}