Permission messages: Add a bunch of missing combination/suppression rules.

Permission messages: Add a bunch of missing combinations/suppressions.

BUG=398257
Committed: https://crrev.com/31aa6eb3acf011688ca971b14b74caf289f2a8fb
Cr-Commit-Position: refs/heads/master@{#347365}

[Marc Treib, 2015-09-01 15:03:07]

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

[Marc Treib, 2015-09-01 15:03:07]

Now fixing all the TODOs introduced in the other CL!

[Marc Treib, 2015-09-01 16:07:14]

Per your comment on the other CL, I've removed the _AND_SESSIONS warnings for
kProcesses and kWebNavigation again -> PS2. Thanks!

[Devlin, 2015-09-01 16:08:51]

https://codereview.chromium.org/1314753004/diff/1/chrome/common/extensions/pe...
File chrome/common/extensions/permissions/chrome_permission_message_rules.cc
(right):

https://codereview.chromium.org/1314753004/diff/1/chrome/common/extensions/pe...
chrome/common/extensions/permissions/chrome_permission_message_rules.cc:326:
PermissionIDSetInitializer(APIPermission::ID a,
This is getting a little nuts here.  Looking briefly at our style guide, I don't
see anything banning variadic functions (did I miss it?).  Using one variadic
function would make this muuuuuch cleaner...

https://codereview.chromium.org/1314753004/diff/1/chrome/common/extensions/pe...
chrome/common/extensions/permissions/chrome_permission_message_rules.cc:479:
{APIPermission::kProcesses, APIPermission::kSessions},
See comment on the other CL - I think I was wrong about these.

[Devlin, 2015-09-01 16:09:53]

https://codereview.chromium.org/1314753004/diff/1/chrome/common/extensions/pe...
File chrome/common/extensions/permissions/chrome_permission_message_rules.cc
(right):

https://codereview.chromium.org/1314753004/diff/1/chrome/common/extensions/pe...
chrome/common/extensions/permissions/chrome_permission_message_rules.cc:479:
{APIPermission::kProcesses, APIPermission::kSessions},
On 2015/09/01 16:08:51, Devlin wrote:
> See comment on the other CL - I think I was wrong about these.

Aaaaand, nevermind. :)

[Marc Treib, 2015-09-01 16:25:50]

https://codereview.chromium.org/1314753004/diff/1/chrome/common/extensions/pe...
File chrome/common/extensions/permissions/chrome_permission_message_rules.cc
(right):

https://codereview.chromium.org/1314753004/diff/1/chrome/common/extensions/pe...
chrome/common/extensions/permissions/chrome_permission_message_rules.cc:326:
PermissionIDSetInitializer(APIPermission::ID a,
On 2015/09/01 16:08:51, Devlin wrote:
> This is getting a little nuts here.  Looking briefly at our style guide, I
don't
> see anything banning variadic functions (did I miss it?).  Using one variadic
> function would make this muuuuuch cleaner...

Hm, seems you're right, I also can't find anything about them in the style
guide. OTOH, I haven't seen a variadic function used in Chrome (or anywhere
else, really) except for printf-style functions. I also don't think I've ever
written one.
I actually think the current mass of ctors is easier to parse than a variadic
function (if only because those are so uncommon). I guess the "correct" solution
would be an std::initializer_list, but those aren't allowed yet. A variadic
template might also work, but will also have readability issues.

Would it be acceptable for you to just put in something like
// TODO(treib): Replace all these ctors by a single initializer_list version
when we have those.
for now?

[Devlin, 2015-09-01 16:33:44]

https://codereview.chromium.org/1314753004/diff/1/chrome/common/extensions/pe...
File chrome/common/extensions/permissions/chrome_permission_message_rules.cc
(right):

https://codereview.chromium.org/1314753004/diff/1/chrome/common/extensions/pe...
chrome/common/extensions/permissions/chrome_permission_message_rules.cc:326:
PermissionIDSetInitializer(APIPermission::ID a,
On 2015/09/01 16:25:50, Marc Treib wrote:
> On 2015/09/01 16:08:51, Devlin wrote:
> > This is getting a little nuts here.  Looking briefly at our style guide, I
> don't
> > see anything banning variadic functions (did I miss it?).  Using one
variadic
> > function would make this muuuuuch cleaner...
> 
> Hm, seems you're right, I also can't find anything about them in the style
> guide. OTOH, I haven't seen a variadic function used in Chrome (or anywhere
> else, really) except for printf-style functions. I also don't think I've ever
> written one.
> I actually think the current mass of ctors is easier to parse than a variadic
> function (if only because those are so uncommon). I guess the "correct"
solution
> would be an std::initializer_list, but those aren't allowed yet. A variadic
> template might also work, but will also have readability issues.
> 
> Would it be acceptable for you to just put in something like
> // TODO(treib): Replace all these ctors by a single initializer_list version
> when we have those.
> for now?

Eh, variadic templates aren't *that* hard to parse, especially when the argument
list is all one type.  And another approach, which would be just as good, is to
pass in a std::set or std::vector of ids which can be brace-initialized, so that
you can still have one constructor, and do PermissionIdSetInitializer({id1, id2,
...idn});, but I think that vector brace initialization is still verboten as
part of uniform initialization (do you know for sure?).

But, in any case, this is mostly just an eyesore, not a safety or performance
concern, so as long as I stop looking at it, we'll be okay. :)

[Marc Treib, 2015-09-02 10:43:56]

https://codereview.chromium.org/1314753004/diff/1/chrome/common/extensions/pe...
File chrome/common/extensions/permissions/chrome_permission_message_rules.cc
(right):

https://codereview.chromium.org/1314753004/diff/1/chrome/common/extensions/pe...
chrome/common/extensions/permissions/chrome_permission_message_rules.cc:326:
PermissionIDSetInitializer(APIPermission::ID a,
On 2015/09/01 16:33:43, Devlin wrote:
> On 2015/09/01 16:25:50, Marc Treib wrote:
> > On 2015/09/01 16:08:51, Devlin wrote:
> > > This is getting a little nuts here.  Looking briefly at our style guide, I
> > don't
> > > see anything banning variadic functions (did I miss it?).  Using one
> variadic
> > > function would make this muuuuuch cleaner...
> > 
> > Hm, seems you're right, I also can't find anything about them in the style
> > guide. OTOH, I haven't seen a variadic function used in Chrome (or anywhere
> > else, really) except for printf-style functions. I also don't think I've
ever
> > written one.
> > I actually think the current mass of ctors is easier to parse than a
variadic
> > function (if only because those are so uncommon). I guess the "correct"
> solution
> > would be an std::initializer_list, but those aren't allowed yet. A variadic
> > template might also work, but will also have readability issues.
> > 
> > Would it be acceptable for you to just put in something like
> > // TODO(treib): Replace all these ctors by a single initializer_list version
> > when we have those.
> > for now?
> 
> Eh, variadic templates aren't *that* hard to parse, especially when the
argument
> list is all one type.  And another approach, which would be just as good, is
to
> pass in a std::set or std::vector of ids which can be brace-initialized, so
that
> you can still have one constructor, and do PermissionIdSetInitializer({id1,
id2,
> ...idn});, but I think that vector brace initialization is still verboten as
> part of uniform initialization (do you know for sure?).
> 
> But, in any case, this is mostly just an eyesore, not a safety or performance
> concern, so as long as I stop looking at it, we'll be okay. :)

Yup, the uniform initialization of set/vector is based on std::initializer_list
which we don't have yet.

But, indeed, variadic templates are less weird-looking than I would've thought.
So I'm now the proud author of my very first variadic template :)

https://codereview.chromium.org/1314753004/diff/40001/chrome/common/extension...
File chrome/common/extensions/permissions/chrome_permission_message_rules.cc
(right):

https://codereview.chromium.org/1314753004/diff/40001/chrome/common/extension...
chrome/common/extensions/permissions/chrome_permission_message_rules.cc:268:
void ExpandHelper(Args&&...) {}
This is the one slightly yucky part - we need this empty wrapper for the
parameter expansion above.

[Devlin, 2015-09-03 16:08:28]

LGTM

On 2015/09/02 10:43:56, Marc Treib wrote:
> But, indeed, variadic templates are less weird-looking than I would've
thought.
> So I'm now the proud author of my very first variadic template :)
> 

Parameter packing FTW.

That said, we should still add a very brief comment explaining what, exactly,
that function is doing (even though it should be pretty obvious, ellipses freak
people [myself included] out). :)

