Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(378)

Issue 562263002: Check media permissions through RenderFrameHostDelegate. (Closed)

Created:
6 years, 3 months ago by Henrik Grunell
Modified:
6 years, 3 months ago
CC:
chromium-reviews, extensions-reviews_chromium.org, creis+watch_chromium.org, posciak+watch_chromium.org, nasko+codewatch_chromium.org, jam, mcasas+watch_chromium.org, feature-media-reviews_chromium.org, darin-cc_chromium.org, chromium-apps-reviews_chromium.org, wjia+watch_chromium.org, miu+watch_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@render_frame_get_sources
Project:
chromium
Visibility:
Public.

Description

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}

Patch Set 1 #

Total comments: 4

Patch Set 2 : Added more implementations of WebContentsDelegate etc. Rebase. #

Patch Set 3 : Cleaning. #

Patch Set 4 : git cl format. Don't blame me. #

Total comments: 14

Patch Set 5 : Code review fixes + rebase. #

Total comments: 12

Patch Set 6 : Updated unit test MediaStreamDispatcherHost. #

Total comments: 2

Patch Set 7 : Code review. Added a unit test for MediaStreamUIProxy. Rebase. #

Patch Set 8 : Rebase. #

Patch Set 9 : Compile fixes for some platforms. #

Patch Set 10 : Compile fix. #

Patch Set 11 : Fix. #

Total comments: 2

Patch Set 12 : Code review fix. #

Patch Set 13 : Rebase #

