Refactor the content interface for RequestMediaAccessPermission.

Refactor the content interface for RequestMediaAccessPermission.

This makes RequestMediaAccessPermission a part of WebContentsDelegate instead
of ContentBrowserClient. The request UI (or lack thereof) needs to be
controlled by the WebContentsDelegate so that platform apps can grant permission
differently than a normal browser. This also seems appropriate because
each request is made on behalf of a particular web contents.

BUG=130216
TEST=navigator.webkitGetUserMedia still works for normal web pages

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=141792

[Evan Stade, 2012-06-09 05:47:38]

http://codereview.chromium.org/10542092/diff/5001/chrome/browser/chrome_conte...
File chrome/browser/chrome_content_browser_client.cc (left):

http://codereview.chromium.org/10542092/diff/5001/chrome/browser/chrome_conte...
chrome/browser/chrome_content_browser_client.cc:1117: 
it also would have been possible to just do

content->GetDelegate()->RequestMediaAcecssPermission(...)

here. That would have required a smaller change, and a little less code overall,
but it would have required two RequestMediaAccessPermission functions in the
content api.

http://codereview.chromium.org/10542092/diff/5001/chrome/browser/ui/extension...
File chrome/browser/ui/extensions/shell_window.cc (right):

http://codereview.chromium.org/10542092/diff/5001/chrome/browser/ui/extension...
chrome/browser/ui/extensions/shell_window.cc:114:
callback.Run(content::MediaStreamDevices());
not yet implemented.

http://codereview.chromium.org/10542092/diff/5001/content/browser/renderer_ho...
File content/browser/renderer_host/render_view_host_delegate.h (right):

http://codereview.chromium.org/10542092/diff/5001/content/browser/renderer_ho...
content/browser/renderer_host/render_view_host_delegate.h:394: // The render
view host has requested access to media devices listed in
s/render view host/render view/

[jochen (gone - plz use gerrit), 2012-06-09 07:55:21]

http://codereview.chromium.org/10542092/diff/5001/chrome/browser/ui/browser.cc
File chrome/browser/ui/browser.cc (right):

http://codereview.chromium.org/10542092/diff/5001/chrome/browser/ui/browser.c...
chrome/browser/ui/browser.cc:3693: TabContentsWrapper* tab =
TabContentsWrapper is called TabContents these days

http://codereview.chromium.org/10542092/diff/5001/content/browser/renderer_ho...
File content/browser/renderer_host/media/media_stream_device_settings.cc
(right):

http://codereview.chromium.org/10542092/diff/5001/content/browser/renderer_ho...
content/browser/renderer_host/media/media_stream_device_settings.cc:339: void
MediaStreamDeviceSettings::DoRequest(
could this be a helper function in an anon namespace?

[jam, 2012-06-11 05:31:57]

http://codereview.chromium.org/10542092/diff/5001/chrome/browser/ui/browser.cc
File chrome/browser/ui/browser.cc (right):

http://codereview.chromium.org/10542092/diff/5001/chrome/browser/ui/browser.c...
chrome/browser/ui/browser.cc:3695: DCHECK(tab);
nit: no need for dcheck, if it's null the crash in the next line is just as
useful of a signal to the developer

http://codereview.chromium.org/10542092/diff/5001/chrome/browser/ui/browser.h
File chrome/browser/ui/browser.h (right):

http://codereview.chromium.org/10542092/diff/5001/chrome/browser/ui/browser.h...
chrome/browser/ui/browser.h:1009: 
nit: please take out this blank line, and the couple of ones above. convention
is not to put blank lines between overridden functions

http://codereview.chromium.org/10542092/diff/5001/content/browser/renderer_ho...
File content/browser/renderer_host/render_view_host_delegate.h (right):

http://codereview.chromium.org/10542092/diff/5001/content/browser/renderer_ho...
content/browser/renderer_host/render_view_host_delegate.h:398: const
content::MediaStreamRequest* request,
nit: here and below, no "content::" since this is already in the content
namespace

http://codereview.chromium.org/10542092/diff/5001/content/public/common/media...
File content/public/common/media_stream_request.h (right):

http://codereview.chromium.org/10542092/diff/5001/content/public/common/media...
content/public/common/media_stream_request.h:49: typedef base::Callback<
void(const MediaStreamDevices&) > MediaResponseCallback;
nit: I realize you put this here because it's used by both
render_view_host_delegate.h and web_contents_delegate.h. but this is an odd
place, since 1) it's only used in \browser, and 2) it's not used here. I think
it would be better to duplicate it in the RVHD.

