Added CoalescedPermissionMessages to ManifestPermissions

Added CoalescedPermissionMessages to ManifestPermissions

Added a new type, CoalescedPermissionMessage, which represents a
permission message made up of 0 or more permissions. Also started a
refactor to move IDs out of APIPermission::ID and PermissionMessage::ID
and into their own common class.

Added a new method GetPermissions() to ManifestPermission, which allows
manifest permissions to specify their own custom permissions (and hence
custom messages) for apps with that permission. Updated all 5 subclasses
(automation, bluetooth, sockets, UI overrides hanlder and the mock
manifest).

Also added another FilterHostPermissions() method to ExtensionsClient
that can create CoalescedPermissionMessages.

BUG=398257
Committed: https://crrev.com/45d827a6ba247ef55275c52ade2dfddbaeb87a9f
Cr-Commit-Position: refs/heads/master@{#307630}

Committed: https://crrev.com/7c08b85486f7bd6af38190138e522d5c278a95ba
Cr-Commit-Position: refs/heads/master@{#307677}

[sashab, 2014-11-27 06:06:19]

[email protected] changed reviewers:
+ [email protected]

[sashab, 2014-11-27 06:06:20]

This is an enormous patch, so I'm happy for just some overall feedback at first
(on the change to ManifestPermission/the new CoalescedPermissionMessage) and a
more thorough review later. Also, see my comments on structuring
CoalescedPermissionMessage.

https://codereview.chromium.org/750353003/diff/20001/extensions/common/permis...
File extensions/common/permissions/coalesced_permission_message.h (right):

https://codereview.chromium.org/750353003/diff/20001/extensions/common/permis...
extensions/common/permissions/coalesced_permission_message.h:34: class
CoalescedPermissionMessage {
The only problem I have with this class is there's no nice way of representing
host permissions. As you can see in FilterHostPermissions(), I'm representing
them with an empty PermissionIDSet. What would be better is if each host
permission was its own PermissionID and the coalescing could happen in
ChromePermissionMessageProvider (upcoming; where all the logic will be).

Maybe the right to do is to make PermissionID a class instead of an enum - it
has two fields, an enum and a string (which together are unique). The string is
empty except for a subclass, HostPermissionID, which uses the string to store
the host. So the permission IDs become like:

{ kFavicon, "" }
{ kBluetooth, "" }
{ kHost, "http://www.google.com/" }
{ kReadOnlyHost, "http://www.foobar.com/" }

Coalescing rules would apply as normal to all permissions, except there is an
extra step at the end that coalesces kHost and kReadOnlyHost permissions,
merging their messages in some clever way.

The only problem I see with this is that as soon as PermissionID isn't a simple
data type, it'll be hard to pass around/create on the fly (will require a
constructor), will be hard to store in sets and stuff. If you can think of a
good way to do this let me know, otherwise maybe what we've got right now really
is the best way (and we can just coalesce host permissions as an extra step,
e.g. have a HostPermissionMessage class that can become a
CoalescedPermissionMessage later).

EDIT: Another crazy idea, maybe we can have a subclass of
CoalescedPermissionMessage (say HostPermissionMessage) which takes a list of
hosts, and constructs a valid CoalescedPermissionMessage. We'd then need to add
something like GetHosts() to manifest permissions (which returns what you need
to construct a HostPermissionMessage). ChromePermissionMessageProvider can then
follow this logic:

1. GetPermissionIDs() for all permissions
2. Construct CoalescedPermissionMessages
3. GetHosts() for all permissions
4. Construct HostPermissionmessages from the union of all hosts

Huh. I might try this. Let me know.

[not at google - send to devlin, 2014-12-01 23:44:25]

[email protected] changed reviewers:
+ [email protected]
- [email protected]

[not at google - send to devlin, 2014-12-01 23:44:25]

Punting to Yoyo :)

[Yoyo Zhou, 2014-12-04 00:37:53]

https://chromiumcodereview.appspot.com/750353003/diff/20001/extensions/common...
File extensions/common/permissions/coalesced_permission_message.h (right):

https://chromiumcodereview.appspot.com/750353003/diff/20001/extensions/common...
extensions/common/permissions/coalesced_permission_message.h:34: class
CoalescedPermissionMessage {
On 2014/11/27 06:06:20, sasha_b wrote:
> The only problem I have with this class is there's no nice way of representing
> host permissions. As you can see in FilterHostPermissions(), I'm representing
> them with an empty PermissionIDSet. What would be better is if each host
> permission was its own PermissionID and the coalescing could happen in
> ChromePermissionMessageProvider (upcoming; where all the logic will be).
> 
> Maybe the right to do is to make PermissionID a class instead of an enum - it
> has two fields, an enum and a string (which together are unique). The string
is
> empty except for a subclass, HostPermissionID, which uses the string to store
> the host. So the permission IDs become like:
> 
> { kFavicon, "" }
> { kBluetooth, "" }
> { kHost, "http://www.google.com/" }
> { kReadOnlyHost, "http://www.foobar.com/" }
> 
> Coalescing rules would apply as normal to all permissions, except there is an
> extra step at the end that coalesces kHost and kReadOnlyHost permissions,
> merging their messages in some clever way.
> 
> The only problem I see with this is that as soon as PermissionID isn't a
simple
> data type, it'll be hard to pass around/create on the fly (will require a
> constructor), will be hard to store in sets and stuff. If you can think of a
> good way to do this let me know, otherwise maybe what we've got right now
really
> is the best way (and we can just coalesce host permissions as an extra step,
> e.g. have a HostPermissionMessage class that can become a
> CoalescedPermissionMessage later).
> 
> EDIT: Another crazy idea, maybe we can have a subclass of
> CoalescedPermissionMessage (say HostPermissionMessage) which takes a list of
> hosts, and constructs a valid CoalescedPermissionMessage. We'd then need to
add
> something like GetHosts() to manifest permissions (which returns what you need
> to construct a HostPermissionMessage). ChromePermissionMessageProvider can
then
> follow this logic:
> 
> 1. GetPermissionIDs() for all permissions
> 2. Construct CoalescedPermissionMessages
> 3. GetHosts() for all permissions
> 4. Construct HostPermissionmessages from the union of all hosts
> 
> Huh. I might try this. Let me know.

What kind of coalescing of host-ish permissions do you need to do? Anything
besides collapsing host and hostreadonly?

It's a little hard to know without being able to see the logic yet. What's the
intended use of CoalescedPermissionMessage, i.e. where do we do the coalescing,
and how many do we end up with? If we have multiple of these coalesced messages
per extension, it might make sense for the permission ID to be "kHosts" and the
submessages to be the list of hosts themselves. (I'm not sure how feasible it
would be to declaratively coalesce these.)

(I'm assuming that, for example, we might have permissions A, B, C, D, E
coalesced into the coalesced messages for {A,B,C}, D, E.)

Is the issue with my suggestion above that host permissions should be displayed
as the main permission, i.e. "Access your data on x.com and y.net", not "Access
your data on: \n - x.com \n - y.net"? Then maybe host-ish permissions should
just completely rewrite the main message when they are coalesced with each
other.

https://chromiumcodereview.appspot.com/750353003/diff/20001/extensions/common...
File extensions/common/permissions/manifest_permission.h (right):

https://chromiumcodereview.appspot.com/750353003/diff/20001/extensions/common...
extensions/common/permissions/manifest_permission.h:51: // NOTE: Do NOT store
actual permission messages here, since these messages
I find this distinction between "extra permission messages" and "actual
permission messages" confusing. For example, with sockets, all of the manifest
permission messages are in here.

[sashab, 2014-12-04 04:06:09]

Alright; looks like it worked. Take a look :)

