New issue
Advanced search Search tips
Note: Color blocks (like █ or █) mean that a user may not be available. Tooltip shows the reason.

Issue 793297 link

Starred by 1 user
Status: Fixed
Owner:
█ c.pa...@samsung.com
Last visit > 30 days ago
Closed: Jan 2018
Cc:
guidou@chromium.org
niklase@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug

Blocking:
issue 787254



Sign in to add a comment

Migrate processing of the devicechange event from content/renderer/media to Blink

Project Member Reported by guidou@chromium.org, Dec 8 2017 
There are two parts.


1. Subscription handling: depends on MediaDevicesDispatcherHost. After the work on  bug 764680  , access to this object from Blink is straightforward.

2. Event handling: This is implemented by a renderer-side mojo object (MediaDevicesListener) implemented in content/renderer/media.
This should be moved to Blink.

The main issue is that this is not only used by the Web platform, but also by PepperMediaDevicesManager in Blink. Moving all the logic to Blink requires making it accessible to content via public/ so that PepperMediaDevicesManager can continue to use the functionality.

Comment 1 by guidou@chromium.org, Dec 8 2017

Cc: c.pa...@samsung.com
Components: Blink>MediaStream
c.padhi@: Since you solved  bug 764680 , would you like to take a look at this as well?

Comment 2 by guidou@chromium.org, Dec 8 2017

Cc: guidou@chromium.org

Comment 3 by c.pa...@samsung.com, Dec 8 2017

Cc: -c.pa...@samsung.com
Owner: c.pa...@samsung.com
Sure. I'll start working on this as well.

Comment 4 by c.pa...@samsung.com, Dec 8 2017

Status: Assigned (was: Untriaged)

Comment 5 by guidou@chromium.org, Dec 12 2017

Blocking: 787254
Project Member

Comment 6 by bugdroid1@chromium.org, Jan 12 2018 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/7db4074e5ec8d6e8e096c48761088c842d399ece

commit 7db4074e5ec8d6e8e096c48761088c842d399ece
Author: Chandan Padhi <c.padhi@samsung.com>
Date: Fri Jan 12 16:17:01 2018

Move MediaDevicesPermissionChecker to MediaDevicesManager

This CL is in preparation for device change event migration to blink.
This change will allow MediaDevicesManager to check for permissions
before notifying renderers of device change.