[macourteau, 2012-06-11 15:07:09]

Moving RequestMediaAccessPermission from ContentBrowserClient to
WebContentsDelegate lgtm

Nit: it might worth adding comments saying that  you're effectively denying the
device request at the few places in the code where the MediaResponseCallback is
called with an empty list of devices (unless you believe that it's clear that
this is what's happening).

[Evan Stade, 2012-06-12 00:00:45]

http://codereview.chromium.org/10542092/diff/5001/chrome/browser/ui/browser.cc
File chrome/browser/ui/browser.cc (right):

http://codereview.chromium.org/10542092/diff/5001/chrome/browser/ui/browser.c...
chrome/browser/ui/browser.cc:3693: TabContentsWrapper* tab =
On 2012/06/09 07:55:21, jochen wrote:
> TabContentsWrapper is called TabContents these days

Done.

http://codereview.chromium.org/10542092/diff/5001/chrome/browser/ui/browser.c...
chrome/browser/ui/browser.cc:3695: DCHECK(tab);
On 2012/06/11 05:31:57, John Abd-El-Malek wrote:
> nit: no need for dcheck, if it's null the crash in the next line is just as
> useful of a signal to the developer

I think of DCHECK as documentation. If we remove it, a future developer will
perhaps conclude that it's an oversight that the NULL tab case isn't handled.

http://codereview.chromium.org/10542092/diff/5001/chrome/browser/ui/browser.h
File chrome/browser/ui/browser.h (right):

http://codereview.chromium.org/10542092/diff/5001/chrome/browser/ui/browser.h...
chrome/browser/ui/browser.h:1009: 
On 2012/06/11 05:31:57, John Abd-El-Malek wrote:
> nit: please take out this blank line, and the couple of ones above. convention
> is not to put blank lines between overridden functions

Done.

http://codereview.chromium.org/10542092/diff/5001/content/browser/renderer_ho...
File content/browser/renderer_host/media/media_stream_device_settings.cc
(right):

http://codereview.chromium.org/10542092/diff/5001/content/browser/renderer_ho...
content/browser/renderer_host/media/media_stream_device_settings.cc:339: void
MediaStreamDeviceSettings::DoRequest(
On 2012/06/09 07:55:21, jochen wrote:
> could this be a helper function in an anon namespace?

indeed I suppose it could

http://codereview.chromium.org/10542092/diff/5001/content/browser/renderer_ho...
File content/browser/renderer_host/render_view_host_delegate.h (right):

http://codereview.chromium.org/10542092/diff/5001/content/browser/renderer_ho...
content/browser/renderer_host/render_view_host_delegate.h:398: const
content::MediaStreamRequest* request,
On 2012/06/11 05:31:57, John Abd-El-Malek wrote:
> nit: here and below, no "content::" since this is already in the content
> namespace

Done.

http://codereview.chromium.org/10542092/diff/5001/content/public/common/media...
File content/public/common/media_stream_request.h (right):

http://codereview.chromium.org/10542092/diff/5001/content/public/common/media...
content/public/common/media_stream_request.h:49: typedef base::Callback<
void(const MediaStreamDevices&) > MediaResponseCallback;
On 2012/06/11 05:31:57, John Abd-El-Malek wrote:
> nit: I realize you put this here because it's used by both
> render_view_host_delegate.h and web_contents_delegate.h. but this is an odd
> place, since 1) it's only used in \browser, and 2) it's not used here. I think
> it would be better to duplicate it in the RVHD.

done, although it leads to the somewhat funky include of web_contents_delegate.h
in media_stream_infobar_delegate.h

[Evan Stade, 2012-06-12 00:05:31]

On 2012/06/11 15:07:09, macourteau wrote:
> Moving RequestMediaAccessPermission from ContentBrowserClient to
> WebContentsDelegate lgtm
> 
> Nit: it might worth adding comments saying that  you're effectively denying
the
> device request at the few places in the code where the MediaResponseCallback
is
> called with an empty list of devices (unless you believe that it's clear that
> this is what's happening).

yea, I think it's apparent.

[jam, 2012-06-12 00:29:06]

http://codereview.chromium.org/10542092/diff/5001/chrome/browser/ui/browser.cc
File chrome/browser/ui/browser.cc (right):

http://codereview.chromium.org/10542092/diff/5001/chrome/browser/ui/browser.c...
chrome/browser/ui/browser.cc:3695: DCHECK(tab);
On 2012/06/12 00:00:46, Evan Stade wrote:
> On 2012/06/11 05:31:57, John Abd-El-Malek wrote:
> > nit: no need for dcheck, if it's null the crash in the next line is just as
> > useful of a signal to the developer
> 
> I think of DCHECK as documentation. If we remove it, a future developer will
> perhaps conclude that it's an oversight that the NULL tab case isn't handled.

it seems that if a developer reading this assumes that whoever wrote the code is
competant, it's also documenting that using a pointer means they expect it to
not be null.

taken to the extreme, what happens with this convention is that every single
function starts with DCHECKs for all the pointers. from expereince in a large
project that did this, it didn't improve quality or reduce crashes, just added a
lot of rote coding.

since this isn't in the style guide, I'll leave it up to you after this
information

http://codereview.chromium.org/10542092/diff/10002/content/public/common/medi...
File content/public/common/media_stream_request.h (right):

http://codereview.chromium.org/10542092/diff/10002/content/public/common/medi...
content/public/common/media_stream_request.h:13: #include "base/callback.h"
nit: not needed anymore

[jam, 2012-06-12 00:29:15]

(meant to say lgtm)

[Mihai Parparita -not on Chrome, 2012-06-12 02:57:31]

http://codereview.chromium.org/10542092/diff/10002/chrome/browser/chrome_cont...
File chrome/browser/chrome_content_browser_client.cc (right):

http://codereview.chromium.org/10542092/diff/10002/chrome/browser/chrome_cont...
chrome/browser/chrome_content_browser_client.cc:1138: if
(io_data->GetExtensionInfoMap()->SecurityOriginHasAPIPermission(
BTW, did you consider this approach (implementing the permission check in
ChromeContentBrowserClient by looking if an extension is making the request)?

The one advantage that I see is that then all extension views would get this new
permission, instead of making all of the WebContentsDelegate implementations in
the extension system repeat the checking code.

[Evan Stade, 2012-06-12 17:34:17]

ping Jochen

http://codereview.chromium.org/10542092/diff/10002/chrome/browser/chrome_cont...
File chrome/browser/chrome_content_browser_client.cc (right):

http://codereview.chromium.org/10542092/diff/10002/chrome/browser/chrome_cont...
chrome/browser/chrome_content_browser_client.cc:1138: if
(io_data->GetExtensionInfoMap()->SecurityOriginHasAPIPermission(
On 2012/06/12 02:57:31, Mihai Parparita wrote:
> BTW, did you consider this approach (implementing the permission check in
> ChromeContentBrowserClient by looking if an extension is making the request)?
> 
> The one advantage that I see is that then all extension views would get this
new
> permission, instead of making all of the WebContentsDelegate implementations
in
> the extension system repeat the checking code.

we could share the checking code without much trouble if we decide that's what
we want, however it's not clear to me that's the case. Hosted apps probably want
to continue going the infobar route to allow users to choose the input device.
(indeed "choosing" the input device requires more LOC than checking the
permissions)

http://codereview.chromium.org/10542092/diff/10002/content/public/common/medi...
File content/public/common/media_stream_request.h (right):

http://codereview.chromium.org/10542092/diff/10002/content/public/common/medi...
content/public/common/media_stream_request.h:13: #include "base/callback.h"
On 2012/06/12 00:29:06, John Abd-El-Malek wrote:
> nit: not needed anymore

Done.

[jochen (gone - plz use gerrit), 2012-06-12 20:44:45]

lgtm

[Evan Stade, 2012-06-12 22:55:11]

Mihai: lgty? (need OWNERS for extensions/)
Scott: OWNERS for ui/browser.* please

[sky, 2012-06-12 23:42:30]

LGTM

[Mihai Parparita -not on Chrome, 2012-06-12 23:48:04]

LGTM