Add PermissionDescriptor to the permissions Mojo interface.

Add PermissionDescriptor to the permissions Mojo interface.

A permission descriptor is a structure describing a permission that an
origin may request. Most permissions are only identified by their names
but some have additional parameters. For example, a Bluetooth device
permission may include the MAC address of the device (or other unique
identifier) so that the permission applies to only the device selected
by the user through some sort of chooser interface.

BUG=638721
Committed: https://crrev.com/dc5d9051abbe865c3d97f75f6e64c0f04a7492a8
Cr-Commit-Position: refs/heads/master@{#422744}

[Reilly Grant (use Gerrit), 2016-08-17 21:07:37]

reillyg@chromium.org changed reviewers:
+ ddorwin@chromium.org, mlamouri@chromium.org, rickyz@chromium.org

[Reilly Grant (use Gerrit), 2016-08-17 21:07:37]

Please take a look.

[Reilly Grant (use Gerrit), 2016-08-17 21:07:42]

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

[commit-bot: I haz the power, 2016-08-17 21:08:12]

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-08-17 21:12:30]

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

[commit-bot: I haz the power, 2016-08-17 21:12:31]

Dry run: Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[mlamouri (slow - plz ping), 2016-08-18 13:49:44]

I'm not against using PermissionDescriptor in principle but how would you handle
the case case of PushPermissionDescriptor and MidiPermissionDescriptor that have
very different fields? I would like to see a solution for this before this land.

[Reilly Grant (use Gerrit), 2016-08-24 00:25:00]

Demonstrate PermissionDescriptor extensibility with MIDI.

[Reilly Grant (use Gerrit), 2016-08-24 00:25:09]

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

[Reilly Grant (use Gerrit), 2016-08-24 00:25:45]

On 2016/08/18 at 13:49:44, mlamouri wrote:
> I'm not against using PermissionDescriptor in principle but how would you
handle the case case of PushPermissionDescriptor and MidiPermissionDescriptor
that have very different fields? I would like to see a solution for this before
this land.

PushPermissionDescriptor doesn't have any currently supported fields but I've
implemented MidiPermissionDescriptor and removed PermissionName::MIDI_SYSEX.

[commit-bot: I haz the power, 2016-08-24 00:25:57]

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-08-24 00:52:02]

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

[commit-bot: I haz the power, 2016-08-24 00:52:03]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[ddorwin, 2016-08-24 02:43:55]

https://codereview.chromium.org/2255933002/diff/20001/content/renderer/media/...
File content/renderer/media/media_permission_dispatcher.cc (right):

https://codereview.chromium.org/2255933002/diff/20001/content/renderer/media/...
content/renderer/media/media_permission_dispatcher.cc:33:
blink::mojom::PermissionName::PROTECTED_MEDIA_IDENTIFIER;
As I note in the .mojom file, the member should probably be initialized to an
invalid value, which would be returned if we don't set the member here.

However, if the constructor is updated to take a name (see next file), then we
would need to provide something. In that case, the switch statement would simply
set a local variable that would then be used in the constructor. (This would
avoid the local descriptor variable.)

https://codereview.chromium.org/2255933002/diff/20001/content/renderer/notifi...
File content/renderer/notification_permission_dispatcher.cc (right):

https://codereview.chromium.org/2255933002/diff/20001/content/renderer/notifi...
content/renderer/notification_permission_dispatcher.cc:38: descriptor->name =
blink::mojom::PermissionName::NOTIFICATIONS;
This pattern occurs frequently. ISTM, the constructor should take the permission
name. See the comment in the .mojom file.

https://codereview.chromium.org/2255933002/diff/20001/third_party/WebKit/publ...
File third_party/WebKit/public/platform/modules/permissions/permission.mojom
(right):

https://codereview.chromium.org/2255933002/diff/20001/third_party/WebKit/publ...
third_party/WebKit/public/platform/modules/permissions/permission.mojom:34:
PermissionName name;
This is uninitialized.

Per my comment in the media file, an invalid enum value may be useful.

