Added PermissionIDSet to APIPermissions

Added PermissionIDSet to APIPermissions

Added a GetPermissions() method to APIPermissions, which returns the
PermissionIDSet for that API permission. This will be used for the new
permission messages system, which can generate
CoalescedPermissionMessages from these permissions.

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

[sashab, 2014-12-10 08:53:27]

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

[sashab, 2014-12-10 08:57:49]

Hi yoz; please review. I plan on:
- Removing the empty CoalescedPermissionMessage() constructor (it's not needed)
- Adding tests for all manifest and API permissions that produce weird
messages... We really should have as many tests for these cases as we can.

https://codereview.chromium.org/795543002/diff/1/extensions/common/permission...
File extensions/common/permissions/api_permission.h (right):

https://codereview.chromium.org/795543002/diff/1/extensions/common/permission...
extensions/common/permissions/api_permission.h:256: virtual PermissionIDSet
GetPermissions() const = 0;
This is the main change in this patch.

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

https://codereview.chromium.org/795543002/diff/1/extensions/common/permission...
extensions/common/permissions/api_permission_set.h:84: const base::string16&
parameter() const { return this->second; }
The readability of id() and parameter() is worth it. first and second just
aren't descriptive enough :)

https://codereview.chromium.org/795543002/diff/1/extensions/common/permission...
extensions/common/permissions/api_permission_set.h:138: APIPermission::ID
permission_six);
These are needed in a later patch :-)

https://codereview.chromium.org/795543002/diff/1/extensions/common/permission...
File extensions/common/permissions/coalesced_permission_message.h (right):

https://codereview.chromium.org/795543002/diff/1/extensions/common/permission...
extensions/common/permissions/coalesced_permission_message.h:43:
CoalescedPermissionMessage();
Not sure if we should include this... Technically, there should be no such thing
as an empty permission message. Will remove in a follow-up patch.

[sashab, 2014-12-10 10:10:19]

Done; ptal :)

[sashab, 2014-12-10 23:19:51]

Patchset #3 (id:40001) has been deleted

[sashab, 2014-12-10 23:19:56]

Patchset #2 (id:20001) has been deleted

[Yoyo Zhou, 2014-12-11 00:54:11]

LGTM with nits

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

https://codereview.chromium.org/795543002/diff/1/extensions/common/permission...
extensions/common/permissions/api_permission_set.h:84: const base::string16&
parameter() const { return this->second; }
On 2014/12/10 08:57:49, sasha_b wrote:
> The readability of id() and parameter() is worth it. first and second just
> aren't descriptive enough :)

Agreed. I always hated that about std::pair.

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

https://codereview.chromium.org/795543002/diff/20002/chrome/browser/extension...
chrome/browser/extensions/permission_message_combinations_unittest.cc:1050:
"unknown devices from Immanuel Electronics Co., Ltd",
These are a little unorthodox for "host permissions". Might be worth a comment
on why ProducesHostPermissions is how we get at them.

https://codereview.chromium.org/795543002/diff/20002/extensions/common/permis...
File extensions/common/permissions/api_permission.h (right):

https://codereview.chromium.org/795543002/diff/20002/extensions/common/permis...
extensions/common/permissions/api_permission.h:253: // The set of permissions
this API permission has. These permissions are used
I would appreciate more explanation here to clarify API permission vs.
Permission IDs, like: For example, a simple API permission will have x. A
complex API permission will have y. Host permissions will have z.

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

https://codereview.chromium.org/795543002/diff/20002/extensions/common/permis...
extensions/common/permissions/api_permission_set.h:67: // permissions, this is
all.
nit: "this is all" is easy to misread. Maybe "there is nothing more"?

https://codereview.chromium.org/795543002/diff/20002/extensions/common/permis...
extensions/common/permissions/api_permission_set.h:143: base::string16
permission_parameter);
nit: Why rename this from detail? "detail" still appears in the comments for
this class/function, so make them consistent.

https://codereview.chromium.org/795543002/diff/20002/extensions/common/permis...
File extensions/common/permissions/media_galleries_permission.h (right):

https://codereview.chromium.org/795543002/diff/20002/extensions/common/permis...
extensions/common/permissions/media_galleries_permission.h:57: // TODO(sashab):
Once GetMessages() is deprecated, move this logic back into
nit: seems like this TODO should go in the implementation. The implementation
could also be an anonymous namespace function.

https://codereview.chromium.org/795543002/diff/20002/extensions/common/permis...
File extensions/common/permissions/socket_permission.cc (right):

https://codereview.chromium.org/795543002/diff/20002/extensions/common/permis...
extensions/common/permissions/socket_permission.cc:41: // TODO(rpaquay): This
function and callees is (almost) a copy/paste from
Do you have thoughts on unifying these? I really get a sense of deja vu here.

https://codereview.chromium.org/795543002/diff/20002/extensions/common/permis...
File extensions/common/permissions/usb_device_permission.h (right):