Bug:  793297 
Change-Id: Idd9524c02e3921e80b1d9f40f332fd2446ea3c16
Reviewed-on: https://chromium-review.googlesource.com/863265
Reviewed-by: Guido Urdaneta <guidou@chromium.org>
Commit-Queue: Chandan Padhi <c.padhi@samsung.com>
Cr-Commit-Position: refs/heads/master@{#528960}
[modify] https://crrev.com/7db4074e5ec8d6e8e096c48761088c842d399ece/content/browser/renderer_host/media/audio_output_authorization_handler.cc
[modify] https://crrev.com/7db4074e5ec8d6e8e096c48761088c842d399ece/content/browser/renderer_host/media/audio_output_authorization_handler.h
[modify] https://crrev.com/7db4074e5ec8d6e8e096c48761088c842d399ece/content/browser/renderer_host/media/media_devices_dispatcher_host.cc
[modify] https://crrev.com/7db4074e5ec8d6e8e096c48761088c842d399ece/content/browser/renderer_host/media/media_devices_dispatcher_host.h
[modify] https://crrev.com/7db4074e5ec8d6e8e096c48761088c842d399ece/content/browser/renderer_host/media/media_devices_dispatcher_host_unittest.cc
[modify] https://crrev.com/7db4074e5ec8d6e8e096c48761088c842d399ece/content/browser/renderer_host/media/media_devices_manager.cc
[modify] https://crrev.com/7db4074e5ec8d6e8e096c48761088c842d399ece/content/browser/renderer_host/media/media_devices_manager.h

Comment 7 by c.pa...@samsung.com, Jan 16 2018 

guidou@ I am trying out the possibility of having MDD return already translated enumeration results based on your suggestion.
If we do this, then I think we can completely remove permission checks and |salt_and_origin_callback_| from MDDH.
I have few queries if you could please help me out.
1. Where should permission checks happen in MDD? Before we enumerate devices i.e. in MDD::EnumerateDevices() or after we have enumeration results in MDD::DevicesEnumerated()?
2. MediaStreamManager and AudioOutputAuthorizationHandler also use MDD::EnumerateDevices. Currently, they don't expect permission checks in MDD. How should we handle these cases?

Comment 8 by guidou@chromium.org, Jan 16 2018 

Since MSM and AOAH require the existing MDD::EnumerateDevices, you cannot remove it.
You can add a new enumeration function to MDD that invokes the old MDD::EnumerateDevices and also performs permission checks and translation. I think we can call it EnumerateDevicesForRenderer or perhaps CheckedEnumerateDevices (feel free to suggest another name). We can even call it EnumerateDevices (since it will have a different set of parameters), although I would prefer a different name.
Project Member

Comment 9 by bugdroid1@chromium.org, Jan 19 2018 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/be1a95495158110835d638e0133e7d2b19ea179b

commit be1a95495158110835d638e0133e7d2b19ea179b
Author: Chandan Padhi <c.padhi@samsung.com>
Date: Fri Jan 19 17:12:42 2018

Move MediaDeviceSaltAndOriginCallback to MediaDevicesManager

This CL is in preparation for device change event migration to blink.
This change will allow MediaDevicesManager to translate enumeration
results before sending them to the renderers.

Bug:  793297 
Change-Id: I30f3533efb7c7eac8b7db5ab97fb6b155b28d99e
Reviewed-on: https://chromium-review.googlesource.com/869638
Commit-Queue: Chandan Padhi <c.padhi@samsung.com>
Reviewed-by: Guido Urdaneta <guidou@chromium.org>
Cr-Commit-Position: refs/heads/master@{#530544}
[modify] https://crrev.com/be1a95495158110835d638e0133e7d2b19ea179b/content/browser/media/media_devices_util.cc
[modify] https://crrev.com/be1a95495158110835d638e0133e7d2b19ea179b/content/browser/media/media_devices_util.h
[modify] https://crrev.com/be1a95495158110835d638e0133e7d2b19ea179b/content/browser/renderer_host/media/media_devices_dispatcher_host.cc
[modify] https://crrev.com/be1a95495158110835d638e0133e7d2b19ea179b/content/browser/renderer_host/media/media_devices_dispatcher_host.h
[modify] https://crrev.com/be1a95495158110835d638e0133e7d2b19ea179b/content/browser/renderer_host/media/media_devices_dispatcher_host_unittest.cc
[modify] https://crrev.com/be1a95495158110835d638e0133e7d2b19ea179b/content/browser/renderer_host/media/media_devices_manager.cc
[modify] https://crrev.com/be1a95495158110835d638e0133e7d2b19ea179b/content/browser/renderer_host/media/media_devices_manager.h
Project Member

Comment 10 by bugdroid1@chromium.org, Jan 19 2018 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/082d2996ba2452dd71b9cef7f3fcb7ec0237ca87

commit 082d2996ba2452dd71b9cef7f3fcb7ec0237ca87
Author: Chandan Padhi <c.padhi@samsung.com>
Date: Fri Jan 19 20:01:33 2018

Migrate MediaDevices devicechange event processing from content to Blink

This CL introduces a new mojom method AddMediaDevicesListener() to
subscribe for device-change notifications. The subscription can be
cancelled by closing the message pipe. This method will eventually
replace the existing separate methods to subscribe and unsubscribe.

Currently, MediaDevices devicechange subscription and event handling
are done in UserMediaClientImpl and MediaDevicesEventDispatcher. Now
that Mojo MediaDevicesDispatcherHost can be accessed from Blink [1],
blink::MediaDevices itself inherits mojom::MediaDevicesListener to
listen to device-change notifications and calls AddMediaDevicesListener
for subscription.
[1] https://crrev.com/c/790119/

Bug:  793297 
Change-Id: I90a873790151e493347c29af81ce3deb9f3915e6
Reviewed-on: https://chromium-review.googlesource.com/844303
Commit-Queue: Chandan Padhi <c.padhi@samsung.com>
Reviewed-by: Guido Urdaneta <guidou@chromium.org>
Reviewed-by: Tom Sepez <tsepez@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Cr-Commit-Position: refs/heads/master@{#530591}
[modify] https://crrev.com/082d2996ba2452dd71b9cef7f3fcb7ec0237ca87/content/browser/renderer_host/media/media_devices_dispatcher_host.cc
[modify] https://crrev.com/082d2996ba2452dd71b9cef7f3fcb7ec0237ca87/content/browser/renderer_host/media/media_devices_dispatcher_host.h
[modify] https://crrev.com/082d2996ba2452dd71b9cef7f3fcb7ec0237ca87/content/browser/renderer_host/media/media_devices_manager.cc
[modify] https://crrev.com/082d2996ba2452dd71b9cef7f3fcb7ec0237ca87/content/browser/renderer_host/media/media_devices_manager.h
[modify] https://crrev.com/082d2996ba2452dd71b9cef7f3fcb7ec0237ca87/content/renderer/media/media_devices_event_dispatcher_unittest.cc
[modify] https://crrev.com/082d2996ba2452dd71b9cef7f3fcb7ec0237ca87/content/renderer/media/user_media_client_impl.cc
[modify] https://crrev.com/082d2996ba2452dd71b9cef7f3fcb7ec0237ca87/content/renderer/media/user_media_client_impl.h
[modify] https://crrev.com/082d2996ba2452dd71b9cef7f3fcb7ec0237ca87/content/renderer/media/user_media_client_impl_unittest.cc
[modify] https://crrev.com/082d2996ba2452dd71b9cef7f3fcb7ec0237ca87/third_party/WebKit/Source/modules/exported/BUILD.gn
[delete] https://crrev.com/b8951ba5f12103e6cfd719f725bbf72c978f8b31/third_party/WebKit/Source/modules/exported/WebMediaDeviceChangeObserver.cpp
[modify] https://crrev.com/082d2996ba2452dd71b9cef7f3fcb7ec0237ca87/third_party/WebKit/Source/modules/mediastream/MediaDevices.cpp
[modify] https://crrev.com/082d2996ba2452dd71b9cef7f3fcb7ec0237ca87/third_party/WebKit/Source/modules/mediastream/MediaDevices.h
[modify] https://crrev.com/082d2996ba2452dd71b9cef7f3fcb7ec0237ca87/third_party/WebKit/Source/modules/mediastream/MediaDevicesTest.cpp
[modify] https://crrev.com/082d2996ba2452dd71b9cef7f3fcb7ec0237ca87/third_party/WebKit/Source/modules/mediastream/UserMediaClient.cpp
[modify] https://crrev.com/082d2996ba2452dd71b9cef7f3fcb7ec0237ca87/third_party/WebKit/Source/modules/mediastream/UserMediaClient.h
[modify] https://crrev.com/082d2996ba2452dd71b9cef7f3fcb7ec0237ca87/third_party/WebKit/Source/modules/mediastream/UserMediaController.h
[modify] https://crrev.com/082d2996ba2452dd71b9cef7f3fcb7ec0237ca87/third_party/WebKit/public/BUILD.gn
[modify] https://crrev.com/082d2996ba2452dd71b9cef7f3fcb7ec0237ca87/third_party/WebKit/public/platform/modules/mediastream/media_devices.mojom
[delete] https://crrev.com/b8951ba5f12103e6cfd719f725bbf72c978f8b31/third_party/WebKit/public/web/WebMediaDeviceChangeObserver.h
[modify] https://crrev.com/082d2996ba2452dd71b9cef7f3fcb7ec0237ca87/third_party/WebKit/public/web/WebUserMediaClient.h
Project Member

Comment 11 by bugdroid1@chromium.org, Jan 20 2018 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a935aae017f50bdd21a37903458b4b5e3f7661e1

commit a935aae017f50bdd21a37903458b4b5e3f7661e1
Author: Chandan Padhi <c.padhi@samsung.com>
Date: Sat Jan 20 05:08:38 2018

Make Pepper subscribe and handle device change event on its own

Currently, device change subscription and event handling are done in
MediaDevicesEventDispatcher. After [1] and [2], Pepper can itself inherit
mojom::MediaDevicesListener to listen to device change notifications and
directly call AddMediaDevicesListener() for subscription.
[1] https://crrev.com/c/790119/790119/
[2] https://crrev.com/c/790119/844303/

Bug:  793297 
Change-Id: I68336592cc71d6cbefbb3be1b9005586d3aaa450
Reviewed-on: https://chromium-review.googlesource.com/844534
Commit-Queue: Chandan Padhi <c.padhi@samsung.com>
Reviewed-by: Guido Urdaneta <guidou@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Reviewed-by: Bill Budge <bbudge@chromium.org>
Reviewed-by: Tom Sepez <tsepez@chromium.org>
Cr-Commit-Position: refs/heads/master@{#530755}
[modify] https://crrev.com/a935aae017f50bdd21a37903458b4b5e3f7661e1/content/browser/renderer_host/media/media_devices_dispatcher_host.cc
[modify] https://crrev.com/a935aae017f50bdd21a37903458b4b5e3f7661e1/content/browser/renderer_host/media/media_devices_dispatcher_host.h
[modify] https://crrev.com/a935aae017f50bdd21a37903458b4b5e3f7661e1/content/browser/renderer_host/media/media_devices_dispatcher_host_unittest.cc
[modify] https://crrev.com/a935aae017f50bdd21a37903458b4b5e3f7661e1/content/browser/renderer_host/media/media_devices_manager.cc
[modify] https://crrev.com/a935aae017f50bdd21a37903458b4b5e3f7661e1/content/browser/renderer_host/media/media_devices_manager.h
[modify] https://crrev.com/a935aae017f50bdd21a37903458b4b5e3f7661e1/content/browser/renderer_host/media/media_devices_manager_unittest.cc
[modify] https://crrev.com/a935aae017f50bdd21a37903458b4b5e3f7661e1/content/renderer/BUILD.gn
[delete] https://crrev.com/7642786ed0982a8c36687dbd7355407aa44d2943/content/renderer/media/media_devices_event_dispatcher.cc
[delete] https://crrev.com/7642786ed0982a8c36687dbd7355407aa44d2943/content/renderer/media/media_devices_event_dispatcher.h
[delete] https://crrev.com/7642786ed0982a8c36687dbd7355407aa44d2943/content/renderer/media/media_devices_event_dispatcher_unittest.cc
[delete] https://crrev.com/7642786ed0982a8c36687dbd7355407aa44d2943/content/renderer/media/media_devices_listener_impl.cc
[delete] https://crrev.com/7642786ed0982a8c36687dbd7355407aa44d2943/content/renderer/media/media_devices_listener_impl.h
[modify] https://crrev.com/a935aae017f50bdd21a37903458b4b5e3f7661e1/content/renderer/media/user_media_client_impl_unittest.cc
[modify] https://crrev.com/a935aae017f50bdd21a37903458b4b5e3f7661e1/content/renderer/pepper/pepper_device_enumeration_host_helper.cc
[modify] https://crrev.com/a935aae017f50bdd21a37903458b4b5e3f7661e1/content/renderer/pepper/pepper_device_enumeration_host_helper.h
[modify] https://crrev.com/a935aae017f50bdd21a37903458b4b5e3f7661e1/content/renderer/pepper/pepper_device_enumeration_host_helper_unittest.cc
[modify] https://crrev.com/a935aae017f50bdd21a37903458b4b5e3f7661e1/content/renderer/pepper/pepper_media_device_manager.cc
[modify] https://crrev.com/a935aae017f50bdd21a37903458b4b5e3f7661e1/content/renderer/pepper/pepper_media_device_manager.h
[modify] https://crrev.com/a935aae017f50bdd21a37903458b4b5e3f7661e1/content/renderer/render_frame_impl.cc
[modify] https://crrev.com/a935aae017f50bdd21a37903458b4b5e3f7661e1/content/test/BUILD.gn
[modify] https://crrev.com/a935aae017f50bdd21a37903458b4b5e3f7661e1/third_party/WebKit/Source/modules/mediastream/MediaDevices.cpp
[modify] https://crrev.com/a935aae017f50bdd21a37903458b4b5e3f7661e1/third_party/WebKit/Source/modules/mediastream/MediaDevices.h
[modify] https://crrev.com/a935aae017f50bdd21a37903458b4b5e3f7661e1/third_party/WebKit/Source/modules/mediastream/MediaDevicesTest.cpp
[modify] https://crrev.com/a935aae017f50bdd21a37903458b4b5e3f7661e1/third_party/WebKit/public/platform/modules/mediastream/media_devices.mojom

Comment 12 by c.pa...@samsung.com, Jan 22 2018 

guidou@: Is there anything else that needs to be done w.r.t this bug?
Also, I would like to continue with my contributions. Please let me know if there's anything interesting that I can take up next. Thanks.

Comment 13 by guidou@chromium.org, Jan 22 2018

Status: Fixed (was: Assigned)
This is done. Another bug you can take a look at is bug 704136.
It's similar to this one, but a bit trickier since it requires moving a log more code to Blink.
►

Sign in to add a comment