https://codereview.chromium.org/750353003/diff/20001/extensions/common/permis...
File extensions/common/permissions/coalesced_permission_message.h (right):

https://codereview.chromium.org/750353003/diff/20001/extensions/common/permis...
extensions/common/permissions/coalesced_permission_message.h:34: class
CoalescedPermissionMessage {
On 2014/12/04 00:37:53, Yoyo Zhou wrote:
> On 2014/11/27 06:06:20, sasha_b wrote:
> > The only problem I have with this class is there's no nice way of
representing
> > host permissions. As you can see in FilterHostPermissions(), I'm
representing
> > them with an empty PermissionIDSet. What would be better is if each host
> > permission was its own PermissionID and the coalescing could happen in
> > ChromePermissionMessageProvider (upcoming; where all the logic will be).
> > 
> > Maybe the right to do is to make PermissionID a class instead of an enum -
it
> > has two fields, an enum and a string (which together are unique). The string
> is
> > empty except for a subclass, HostPermissionID, which uses the string to
store
> > the host. So the permission IDs become like:
> > 
> > { kFavicon, "" }
> > { kBluetooth, "" }
> > { kHost, "http://www.google.com/" }
> > { kReadOnlyHost, "http://www.foobar.com/" }
> > 
> > Coalescing rules would apply as normal to all permissions, except there is
an
> > extra step at the end that coalesces kHost and kReadOnlyHost permissions,
> > merging their messages in some clever way.
> > 
> > The only problem I see with this is that as soon as PermissionID isn't a
> simple
> > data type, it'll be hard to pass around/create on the fly (will require a
> > constructor), will be hard to store in sets and stuff. If you can think of a
> > good way to do this let me know, otherwise maybe what we've got right now
> really
> > is the best way (and we can just coalesce host permissions as an extra step,
> > e.g. have a HostPermissionMessage class that can become a
> > CoalescedPermissionMessage later).
> > 
> > EDIT: Another crazy idea, maybe we can have a subclass of
> > CoalescedPermissionMessage (say HostPermissionMessage) which takes a list of
> > hosts, and constructs a valid CoalescedPermissionMessage. We'd then need to
> add
> > something like GetHosts() to manifest permissions (which returns what you
need
> > to construct a HostPermissionMessage). ChromePermissionMessageProvider can
> then
> > follow this logic:
> > 
> > 1. GetPermissionIDs() for all permissions
> > 2. Construct CoalescedPermissionMessages
> > 3. GetHosts() for all permissions
> > 4. Construct HostPermissionmessages from the union of all hosts
> > 
> > Huh. I might try this. Let me know.
> 
> What kind of coalescing of host-ish permissions do you need to do? Anything
> besides collapsing host and hostreadonly?
> 
> It's a little hard to know without being able to see the logic yet. What's the
> intended use of CoalescedPermissionMessage, i.e. where do we do the
coalescing,
> and how many do we end up with? If we have multiple of these coalesced
messages
> per extension, it might make sense for the permission ID to be "kHosts" and
the
> submessages to be the list of hosts themselves. (I'm not sure how feasible it
> would be to declaratively coalesce these.)
> 
> (I'm assuming that, for example, we might have permissions A, B, C, D, E
> coalesced into the coalesced messages for {A,B,C}, D, E.)
> 
> Is the issue with my suggestion above that host permissions should be
displayed
> as the main permission, i.e. "Access your data on x.com and y.net", not
"Access
> your data on: \n - x.com \n - y.net"? Then maybe host-ish permissions should
> just completely rewrite the main message when they are coalesced with each
> other.

