app_shell: Add audio and video capture support

app_shell: Add audio and video capture support

This allows HTML5/WebRTC audio and video capture to work inside apps.

* Apps in app_shell are given access to the first available
 camera and microphone via WebContentsDelegate::
 ProcessMediaAccessRequest().
* Introduce media_capture_util so code can be shared 
 between ShellExtensionHostDelegate (for audio capture from 
 background pages) and ShellAppWindow (for normal windows).
* Extract ShellExtensionHostDelegate into its own file 
 because it's a little too big to sit where it is.

BUG=404299
TEST=manual, see app attached to bug

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

[James Cook, 2014-08-20 18:17:16]

derat, PTAL

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

https://codereview.chromium.org/493453004/diff/20001/extensions/shell/browser...
extensions/shell/browser/media_capture_util.cc:18: // See also Chrome's
MediaCaptureDevicesDispatcher.
I looked at refactoring MediaCaptureDevicesDispatcher but it's tied to Chrome UI
concepts, tab/screen capture, injection of test devices, etc.  In theory some of
it could be pushed down into src/content and some pulled here, but I think it
would make all the pieces harder to understand.

https://codereview.chromium.org/493453004/diff/20001/extensions/shell/browser...
File extensions/shell/browser/shell_extension_host_delegate.cc (right):

