Introduce MediaDevicesPermissionChecker.

Introduce MediaDevicesPermissionChecker.

This class makes it easier to check device permissions.
It is similar to MediaStreamUIProxy, but it has the following advantages:
* Easier to check permissions for multiple device types in a single call.
* Uses MediaDeviceType instead of MediaStreamType to refer to device types.
* Simpler design.

Current users are MediaDevicesDispatcherHost and AudioRendererHost.
This will make it easier to check for permissions in the mojo version of
devicechange notifications.

Once MediaDevice permissions are revamped, maybe this class might substitute
MediaStreamUIProxy.

Originally, the intention was to use PermissionManager, but several tests expect
permissions to be checked via the RenderFrameHostDelegate, so this will have
to wait.

Also, the plan was to place this in content/browser/renderer_host/media since
it is used only by *Host classes, but it is illegal to include
"content/browser/frame_host" from there (MediaStreamUIProxy got a free pass).

BUG=648183
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/df036b3c1846ea332f5196edd6f578f802cb8ef7
Cr-Commit-Position: refs/heads/master@{#428709}

[Guido Urdaneta, 2016-10-20 17:39:23]

Description was changed from

==========
Introduce MediaDevicesPermissionChecker.

This class makes it easier to check device permissions.
It is similar to MediaStreamUIProxy, but it has the following advantages:
* Easier to check permissions for multiple device types in a single call.
* Uses MediaDeviceType instead of MediaStreamType to refer to device types.
* Simpler design.

Current users are MediaDevicesDispatcherHost and AudioRendererHost.
This will make it easier to check for permissions in the mojo version of
devicechange notifications.

Once MediaDevice permissions are revamped, maybe this class might substitute
MediaStreamUIProxy.

Originally, the intention was to use PermissionManager, but several tests expect
permissions to be checked via the RenderFrameHostDelegate, so this will have
to wait.

Also, the plan was to place this in content/browser/renderer_host/media since
it is used only by *Host classes, but it is illegal to include
"content/broser/frame_host" from there (MediaStreamUIProxy got a free pass).

BUG=648183
==========

to

==========
Introduce MediaDevicesPermissionChecker.

This class makes it easier to check device permissions.
It is similar to MediaStreamUIProxy, but it has the following advantages:
* Easier to check permissions for multiple device types in a single call.
* Uses MediaDeviceType instead of MediaStreamType to refer to device types.
* Simpler design.

Current users are MediaDevicesDispatcherHost and AudioRendererHost.
This will make it easier to check for permissions in the mojo version of
devicechange notifications.

Once MediaDevice permissions are revamped, maybe this class might substitute
MediaStreamUIProxy.

Originally, the intention was to use PermissionManager, but several tests expect
permissions to be checked via the RenderFrameHostDelegate, so this will have
to wait.

Also, the plan was to place this in content/browser/renderer_host/media since
it is used only by *Host classes, but it is illegal to include
"content/broser/frame_host" from there (MediaStreamUIProxy got a free pass).

BUG=648183
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[Guido Urdaneta, 2016-10-20 17:42:10]

Description was changed from

==========
Introduce MediaDevicesPermissionChecker.

This class makes it easier to check device permissions.
It is similar to MediaStreamUIProxy, but it has the following advantages:
* Easier to check permissions for multiple device types in a single call.
* Uses MediaDeviceType instead of MediaStreamType to refer to device types.
* Simpler design.

Current users are MediaDevicesDispatcherHost and AudioRendererHost.
This will make it easier to check for permissions in the mojo version of
devicechange notifications.

Once MediaDevice permissions are revamped, maybe this class might substitute
MediaStreamUIProxy.

Originally, the intention was to use PermissionManager, but several tests expect
permissions to be checked via the RenderFrameHostDelegate, so this will have
to wait.

Also, the plan was to place this in content/browser/renderer_host/media since
it is used only by *Host classes, but it is illegal to include
"content/broser/frame_host" from there (MediaStreamUIProxy got a free pass).

BUG=648183
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Introduce MediaDevicesPermissionChecker.

This class makes it easier to check device permissions.
It is similar to MediaStreamUIProxy, but it has the following advantages:
* Easier to check permissions for multiple device types in a single call.
* Uses MediaDeviceType instead of MediaStreamType to refer to device types.
* Simpler design.

Current users are MediaDevicesDispatcherHost and AudioRendererHost.
This will make it easier to check for permissions in the mojo version of
devicechange notifications.

Once MediaDevice permissions are revamped, maybe this class might substitute
MediaStreamUIProxy.

Originally, the intention was to use PermissionManager, but several tests expect
permissions to be checked via the RenderFrameHostDelegate, so this will have
to wait.

Also, the plan was to place this in content/browser/renderer_host/media since
it is used only by *Host classes, but it is illegal to include
"content/broser/frame_host" from there (MediaStreamUIProxy got a free pass).

BUG=648183
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[Guido Urdaneta, 2016-10-20 17:42:10]

guidou@chromium.org changed reviewers:
+ tommi@chromium.org

[Guido Urdaneta, 2016-10-20 17:42:17]

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

[commit-bot: I haz the power, 2016-10-20 17:42:43]

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

[Guido Urdaneta, 2016-10-20 17:43:18]

Hi, PTAL

[commit-bot: I haz the power, 2016-10-20 19:43:20]

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

[commit-bot: I haz the power, 2016-10-20 19:43:21]

Dry run: This issue passed the CQ dry run.

[Guido Urdaneta, 2016-10-21 09:10:51]

Description was changed from

==========
Introduce MediaDevicesPermissionChecker.

This class makes it easier to check device permissions.
It is similar to MediaStreamUIProxy, but it has the following advantages:
* Easier to check permissions for multiple device types in a single call.
* Uses MediaDeviceType instead of MediaStreamType to refer to device types.
* Simpler design.

Current users are MediaDevicesDispatcherHost and AudioRendererHost.
This will make it easier to check for permissions in the mojo version of
devicechange notifications.

Once MediaDevice permissions are revamped, maybe this class might substitute
MediaStreamUIProxy.

Originally, the intention was to use PermissionManager, but several tests expect
permissions to be checked via the RenderFrameHostDelegate, so this will have
to wait.

Also, the plan was to place this in content/browser/renderer_host/media since
it is used only by *Host classes, but it is illegal to include
"content/broser/frame_host" from there (MediaStreamUIProxy got a free pass).

BUG=648183
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Introduce MediaDevicesPermissionChecker.

This class makes it easier to check device permissions.
It is similar to MediaStreamUIProxy, but it has the following advantages:
* Easier to check permissions for multiple device types in a single call.
* Uses MediaDeviceType instead of MediaStreamType to refer to device types.
* Simpler design.

Current users are MediaDevicesDispatcherHost and AudioRendererHost.
This will make it easier to check for permissions in the mojo version of
devicechange notifications.

Once MediaDevice permissions are revamped, maybe this class might substitute
MediaStreamUIProxy.

Originally, the intention was to use PermissionManager, but several tests expect
permissions to be checked via the RenderFrameHostDelegate, so this will have
to wait.

Also, the plan was to place this in content/browser/renderer_host/media since
it is used only by *Host classes, but it is illegal to include
"content/browser/frame_host" from there (MediaStreamUIProxy got a free pass).

BUG=648183
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[Guido Urdaneta, 2016-10-21 15:42:16]

guidou@chromium.org changed reviewers:
+ alexmos@chromium.org

[Guido Urdaneta, 2016-10-21 15:42:17]

alexmos@chromium.org: please review changes in content/browser/frame_host

[Guido Urdaneta, 2016-10-28 08:51:18]

guidou@chromium.org changed reviewers:
+ xhwang@chromium.org

[Guido Urdaneta, 2016-10-28 08:53:56]

xhwang@chromium.org: please review changes in content/browser/media
alexmos@chromium.org: please review changes in content/browser/frame_host

[tommi (sloooow) - chröme, 2016-10-28 09:36:52]

lgtm

https://codereview.chromium.org/2436113002/diff/1/content/browser/media/media...
File content/browser/media/media_devices_permission_checker.cc (right):

https://codereview.chromium.org/2436113002/diff/1/content/browser/media/media...
content/browser/media/media_devices_permission_checker.cc:150: use_override_ =
true;
nit: would it make sense to use Optional?

[Guido Urdaneta, 2016-10-28 10:06:55]

https://codereview.chromium.org/2436113002/diff/1/content/browser/media/media...
File content/browser/media/media_devices_permission_checker.cc (right):

https://codereview.chromium.org/2436113002/diff/1/content/browser/media/media...
content/browser/media/media_devices_permission_checker.cc:150: use_override_ =
true;
On 2016/10/28 09:36:52, tommi (chrömium) wrote:
> nit: would it make sense to use Optional?

I would work, but I think the code would become less obvious and more error
prone since both |override| and |*override| would be (implicitly) bool.

[alexmos, 2016-10-28 17:01:54]

frame_host LGTM

[xhwang, 2016-10-28 17:14:56]

lgtm % nits

https://codereview.chromium.org/2436113002/diff/1/content/browser/media/media...
File content/browser/media/media_devices_permission_checker.h (right):

https://codereview.chromium.org/2436113002/diff/1/content/browser/media/media...
content/browser/media/media_devices_permission_checker.h:5: #ifndef
CONTENT_BROWSER_MEDIA_MEDIA_DEVICES_PERMISSION_CHECKER_H_
OOC, can we put this file in content/browser/renderer_host/media/ since this is
only used for "renderer host"?

https://codereview.chromium.org/2436113002/diff/1/content/browser/media/media...
content/browser/media/media_devices_permission_checker.h:20: // This class
provides has various utility functions to check if a render frame
"provides has"?

https://codereview.chromium.org/2436113002/diff/1/content/browser/media/media...
content/browser/media/media_devices_permission_checker.h:31: bool
CheckPermission(MediaDeviceType device_type,
Does this make sense to name this CheckPermissionOnUIThread?

https://google.github.io/styleguide/cppguide.html#Function_Overloading

https://codereview.chromium.org/2436113002/diff/1/content/browser/media/media...
content/browser/media/media_devices_permission_checker.h:33: int routing_id,
It seems less ambiguouse to use render_frame_id when we are out of RenderFrame*
context:

https://cs.chromium.org/search/?q=render_frame_id&sq=package:chromium&type=cs

https://codereview.chromium.org/2436113002/diff/1/content/browser/media/media...
content/browser/media/media_devices_permission_checker.h:39: // This method can
be called on any thread.
nit: might worth commenting that the |callback| will always be fired on the
thread this method is called on.

https://codereview.chromium.org/2436113002/diff/1/content/browser/media/media...
content/browser/media/media_devices_permission_checker.h:77: void
OverridePermissions(bool override_value);
Could you please document the use case for this? It's a bit odd to override
permission values.

[Guido Urdaneta, 2016-10-29 19:34:38]

https://codereview.chromium.org/2436113002/diff/1/content/browser/media/media...
File content/browser/media/media_devices_permission_checker.h (right):

https://codereview.chromium.org/2436113002/diff/1/content/browser/media/media...
content/browser/media/media_devices_permission_checker.h:5: #ifndef
CONTENT_BROWSER_MEDIA_MEDIA_DEVICES_PERMISSION_CHECKER_H_
On 2016/10/28 17:14:56, xhwang wrote:
> OOC, can we put this file in content/browser/renderer_host/media/ since this
is
> only used for "renderer host"?

That would be my preference, but the .cc includes "content/browser/frame_host",
and it is forbidden to include that directory from
"content/browser/renderer_host/media".
Any workaround?

https://codereview.chromium.org/2436113002/diff/1/content/browser/media/media...
content/browser/media/media_devices_permission_checker.h:20: // This class
provides has various utility functions to check if a render frame
On 2016/10/28 17:14:56, xhwang wrote:
> "provides has"?

Done.

https://codereview.chromium.org/2436113002/diff/1/content/browser/media/media...
content/browser/media/media_devices_permission_checker.h:31: bool
CheckPermission(MediaDeviceType device_type,
On 2016/10/28 17:14:56, xhwang wrote:
> Does this make sense to name this CheckPermissionOnUIThread?
> 
> https://google.github.io/styleguide/cppguide.html#Function_Overloading

Done.

https://codereview.chromium.org/2436113002/diff/1/content/browser/media/media...
content/browser/media/media_devices_permission_checker.h:33: int routing_id,
On 2016/10/28 17:14:56, xhwang wrote:
> It seems less ambiguouse to use render_frame_id when we are out of
RenderFrame*
> context:
> 
> https://cs.chromium.org/search/?q=render_frame_id&sq=package:chromium&type=cs

Done.

https://codereview.chromium.org/2436113002/diff/1/content/browser/media/media...
content/browser/media/media_devices_permission_checker.h:39: // This method can
be called on any thread.
On 2016/10/28 17:14:56, xhwang wrote:
> nit: might worth commenting that the |callback| will always be fired on the
> thread this method is called on.

Done.

https://codereview.chromium.org/2436113002/diff/1/content/browser/media/media...
content/browser/media/media_devices_permission_checker.h:77: void
OverridePermissions(bool override_value);
On 2016/10/28 17:14:56, xhwang wrote:
> Could you please document the use case for this? It's a bit odd to override
> permission values.

Done. Renamed the method to OverridePermissionForTesting.

[xhwang, 2016-10-30 16:18:27]

lgtm++

https://codereview.chromium.org/2436113002/diff/1/content/browser/media/media...
File content/browser/media/media_devices_permission_checker.h (right):

https://codereview.chromium.org/2436113002/diff/1/content/browser/media/media...
content/browser/media/media_devices_permission_checker.h:5: #ifndef
CONTENT_BROWSER_MEDIA_MEDIA_DEVICES_PERMISSION_CHECKER_H_
On 2016/10/29 19:34:38, Guido Urdaneta wrote:
> On 2016/10/28 17:14:56, xhwang wrote:
> > OOC, can we put this file in content/browser/renderer_host/media/ since this
> is
> > only used for "renderer host"?
> 
> That would be my preference, but the .cc includes
"content/browser/frame_host",
> and it is forbidden to include that directory from
> "content/browser/renderer_host/media".
> Any workaround?

I see. I am not familiar with this part. A content/ owner should know.

[Guido Urdaneta, 2016-10-31 09:38:25]

guidou@chromium.org changed reviewers:
+ avi@chromium.org

[Guido Urdaneta, 2016-10-31 09:38:26]

avi@chromium.org: Can you rs? 

Also, do you have any advice wrt xhwangs@'s comment about placing the
media_devices_permission_checker.* files in
"content/browser/renderer_host/media" and the problem with the inclusion of
"content/browser/frame_host/"?

[Avi (use Gerrit), 2016-10-31 13:22:15]

On 2016/10/31 09:38:26, Guido Urdaneta wrote:
> mailto:avi@chromium.org: Can you rs? 
> 
> Also, do you have any advice wrt xhwangs@'s comment about placing the
> media_devices_permission_checker.* files in
> "content/browser/renderer_host/media" and the problem with the inclusion of
> "content/browser/frame_host/"?

LGTM stamp as is.

Please respect the dependency requirements as stated in the DEPS files.

[Guido Urdaneta, 2016-10-31 13:23:01]

The CQ bit was checked by guidou@chromium.org

[Guido Urdaneta, 2016-10-31 13:23:02]

The patchset sent to the CQ was uploaded after l-g-t-m from tommi@chromium.org,
alexmos@chromium.org
Link to the patchset: https://codereview.chromium.org/2436113002/#ps20001
(title: "address xhwang's comments")

[commit-bot: I haz the power, 2016-10-31 13:23:13]

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-31 14:51:43]

Description was changed from

==========
Introduce MediaDevicesPermissionChecker.

This class makes it easier to check device permissions.
It is similar to MediaStreamUIProxy, but it has the following advantages:
* Easier to check permissions for multiple device types in a single call.
* Uses MediaDeviceType instead of MediaStreamType to refer to device types.
* Simpler design.

Current users are MediaDevicesDispatcherHost and AudioRendererHost.
This will make it easier to check for permissions in the mojo version of
devicechange notifications.

Once MediaDevice permissions are revamped, maybe this class might substitute
MediaStreamUIProxy.

Originally, the intention was to use PermissionManager, but several tests expect
permissions to be checked via the RenderFrameHostDelegate, so this will have
to wait.

Also, the plan was to place this in content/browser/renderer_host/media since
it is used only by *Host classes, but it is illegal to include
"content/browser/frame_host" from there (MediaStreamUIProxy got a free pass).

BUG=648183
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Introduce MediaDevicesPermissionChecker.

This class makes it easier to check device permissions.
It is similar to MediaStreamUIProxy, but it has the following advantages:
* Easier to check permissions for multiple device types in a single call.
* Uses MediaDeviceType instead of MediaStreamType to refer to device types.
* Simpler design.

Current users are MediaDevicesDispatcherHost and AudioRendererHost.
This will make it easier to check for permissions in the mojo version of
devicechange notifications.

Once MediaDevice permissions are revamped, maybe this class might substitute
MediaStreamUIProxy.

Originally, the intention was to use PermissionManager, but several tests expect
permissions to be checked via the RenderFrameHostDelegate, so this will have
to wait.

Also, the plan was to place this in content/browser/renderer_host/media since
it is used only by *Host classes, but it is illegal to include
"content/browser/frame_host" from there (MediaStreamUIProxy got a free pass).

BUG=648183
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[commit-bot: I haz the power, 2016-10-31 14:51:45]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-10-31 14:54:59]

Description was changed from

==========
Introduce MediaDevicesPermissionChecker.

This class makes it easier to check device permissions.
It is similar to MediaStreamUIProxy, but it has the following advantages:
* Easier to check permissions for multiple device types in a single call.
* Uses MediaDeviceType instead of MediaStreamType to refer to device types.
* Simpler design.

Current users are MediaDevicesDispatcherHost and AudioRendererHost.
This will make it easier to check for permissions in the mojo version of
devicechange notifications.

Once MediaDevice permissions are revamped, maybe this class might substitute
MediaStreamUIProxy.

Originally, the intention was to use PermissionManager, but several tests expect
permissions to be checked via the RenderFrameHostDelegate, so this will have
to wait.

Also, the plan was to place this in content/browser/renderer_host/media since
it is used only by *Host classes, but it is illegal to include
"content/browser/frame_host" from there (MediaStreamUIProxy got a free pass).

BUG=648183
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Introduce MediaDevicesPermissionChecker.

This class makes it easier to check device permissions.
It is similar to MediaStreamUIProxy, but it has the following advantages:
* Easier to check permissions for multiple device types in a single call.
* Uses MediaDeviceType instead of MediaStreamType to refer to device types.
* Simpler design.

Current users are MediaDevicesDispatcherHost and AudioRendererHost.
This will make it easier to check for permissions in the mojo version of
devicechange notifications.

Once MediaDevice permissions are revamped, maybe this class might substitute
MediaStreamUIProxy.

Originally, the intention was to use PermissionManager, but several tests expect
permissions to be checked via the RenderFrameHostDelegate, so this will have
to wait.

Also, the plan was to place this in content/browser/renderer_host/media since
it is used only by *Host classes, but it is illegal to include
"content/browser/frame_host" from there (MediaStreamUIProxy got a free pass).

BUG=648183
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

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

[commit-bot: I haz the power, 2016-10-31 14:55:00]

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