On the other hand, as noted earlier, providing a constructor that accepts a name
would be nice. This could also prevent a permission from being created without a
valid name. You'd need to make this private (possibly making the struct a
class), expose a getter, and define a constructor.

[Reilly Grant (use Gerrit), 2016-08-24 18:28:53]

https://codereview.chromium.org/2255933002/diff/20001/third_party/WebKit/publ...
File third_party/WebKit/public/platform/modules/permissions/permission.mojom
(right):

https://codereview.chromium.org/2255933002/diff/20001/third_party/WebKit/publ...
third_party/WebKit/public/platform/modules/permissions/permission.mojom:34:
PermissionName name;
On 2016/08/24 at 02:43:55, ddorwin wrote:
> This is uninitialized.
> 
> Per my comment in the media file, an invalid enum value may be useful.
> 
> On the other hand, as noted earlier, providing a constructor that accepts a
name would be nice. This could also prevent a permission from being created
without a valid name. You'd need to make this private (possibly making the
struct a class), expose a getter, and define a constructor.

This is a Mojo file. We cannot specify custom constructors.

[mlamouri (slow - plz ping), 2016-08-26 17:19:29]

I had another quick look. The general idea sounds fine but as ddorwin@ pointed,
we might want a helper function to help build these descriptors. Even setting
the extension is a bit odd (ie.
`permission.extension.set_midi(midiPermission)`).

I don't have the bandwidth to do a full review today and I will be out for a few
days. Feel free to get other reviewers to l-g-t-m the different files.

https://codereview.chromium.org/2255933002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/permissions/PermissionStatus.h (right):

https://codereview.chromium.org/2255933002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/permissions/PermissionStatus.h:65:
MojoPermissionDescriptor m_query;
nit: m_descriptor?

[Reilly Grant (use Gerrit), 2016-08-26 18:16:46]

On 2016/08/26 at 17:19:29, mlamouri wrote:
> I had another quick look. The general idea sounds fine but as ddorwin@
pointed, we might want a helper function to help build these descriptors. Even
setting the extension is a bit odd (ie.
`permission.extension.set_midi(midiPermission)`).

I wish Mojo let me do that. Maybe I'll add helpers to Permissions.h.

[Reilly Grant (use Gerrit), 2016-08-26 18:17:16]

https://codereview.chromium.org/2255933002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/permissions/PermissionStatus.h (right):

https://codereview.chromium.org/2255933002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/permissions/PermissionStatus.h:65:
MojoPermissionDescriptor m_query;
On 2016/08/26 at 17:19:29, mlamouri (slow) wrote:
> nit: m_descriptor?

I stole the word "query" from the Permission API spec but descriptor is probably
better here.

[mlamouri (slow - plz ping), 2016-09-05 09:12:44]

On 2016/08/26 at 17:19:29, mlamouri (slow) wrote:
> I don't have the bandwidth to do a full review today and I will be out for a
few days. Feel free to get other reviewers to l-g-t-m the different files.

I'm back to normal. Will have a look at this CL tomorrow.

[mlamouri (slow - plz ping), 2016-09-06 10:52:40]

https://codereview.chromium.org/2255933002/diff/20001/content/browser/permiss...
File content/browser/permissions/permission_service_impl.h (right):

https://codereview.chromium.org/2255933002/diff/20001/content/browser/permiss...
content/browser/permissions/permission_service_impl.h:83:
std::vector<blink::mojom::PermissionDescriptorPtr> permissions,
Why did this become by value?

https://codereview.chromium.org/2255933002/diff/20001/third_party/WebKit/publ...
File third_party/WebKit/public/platform/modules/permissions/permission.mojom
(right):