https://codereview.chromium.org/493453004/diff/20001/extensions/shell/browser...
extensions/shell/browser/shell_extension_host_delegate.cc:51:
media_capture_util::GrantMediaStreamRequestWithFirstDevice(
This is the only line I added -- the rest is just moved.

[Daniel Erat, 2014-08-20 21:09:51]

lgtm! you might want to ask a media person to glance at it too, though.

[James Cook, 2014-08-20 21:25:26]

xians, PTAL at extensions/shell/browser/media_capture_util.*

[James Cook, 2014-08-21 23:14:02]

On 2014/08/20 21:25:26, James Cook wrote:
> xians, PTAL at extensions/shell/browser/media_capture_util.*

xians, friendly ping?

[Daniel Erat, 2014-08-21 23:15:40]

On 2014/08/21 23:14:02, James Cook wrote:
> On 2014/08/20 21:25:26, James Cook wrote:
> > xians, PTAL at extensions/shell/browser/media_capture_util.*
> 
> xians, friendly ping?

if it works, i think it's fine to check it in now and apply media-specific
comments in a followup change.

[no longer working on chromium, 2014-08-22 13:09:53]

https://codereview.chromium.org/493453004/diff/20001/extensions/shell/browser...
File extensions/shell/browser/shell_extension_host_delegate.cc (right):

https://codereview.chromium.org/493453004/diff/20001/extensions/shell/browser...
extensions/shell/browser/shell_extension_host_delegate.cc:51:
media_capture_util::GrantMediaStreamRequestWithFirstDevice(
I am not very sure, but shouldn't you do some kind of permission control and ui
notification here?
like ChromeExtensionHostDelegate::ProcessMediaAccessRequest, which will
eventually call 
MediaCaptureDevicesDispatcher::ProcessMediaAccessRequestFromPlatformAppOrExtension()

[James Cook, 2014-08-22 15:39:12]

On 2014/08/22 13:09:53, xians1 wrote:
>
https://codereview.chromium.org/493453004/diff/20001/extensions/shell/browser...
> File extensions/shell/browser/shell_extension_host_delegate.cc (right):
> 
>
https://codereview.chromium.org/493453004/diff/20001/extensions/shell/browser...
> extensions/shell/browser/shell_extension_host_delegate.cc:51:
> media_capture_util::GrantMediaStreamRequestWithFirstDevice(
> I am not very sure, but shouldn't you do some kind of permission control and
ui
> notification here?
> like ChromeExtensionHostDelegate::ProcessMediaAccessRequest, which will
> eventually call 
>
MediaCaptureDevicesDispatcher::ProcessMediaAccessRequestFromPlatformAppOrExtension()

This code can't call MediaCaptureDevicesDispatcher code because src/extensions
cannot depend on src/chrome.  Also, the app has permissions granted at install
time, so it doesn't need to show UI.  I'll look into doing the app permission
requests.  They're not strictly needed (app_shell only runs trusted apps, it's
not an arbitrary app runtime) but it might be nice to add.

[James Cook, 2014-08-22 16:52:14]

derat, please take another look at extensions/shell. I had to plumb an
Extension* into the ShellAppWindow so I could use it to check for the capture
permissions.

oshima, PTAL at athena.

[Daniel Erat, 2014-08-22 17:09:55]

lgtm for extensions/shell

https://codereview.chromium.org/493453004/diff/60001/extensions/shell/browser...
File extensions/shell/browser/shell_app_window.h (right):

https://codereview.chromium.org/493453004/diff/60001/extensions/shell/browser...
extensions/shell/browser/shell_app_window.h:48: gfx::Size initial_size);
nit: const ref, maybe

[oshima, 2014-08-22 17:13:59]

athena/ lgtm

[James Cook, 2014-08-22 17:16:57]

yoz, can I get OWNERS for chrome/common/extensions? I had to move a permissions
constant and strings to src/extensions.

[no longer working on chromium, 2014-08-22 19:22:27]

lgtm with two nits and one question. Thanks for fixing it.

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

https://codereview.chromium.org/493453004/diff/80001/extensions/shell/browser...
extensions/shell/browser/media_capture_util.cc:11: using
content::MediaCaptureDevices;
nit, add an empty line before using content::MediaCaptureDevices;

https://codereview.chromium.org/493453004/diff/80001/extensions/shell/browser...
extensions/shell/browser/media_capture_util.cc:36: if
(permissions_data->HasAPIPermission(APIPermission::kAudioCapture)) {
If I understand your comment correctly, app_shell only runs trusted apps, that
says the trust apps should only call this method if they have permission, if so,
should we add a CHECK here instead?

https://codereview.chromium.org/493453004/diff/80001/extensions/shell/browser...
extensions/shell/browser/media_capture_util.cc:63: callback.Run(devices,
Just FTR, for a website, Chrome will show some notification UIs (a red favicon
and a camera icon on the right of URL ) to notify the users that the website is
doing recording, even though the website has permission (considered to be
trusted in some way).

I do understand that things might be different for an app, but just wondering if
we should somehow notify the users that the recording is ongoing, or is it
possible at all?

[Yoyo Zhou, 2014-08-22 19:39:26]

On 2014/08/22 17:16:57, James Cook wrote:
> yoz, can I get OWNERS for chrome/common/extensions? I had to move a
permissions
> constant and strings to src/extensions.

c/c/e LGTM

[James Cook, 2014-08-22 20:11:00]

xians, thanks for the late night review!

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

https://codereview.chromium.org/493453004/diff/80001/extensions/shell/browser...
extensions/shell/browser/media_capture_util.cc:11: using
content::MediaCaptureDevices;
On 2014/08/22 19:22:27, xians1 wrote:
> nit, add an empty line before using content::MediaCaptureDevices;

Oops, done.

https://codereview.chromium.org/493453004/diff/80001/extensions/shell/browser...
extensions/shell/browser/media_capture_util.cc:36: if
(permissions_data->HasAPIPermission(APIPermission::kAudioCapture)) {
On 2014/08/22 19:22:26, xians1 wrote:
> If I understand your comment correctly, app_shell only runs trusted apps, that
> says the trust apps should only call this method if they have permission, if
so,
> should we add a CHECK here instead?

Yes, I think a CHECK here is better. Done.

https://codereview.chromium.org/493453004/diff/80001/extensions/shell/browser...
extensions/shell/browser/media_capture_util.cc:63: callback.Run(devices,
On 2014/08/22 19:22:26, xians1 wrote:
> Just FTR, for a website, Chrome will show some notification UIs (a red favicon
> and a camera icon on the right of URL ) to notify the users that the website
is
> doing recording, even though the website has permission (considered to be
> trusted in some way).
> 
> I do understand that things might be different for an app, but just wondering
if
> we should somehow notify the users that the recording is ongoing, or is it
> possible at all?

Yeah, unfortunately we don't have a good UI surface for an indicator. Packaged
apps don't have a URL bar.  On desktop we show an icon in the system tray, but
app_shell apps are fullscreen.

Fortunately for the internal project we're considering the camera will have a
physical "recording" light. For other apps I think we have to rely on the app
author so show a recording indicator.

Added a TODO here -- I don't have a good solution.

[James Cook, 2014-08-22 20:31:13]

The CQ bit was checked by jamescook@chromium.org

[commit-bot: I haz the power, 2014-08-22 20:31:45]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jamescook@chromium.org/493453004/120001

[commit-bot: I haz the power, 2014-08-22 21:42:10]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_chromium_gn_compile_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
  mac_chromium_compile_dbg on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  win_chromium_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[commit-bot: I haz the power, 2014-08-22 22:28:37]

Committed patchset #7 (120001) as 291534