Integrate MediaPermission with PermissionManager by using PermissionContext.

Integrate MediaPermission with PermissionManager by using PermissionContext.

Use PermissionContext in MediaPermission. MediaPermission is a superset of PermissionContext as it contains check based on request type and device Id. It also covers use case like Flash in addition to audio/video streaming.

BUG=520101
Committed: https://crrev.com/4461ecf316f6e11d24358e6cd79c59c63e2229fa
Cr-Commit-Position: refs/heads/master@{#346857}

[guoweis_left_chromium, 2015-08-28 00:06:49]

guoweis@chromium.org changed reviewers:
+ mlamouri@chromium.org, raymes@chromium.org, xhwang@chromium.org

[guoweis_left_chromium, 2015-08-28 00:06:50]

PTAL.

[mlamouri (slow - plz ping), 2015-08-28 10:51:36]

https://codereview.chromium.org/1318173002/diff/20001/chrome/browser/media/me...
File chrome/browser/media/media_permission.cc (right):

https://codereview.chromium.org/1318173002/diff/20001/chrome/browser/media/me...
chrome/browser/media/media_permission.cc:91: ContentSetting
MediaPermission::GetStoredContentSetting() const {
Could you change MediaPermission::GetStoredContentSetting() to
MediaPermission::GetPermissionStatus() and return a PermissionStatus? It seems
that it would simply this code greatly.

https://codereview.chromium.org/1318173002/diff/20001/chrome/browser/media/me...
chrome/browser/media/media_permission.cc:93: if (profile_) {
Why are you checking for |profile_| while the previous code didn't?

https://codereview.chromium.org/1318173002/diff/20001/chrome/browser/media/me...
chrome/browser/media/media_permission.cc:96: if (permission_manager) {
Maybe:
if (!permission_manager)
  return CONTENT_SETTING_BLOCK;

https://codereview.chromium.org/1318173002/diff/20001/chrome/browser/media/me...
chrome/browser/media/media_permission.cc:100: origin_));
Shouldn't you take into account the embedder?

[raymes, 2015-08-29 00:09:02]

https://codereview.chromium.org/1318173002/diff/20001/chrome/browser/media/me...
File chrome/browser/media/media_permission.cc (right):

https://codereview.chromium.org/1318173002/diff/20001/chrome/browser/media/me...
chrome/browser/media/media_permission.cc:91: ContentSetting
MediaPermission::GetStoredContentSetting() const {
This unfortunately comes as a result of doing a partial refactoring.
ContentSetting values are used throughout MediaStreamDevicesController which
uses this file. The end state that we want to get to is to remove
MediaStreamDevicesController altogether and have the callsites access the
PermissionContexts for mic/cam directly. 

I agree, though, that it seems nasty to have to do the
permissionstatus/contentsetting conversion in this file too. I'll have to have a
bit more of a think about how we can make this nicer in the short term. I'll try
to add more comments on Monday.

https://codereview.chromium.org/1318173002/diff/20001/chrome/browser/media/me...
chrome/browser/media/media_permission.cc:100: origin_));
Mounir, you should read my replies :) As I mentioned in the other CL, the
current behavior for microphone and video permissions is not to consider the
embedder. Even if we wanted to make the behavioral change, this CL wouldn't be
the best place to do it :)

[raymes, 2015-08-31 01:51:46]

https://codereview.chromium.org/1318173002/diff/20001/chrome/browser/media/me...
File chrome/browser/media/media_permission.cc (right):