https://codereview.chromium.org/2255933002/diff/20001/third_party/WebKit/publ...
third_party/WebKit/public/platform/modules/permissions/permission.mojom:34:
PermissionName name;
On 2016/08/24 at 18:28:53, Reilly Grant wrote:
> On 2016/08/24 at 02:43:55, ddorwin wrote:
> > This is uninitialized.
> > 
> > Per my comment in the media file, an invalid enum value may be useful.
> > 
> > On the other hand, as noted earlier, providing a constructor that accepts a
name would be nice. This could also prevent a permission from being created
without a valid name. You'd need to make this private (possibly making the
struct a class), expose a getter, and define a constructor.
> 
> This is a Mojo file. We cannot specify custom constructors.

Can you add an util file in Blink that would have these methods:

PermissionDescriptor createPermissionDescriptor(PermissionName);
PermissionDescriptor createMidiPermissionDescriptor(bool);

If you add this in public/, content/ should be able to access it.

[Reilly Grant (use Gerrit), 2016-09-08 22:09:46]

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

[Reilly Grant (use Gerrit), 2016-09-08 22:09:52]

https://codereview.chromium.org/2255933002/diff/20001/content/browser/permiss...
File content/browser/permissions/permission_service_impl.h (right):

https://codereview.chromium.org/2255933002/diff/20001/content/browser/permiss...
content/browser/permissions/permission_service_impl.h:83:
std::vector<blink::mojom::PermissionDescriptorPtr> permissions,
On 2016/09/06 at 10:52:40, mlamouri wrote:
> Why did this become by value?

PermissionDescriptor is a Mojo struct and those are move-only. To allow the
function to pass ownership of individual elements of the vector to other
functions the parameter becomes pass-by-value (and is move-only).

https://codereview.chromium.org/2255933002/diff/20001/third_party/WebKit/publ...
File third_party/WebKit/public/platform/modules/permissions/permission.mojom
(right):

https://codereview.chromium.org/2255933002/diff/20001/third_party/WebKit/publ...
third_party/WebKit/public/platform/modules/permissions/permission.mojom:34:
PermissionName name;
On 2016/09/06 at 10:52:40, mlamouri wrote:
> On 2016/08/24 at 18:28:53, Reilly Grant wrote:
> > On 2016/08/24 at 02:43:55, ddorwin wrote:
> > > This is uninitialized.
> > > 
> > > Per my comment in the media file, an invalid enum value may be useful.
> > > 
> > > On the other hand, as noted earlier, providing a constructor that accepts
a name would be nice. This could also prevent a permission from being created
without a valid name. You'd need to make this private (possibly making the
struct a class), expose a getter, and define a constructor.
> > 
> > This is a Mojo file. We cannot specify custom constructors.
> 
> Can you add an util file in Blink that would have these methods:
> 
> PermissionDescriptor createPermissionDescriptor(PermissionName);
> PermissionDescriptor createMidiPermissionDescriptor(bool);
> 
> If you add this in public/, content/ should be able to access it.

Unfortunately content/ can't include it because the Blink variant of the Mojo
bindings includes WTF headers. There's only one place it would be used from
content/ though (and I want to get rid of media_permission_dispatcher.cc
anyways) so it's not a big deal.

[Reilly Grant (use Gerrit), 2016-09-08 22:10:09]

Patchset #3 (id:40001) has been deleted

[Reilly Grant (use Gerrit), 2016-09-08 22:10:24]

The CQ bit was unchecked by reillyg@chromium.org

[commit-bot: I haz the power, 2016-09-08 22:10:27]

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

[haraken, 2016-09-09 00:49:01]

haraken@chromium.org changed reviewers:
+ haraken@chromium.org

[haraken, 2016-09-09 00:49:02]

https://codereview.chromium.org/2255933002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/permissions/PermissionUtils.cpp (right):

https://codereview.chromium.org/2255933002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/permissions/PermissionUtils.cpp:27:
interfaceProvider = Platform::current()->interfaceProvider();

Is there any reason you don't always use
Platform::current()->interfaceProvider()?

[mlamouri (slow - plz ping), 2016-09-13 09:57:50]

https://codereview.chromium.org/2255933002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/permissions/PermissionUtils.cpp (right):