As per our other discussion, extended PermissionIDSet to store an additional
string (a 'detail') for the permission, as well as the ID of the permission. Now
it can support host permissions, and hence all permissions, in Chrome. :D!!

Added some convenience insert() methods so it's easier to add items to the set
than ids.insert(std::pair<PermissioniD>(foo, "bar")).

https://codereview.chromium.org/750353003/diff/20001/extensions/common/permis...
File extensions/common/permissions/manifest_permission.h (right):

https://codereview.chromium.org/750353003/diff/20001/extensions/common/permis...
extensions/common/permissions/manifest_permission.h:51: // NOTE: Do NOT store
actual permission messages here, since these messages
On 2014/12/04 00:37:53, Yoyo Zhou wrote:
> I find this distinction between "extra permission messages" and "actual
> permission messages" confusing. For example, with sockets, all of the manifest
> permission messages are in here.

The reason this was needed is because there was no way of representing host
permissions in a PermissionIDSet. Now that PermissionIDSet can store host
permissions, we can merge them into one method :)

[Yoyo Zhou, 2014-12-04 23:00:47]

I like this better. On the naming front, I'm not sure if PermissionID really
describes what you have any more... I think we've used up too many generic words
for PermissionFoo though that we might not have any better ones.

https://chromiumcodereview.appspot.com/750353003/diff/40001/extensions/common...
File extensions/common/api/sockets/sockets_manifest_permission.cc (right):