https://codereview.chromium.org/1318173002/diff/20001/chrome/browser/media/me...
chrome/browser/media/media_permission.cc:16: namespace {
nit: newline below

https://codereview.chromium.org/1318173002/diff/20001/chrome/browser/media/me...
chrome/browser/media/media_permission.cc:91: ContentSetting
MediaPermission::GetStoredContentSetting() const {
It's probably possible to do some refactoring in MediaStreamDevicesController to
use PermissionType but it would take some effort and I'm not sure if it's worth
the benefit without doing the full refactor to remove the class altogether.

Mounir, do you have anyone who might be interested in doing this refactoring to
replace MediaStreamDevicesController with PermissionContext? It would be a
pre-requisite for making the media PermissionContexts work properly.

https://codereview.chromium.org/1318173002/diff/20001/chrome/browser/media/me...
chrome/browser/media/media_permission.cc:115: }
I think this should be moved into the PermissionContext too, otherwise it's
going to return the wrong result for some requests. We can't pass in
request_type_ but what we can do is create an extra method hanging off the
PermissionContext: GetPermissionStatusForPepper() which would be called if
request_type == content:MEDIA_OPEN_DEVICE. We would change
ShouldPersistContentSetting to accept a bool is_pepper_request instead of
request_type.

[mlamouri (slow - plz ping), 2015-08-31 13:09:58]

https://codereview.chromium.org/1318173002/diff/20001/chrome/browser/media/me...
File chrome/browser/media/media_permission.cc (right):

https://codereview.chromium.org/1318173002/diff/20001/chrome/browser/media/me...
chrome/browser/media/media_permission.cc:91: ContentSetting
MediaPermission::GetStoredContentSetting() const {
On 2015/08/31 at 01:51:45, raymes wrote:
> It's probably possible to do some refactoring in MediaStreamDevicesController
to use PermissionType but it would take some effort and I'm not sure if it's
worth the benefit without doing the full refactor to remove the class
altogether.
> 
> Mounir, do you have anyone who might be interested in doing this refactoring
to replace MediaStreamDevicesController with PermissionContext? It would be a
pre-requisite for making the media PermissionContexts work properly.

Hmmm, my comment was only applying to GetStoredContentSettings but I didn't
realise that GetPermissionStatus above was using the returned value as a
ContentSettings which was used in more places.

I guess it's fine to keep that as-is then. Unfortunately, I can't think of any
one with cycles to take care of this refactoring.

https://codereview.chromium.org/1318173002/diff/20001/chrome/browser/media/me...
chrome/browser/media/media_permission.cc:100: origin_));
On 2015/08/29 at 00:09:02, raymes wrote:
> Mounir, you should read my replies :) As I mentioned in the other CL, the
current behavior for microphone and video permissions is not to consider the
embedder. Even if we wanted to make the behavioral change, this CL wouldn't be
the best place to do it :)

I remember your comment. Though, I would prefer the PermissionContext to make
that decision, not the callers. That way, we can easily change the policy
without having to track all the callers.

[raymes, 2015-08-31 23:32:49]

https://codereview.chromium.org/1318173002/diff/20001/chrome/browser/media/me...
File chrome/browser/media/media_permission.cc (right):

https://codereview.chromium.org/1318173002/diff/20001/chrome/browser/media/me...
chrome/browser/media/media_permission.cc:100: origin_));
Ahh, sorry, that makes sense and I agree with you. 

Unfortunately we don't have access to the embedder easily here - we would have
to modify MediaStreamRequest and pass it through. We should add a TODO at least
but it might be best done separately.

[guoweis_left_chromium, 2015-08-31 23:40:13]

I was thinking to use web_contents_->GetLastCommittedURL().GetOrigin() for
embedding origin. Does this work? I haven't tested myself.

On Mon, Aug 31, 2015 at 4:32 PM, <raymes@chromium.org> wrote:

>
>
>
https://codereview.chromium.org/1318173002/diff/20001/chrome/browser/media/me...
> File chrome/browser/media/media_permission.cc (right):
>
>
>
https://codereview.chromium.org/1318173002/diff/20001/chrome/browser/media/me...
> chrome/browser/media/media_permission.cc:100: origin_));
> Ahh, sorry, that makes sense and I agree with you.
>
> Unfortunately we don't have access to the embedder easily here - we
> would have to modify MediaStreamRequest and pass it through. We should
> add a TODO at least but it might be best done separately.
>
> https://codereview.chromium.org/1318173002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[raymes, 2015-09-01 00:01:10]

I think that could work but I'm uncertain too.

On Tue, 1 Sep 2015 at 09:40 Guo-wei Shieh <guoweis@chromium.org> wrote:

> I was thinking to use web_contents_->GetLastCommittedURL().GetOrigin()
> for embedding origin. Does this work? I haven't tested myself.
>
> On Mon, Aug 31, 2015 at 4:32 PM, <raymes@chromium.org> wrote:
>
>>
>>
>>
https://codereview.chromium.org/1318173002/diff/20001/chrome/browser/media/me...
>> File chrome/browser/media/media_permission.cc (right):
>>
>>
>>
https://codereview.chromium.org/1318173002/diff/20001/chrome/browser/media/me...
>> chrome/browser/media/media_permission.cc:100: origin_));
>> Ahh, sorry, that makes sense and I agree with you.
>>
>> Unfortunately we don't have access to the embedder easily here - we
>> would have to modify MediaStreamRequest and pass it through. We should
>> add a TODO at least but it might be best done separately.
>>
>> https://codereview.chromium.org/1318173002/
>>
>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[guoweis_left_chromium, 2015-09-01 07:08:11]

Patchset #3 (id:40001) has been deleted

[guoweis_left_chromium, 2015-09-01 07:08:52]

PTAL.

Will need to add test case later.

https://codereview.chromium.org/1318173002/diff/20001/chrome/browser/media/me...
File chrome/browser/media/media_permission.cc (right):

https://codereview.chromium.org/1318173002/diff/20001/chrome/browser/media/me...
chrome/browser/media/media_permission.cc:16: namespace {
On 2015/08/31 01:51:45, raymes wrote:
> nit: newline below

Done.

https://codereview.chromium.org/1318173002/diff/20001/chrome/browser/media/me...
chrome/browser/media/media_permission.cc:93: if (profile_) {
On 2015/08/28 10:51:35, Mounir Lamouri wrote:
> Why are you checking for |profile_| while the previous code didn't?

Done.

https://codereview.chromium.org/1318173002/diff/20001/chrome/browser/media/me...
chrome/browser/media/media_permission.cc:96: if (permission_manager) {
On 2015/08/28 10:51:35, Mounir Lamouri wrote:
> Maybe:
> if (!permission_manager)
>   return CONTENT_SETTING_BLOCK;

Done.

https://codereview.chromium.org/1318173002/diff/20001/chrome/browser/media/me...
chrome/browser/media/media_permission.cc:100: origin_));
On 2015/08/31 13:09:58, Mounir Lamouri wrote:
> On 2015/08/29 at 00:09:02, raymes wrote:
> > Mounir, you should read my replies :) As I mentioned in the other CL, the
> current behavior for microphone and video permissions is not to consider the
> embedder. Even if we wanted to make the behavioral change, this CL wouldn't be
> the best place to do it :)
> 
> I remember your comment. Though, I would prefer the PermissionContext to make
> that decision, not the callers. That way, we can easily change the policy
> without having to track all the callers.

Done.

https://codereview.chromium.org/1318173002/diff/20001/chrome/browser/media/me...
chrome/browser/media/media_permission.cc:115: }
On 2015/08/31 01:51:45, raymes wrote:
> I think this should be moved into the PermissionContext too, otherwise it's
> going to return the wrong result for some requests. We can't pass in
> request_type_ but what we can do is create an extra method hanging off the
> PermissionContext: GetPermissionStatusForPepper() which would be called if
> request_type == content:MEDIA_OPEN_DEVICE. We would change
> ShouldPersistContentSetting to accept a bool is_pepper_request instead of
> request_type.

We can't use PermissionManager for this. Instead, we use PermissionContext::Get
and cast it to the MediaStreamDevicePermissionContext type for this.

[guoweis_left_chromium, 2015-09-01 19:16:59]

guoweis@chromium.org changed reviewers:
- xhwang@chromium.org

[guoweis_left_chromium, 2015-09-01 20:54:06]

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

[guoweis_left_chromium, 2015-09-01 20:54:06]

tommi@chromium.org: PTAL. This one is based on
https://codereview.chromium.org/1311783007

[tommi (sloooow) - chröme, 2015-09-01 21:30:45]

lgtm

[guoweis_left_chromium, 2015-09-01 23:31:18]

test cases added for pepper scenarios.