Unified diffs Side-by-side diffs Delta from patch set Stats (+583 lines, -116 lines) Patch
M apps/custom_launcher_page_contents.h View 1 1 chunk +4 lines, -0 lines 0 comments Download
M apps/custom_launcher_page_contents.cc View 1 1 chunk +8 lines, -0 lines 0 comments Download
M athena/extensions/chrome/athena_app_delegate.h View 1 1 chunk +5 lines, -0 lines 0 comments Download
M athena/extensions/chrome/athena_app_delegate.cc View 1 1 chunk +10 lines, -0 lines 0 comments Download
M chrome/browser/android/chrome_web_contents_delegate_android.h View 1 1 chunk +4 lines, -0 lines 0 comments Download
M chrome/browser/android/chrome_web_contents_delegate_android.cc View 1 2 3 4 5 6 1 chunk +8 lines, -0 lines 0 comments Download
M chrome/browser/chromeos/login/ui/webui_login_view.h View 1 1 chunk +4 lines, -0 lines 0 comments Download
M chrome/browser/chromeos/login/ui/webui_login_view.cc View 1 2 3 4 5 6 7 8 2 chunks +9 lines, -0 lines 0 comments Download
M chrome/browser/extensions/chrome_extension_host_delegate.h View 1 chunk +4 lines, -0 lines 0 comments Download
M chrome/browser/extensions/chrome_extension_host_delegate.cc View 1 chunk +10 lines, -0 lines 0 comments Download
M chrome/browser/guest_view/web_view/chrome_web_view_permission_helper_delegate.h View 1 chunk +4 lines, -0 lines 0 comments Download
M chrome/browser/guest_view/web_view/chrome_web_view_permission_helper_delegate.cc View 1 2 3 4 1 chunk +12 lines, -1 line 0 comments Download
M chrome/browser/media/media_capture_devices_dispatcher.h View 1 2 3 4 5 6 7 8 9 10 1 chunk +14 lines, -0 lines 0 comments Download
M chrome/browser/media/media_capture_devices_dispatcher.cc View 1 2 3 4 5 6 1 chunk +63 lines, -0 lines 0 comments Download
M chrome/browser/ui/app_list/start_page_service.cc View 1 2 3 4 5 6 1 chunk +8 lines, -0 lines 0 comments Download
M chrome/browser/ui/apps/chrome_app_delegate.h View 1 chunk +5 lines, -0 lines 0 comments Download
M chrome/browser/ui/apps/chrome_app_delegate.cc View 1 1 chunk +10 lines, -0 lines 0 comments Download
M chrome/browser/ui/browser.h View 1 2 3 4 5 6 1 chunk +4 lines, -0 lines 0 comments Download
M chrome/browser/ui/browser.cc View 1 2 3 4 5 6 1 chunk +6 lines, -0 lines 0 comments Download
M chrome/browser/ui/media_utils.h View 2 chunks +6 lines, -0 lines 0 comments Download
M chrome/browser/ui/media_utils.cc View 1 2 3 4 5 6 7 8 9 10 11 3 chunks +39 lines, -11 lines 0 comments Download
M content/browser/frame_host/render_frame_host_delegate.h View 1 2 3 4 5 6 1 chunk +6 lines, -0 lines 0 comments Download
M content/browser/frame_host/render_frame_host_delegate.cc View 1 2 3 4 5 6 1 chunk +8 lines, -0 lines 0 comments Download
M content/browser/renderer_host/media/media_stream_dispatcher_host_unittest.cc View 1 2 3 4 5 5 chunks +12 lines, -41 lines 0 comments Download
M content/browser/renderer_host/media/media_stream_manager.h View 1 2 1 chunk +0 lines, -5 lines 0 comments Download
M content/browser/renderer_host/media/media_stream_manager.cc View 1 2 2 chunks +13 lines, -29 lines 0 comments Download
M content/browser/renderer_host/media/media_stream_ui_proxy.h View 1 2 3 4 5 3 chunks +24 lines, -0 lines 0 comments Download
M content/browser/renderer_host/media/media_stream_ui_proxy.cc View 1 2 3 4 11 chunks +108 lines, -17 lines 0 comments Download
M content/browser/renderer_host/media/media_stream_ui_proxy_unittest.cc View 1 2 3 4 5 6 3 chunks +16 lines, -0 lines 0 comments Download
M content/browser/web_contents/web_contents_impl.h View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +2 lines, -0 lines 0 comments Download
M content/browser/web_contents/web_contents_impl.cc View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +8 lines, -0 lines 0 comments Download
M content/public/browser/web_contents_delegate.h View 1 chunk +7 lines, -0 lines 0 comments Download
M content/public/browser/web_contents_delegate.cc View 1 chunk +9 lines, -0 lines 0 comments Download
M extensions/browser/app_window/app_delegate.h View 1 2 3 1 chunk +4 lines, -0 lines 0 comments Download
M extensions/browser/app_window/app_web_contents_helper.h View 1 chunk +5 lines, -0 lines 0 comments Download
M extensions/browser/app_window/app_web_contents_helper.cc View 1 chunk +11 lines, -0 lines 0 comments Download
M extensions/browser/app_window/app_window.h View 1 chunk +4 lines, -0 lines 0 comments Download
M extensions/browser/app_window/app_window.cc View 1 chunk +7 lines, -0 lines 0 comments Download
M extensions/browser/extension_host.h View 1 chunk +4 lines, -0 lines 0 comments Download
M extensions/browser/extension_host.cc View 1 2 1 chunk +8 lines, -0 lines 0 comments Download
M extensions/browser/extension_host_delegate.h View 1 2 3 4 1 chunk +8 lines, -0 lines 0 comments Download
M extensions/browser/guest_view/web_view/web_view_guest.h View 1 chunk +4 lines, -0 lines 0 comments Download
M extensions/browser/guest_view/web_view/web_view_guest.cc View 1 2 3 4 5 6 1 chunk +7 lines, -0 lines 0 comments Download
M extensions/browser/guest_view/web_view/web_view_permission_helper.h View 1 2 3 4 5 6 1 chunk +3 lines, -0 lines 0 comments Download
M extensions/browser/guest_view/web_view/web_view_permission_helper.cc View 1 chunk +9 lines, -1 line 0 comments Download
M extensions/browser/guest_view/web_view/web_view_permission_helper_delegate.h View 1 2 3 1 chunk +4 lines, -0 lines 0 comments Download
M extensions/browser/guest_view/web_view/web_view_permission_helper_delegate.cc View 1 2 3 4 1 chunk +8 lines, -0 lines 0 comments Download
M extensions/shell/browser/media_capture_util.h View 1 1 chunk +4 lines, -0 lines 0 comments Download
M extensions/shell/browser/media_capture_util.cc View 1 2 3 4 5 6 7 8 3 chunks +17 lines, -10 lines 0 comments Download
M extensions/shell/browser/shell_app_delegate.h View 1 2 3 4 5 6 7 8 1 chunk +4 lines, -0 lines 0 comments Download
M extensions/shell/browser/shell_app_delegate.cc View 1 2 1 chunk +9 lines, -0 lines 0 comments Download
M extensions/shell/browser/shell_extension_host_delegate.h View 1 2 3 4 5 6 7 8 1 chunk +4 lines, -0 lines 0 comments Download
M extensions/shell/browser/shell_extension_host_delegate.cc View 1 2 3 4 5 6 7 8 9 1 chunk +9 lines, -1 line 0 comments Download