https://chromiumcodereview.appspot.com/750353003/diff/40001/extensions/common...
extensions/common/api/sockets/sockets_manifest_permission.cc:162:
PermissionIDSet SocketsManifestPermission::GetPermissions() const {
Can you make this and GetMessages share more of the implementation?

https://chromiumcodereview.appspot.com/750353003/diff/40001/extensions/common...
File extensions/common/api/sockets/sockets_manifest_permission.h (right):

https://chromiumcodereview.appspot.com/750353003/diff/40001/extensions/common...
extensions/common/api/sockets/sockets_manifest_permission.h:57: bool
AddAnyHostMessage(PermissionMessages& messages,
Add a TODO to deprecate the PermissionMessages argument?

https://chromiumcodereview.appspot.com/750353003/diff/40001/extensions/common...
File extensions/common/permissions/api_permission_set.h (right):

https://chromiumcodereview.appspot.com/750353003/diff/40001/extensions/common...
extensions/common/permissions/api_permission_set.h:81: class PermissionIDSet :
public std::set<PermissionID> {
Seems like at this point, you don't benefit from inheritance (vs. composition).

https://chromiumcodereview.appspot.com/750353003/diff/40001/extensions/common...
extensions/common/permissions/api_permission_set.h:99: void
insertPermission(APIPermission::ID permission);
nit: this should be named like insert_permission (or just insert)

https://chromiumcodereview.appspot.com/750353003/diff/40001/extensions/common...
File extensions/common/permissions/coalesced_permission_message.h (right):

https://chromiumcodereview.appspot.com/750353003/diff/40001/extensions/common...
extensions/common/permissions/coalesced_permission_message.h:26: // Some
permissions may contains extra details, stored in |submessages|. These
Do we still need submessages if PermissionIDs can contain details? (I could see
this going either way.)

https://chromiumcodereview.appspot.com/750353003/diff/40001/extensions/common...
File extensions/common/permissions/permission_message_util.cc (right):

https://chromiumcodereview.appspot.com/750353003/diff/40001/extensions/common...
extensions/common/permissions/permission_message_util.cc:113:
extensions::PermissionMessage message = CreateFromHostList(hosts, properties);
Note that if there are 3 or fewer host permissions, you won't get one per line
from the implementation above. i.e. still the caveat about the message varying
based on number of hosts.

[sashab, 2014-12-08 22:47:53]

Changes discussed in comments.

https://codereview.chromium.org/750353003/diff/40001/extensions/common/api/so...
File extensions/common/api/sockets/sockets_manifest_permission.cc (right):

https://codereview.chromium.org/750353003/diff/40001/extensions/common/api/so...
extensions/common/api/sockets/sockets_manifest_permission.cc:162:
PermissionIDSet SocketsManifestPermission::GetPermissions() const {
On 2014/12/04 23:00:47, Yoyo Zhou wrote:
> Can you make this and GetMessages share more of the implementation?

Done. Also added a unit test for the sockets permission messages since they are
complex enough they should have their own tests.

https://codereview.chromium.org/750353003/diff/40001/extensions/common/api/so...
File extensions/common/api/sockets/sockets_manifest_permission.h (right):

https://codereview.chromium.org/750353003/diff/40001/extensions/common/api/so...
extensions/common/api/sockets/sockets_manifest_permission.h:57: bool
AddAnyHostMessage(PermissionMessages& messages,
On 2014/12/04 23:00:47, Yoyo Zhou wrote:
> Add a TODO to deprecate the PermissionMessages argument?

Done.

https://codereview.chromium.org/750353003/diff/40001/extensions/common/permis...
File extensions/common/permissions/api_permission_set.h (right):

https://codereview.chromium.org/750353003/diff/40001/extensions/common/permis...
extensions/common/permissions/api_permission_set.h:81: class PermissionIDSet :
public std::set<PermissionID> {
On 2014/12/04 23:00:47, Yoyo Zhou wrote:
> Seems like at this point, you don't benefit from inheritance (vs.
composition).

Might need to go back to composition later, to get some of the convenience set
operators (union, difference, etc), but I could always expose those as helper
methods from this class. Will cross that bridge when we get to it. For now
composition sounds good :)

Also added a TODO to rename this class. In the end, we should end up with just;

APIPermission::ID
ManifestPermission
PermissionIDSet
CoalescedPermissionMessage

which we can then rename to... (all of these names are currently taken)
APIPermission::ID => PermissionID
ManifestPermission
PermissionIDSet => PermissionSet
CoalescedPermissionMessage => PermissionMessage

Then the picture will be simple: A PermissionSet contains PermissionIDs, which
are used to generate PermissionMessages.

https://codereview.chromium.org/750353003/diff/40001/extensions/common/permis...
extensions/common/permissions/api_permission_set.h:99: void
insertPermission(APIPermission::ID permission);
On 2014/12/04 23:00:47, Yoyo Zhou wrote:
> nit: this should be named like insert_permission (or just insert)

Agreed :) Actually couldn't do this before because set<> already had an insert()
method that was needed. Done.

https://codereview.chromium.org/750353003/diff/40001/extensions/common/permis...
File extensions/common/permissions/coalesced_permission_message.h (right):

https://codereview.chromium.org/750353003/diff/40001/extensions/common/permis...
extensions/common/permissions/coalesced_permission_message.h:26: // Some
permissions may contains extra details, stored in |submessages|. These
On 2014/12/04 23:00:47, Yoyo Zhou wrote:
> Do we still need submessages if PermissionIDs can contain details? (I could
see
> this going either way.)

Yes - this represents a message in the final dialog, where submessages are
nested bullets. PermisisonIDs can only have a single 'detail' string and its
more of a parameter to the permission. Updated the comments of both classes to
reflect this.

In fact, this class should really be immutable - once constructed, a
PermissionMessage is read-only. Updated the comment to say 'immutable' and made
all the member variables const.

https://codereview.chromium.org/750353003/diff/40001/extensions/common/permis...
File extensions/common/permissions/permission_message_util.cc (right):

https://codereview.chromium.org/750353003/diff/40001/extensions/common/permis...
extensions/common/permissions/permission_message_util.cc:113:
extensions::PermissionMessage message = CreateFromHostList(hosts, properties);
On 2014/12/04 23:00:47, Yoyo Zhou wrote:
> Note that if there are 3 or fewer host permissions, you won't get one per line
> from the implementation above. i.e. still the caveat about the message varying
> based on number of hosts.

Ahh, nice find. Abstracted out the logic that generates the host list into
GetHostListFromHosts(), and now both functions call that. Also had to make the
messages variables static since I need access to kNumMessages in both.

[Yoyo Zhou, 2014-12-08 23:25:21]

LGTM with nits

https://codereview.chromium.org/750353003/diff/40001/extensions/common/permis...
File extensions/common/permissions/api_permission_set.h (right):

https://codereview.chromium.org/750353003/diff/40001/extensions/common/permis...
extensions/common/permissions/api_permission_set.h:81: class PermissionIDSet :
public std::set<PermissionID> {
On 2014/12/08 22:47:52, sasha_b wrote:
> On 2014/12/04 23:00:47, Yoyo Zhou wrote:
> > Seems like at this point, you don't benefit from inheritance (vs.
> composition).
> 
> Might need to go back to composition later, to get some of the convenience set
> operators (union, difference, etc), but I could always expose those as helper
> methods from this class. Will cross that bridge when we get to it. For now
> composition sounds good :)

I forget which classes we have for permissions that already have their own
union/difference implementations that just call the underlying set
union/difference for their set member variable. There is precedent in this
regard.

> Also added a TODO to rename this class. In the end, we should end up with
just;
> 
> APIPermission::ID
> ManifestPermission
> PermissionIDSet
> CoalescedPermissionMessage
> 
> which we can then rename to... (all of these names are currently taken)
> APIPermission::ID => PermissionID
> ManifestPermission
> PermissionIDSet => PermissionSet
> CoalescedPermissionMessage => PermissionMessage
> 
> Then the picture will be simple: A PermissionSet contains PermissionIDs, which
> are used to generate PermissionMessages.

This will be nice, for sure. We have way too many of these things already. =)

https://codereview.chromium.org/750353003/diff/60001/chrome/browser/extension...
File chrome/browser/extensions/permission_message_combinations_unittest.cc
(right):

https://codereview.chromium.org/750353003/diff/60001/chrome/browser/extension...
chrome/browser/extensions/permission_message_combinations_unittest.cc:740:
ASSERT_TRUE(CheckManifestProducesHostPermissions());
How about a test case
'tcp': (any server + port number)
'tcpServer': (list of servers)
with expected result 'exchange data with any computer...'

https://codereview.chromium.org/750353003/diff/60001/extensions/common/api/so...
File extensions/common/api/sockets/sockets_manifest_permission.h (right):

https://codereview.chromium.org/750353003/diff/60001/extensions/common/api/so...
extensions/common/api/sockets/sockets_manifest_permission.h:59: // the
GetAllHostMessages() function (move all the logic into GetPermissions()
There isn't a GetAllHostMessages.

https://codereview.chromium.org/750353003/diff/60001/extensions/common/permis...
File extensions/common/permissions/api_permission_set.h (right):

https://codereview.chromium.org/750353003/diff/60001/extensions/common/permis...
extensions/common/permissions/api_permission_set.h:83: // TODO(sashab): Move
this to it's own file and rename it to PermissionSet after
nit: its

https://codereview.chromium.org/750353003/diff/60001/extensions/common/permis...
File extensions/common/permissions/coalesced_permission_message.h (right):

https://codereview.chromium.org/750353003/diff/60001/extensions/common/permis...
extensions/common/permissions/coalesced_permission_message.h:20: // bullet in
the list of an app or extension's permissions. It contains the
This is a great description.

https://codereview.chromium.org/750353003/diff/60001/extensions/common/permis...
extensions/common/permissions/coalesced_permission_message.h:29: // the ones
'contained' in this message - if this set is taken is called for all
There are too many 'is' here, something is amiss with this sentence.

https://codereview.chromium.org/750353003/diff/60001/extensions/common/permis...
File extensions/common/permissions/permission_message_util.cc (right):

https://codereview.chromium.org/750353003/diff/60001/extensions/common/permis...
extensions/common/permissions/permission_message_util.cc:43: static const
MsgPair kReadWriteMessagesList[] = {
Be careful that this doesn't introduce new static initializers.

[sashab, 2014-12-09 22:18:42]

Patchset #8 (id:140001) has been deleted

[sashab, 2014-12-09 22:18:47]

Patchset #7 (id:120001) has been deleted

[sashab, 2014-12-09 22:18:54]

Patchset #6 (id:100001) has been deleted

[sashab, 2014-12-10 02:25:06]

The CQ bit was checked by [email protected]

[commit-bot: I haz the power, 2014-12-10 02:25:41]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/750353003/160001

[commit-bot: I haz the power, 2014-12-10 03:12:36]

Committed patchset #6 (id:160001)

[commit-bot: I haz the power, 2014-12-10 03:14:26]

Patchset 6 (id:??) landed as
https://crrev.com/45d827a6ba247ef55275c52ade2dfddbaeb87a9f
Cr-Commit-Position: refs/heads/master@{#307630}

[sashab, 2014-12-10 08:24:14]

https://codereview.chromium.org/750353003/diff/60001/chrome/browser/extension...
File chrome/browser/extensions/permission_message_combinations_unittest.cc
(right):

https://codereview.chromium.org/750353003/diff/60001/chrome/browser/extension...
chrome/browser/extensions/permission_message_combinations_unittest.cc:740:
ASSERT_TRUE(CheckManifestProducesHostPermissions());
On 2014/12/08 23:25:21, Yoyo Zhou wrote:
> How about a test case
> 'tcp': (any server + port number)
> 'tcpServer': (list of servers)
> with expected result 'exchange data with any computer...'

Good idea. Done.

https://codereview.chromium.org/750353003/diff/60001/extensions/common/api/so...
File extensions/common/api/sockets/sockets_manifest_permission.h (right):

https://codereview.chromium.org/750353003/diff/60001/extensions/common/api/so...
extensions/common/api/sockets/sockets_manifest_permission.h:59: // the
GetAllHostMessages() function (move all the logic into GetPermissions()
On 2014/12/08 23:25:21, Yoyo Zhou wrote:
> There isn't a GetAllHostMessages.

Oops, meant AddAllHostMessages. Done.

https://codereview.chromium.org/750353003/diff/60001/extensions/common/permis...
File extensions/common/permissions/api_permission_set.h (right):

https://codereview.chromium.org/750353003/diff/60001/extensions/common/permis...
extensions/common/permissions/api_permission_set.h:83: // TODO(sashab): Move
this to it's own file and rename it to PermissionSet after
On 2014/12/08 23:25:21, Yoyo Zhou wrote:
> nit: its

Haha nice find :) Done.

https://codereview.chromium.org/750353003/diff/60001/extensions/common/permis...
File extensions/common/permissions/coalesced_permission_message.h (right):

https://codereview.chromium.org/750353003/diff/60001/extensions/common/permis...
extensions/common/permissions/coalesced_permission_message.h:20: // bullet in
the list of an app or extension's permissions. It contains the
On 2014/12/08 23:25:21, Yoyo Zhou wrote:
> This is a great description.

Thanks :^)

https://codereview.chromium.org/750353003/diff/60001/extensions/common/permis...
extensions/common/permissions/coalesced_permission_message.h:29: // the ones
'contained' in this message - if this set is taken is called for all
On 2014/12/08 23:25:21, Yoyo Zhou wrote:
> There are too many 'is' here, something is amiss with this sentence.

Ah, nice, fixed. Also changed PermissionIDSet to |permissions| to be more
specific in its reference.

[jochen (gone - plz use gerrit), 2014-12-10 08:58:17]

A revert of this CL (patchset #6 id:160001) has been created in
https://codereview.chromium.org/793633003/ by [email protected].

The reason for reverting is: Static initializers in permission message utils.

[sashab, 2014-12-10 09:08:37]

The CQ bit was checked by [email protected]

[commit-bot: I haz the power, 2014-12-10 09:09:23]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/750353003/180001

[commit-bot: I haz the power, 2014-12-10 10:54:56]

Committed patchset #7 (id:180001)

[commit-bot: I haz the power, 2014-12-10 10:55:43]

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