Mounir and Raymes, PTAL before I land this. Thanks.

[raymes, 2015-09-02 04:15:34]

This looks really good to me! Thank you! :)

A few small comments below but otherwise lgtm.

https://codereview.chromium.org/1318173002/diff/80001/chrome/browser/media/me...
File chrome/browser/media/media_permission.cc (right):

https://codereview.chromium.org/1318173002/diff/80001/chrome/browser/media/me...
chrome/browser/media/media_permission.cc:31: }
Can you make this not a switch statement? I'm looking at making
ContentSettingsType not be an enum soon. Plus I think it would be simpler
anyway:

if (content_setting == CONTENT_SETTINGS_TYPE_MEDIASTREAM_MIC) {
  return content::PermissionType::AUDIO_CAPTURE;
} else {
  DCHECK_EQ(CONTENT_SETTINGS_TYPE_MEDIASTREAM_CAMERA, content_setting);
  return content::PermissionType::VIDEO_CAPTURE;
}

https://codereview.chromium.org/1318173002/diff/80001/chrome/browser/media/me...
File chrome/browser/media/media_stream_device_permission_context.h (right):

https://codereview.chromium.org/1318173002/diff/80001/chrome/browser/media/me...
chrome/browser/media/media_stream_device_permission_context.h:36: // For Pepper
request, insecure origin is allowed to be persisted, hence we
insecure origins are allowed to be persisted

https://codereview.chromium.org/1318173002/diff/80001/chrome/browser/media/me...
chrome/browser/media/media_stream_device_permission_context.h:37: // can't just
ask for permission when the requesting origin is insecure. See
can't just call GetPermissionStatus when the requesting origin is insecure.

[guoweis_left_chromium, 2015-09-02 05:54:12]

Thanks. I'm landing this now.

https://codereview.chromium.org/1318173002/diff/80001/chrome/browser/media/me...
File chrome/browser/media/media_permission.cc (right):

https://codereview.chromium.org/1318173002/diff/80001/chrome/browser/media/me...
chrome/browser/media/media_permission.cc:31: }
On 2015/09/02 04:15:33, raymes wrote:
> Can you make this not a switch statement? I'm looking at making
> ContentSettingsType not be an enum soon. Plus I think it would be simpler
> anyway:
> 
> if (content_setting == CONTENT_SETTINGS_TYPE_MEDIASTREAM_MIC) {
>   return content::PermissionType::AUDIO_CAPTURE;
> } else {
>   DCHECK_EQ(CONTENT_SETTINGS_TYPE_MEDIASTREAM_CAMERA, content_setting);
>   return content::PermissionType::VIDEO_CAPTURE;
> }

Done.

https://codereview.chromium.org/1318173002/diff/80001/chrome/browser/media/me...
File chrome/browser/media/media_stream_device_permission_context.h (right):

https://codereview.chromium.org/1318173002/diff/80001/chrome/browser/media/me...
chrome/browser/media/media_stream_device_permission_context.h:36: // For Pepper
request, insecure origin is allowed to be persisted, hence we
On 2015/09/02 04:15:33, raymes wrote:
> insecure origins are allowed to be persisted

Done.

https://codereview.chromium.org/1318173002/diff/80001/chrome/browser/media/me...
chrome/browser/media/media_stream_device_permission_context.h:37: // can't just
ask for permission when the requesting origin is insecure. See
On 2015/09/02 04:15:33, raymes wrote:
> can't just call GetPermissionStatus when the requesting origin is insecure.

Done.

[guoweis_left_chromium, 2015-09-02 06:21:46]

The CQ bit was checked by guoweis@chromium.org

[guoweis_left_chromium, 2015-09-02 06:21:46]

The patchset sent to the CQ was uploaded after l-g-t-m from tommi@chromium.org,
raymes@chromium.org
Link to the patchset: https://codereview.chromium.org/1318173002/#ps100001
(title: " ")

[commit-bot: I haz the power, 2015-09-02 06:22:01]

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

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

Committed patchset #5 (id:100001)

[commit-bot: I haz the power, 2015-09-02 07:05:00]

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