>
https://codereview.chromium.org/1314753004/diff/40001/chrome/common/extension...
> File chrome/common/extensions/permissions/chrome_permission_message_rules.cc
> (right):
> 
>
https://codereview.chromium.org/1314753004/diff/40001/chrome/common/extension...
> chrome/common/extensions/permissions/chrome_permission_message_rules.cc:268:
> void ExpandHelper(Args&&...) {}
> This is the one slightly yucky part - we need this empty wrapper for the
> parameter expansion above.

[Marc Treib, 2015-09-03 16:16:54]

On 2015/09/03 16:08:28, Devlin wrote:
> LGTM
> 
> On 2015/09/02 10:43:56, Marc Treib wrote:
> > But, indeed, variadic templates are less weird-looking than I would've
> thought.
> > So I'm now the proud author of my very first variadic template :)
> > 
> 
> Parameter packing FTW.
> 
> That said, we should still add a very brief comment explaining what, exactly,
> that function is doing (even though it should be pretty obvious, ellipses
freak
> people [myself included] out). :)
> 

Added "This effectively calls insert() with each of the ids." above the
"ExpandHelper(insert(ids)...)".
Is that what you meant?

> >
>
https://codereview.chromium.org/1314753004/diff/40001/chrome/common/extension...
> > File chrome/common/extensions/permissions/chrome_permission_message_rules.cc
> > (right):
> > 
> >
>
https://codereview.chromium.org/1314753004/diff/40001/chrome/common/extension...
> > chrome/common/extensions/permissions/chrome_permission_message_rules.cc:268:
> > void ExpandHelper(Args&&...) {}
> > This is the one slightly yucky part - we need this empty wrapper for the
> > parameter expansion above.