Messages

Total messages: 43 (9 generated)
Henrik Grunell
This CL isn't entirely ready yet. Per: can you take a quick overview look? fsamuel@: ...
6 years, 3 months ago (2014-09-11 16:28:41 UTC) #2
Henrik Grunell
Hi! This CL is ready for review. It's a little monster, but most files are ...
6 years, 3 months ago (2014-09-12 15:18:12 UTC) #5
Fady Samuel
https://codereview.chromium.org/562263002/diff/1/chrome/browser/guest_view/web_view/chrome_web_view_permission_helper_delegate.cc 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/web_view/chrome_web_view_permission_helper_delegate.cc#newcode145 chrome/browser/guest_view/web_view/chrome_web_view_permission_helper_delegate.cc:145: // TODO - BEFORE COMMIT: I guess we need ...
6 years, 3 months ago (2014-09-12 15:19:14 UTC) #6
Avi (use Gerrit)
lgtm
6 years, 3 months ago (2014-09-12 15:44:16 UTC) #7
sky
sky->oshima (for athena)
6 years, 3 months ago (2014-09-12 15:59:59 UTC) #9
Henrik Grunell
On 2014/09/12 15:59:59, sky wrote: > sky->oshima (for athena) Scott, would you mind reviewing //chrome?
6 years, 3 months ago (2014-09-12 16:04:57 UTC) #11
Henrik Grunell
https://codereview.chromium.org/562263002/diff/1/chrome/browser/guest_view/web_view/chrome_web_view_permission_helper_delegate.cc 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/web_view/chrome_web_view_permission_helper_delegate.cc#newcode145 chrome/browser/guest_view/web_view/chrome_web_view_permission_helper_delegate.cc:145: // TODO - BEFORE COMMIT: I guess we need ...
6 years, 3 months ago (2014-09-12 16:10:35 UTC) #12
sky
https://codereview.chromium.org/562263002/diff/60001/chrome/browser/guest_view/web_view/chrome_web_view_permission_helper_delegate.cc 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_view/web_view/chrome_web_view_permission_helper_delegate.cc#newcode145 chrome/browser/guest_view/web_view/chrome_web_view_permission_helper_delegate.cc:145: // TODO - BEFORE COMMIT: I guess we need ...
6 years, 3 months ago (2014-09-12 16:19:14 UTC) #13
Yoyo Zhou
The extensions and apps mostly LGTM, but +jamescook to sanity check media_capture_util https://codereview.chromium.org/562263002/diff/60001/extensions/browser/extension_host_delegate.h File extensions/browser/extension_host_delegate.h ...
6 years, 3 months ago (2014-09-12 18:30:41 UTC) #15
oshima
athena/ lgtm
6 years, 3 months ago (2014-09-12 20:14:59 UTC) #16
James Cook
LGTM with one optional suggestion for media_capture_util.cc https://codereview.chromium.org/562263002/diff/60001/extensions/shell/browser/media_capture_util.cc File extensions/shell/browser/media_capture_util.cc (right): https://codereview.chromium.org/562263002/diff/60001/extensions/shell/browser/media_capture_util.cc#newcode48 extensions/shell/browser/media_capture_util.cc:48: // app_shell ...
6 years, 3 months ago (2014-09-12 20:43:12 UTC) #17
Fady Samuel
https://codereview.chromium.org/562263002/diff/1/chrome/browser/guest_view/web_view/chrome_web_view_permission_helper_delegate.cc 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/web_view/chrome_web_view_permission_helper_delegate.cc#newcode145 chrome/browser/guest_view/web_view/chrome_web_view_permission_helper_delegate.cc:145: // TODO - BEFORE COMMIT: I guess we need ...
6 years, 3 months ago (2014-09-12 20:46:33 UTC) #18
Henrik Grunell
https://codereview.chromium.org/562263002/diff/1/chrome/browser/guest_view/web_view/chrome_web_view_permission_helper_delegate.cc 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/web_view/chrome_web_view_permission_helper_delegate.cc#newcode145 chrome/browser/guest_view/web_view/chrome_web_view_permission_helper_delegate.cc:145: // TODO - BEFORE COMMIT: I guess we need ...
6 years, 3 months ago (2014-09-15 11:33:48 UTC) #19
perkj_chrome
https://codereview.chromium.org/562263002/diff/80001/content/browser/frame_host/render_frame_host_delegate.cc File content/browser/frame_host/render_frame_host_delegate.cc (right): https://codereview.chromium.org/562263002/diff/80001/content/browser/frame_host/render_frame_host_delegate.cc#newcode50 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_host/media/media_stream_dispatcher_host_unittest.cc File content/browser/renderer_host/media/media_stream_dispatcher_host_unittest.cc (right): ...
6 years, 3 months ago (2014-09-15 12:06:25 UTC) #20
Henrik Grunell
https://codereview.chromium.org/562263002/diff/80001/content/browser/frame_host/render_frame_host_delegate.cc File content/browser/frame_host/render_frame_host_delegate.cc (right): https://codereview.chromium.org/562263002/diff/80001/content/browser/frame_host/render_frame_host_delegate.cc#newcode50 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: ...
6 years, 3 months ago (2014-09-15 13:45:17 UTC) #21
perkj_chrome
https://codereview.chromium.org/562263002/diff/80001/content/browser/renderer_host/media/media_stream_ui_proxy.h File content/browser/renderer_host/media/media_stream_ui_proxy.h (right): https://codereview.chromium.org/562263002/diff/80001/content/browser/renderer_host/media/media_stream_ui_proxy.h#newcode45 content/browser/renderer_host/media/media_stream_ui_proxy.h:45: // this does not query the user. |type| must ...
6 years, 3 months ago (2014-09-15 15:08:55 UTC) #22
Henrik Grunell
https://codereview.chromium.org/562263002/diff/80001/content/browser/renderer_host/media/media_stream_ui_proxy.h File content/browser/renderer_host/media/media_stream_ui_proxy.h (right): https://codereview.chromium.org/562263002/diff/80001/content/browser/renderer_host/media/media_stream_ui_proxy.h#newcode45 content/browser/renderer_host/media/media_stream_ui_proxy.h:45: // this does not query the user. |type| must ...
6 years, 3 months ago (2014-09-15 18:20:16 UTC) #23
Fady Samuel
After a chat offline, guest_view lgtm.
6 years, 3 months ago (2014-09-15 18:43:54 UTC) #24
Henrik Grunell
Updated unit test. Per, ptal.
6 years, 3 months ago (2014-09-16 09:54:55 UTC) #25
perkj_chrome
On 2014/09/16 09:54:55, Henrik Grunell wrote: > Updated unit test. Per, ptal. renderer_host/media lgtm.
6 years, 3 months ago (2014-09-16 10:39:04 UTC) #26
Henrik Grunell
Scott, ptal.
6 years, 3 months ago (2014-09-16 12:35:31 UTC) #27
sky
https://codereview.chromium.org/562263002/diff/100001/chrome/browser/ui/media_utils.cc File chrome/browser/ui/media_utils.cc (right): https://codereview.chromium.org/562263002/diff/100001/chrome/browser/ui/media_utils.cc#newcode52 chrome/browser/ui/media_utils.cc:52: const extensions::Extension* extension = NULL; If ENABLE_EXTENSIONS is false, ...
6 years, 3 months ago (2014-09-16 16:33:25 UTC) #28
Henrik Grunell
https://codereview.chromium.org/562263002/diff/100001/chrome/browser/ui/media_utils.cc File chrome/browser/ui/media_utils.cc (right): https://codereview.chromium.org/562263002/diff/100001/chrome/browser/ui/media_utils.cc#newcode52 chrome/browser/ui/media_utils.cc:52: const extensions::Extension* extension = NULL; On 2014/09/16 16:33:25, sky ...
6 years, 3 months ago (2014-09-16 17:14:15 UTC) #29
sky
Seems clearer to have a function that doesn't take an Exension if ENABLE_EXTENSIONS is false. ...
6 years, 3 months ago (2014-09-16 17:50:43 UTC) #30
Henrik Grunell
Any other comments besides this? Den 16 sep 2014 19:50 skrev "Scott Violet" <sky@chromium.org>: > ...
6 years, 3 months ago (2014-09-16 20:30:15 UTC) #31
sky
No, that is my only remaining comment. On Tue, Sep 16, 2014 at 1:30 PM, ...
6 years, 3 months ago (2014-09-16 22:05:00 UTC) #32
Henrik Grunell
On 2014/09/16 17:50:43, sky wrote: > Seems clearer to have a function that doesn't take ...
6 years, 3 months ago (2014-09-17 08:27:30 UTC) #33
sky
LGTM https://codereview.chromium.org/562263002/diff/200001/chrome/browser/ui/media_utils.cc File chrome/browser/ui/media_utils.cc (right): https://codereview.chromium.org/562263002/diff/200001/chrome/browser/ui/media_utils.cc#newcode61 chrome/browser/ui/media_utils.cc:61: } else { nit: no else after a ...
6 years, 3 months ago (2014-09-17 15:46:48 UTC) #34
Henrik Grunell
https://codereview.chromium.org/562263002/diff/200001/chrome/browser/ui/media_utils.cc File chrome/browser/ui/media_utils.cc (right): https://codereview.chromium.org/562263002/diff/200001/chrome/browser/ui/media_utils.cc#newcode61 chrome/browser/ui/media_utils.cc:61: } else { On 2014/09/17 15:46:47, sky wrote: > ...
6 years, 3 months ago (2014-09-17 20:00:17 UTC) #35
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patchset/562263002/240001
6 years, 3 months ago (2014-09-17 20:01:52 UTC) #37
commit-bot: I haz the power
Exceeded time limit waiting for builds to trigger.
6 years, 3 months ago (2014-09-17 22:02:45 UTC) #39
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patchset/562263002/240001
6 years, 3 months ago (2014-09-17 23:39:08 UTC) #41
commit-bot: I haz the power
Committed patchset #13 (id:240001) as 829d93f2dbddc7c5a7b64d2ab59862e745b8674d
6 years, 3 months ago (2014-09-18 00:09:51 UTC) #42
commit-bot: I haz the power
6 years, 3 months ago (2014-09-18 00:10:27 UTC) #43
Message was sent while issue was closed.
Patchset 13 (id:??) landed as
https://crrev.com/657d4d8c7d26c89c0846479ac322931bfaba4f86
Cr-Commit-Position: refs/heads/master@{#295379}

Powered by Google App Engine
This is Rietveld 408576698