https://codereview.chromium.org/795543002/diff/20002/extensions/common/permis...
extensions/common/permissions/usb_device_permission.h:39: void
AddPermissionsToLists(PermissionIDSet& ids,
Similarly, this could be an anonymous namespace function and not appear in the
header.

[sashab, 2014-12-11 04:15:37]

Thanks for your feedback on this - updated comments and removed files from
headers where noted. Also started work on removing the duplicated code from
SocketsPermission and SocketsManifestPermission in crrev.com/794063002.

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

https://codereview.chromium.org/795543002/diff/20002/chrome/browser/extension...
chrome/browser/extensions/permission_message_combinations_unittest.cc:1050:
"unknown devices from Immanuel Electronics Co., Ltd",
On 2014/12/11 00:54:11, Yoyo Zhou wrote:
> These are a little unorthodox for "host permissions". Might be worth a comment
> on why ProducesHostPermissions is how we get at them.

Hm; this is a good point. Technically, we should be checking if the given
details are in for a particular permission message, since with USB devices you
can have more than 1 'host' permissions.

Added a todo to enhance the test system in this file to better handle this. It's
not needed right now since no tests produce more than 1 list of host
permissions, so added a todo to add a test for that as well :)

https://codereview.chromium.org/795543002/diff/20002/extensions/common/permis...
File extensions/common/permissions/api_permission.h (right):

https://codereview.chromium.org/795543002/diff/20002/extensions/common/permis...
extensions/common/permissions/api_permission.h:253: // The set of permissions
this API permission has. These permissions are used
On 2014/12/11 00:54:11, Yoyo Zhou wrote:
> I would appreciate more explanation here to clarify API permission vs.
> Permission IDs, like: For example, a simple API permission will have x. A
> complex API permission will have y. Host permissions will have z.

Good point, and the main thing to get across here is that the messages are
generated in PermissionMessageProvider. Done.

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

https://codereview.chromium.org/795543002/diff/20002/extensions/common/permis...
extensions/common/permissions/api_permission_set.h:67: // permissions, this is
all.
On 2014/12/11 00:54:11, Yoyo Zhou wrote:
> nit: "this is all" is easy to misread. Maybe "there is nothing more"?

Yeah... Couldn't think of a better way to say this. Changed to 'For most
permissions, this is all they have.'

https://codereview.chromium.org/795543002/diff/20002/extensions/common/permis...
extensions/common/permissions/api_permission_set.h:143: base::string16
permission_parameter);
On 2014/12/11 00:54:11, Yoyo Zhou wrote:
> nit: Why rename this from detail? "detail" still appears in the comments for
> this class/function, so make them consistent.

'parameter' now matches the name in PermissionID, which is exactly what its used
for. Changed the comments; good find.

https://codereview.chromium.org/795543002/diff/20002/extensions/common/permis...
File extensions/common/permissions/media_galleries_permission.h (right):

https://codereview.chromium.org/795543002/diff/20002/extensions/common/permis...
extensions/common/permissions/media_galleries_permission.h:57: // TODO(sashab):
Once GetMessages() is deprecated, move this logic back into
On 2014/12/11 00:54:11, Yoyo Zhou wrote:
> nit: seems like this TODO should go in the implementation. The implementation
> could also be an anonymous namespace function.

Done. Also renamed to AddPermissionsToLists() and shortened the parameter names
to match USBDevicePermission.

https://codereview.chromium.org/795543002/diff/20002/extensions/common/permis...
File extensions/common/permissions/socket_permission.cc (right):

https://codereview.chromium.org/795543002/diff/20002/extensions/common/permis...
extensions/common/permissions/socket_permission.cc:41: // TODO(rpaquay): This
function and callees is (almost) a copy/paste from
On 2014/12/11 00:54:11, Yoyo Zhou wrote:
> Do you have thoughts on unifying these? I really get a sense of deja vu here.

I have some ideas; let's do it in a follow-up patch :)

https://codereview.chromium.org/795543002/diff/20002/extensions/common/permis...
File extensions/common/permissions/usb_device_permission.h (right):

https://codereview.chromium.org/795543002/diff/20002/extensions/common/permis...
extensions/common/permissions/usb_device_permission.h:39: void
AddPermissionsToLists(PermissionIDSet& ids,
On 2014/12/11 00:54:11, Yoyo Zhou wrote:
> Similarly, this could be an anonymous namespace function and not appear in the
> header.

Done. Also made it slightly more efficient by taking pointers instead of
references, so the callsites can pass NULL if they don't want a particular type
of message, like in MediaGalleriesPermission.

[sashab, 2014-12-11 04:15:48]

The CQ bit was checked by sashab@chromium.org

[commit-bot: I haz the power, 2014-12-11 04:16:15]

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

[commit-bot: I haz the power, 2014-12-11 05:06:43]

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

[commit-bot: I haz the power, 2014-12-11 05:06:44]

Try jobs failed on following builders:
  win_chromium_x64_rel_ng on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[sashab, 2014-12-11 05:33:51]

The CQ bit was checked by sashab@chromium.org

[commit-bot: I haz the power, 2014-12-11 05:34:31]

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

[commit-bot: I haz the power, 2014-12-11 06:23:55]

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

[commit-bot: I haz the power, 2014-12-11 06:23:57]

Try jobs failed on following builders:
  linux_chromium_rel_ng on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[sashab, 2014-12-11 06:41:45]

The CQ bit was checked by sashab@chromium.org

[commit-bot: I haz the power, 2014-12-11 06:42:47]

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

[commit-bot: I haz the power, 2014-12-11 07:45:42]

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

[commit-bot: I haz the power, 2014-12-11 07:45:43]

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

[sashab, 2014-12-11 22:57:26]

The CQ bit was checked by sashab@chromium.org

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

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

[commit-bot: I haz the power, 2014-12-12 02:46:28]

Committed patchset #6 (id:130001)

[commit-bot: I haz the power, 2014-12-12 02:47:20]

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