https://codereview.chromium.org/2255933002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/permissions/PermissionUtils.cpp:27:
interfaceProvider = Platform::current()->interfaceProvider();
On 2016/09/09 at 00:49:02, haraken wrote:
> Is there any reason you don't always use
Platform::current()->interfaceProvider()?

The service running on the Frame is able to show a UI (for request()). Not the
one on Platform which can only query permission.

https://codereview.chromium.org/2255933002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/permissions/Permissions.cpp (right):

https://codereview.chromium.org/2255933002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/permissions/Permissions.cpp:84: return
createPermissionDescriptor(permissionName);
I find it a bit confusing that some if statements end up returning and some will
set `permissionName` which is used here for the final return. Given that this
helper is a one-liner, can we make this a bit easier to read with a flow like:

```
if (name == "geolocation")
  return createPermissionDescriptor(PermissionName::GEOLOCATION)

if (name == "notifications")
  return createPermissionDescriptor(PermissionName::NOTIFICATIONS)

[...]
```

https://codereview.chromium.org/2255933002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/permissions/Permissions.cpp:92: auto
descriptor = parsePermission(scriptState, rawPermission, exceptionState);
can you explicitly put `PermissionDescriptor` instead of `auto`?

https://codereview.chromium.org/2255933002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/permissions/Permissions.cpp:109: auto
descriptorCopy = descriptor->Clone();
ditto and all the instances below.

https://codereview.chromium.org/2255933002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/permissions/Permissions.cpp:170: int
internalIndex = -1;
What about using `kNotFound`?

https://codereview.chromium.org/2255933002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/permissions/Permissions.cpp:177: if
(internalIndex == -1) {
... and here you can do `if (internalIndex == kNotFound)`

[Reilly Grant (use Gerrit), 2016-09-27 10:24:13]

Addressed mlamouri@'s comments.

[Reilly Grant (use Gerrit), 2016-09-27 10:34:19]

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

[Reilly Grant (use Gerrit), 2016-09-27 10:34:28]

https://codereview.chromium.org/2255933002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/permissions/Permissions.cpp (right):

https://codereview.chromium.org/2255933002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/permissions/Permissions.cpp:84: return
createPermissionDescriptor(permissionName);
On 2016/09/13 at 09:57:50, mlamouri (slow) wrote:
> I find it a bit confusing that some if statements end up returning and some
will set `permissionName` which is used here for the final return. Given that
this helper is a one-liner, can we make this a bit easier to read with a flow
like:
> 
> ```
> if (name == "geolocation")
>   return createPermissionDescriptor(PermissionName::GEOLOCATION)
> 
> if (name == "notifications")
>   return createPermissionDescriptor(PermissionName::NOTIFICATIONS)
> 
> [...]
> ```

Done. You're right, that's cleaner.

https://codereview.chromium.org/2255933002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/permissions/Permissions.cpp:92: auto
descriptor = parsePermission(scriptState, rawPermission, exceptionState);
On 2016/09/13 at 09:57:50, mlamouri (slow) wrote:
> can you explicitly put `PermissionDescriptor` instead of `auto`?

Done.

https://codereview.chromium.org/2255933002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/permissions/Permissions.cpp:109: auto
descriptorCopy = descriptor->Clone();
On 2016/09/13 at 09:57:50, mlamouri (slow) wrote:
> ditto and all the instances below.

Done.

https://codereview.chromium.org/2255933002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/permissions/Permissions.cpp:170: int
internalIndex = -1;
On 2016/09/13 at 09:57:50, mlamouri (slow) wrote:
> What about using `kNotFound`?

I didn't know kNotFound was a common type. Neat.

https://codereview.chromium.org/2255933002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/permissions/Permissions.cpp:177: if
(internalIndex == -1) {
On 2016/09/13 at 09:57:50, mlamouri (slow) wrote:
> ... and here you can do `if (internalIndex == kNotFound)`

Done.

[commit-bot: I haz the power, 2016-09-27 10:34:32]

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-27 11:04:29]

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

[commit-bot: I haz the power, 2016-09-27 11:04:30]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[haraken, 2016-09-27 13:35:55]

https://codereview.chromium.org/2255933002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/permissions/PermissionUtils.cpp (right):

https://codereview.chromium.org/2255933002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/permissions/PermissionUtils.cpp:27:
interfaceProvider = Platform::current()->interfaceProvider();

Is it worth distinguishing the interface providers? i.e., what's a disadvantage
of using Platform's interface provider always?

[Reilly Grant (use Gerrit), 2016-09-28 05:22:51]

https://codereview.chromium.org/2255933002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/permissions/PermissionUtils.cpp (right):

https://codereview.chromium.org/2255933002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/permissions/PermissionUtils.cpp:27:
interfaceProvider = Platform::current()->interfaceProvider();
On 2016/09/27 at 13:35:54, haraken wrote:
> Is it worth distinguishing the interface providers? i.e., what's a
disadvantage of using Platform's interface provider always?

Platform::interfaceProvider() returns services registered to the
RenderProcessHost, LocalFrame::interfaceProvider() returns services registered
to the RenderFrameHost. Only the later can provide the service with access to
the content::WebContents which is required for displaying UI. A
RenderProcessHost may represent the renderer for multiple tabs.

[haraken, 2016-09-28 05:36:49]

On 2016/09/28 05:22:51, Reilly Grant wrote:
>
https://codereview.chromium.org/2255933002/diff/80001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/modules/permissions/PermissionUtils.cpp
(right):
> 
>
https://codereview.chromium.org/2255933002/diff/80001/third_party/WebKit/Sour...
> third_party/WebKit/Source/modules/permissions/PermissionUtils.cpp:27:
> interfaceProvider = Platform::current()->interfaceProvider();
> On 2016/09/27 at 13:35:54, haraken wrote:
> > Is it worth distinguishing the interface providers? i.e., what's a
> disadvantage of using Platform's interface provider always?
> 
> Platform::interfaceProvider() returns services registered to the
> RenderProcessHost, LocalFrame::interfaceProvider() returns services registered
> to the RenderFrameHost. Only the later can provide the service with access to
> the content::WebContents which is required for displaying UI. A
> RenderProcessHost may represent the renderer for multiple tabs.

Makes sense.

[Reilly Grant (use Gerrit), 2016-09-28 06:22:46]

Fixing tests.

[Reilly Grant (use Gerrit), 2016-09-28 06:24:08]

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

[commit-bot: I haz the power, 2016-09-28 06:24:20]

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-28 08:13:28]

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

