Check media permissions through RenderFrameHostDelegate.

Check media permissions through RenderFrameHostDelegate.

* Add function CheckMediaAccessPermission on RenderFrameHostDelegate, WebContentsDelegate, classes implementing those and chain it to MediaCaptureDevicesDispatcher::CheckMediaAccessPermission through the different paths. (Essentially add this function side-by-side with RequestMediaAccessPermission.)
* Change MediaStreamManager to use this function for checking permission when determining if device labels should be visible.

BUG=406094
Committed: https://crrev.com/657d4d8c7d26c89c0846479ac322931bfaba4f86
Cr-Commit-Position: refs/heads/master@{#295379}

[Henrik Grunell, 2014-09-11 16:23:27]

grunell@chromium.org changed reviewers:
+ fsamuel@chromium.org, perkj@chromium.org

[Henrik Grunell, 2014-09-11 16:28:41]

This CL isn't entirely ready yet.

Per: can you take a quick overview look?

fsamuel@: I have a question about

chrome/browser/guest_view/web_view/chrome_web_view_permission_helper_delegate.cc

See my todo, what does web_view_permission_helper()->RequestPermission() do?
What's the purpose of that? The addition I'm doing is to check permission, not
request. Do I need to do something else than just call the delegate as I'm doing
now?

I also need to:
* Look at all WebContentsDelegate implementations and see if the new function
should be added there. (Everywhere we have correspinding Request... function.)
* Clean up.
* Unit tests where applicable.

[Henrik Grunell, 2014-09-12 15:06:04]

grunell@chromium.org changed reviewers:
+ jochen@chromium.org, sky@chromium.org, yoz@chromium.org

[Henrik Grunell, 2014-09-12 15:10:57]

grunell@chromium.org changed reviewers:
+ avi@chromium.org
- jochen@chromium.org

[Henrik Grunell, 2014-09-12 15:18:12]

Hi! This CL is ready for review. It's a little monster, but most files are just
pass-through.

yoz@: apps/ and extensions/
perkj@: content/browser/renderer_host/media/
avi@: content/ except above.
sky@: athena/ och chrome/

One particular question I have is for
chrome/browser/guest_view/web_view/chrome_web_view_permission_helper_delegate.cc

What does web_view_permission_helper()->RequestPermission() do? What's the
purpose of that? The addition I'm doing is to check permission, not request. Do
I need to do something else than just call the delegate as I'm doing now?

[Fady Samuel, 2014-09-12 15:19:14]

https://codereview.chromium.org/562263002/diff/1/chrome/browser/guest_view/we...
File
chrome/browser/guest_view/web_view/chrome_web_view_permission_helper_delegate.cc
(right):

https://codereview.chromium.org/562263002/diff/1/chrome/browser/guest_view/we...
chrome/browser/guest_view/web_view/chrome_web_view_permission_helper_delegate.cc:145:
// TODO - BEFORE COMMIT: I guess we need to call web_view_permission_helper()
RequestPermission triggers the webview permissionrequest API

https://developer.chrome.com/apps/tags/webview#type-MediaPermissionRequest

[Avi (use Gerrit), 2014-09-12 15:44:16]

lgtm

[sky, 2014-09-12 15:59:46]

sky@chromium.org changed reviewers:
+ oshima@chromium.org
- sky@chromium.org

[sky, 2014-09-12 15:59:59]

sky->oshima (for athena)

[Henrik Grunell, 2014-09-12 16:04:26]

grunell@chromium.org changed reviewers:
+ sky@chromium.org

[Henrik Grunell, 2014-09-12 16:04:57]

On 2014/09/12 15:59:59, sky wrote:
> sky->oshima (for athena)

Scott, would you mind reviewing //chrome?

[Henrik Grunell, 2014-09-12 16:10:35]

https://codereview.chromium.org/562263002/diff/1/chrome/browser/guest_view/we...
File
chrome/browser/guest_view/web_view/chrome_web_view_permission_helper_delegate.cc
(right):

https://codereview.chromium.org/562263002/diff/1/chrome/browser/guest_view/we...
chrome/browser/guest_view/web_view/chrome_web_view_permission_helper_delegate.cc:145:
// TODO - BEFORE COMMIT: I guess we need to call web_view_permission_helper()
On 2014/09/12 15:19:13, Fady Samuel wrote:
> RequestPermission triggers the webview permissionrequest API
> 
> https://developer.chrome.com/apps/tags/webview#type-MediaPermissionRequest

OK. This check should not trigger any request, just check what permission is
currently granted or not. It sounds like I'll just skip that then.

But I wonder why RequestPermission (in RequestMediaAccessPermission) is called
before RequestMediaAccessPermission (in OnMediaPermissionResponse)? When
RequestMediaAccessPermission is called, e.g. the policy will be checked.
Shouldn't that be done before?

[sky, 2014-09-12 16:19:14]

https://codereview.chromium.org/562263002/diff/60001/chrome/browser/guest_vie...
File
chrome/browser/guest_view/web_view/chrome_web_view_permission_helper_delegate.cc
(right):

https://codereview.chromium.org/562263002/diff/60001/chrome/browser/guest_vie...
chrome/browser/guest_view/web_view/chrome_web_view_permission_helper_delegate.cc:145:
// TODO - BEFORE COMMIT: I guess we need to call web_view_permission_helper()
You need to resolve this.

https://codereview.chromium.org/562263002/diff/60001/chrome/browser/ui/media_...
File chrome/browser/ui/media_utils.cc (right):

https://codereview.chromium.org/562263002/diff/60001/chrome/browser/ui/media_...
chrome/browser/ui/media_utils.cc:24: #if defined(ENABLE_EXTENSIONS)
How come you check ENABLE_EXTENSIONS here, but not @ 46 or 55-62? Aren't those
places using extensions bits too?

https://codereview.chromium.org/562263002/diff/60001/content/browser/web_cont...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/562263002/diff/60001/content/browser/web_cont...
content/browser/web_contents/web_contents_impl.cc:1760: else
nit: no else here, or you can make a single return for the whole function, eg:
return delegate_ && delegate->CheckM...

[Yoyo Zhou, 2014-09-12 18:30:40]

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

[Yoyo Zhou, 2014-09-12 18:30:41]

The extensions and apps mostly LGTM, but +jamescook to sanity check
media_capture_util

https://codereview.chromium.org/562263002/diff/60001/extensions/browser/exten...
File extensions/browser/extension_host_delegate.h (right):

https://codereview.chromium.org/562263002/diff/60001/extensions/browser/exten...
extensions/browser/extension_host_delegate.h:60: virtual bool
CheckMediaAccessPermission(content::WebContents* web_contents,
needs function documentation

https://codereview.chromium.org/562263002/diff/60001/extensions/browser/guest...
File
extensions/browser/guest_view/web_view/web_view_permission_helper_delegate.cc
(right):

https://codereview.chromium.org/562263002/diff/60001/extensions/browser/guest...
extensions/browser/guest_view/web_view/web_view_permission_helper_delegate.cc:25:
return false;
I'm not sure why this would return false, so it's best to document it.

[oshima, 2014-09-12 20:14:59]

athena/ lgtm

[James Cook, 2014-09-12 20:43:12]

LGTM with one optional suggestion for media_capture_util.cc

https://codereview.chromium.org/562263002/diff/60001/extensions/shell/browser...
File extensions/shell/browser/media_capture_util.cc (right):

https://codereview.chromium.org/562263002/diff/60001/extensions/shell/browser...
extensions/shell/browser/media_capture_util.cc:48: // app_shell has no UI
surface to show an error, and on an embedded device
Why not call VerifyMediaAccessPermission(request.audio_type, extension) here?

https://codereview.chromium.org/562263002/diff/60001/extensions/shell/browser...
extensions/shell/browser/media_capture_util.cc:61: // See
APIPermission::kAudioCapture check above.
ditto for request.video_type here?

[Fady Samuel, 2014-09-12 20:46:33]

https://codereview.chromium.org/562263002/diff/1/chrome/browser/guest_view/we...
File
chrome/browser/guest_view/web_view/chrome_web_view_permission_helper_delegate.cc
(right):

https://codereview.chromium.org/562263002/diff/1/chrome/browser/guest_view/we...
chrome/browser/guest_view/web_view/chrome_web_view_permission_helper_delegate.cc:145:
// TODO - BEFORE COMMIT: I guess we need to call web_view_permission_helper()
On 2014/09/12 16:10:34, Henrik Grunell wrote:
> On 2014/09/12 15:19:13, Fady Samuel wrote:
> > RequestPermission triggers the webview permissionrequest API
> > 
> > https://developer.chrome.com/apps/tags/webview#type-MediaPermissionRequest
> 
> OK. This check should not trigger any request, just check what permission is
> currently granted or not. It sounds like I'll just skip that then.
> 
> But I wonder why RequestPermission (in RequestMediaAccessPermission) is called
> before RequestMediaAccessPermission (in OnMediaPermissionResponse)? When
> RequestMediaAccessPermission is called, e.g. the policy will be checked.
> Shouldn't that be done before?

Could you please remind me when this code path is triggered?

Yes, we should be checking whether the embedder has permission before initiating
the javascript API. Is this possible though? We would want to do that check
before returning to the guest.

[Henrik Grunell, 2014-09-15 11:33:48]

https://codereview.chromium.org/562263002/diff/1/chrome/browser/guest_view/we...
File
chrome/browser/guest_view/web_view/chrome_web_view_permission_helper_delegate.cc
(right):

https://codereview.chromium.org/562263002/diff/1/chrome/browser/guest_view/we...
chrome/browser/guest_view/web_view/chrome_web_view_permission_helper_delegate.cc:145:
// TODO - BEFORE COMMIT: I guess we need to call web_view_permission_helper()
On 2014/09/12 20:46:33, Fady Samuel wrote:
> On 2014/09/12 16:10:34, Henrik Grunell wrote:
> > On 2014/09/12 15:19:13, Fady Samuel wrote:
> > > RequestPermission triggers the webview permissionrequest API
> > > 
> > > https://developer.chrome.com/apps/tags/webview#type-MediaPermissionRequest
> > 
> > OK. This check should not trigger any request, just check what permission is
> > currently granted or not. It sounds like I'll just skip that then.
> > 
> > But I wonder why RequestPermission (in RequestMediaAccessPermission) is
called
> > before RequestMediaAccessPermission (in OnMediaPermissionResponse)? When
> > RequestMediaAccessPermission is called, e.g. the policy will be checked.
> > Shouldn't that be done before?
> 
> Could you please remind me when this code path is triggered?

For the RequestMediaAccessPermission case, that's triggered when the JS app
calls Navigator.getUserMedia to be able to access camera and/or mic.

For the CheckMediaAccessPermission case, it's when the JS app calls
MediaStreamTrack.getSources (static) to enumerate mic and camera devices.

Both cases go through MediaStreamManager.

> 
> Yes, we should be checking whether the embedder has permission before
initiating
> the javascript API. Is this possible though? We would want to do that check
> before returning to the guest.

I honestly don't quite follow this. I'll ping you to chat about this to sort it
out.

https://codereview.chromium.org/562263002/diff/60001/chrome/browser/guest_vie...
File
chrome/browser/guest_view/web_view/chrome_web_view_permission_helper_delegate.cc
(right):

https://codereview.chromium.org/562263002/diff/60001/chrome/browser/guest_vie...
chrome/browser/guest_view/web_view/chrome_web_view_permission_helper_delegate.cc:145:
// TODO - BEFORE COMMIT: I guess we need to call web_view_permission_helper()
On 2014/09/12 16:19:14, sky wrote:
> You need to resolve this.

Done.

https://codereview.chromium.org/562263002/diff/60001/chrome/browser/ui/media_...
File chrome/browser/ui/media_utils.cc (right):

https://codereview.chromium.org/562263002/diff/60001/chrome/browser/ui/media_...
chrome/browser/ui/media_utils.cc:24: #if defined(ENABLE_EXTENSIONS)
On 2014/09/12 16:19:14, sky wrote:
> How come you check ENABLE_EXTENSIONS here, but not @ 46 or 55-62? Aren't those
> places using extensions bits too?

The ProcessMediaAccessRequest and CheckMediaAccessPermission should take
|extension| == NULL as argument if the origin is not an extension. So 55-58
could use it. Changed.

https://codereview.chromium.org/562263002/diff/60001/content/browser/web_cont...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/562263002/diff/60001/content/browser/web_cont...
content/browser/web_contents/web_contents_impl.cc:1760: else
On 2014/09/12 16:19:14, sky wrote:
> nit: no else here, or you can make a single return for the whole function, eg:
> return delegate_ && delegate->CheckM...

Done.

https://codereview.chromium.org/562263002/diff/60001/extensions/browser/exten...
File extensions/browser/extension_host_delegate.h (right):

https://codereview.chromium.org/562263002/diff/60001/extensions/browser/exten...
extensions/browser/extension_host_delegate.h:60: virtual bool
CheckMediaAccessPermission(content::WebContents* web_contents,
On 2014/09/12 18:30:40, Yoyo Zhou wrote:
> needs function documentation

Done.

https://codereview.chromium.org/562263002/diff/60001/extensions/browser/guest...
File
extensions/browser/guest_view/web_view/web_view_permission_helper_delegate.cc
(right):

https://codereview.chromium.org/562263002/diff/60001/extensions/browser/guest...
extensions/browser/guest_view/web_view/web_view_permission_helper_delegate.cc:25:
return false;
On 2014/09/12 18:30:40, Yoyo Zhou wrote:
> I'm not sure why this would return false, so it's best to document it.

The default implementation should be defensive for privacy reasons. Added
comment.

https://codereview.chromium.org/562263002/diff/60001/extensions/shell/browser...
File extensions/shell/browser/media_capture_util.cc (right):

https://codereview.chromium.org/562263002/diff/60001/extensions/shell/browser...
extensions/shell/browser/media_capture_util.cc:48: // app_shell has no UI
surface to show an error, and on an embedded device
On 2014/09/12 20:43:11, James Cook wrote:
> Why not call VerifyMediaAccessPermission(request.audio_type, extension) here?

Yep, thanks, Done.

https://codereview.chromium.org/562263002/diff/60001/extensions/shell/browser...
extensions/shell/browser/media_capture_util.cc:61: // See
APIPermission::kAudioCapture check above.
On 2014/09/12 20:43:11, James Cook wrote:
> ditto for request.video_type here?

Done.

[perkj_chrome, 2014-09-15 12:06:25]

https://codereview.chromium.org/562263002/diff/80001/content/browser/frame_ho...
File content/browser/frame_host/render_frame_host_delegate.cc (right):

https://codereview.chromium.org/562263002/diff/80001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_delegate.cc:50: DCHECK(type ==
MEDIA_DEVICE_AUDIO_CAPTURE ||
NOTREACHED()?

https://codereview.chromium.org/562263002/diff/80001/content/browser/renderer...
File
content/browser/renderer_host/media/media_stream_dispatcher_host_unittest.cc
(right):

https://codereview.chromium.org/562263002/diff/80001/content/browser/renderer...
content/browser/renderer_host/media/media_stream_dispatcher_host_unittest.cc:233:
/*
fix

https://codereview.chromium.org/562263002/diff/80001/content/browser/renderer...
content/browser/renderer_host/media/media_stream_dispatcher_host_unittest.cc:309:
//        static_cast<FakeMediaStreamUIProxy*>(stream_ui_));
fix

https://codereview.chromium.org/562263002/diff/80001/content/browser/renderer...
File content/browser/renderer_host/media/media_stream_ui_proxy.h (right):

https://codereview.chromium.org/562263002/diff/80001/content/browser/renderer...
content/browser/renderer_host/media/media_stream_ui_proxy.h:45: // this does not
query the user. |type| must be MEDIA_DEVICE_AUDIO_CAPTURE
Can you expand this. Ie, When should this be called and what does it mean that
this returns true or false? Ie, does false mean we should ask the user for
permission?

https://codereview.chromium.org/562263002/diff/80001/content/public/browser/w...
File content/public/browser/web_contents_delegate.cc (right):

https://codereview.chromium.org/562263002/diff/80001/content/public/browser/w...
content/public/browser/web_contents_delegate.cc:176: DCHECK(type ==
MEDIA_DEVICE_AUDIO_CAPTURE ||
Why are we ever getting here? Should this be NOTREACHED()?

[Henrik Grunell, 2014-09-15 13:45:17]

https://codereview.chromium.org/562263002/diff/80001/content/browser/frame_ho...
File content/browser/frame_host/render_frame_host_delegate.cc (right):

https://codereview.chromium.org/562263002/diff/80001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_delegate.cc:50: DCHECK(type ==
MEDIA_DEVICE_AUDIO_CAPTURE ||
On 2014/09/15 12:06:25, perkj wrote:
> NOTREACHED()?

I don't think we should force implementors to implement this function. (In that
case it should be pure virtual anyway.)

https://codereview.chromium.org/562263002/diff/80001/content/browser/renderer...
File
content/browser/renderer_host/media/media_stream_dispatcher_host_unittest.cc
(right):

https://codereview.chromium.org/562263002/diff/80001/content/browser/renderer...
content/browser/renderer_host/media/media_stream_dispatcher_host_unittest.cc:233:
/*
On 2014/09/15 12:06:25, perkj wrote:
> fix

Oh right, forgot to say that this test isn't done yet. Sorry. Will update it
later today.

https://codereview.chromium.org/562263002/diff/80001/content/browser/renderer...
content/browser/renderer_host/media/media_stream_dispatcher_host_unittest.cc:309:
//        static_cast<FakeMediaStreamUIProxy*>(stream_ui_));
On 2014/09/15 12:06:25, perkj wrote:
> fix

Same here.

https://codereview.chromium.org/562263002/diff/80001/content/browser/renderer...
File content/browser/renderer_host/media/media_stream_ui_proxy.h (right):

https://codereview.chromium.org/562263002/diff/80001/content/browser/renderer...
content/browser/renderer_host/media/media_stream_ui_proxy.h:45: // this does not
query the user. |type| must be MEDIA_DEVICE_AUDIO_CAPTURE
On 2014/09/15 12:06:25, perkj wrote:
> Can you expand this. Ie, When should this be called and what does it mean that
> this returns true or false? Ie, does false mean we should ask the user for
> permission?

I added that this checks any stored settings. Not sure how to explain further.
What to do with the result depends on who called it and why. The only use case
now is device enumeration; we just want to know if there's permission granted or
not.

https://codereview.chromium.org/562263002/diff/80001/content/public/browser/w...
File content/public/browser/web_contents_delegate.cc (right):

https://codereview.chromium.org/562263002/diff/80001/content/public/browser/w...
content/public/browser/web_contents_delegate.cc:176: DCHECK(type ==
MEDIA_DEVICE_AUDIO_CAPTURE ||
On 2014/09/15 12:06:25, perkj wrote:
> Why are we ever getting here? Should this be NOTREACHED()?

Not all classes that implement WebContentsDelegate have to implement this
function, but are happy with this default one.

[perkj_chrome, 2014-09-15 15:08:55]

https://codereview.chromium.org/562263002/diff/80001/content/browser/renderer...
File content/browser/renderer_host/media/media_stream_ui_proxy.h (right):

https://codereview.chromium.org/562263002/diff/80001/content/browser/renderer...
content/browser/renderer_host/media/media_stream_ui_proxy.h:45: // this does not
query the user. |type| must be MEDIA_DEVICE_AUDIO_CAPTURE
On 2014/09/15 13:45:17, Henrik Grunell wrote:
> On 2014/09/15 12:06:25, perkj wrote:
> > Can you expand this. Ie, When should this be called and what does it mean
that
> > this returns true or false? Ie, does false mean we should ask the user for
> > permission?
> 
> I added that this checks any stored settings. Not sure how to explain further.
> What to do with the result depends on who called it and why. The only use case
> now is device enumeration; we just want to know if there's permission granted
or
> not.

I was wondering about the " Note that
  45   // this does not query the user" part. What is it that make this return
true? |security_origin| has been granted access before by a user gesture? Or
since its an packaged app with the correct permissions?

[Henrik Grunell, 2014-09-15 18:20:16]

https://codereview.chromium.org/562263002/diff/80001/content/browser/renderer...
File content/browser/renderer_host/media/media_stream_ui_proxy.h (right):

https://codereview.chromium.org/562263002/diff/80001/content/browser/renderer...
content/browser/renderer_host/media/media_stream_ui_proxy.h:45: // this does not
query the user. |type| must be MEDIA_DEVICE_AUDIO_CAPTURE
On 2014/09/15 15:08:55, perkj wrote:
> On 2014/09/15 13:45:17, Henrik Grunell wrote:
> > On 2014/09/15 12:06:25, perkj wrote:
> > > Can you expand this. Ie, When should this be called and what does it mean
> that
> > > this returns true or false? Ie, does false mean we should ask the user for
> > > permission?
> > 
> > I added that this checks any stored settings. Not sure how to explain
further.
> > What to do with the result depends on who called it and why. The only use
case
> > now is device enumeration; we just want to know if there's permission
granted
> or
> > not.
> 
> I was wondering about the " Note that
>   45   // this does not query the user" part. What is it that make this return
> true? |security_origin| has been granted access before by a user gesture? Or
> since its an packaged app with the correct permissions? 

Yes, exactly. Either if the user has granted access before and it's stored in
the content settings, or the policy allows it, or the packaged app contains the
permission. It's exactly the same as for RequestAccess above, but without asking
the user.

[Fady Samuel, 2014-09-15 18:43:54]

After a chat offline, guest_view lgtm.

[Henrik Grunell, 2014-09-16 09:54:55]

Updated unit test. Per, ptal.

[perkj_chrome, 2014-09-16 10:39:04]

On 2014/09/16 09:54:55, Henrik Grunell wrote:
> Updated unit test. Per, ptal.

renderer_host/media lgtm.

[Henrik Grunell, 2014-09-16 12:35:31]

Scott, ptal.

[sky, 2014-09-16 16:33:25]

https://codereview.chromium.org/562263002/diff/100001/chrome/browser/ui/media...
File chrome/browser/ui/media_utils.cc (right):

https://codereview.chromium.org/562263002/diff/100001/chrome/browser/ui/media...
chrome/browser/ui/media_utils.cc:52: const extensions::Extension* extension =
NULL;
If ENABLE_EXTENSIONS is false, why are you passing around extensions::Extension?

[Henrik Grunell, 2014-09-16 17:14:15]

https://codereview.chromium.org/562263002/diff/100001/chrome/browser/ui/media...
File chrome/browser/ui/media_utils.cc (right):

https://codereview.chromium.org/562263002/diff/100001/chrome/browser/ui/media...
chrome/browser/ui/media_utils.cc:52: const extensions::Extension* extension =
NULL;
On 2014/09/16 16:33:25, sky wrote:
> If ENABLE_EXTENSIONS is false, why are you passing around
extensions::Extension?

Do you think yet another function is better or did you have anything else in
mind?

[sky, 2014-09-16 17:50:43]

Seems clearer to have a function that doesn't take an Exension if
ENABLE_EXTENSIONS is false.

On Tue, Sep 16, 2014 at 10:14 AM,  <grunell@chromium.org> wrote:
>
>
https://codereview.chromium.org/562263002/diff/100001/chrome/browser/ui/media...
> File chrome/browser/ui/media_utils.cc (right):
>
>
https://codereview.chromium.org/562263002/diff/100001/chrome/browser/ui/media...
> chrome/browser/ui/media_utils.cc:52: const extensions::Extension*
> extension = NULL;
> On 2014/09/16 16:33:25, sky wrote:
>>
>> If ENABLE_EXTENSIONS is false, why are you passing around
>
> extensions::Extension?
>
> Do you think yet another function is better or did you have anything
> else in mind?
>
> https://codereview.chromium.org/562263002/

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

[Henrik Grunell, 2014-09-16 20:30:15]

Any other comments besides this?
Den 16 sep 2014 19:50 skrev "Scott Violet" <sky@chromium.org>:

> Seems clearer to have a function that doesn't take an Exension if
> ENABLE_EXTENSIONS is false.
>
> On Tue, Sep 16, 2014 at 10:14 AM,  <grunell@chromium.org> wrote:
> >
> >
>
https://codereview.chromium.org/562263002/diff/100001/chrome/browser/ui/media...
> > File chrome/browser/ui/media_utils.cc (right):
> >
> >
>
https://codereview.chromium.org/562263002/diff/100001/chrome/browser/ui/media...
> > chrome/browser/ui/media_utils.cc:52: const extensions::Extension*
> > extension = NULL;
> > On 2014/09/16 16:33:25, sky wrote:
> >>
> >> If ENABLE_EXTENSIONS is false, why are you passing around
> >
> > extensions::Extension?
> >
> > Do you think yet another function is better or did you have anything
> > else in mind?
> >
> > https://codereview.chromium.org/562263002/
>

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

[sky, 2014-09-16 22:05:00]

No, that is my only remaining comment.

On Tue, Sep 16, 2014 at 1:30 PM, Henrik Grunell <grunell@chromium.org> wrote:
> Any other comments besides this?
>
> Den 16 sep 2014 19:50 skrev "Scott Violet" <sky@chromium.org>:
>
>> Seems clearer to have a function that doesn't take an Exension if
>> ENABLE_EXTENSIONS is false.
>>
>> On Tue, Sep 16, 2014 at 10:14 AM,  <grunell@chromium.org> wrote:
>> >
>> >
>> >
https://codereview.chromium.org/562263002/diff/100001/chrome/browser/ui/media...
>> > File chrome/browser/ui/media_utils.cc (right):
>> >
>> >
>> >
https://codereview.chromium.org/562263002/diff/100001/chrome/browser/ui/media...
>> > chrome/browser/ui/media_utils.cc:52: const extensions::Extension*
>> > extension = NULL;
>> > On 2014/09/16 16:33:25, sky wrote:
>> >>
>> >> If ENABLE_EXTENSIONS is false, why are you passing around
>> >
>> > extensions::Extension?
>> >
>> > Do you think yet another function is better or did you have anything
>> > else in mind?
>> >
>> > https://codereview.chromium.org/562263002/

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

[Henrik Grunell, 2014-09-17 08:27:30]

On 2014/09/16 17:50:43, sky wrote:
> Seems clearer to have a function that doesn't take an Exension if
> ENABLE_EXTENSIONS is false.

Done. Also added unite test for MediaStreamUIProxy.

> 
> On Tue, Sep 16, 2014 at 10:14 AM,  <mailto:grunell@chromium.org> wrote:
> >
> >
>
https://codereview.chromium.org/562263002/diff/100001/chrome/browser/ui/media...
> > File chrome/browser/ui/media_utils.cc (right):
> >
> >
>
https://codereview.chromium.org/562263002/diff/100001/chrome/browser/ui/media...
> > chrome/browser/ui/media_utils.cc:52: const extensions::Extension*
> > extension = NULL;
> > On 2014/09/16 16:33:25, sky wrote:
> >>
> >> If ENABLE_EXTENSIONS is false, why are you passing around
> >
> > extensions::Extension?
> >
> > Do you think yet another function is better or did you have anything
> > else in mind?
> >
> > https://codereview.chromium.org/562263002/
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[sky, 2014-09-17 15:46:48]

LGTM

https://codereview.chromium.org/562263002/diff/200001/chrome/browser/ui/media...
File chrome/browser/ui/media_utils.cc (right):

https://codereview.chromium.org/562263002/diff/200001/chrome/browser/ui/media...
chrome/browser/ui/media_utils.cc:61: } else {
nit: no else after a return (see style guide).

[Henrik Grunell, 2014-09-17 20:00:17]

https://codereview.chromium.org/562263002/diff/200001/chrome/browser/ui/media...
File chrome/browser/ui/media_utils.cc (right):

https://codereview.chromium.org/562263002/diff/200001/chrome/browser/ui/media...
chrome/browser/ui/media_utils.cc:61: } else {
On 2014/09/17 15:46:47, sky wrote:
> nit: no else after a return (see style guide).

Done.

[Henrik Grunell, 2014-09-17 20:01:00]

The CQ bit was checked by grunell@chromium.org

[commit-bot: I haz the power, 2014-09-17 20:01:52]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/562263002/240001

[commit-bot: I haz the power, 2014-09-17 22:02:43]

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

[commit-bot: I haz the power, 2014-09-17 22:02:45]

Exceeded time limit waiting for builds to trigger.

[Fady Samuel, 2014-09-17 23:38:28]

The CQ bit was checked by fsamuel@chromium.org

[commit-bot: I haz the power, 2014-09-17 23:39:08]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/562263002/240001

[commit-bot: I haz the power, 2014-09-18 00:09:51]

Committed patchset #13 (id:240001) as 829d93f2dbddc7c5a7b64d2ab59862e745b8674d

[commit-bot: I haz the power, 2014-09-18 00:10:27]

Patchset 13 (id:??) landed as
https://crrev.com/657d4d8c7d26c89c0846479ac322931bfaba4f86
Cr-Commit-Position: refs/heads/master@{#295379}