[Devlin, 2015-09-03 16:41:13]

On 2015/09/03 16:16:54, Marc Treib wrote:
> On 2015/09/03 16:08:28, Devlin wrote:
> > LGTM
> > 
> > On 2015/09/02 10:43:56, Marc Treib wrote:
> > > But, indeed, variadic templates are less weird-looking than I would've
> > thought.
> > > So I'm now the proud author of my very first variadic template :)
> > > 
> > 
> > Parameter packing FTW.
> > 
> > That said, we should still add a very brief comment explaining what,
exactly,
> > that function is doing (even though it should be pretty obvious, ellipses
> freak
> > people [myself included] out). :)
> > 
> 
> Added "This effectively calls insert() with each of the ids." above the
> "ExpandHelper(insert(ids)...)".
> Is that what you meant?

Yep, that'll do.

[Marc Treib, 2015-09-04 07:35:09]

The CQ bit was checked by treib@chromium.org

[Marc Treib, 2015-09-04 07:35:09]

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/1314753004/#ps80001
(title: "rebase")

[commit-bot: I haz the power, 2015-09-04 07:35:22]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1314753004/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1314753004/80001

[commit-bot: I haz the power, 2015-09-04 09:02:11]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-09-04 09:02:43]

Patchset 5 (id:??) landed as
https://crrev.com/31aa6eb3acf011688ca971b14b74caf289f2a8fb
Cr-Commit-Position: refs/heads/master@{#347365}