[commit-bot: I haz the power, 2016-09-28 08:13:30]

Dry run: This issue passed the CQ dry run.

[mlamouri (slow - plz ping), 2016-09-28 12:24:33]

lgtm

Thanks! :)

https://codereview.chromium.org/2255933002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/permissions/Permissions.cpp (right):

https://codereview.chromium.org/2255933002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/permissions/Permissions.cpp:56: if (name ==
"geolocation") {
style: no { } for one line statements.

https://codereview.chromium.org/2255933002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/permissions/Permissions.cpp:59: if (name ==
"notifications") {
ditto (and maybe below)

[Reilly Grant (use Gerrit), 2016-09-29 07:47:19]

Remove braces around single-line ifs.

[Reilly Grant (use Gerrit), 2016-09-29 07:53:15]

https://codereview.chromium.org/2255933002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/permissions/Permissions.cpp (right):

https://codereview.chromium.org/2255933002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/permissions/Permissions.cpp:56: if (name ==
"geolocation") {
On 2016/09/28 at 12:24:33, mlamouri (slow) wrote:
> style: no { } for one line statements.

Done.

https://codereview.chromium.org/2255933002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/permissions/Permissions.cpp:59: if (name ==
"notifications") {
On 2016/09/28 at 12:24:33, mlamouri (slow) wrote:
> ditto (and maybe below)

Done.

[Reilly Grant (use Gerrit), 2016-09-29 07:55:36]

ddorwin@, please review content/renderer/media/media_permission_dispatcher.cc
rickyz@, please review
third_party/WebKit/public/platform/modules/permissions/permission.mojom

[rickyz (no longer on Chrome), 2016-09-30 01:24:27]

mojom lgtm

[Reilly Grant (use Gerrit), 2016-09-30 01:53:10]

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

[commit-bot: I haz the power, 2016-09-30 01:53:44]

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-30 04:14:28]

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

