Extensions: Switch to new permission message system, part II

Extensions: Switch to new permission message system, part II

- Update some remaining tests to check both legacy and new system
- Add some missing coalescing rules in the new system

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

[Marc Treib, 2015-04-02 11:07:23]

treib@chromium.org changed reviewers:
+ yoz@chromium.org

[Marc Treib, 2015-04-02 11:07:23]

treib@chromium.org changed reviewers:
+ yoz@chromium.org

[Marc Treib, 2015-04-02 11:07:23]

Sending this out on the off chance you still have some time to look at it ;)
Maybe more realistically, can you recommend someone else to take over reviewing
this? Thanks!

[Marc Treib, 2015-04-02 11:07:24]

Sending this out on the off chance you still have some time to look at it ;)
Maybe more realistically, can you recommend someone else to take over reviewing
this? Thanks!

[Yoyo Zhou, 2015-04-02 17:46:46]

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

[Yoyo Zhou, 2015-04-02 17:46:47]

Maybe Devlin would be a good reviewer? I'm not sure who has looked at
permissions code recently. I don't expect to get to this one, unfortunately.

[Devlin, 2015-04-02 21:00:24]

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

https://codereview.chromium.org/1042793003/diff/1/chrome/common/extensions/pe...
chrome/common/extensions/permissions/permission_set_unittest.cc:792:
PermissionIDSet MakePermissionIDSet(APIPermission::ID id1) {
Can you move all of these to the anonymous namespace above?

https://codereview.chromium.org/1042793003/diff/1/chrome/common/extensions/pe...
chrome/common/extensions/permissions/permission_set_unittest.cc:814: if
(ids.empty())
nit: JoinString just returns "" if there are no strings, so you don't need the
if (empty) here or below.

https://codereview.chromium.org/1042793003/diff/1/chrome/common/extensions/pe...
chrome/common/extensions/permissions/permission_set_unittest.cc:819: return "[ "
+ JoinString(strs, ", ") + " ]";
nit: please use base::StringPrintf() (as a rule of thumb, any time it needs more
than one '+')

https://codereview.chromium.org/1042793003/diff/1/chrome/common/extensions/pe...
chrome/common/extensions/permissions/permission_set_unittest.cc:839:
testing::AssertionResult CheckPermissionIDs(
Add function comments.

https://codereview.chromium.org/1042793003/diff/1/chrome/common/extensions/pe...
chrome/common/extensions/permissions/permission_set_unittest.cc:839:
testing::AssertionResult CheckPermissionIDs(
This really only pertains to situations with Hidden/Suppressed permissions,
right? (otherwise we'd need to handle multiple permission messages.)  Should the
name of the function reflect as much?

https://codereview.chromium.org/1042793003/diff/1/chrome/common/extensions/pe...
chrome/common/extensions/permissions/permission_set_unittest.cc:849: <<
"Expected single legacy permission ID " << expected_legacy_id
indentation off? (Also at other returns)

https://codereview.chromium.org/1042793003/diff/1/chrome/common/extensions/pe...
chrome/common/extensions/permissions/permission_set_unittest.cc:853: const
PermissionMessageProvider* provider = PermissionMessageProvider::Get();
Cache this above, since you also use it on line 845

https://codereview.chromium.org/1042793003/diff/1/extensions/common/permissio...
File extensions/common/permissions/api_permission_set.h (right):

https://codereview.chromium.org/1042793003/diff/1/extensions/common/permissio...
extensions/common/permissions/api_permission_set.h:114: using const_iterator =
std::set<PermissionID>::const_iterator;
Makes me wonder if this can/should inherit from BaseSetOperators.  Ah well -
that's out of this CL's scope.

[Marc Treib, 2015-04-07 11:34:47]

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

https://codereview.chromium.org/1042793003/diff/1/chrome/common/extensions/pe...
chrome/common/extensions/permissions/permission_set_unittest.cc:792:
PermissionIDSet MakePermissionIDSet(APIPermission::ID id1) {
On 2015/04/02 21:00:23, Devlin wrote:
> Can you move all of these to the anonymous namespace above?

Done.

https://codereview.chromium.org/1042793003/diff/1/chrome/common/extensions/pe...
chrome/common/extensions/permissions/permission_set_unittest.cc:814: if
(ids.empty())
On 2015/04/02 21:00:23, Devlin wrote:
> nit: JoinString just returns "" if there are no strings, so you don't need the
> if (empty) here or below.

Okay, removed. We'll get some extra spaces in the brackets this way, but I guess
that doesn't really matter.

https://codereview.chromium.org/1042793003/diff/1/chrome/common/extensions/pe...
chrome/common/extensions/permissions/permission_set_unittest.cc:819: return "[ "
+ JoinString(strs, ", ") + " ]";
On 2015/04/02 21:00:23, Devlin wrote:
> nit: please use base::StringPrintf() (as a rule of thumb, any time it needs
more
> than one '+')

Done.

https://codereview.chromium.org/1042793003/diff/1/chrome/common/extensions/pe...
chrome/common/extensions/permissions/permission_set_unittest.cc:839:
testing::AssertionResult CheckPermissionIDs(
On 2015/04/02 21:00:23, Devlin wrote:
> This really only pertains to situations with Hidden/Suppressed permissions,
> right? (otherwise we'd need to handle multiple permission messages.)  Should
the
> name of the function reflect as much?

I've renamed it to PermissionSetProducesMessage. Open for better suggestions :)

https://codereview.chromium.org/1042793003/diff/1/chrome/common/extensions/pe...
chrome/common/extensions/permissions/permission_set_unittest.cc:839:
testing::AssertionResult CheckPermissionIDs(
On 2015/04/02 21:00:23, Devlin wrote:
> Add function comments.

Done.

https://codereview.chromium.org/1042793003/diff/1/chrome/common/extensions/pe...
chrome/common/extensions/permissions/permission_set_unittest.cc:849: <<
"Expected single legacy permission ID " << expected_legacy_id
On 2015/04/02 21:00:23, Devlin wrote:
> indentation off? (Also at other returns)

This is what "git cl format" produced (weird though it looks), and presubmit
even complains if I format it differently.

https://codereview.chromium.org/1042793003/diff/1/chrome/common/extensions/pe...
chrome/common/extensions/permissions/permission_set_unittest.cc:853: const
PermissionMessageProvider* provider = PermissionMessageProvider::Get();
On 2015/04/02 21:00:23, Devlin wrote:
> Cache this above, since you also use it on line 845

Done.

https://codereview.chromium.org/1042793003/diff/1/extensions/common/permissio...
File extensions/common/permissions/api_permission_set.h (right):

https://codereview.chromium.org/1042793003/diff/1/extensions/common/permissio...
extensions/common/permissions/api_permission_set.h:114: using const_iterator =
std::set<PermissionID>::const_iterator;
On 2015/04/02 21:00:23, Devlin wrote:
> Makes me wonder if this can/should inherit from BaseSetOperators.  Ah well -
> that's out of this CL's scope.

It probably should (eventually), see the TODO above.

[Devlin, 2015-04-07 15:55:56]

lgtm

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

https://codereview.chromium.org/1042793003/diff/1/chrome/common/extensions/pe...
chrome/common/extensions/permissions/permission_set_unittest.cc:849: <<
"Expected single legacy permission ID " << expected_legacy_id
On 2015/04/07 11:34:47, Marc Treib wrote:
> On 2015/04/02 21:00:23, Devlin wrote:
> > indentation off? (Also at other returns)
> 
> This is what "git cl format" produced (weird though it looks), and presubmit
> even complains if I format it differently.

:_( Why 8-space indentation?  Grrr.... Well, if git cl format does it.

https://codereview.chromium.org/1042793003/diff/1/extensions/common/permissio...
File extensions/common/permissions/api_permission_set.h (right):

https://codereview.chromium.org/1042793003/diff/1/extensions/common/permissio...
extensions/common/permissions/api_permission_set.h:114: using const_iterator =
std::set<PermissionID>::const_iterator;
On 2015/04/07 11:34:47, Marc Treib wrote:
> On 2015/04/02 21:00:23, Devlin wrote:
> > Makes me wonder if this can/should inherit from BaseSetOperators.  Ah well -
> > that's out of this CL's scope.
> 
> It probably should (eventually), see the TODO above.

Haha oh, look at that. :)

https://codereview.chromium.org/1042793003/diff/20001/extensions/common/permi...
File extensions/common/permissions/api_permission_set.h (right):

https://codereview.chromium.org/1042793003/diff/20001/extensions/common/permi...
extensions/common/permissions/api_permission_set.h:152:
PermissionIDSet(std::set<PermissionID> permissions);
Not your doing, but can you make this const std::set<PermissionID>&?

[Marc Treib, 2015-04-07 16:15:55]

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

https://codereview.chromium.org/1042793003/diff/1/chrome/common/extensions/pe...
chrome/common/extensions/permissions/permission_set_unittest.cc:849: <<
"Expected single legacy permission ID " << expected_legacy_id
On 2015/04/07 15:55:56, Devlin wrote:
> On 2015/04/07 11:34:47, Marc Treib wrote:
> > On 2015/04/02 21:00:23, Devlin wrote:
> > > indentation off? (Also at other returns)
> > 
> > This is what "git cl format" produced (weird though it looks), and presubmit
> > even complains if I format it differently.
> 
> :_( Why 8-space indentation?  Grrr.... Well, if git cl format does it.

I think there's some special case thingie to line stuff up after the "return"...

https://codereview.chromium.org/1042793003/diff/20001/extensions/common/permi...
File extensions/common/permissions/api_permission_set.h (right):

https://codereview.chromium.org/1042793003/diff/20001/extensions/common/permi...
extensions/common/permissions/api_permission_set.h:152:
PermissionIDSet(std::set<PermissionID> permissions);
On 2015/04/07 15:55:56, Devlin wrote:
> Not your doing, but can you make this const std::set<PermissionID>&?

Done. (Good catch!)

[Marc Treib, 2015-04-07 16:17:32]

The CQ bit was checked by treib@chromium.org

[Marc Treib, 2015-04-07 16:17:33]

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/1042793003/#ps40001
(title: "nit")

[commit-bot: I haz the power, 2015-04-07 16:18:13]

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

[commit-bot: I haz the power, 2015-04-07 17:58:04]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-04-07 17:59:45]

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