[commit-bot: I haz the power, 2016-09-30 04:14:29]

Dry run: This issue passed the CQ dry run.

[Reilly Grant (use Gerrit), 2016-10-03 03:43:31]

Rebased over the Blink reformatting.

[Reilly Grant (use Gerrit), 2016-10-03 03:52:12]

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

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

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

[Reilly Grant (use Gerrit), 2016-10-03 05:10:03]

reillyg@chromium.org changed reviewers:
+ mcasas@chromium.org
- ddorwin@chromium.org

[Reilly Grant (use Gerrit), 2016-10-03 05:10:04]

mcasas@, can you review //content/renderer/media?

[commit-bot: I haz the power, 2016-10-03 05:14:34]

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

[commit-bot: I haz the power, 2016-10-03 05:14:36]

Dry run: Exceeded global retry quota

[mcasas, 2016-10-03 16:00:10]

content/renderer/media lgtm

https://codereview.chromium.org/2255933002/diff/140001/content/renderer/media...
File content/renderer/media/media_permission_dispatcher.cc (right):

https://codereview.chromium.org/2255933002/diff/140001/content/renderer/media...
content/renderer/media/media_permission_dispatcher.cc:34: NOTREACHED();
nit: NOTREACHED() << type; ?
(mojom defines stringifiers, let's use them).

[Reilly Grant (use Gerrit), 2016-10-04 07:32:12]

Print the unexpected permission type.

[Reilly Grant (use Gerrit), 2016-10-04 07:42:36]

The CQ bit was checked by reillyg@chromium.org

[Reilly Grant (use Gerrit), 2016-10-04 07:42:37]

https://codereview.chromium.org/2255933002/diff/140001/content/renderer/media...
File content/renderer/media/media_permission_dispatcher.cc (right):

https://codereview.chromium.org/2255933002/diff/140001/content/renderer/media...
content/renderer/media/media_permission_dispatcher.cc:34: NOTREACHED();
On 2016/10/03 at 16:00:10, mcasas wrote:
> nit: NOTREACHED() << type; ?
> (mojom defines stringifiers, let's use them).

Awesome. Done.

[Reilly Grant (use Gerrit), 2016-10-04 07:42:37]

The patchset sent to the CQ was uploaded after l-g-t-m from
mlamouri@chromium.org, rickyz@chromium.org, mcasas@chromium.org
Link to the patchset: https://codereview.chromium.org/2255933002/#ps160001
(title: "Print the unexpected permission type.")

[commit-bot: I haz the power, 2016-10-04 07:42:49]

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-04 10:43:52]

Committed patchset #8 (id:160001)

[commit-bot: I haz the power, 2016-10-04 10:47:08]

Description was changed from

==========
Add PermissionDescriptor to the permissions Mojo interface.

A permission descriptor is a structure describing a permission that an
origin may request. Most permissions are only identified by their names
but some have additional parameters. For example, a Bluetooth device
permission may include the MAC address of the device (or other unique
identifier) so that the permission applies to only the device selected
by the user through some sort of chooser interface.

BUG=638721
==========

to

==========
Add PermissionDescriptor to the permissions Mojo interface.

A permission descriptor is a structure describing a permission that an
origin may request. Most permissions are only identified by their names
but some have additional parameters. For example, a Bluetooth device
permission may include the MAC address of the device (or other unique
identifier) so that the permission applies to only the device selected
by the user through some sort of chooser interface.

BUG=638721

Committed: https://crrev.com/dc5d9051abbe865c3d97f75f6e64c0f04a7492a8
Cr-Commit-Position: refs/heads/master@{#422744}
==========

[commit-bot: I haz the power, 2016-10-04 10:47:11]

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