Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2629383002:
Add functionality to get default media device IDs from user preferences. (Closed)
|
Created:
3 years, 11 months ago by Guido Urdaneta Modified:
3 years, 11 months ago CC:
chromium-reviews, creis+watch_chromium.org, chfremer+watch_chromium.org, nasko+codewatch_chromium.org, jam, feature-media-reviews_chromium.org, darin-cc_chromium.org, mcasas+watch+vc_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd functionality to get default media device from user preferences.
The motivation for this is that constraint algorithms for getUserMedia
and related functions need this information.
The existing mechanism to get the default device (requesting permission)
is insufficient as some algorithms, such as device selection, need this
information before requesting permission.
The WebContentsDelegate::GetDefaultMediaDeviceID is used in follow-up CL https://codereview.chromium.org/2629383002/.
BUG=623104
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
Review-Url: https://codereview.chromium.org/2629383002
Cr-Commit-Position: refs/heads/master@{#445692}
Committed: https://chromium.googlesource.com/chromium/src/+/52a3de796ac586a2ab7d6123663b01415e34966f
Patch Set 1 #
Total comments: 5
Patch Set 2 : Address comments from reviewers #Patch Set 3 : Roll back changes to content/browser #Patch Set 4 : Rename MediaDeviceId -> MediaDeviceID for consistency with other code #Patch Set 5 : rebase #
Total comments: 6
Patch Set 6 : address pkasting@'s comments #
Dependent Patchsets: Messages
Total messages: 35 (18 generated)
Description was changed from ========== Add functionality to get default media device from user preferences. The motivation for this is that constraint algorithms for getUserMedia and related functions need this information. The existing mechanism to get the default device (requesting permission) is insufficient as some algorithms, such as device selection, need this information before requesting permission. BUG=623104 ========== to ========== Add functionality to get default media device from user preferences. The motivation for this is that constraint algorithms for getUserMedia and related functions need this information. The existing mechanism to get the default device (requesting permission) is insufficient as some algorithms, such as device selection, need this information before requesting permission. BUG=623104 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
guidou@chromium.org changed reviewers: + clamy@chromium.org, pkasting@chromium.org, tommi@chromium.org
tommi@chromium.org: Please review changes in chrome/browser/media/webrtc pkasting@chromium.org: Please review changes in chrome/browser/ui clamy@chromium.org: Please review changes in content/
Description was changed from ========== Add functionality to get default media device from user preferences. The motivation for this is that constraint algorithms for getUserMedia and related functions need this information. The existing mechanism to get the default device (requesting permission) is insufficient as some algorithms, such as device selection, need this information before requesting permission. BUG=623104 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Add functionality to get default media device from user preferences. The motivation for this is that constraint algorithms for getUserMedia and related functions need this information. The existing mechanism to get the default device (requesting permission) is insufficient as some algorithms, such as device selection, need this information before requesting permission. BUG=623104 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
lgtm
Sorry I didn't have time to look at this today. Will do a review on Monday.
Thanks! The code in content/ looks good, but I'm concerned that none of the functions introduced in this patch are used. Please consider merging the CL where they will be used with this one, or at least add unit tests that will exercise the code path if the former is too complex. https://codereview.chromium.org/2629383002/diff/1/content/browser/frame_host/... File content/browser/frame_host/render_frame_host_delegate.h (right): https://codereview.chromium.org/2629383002/diff/1/content/browser/frame_host/... content/browser/frame_host/render_frame_host_delegate.h:164: virtual std::string GetDefaultMediaDeviceId(MediaStreamType type); It seems to me that there are no instances of this function being used in the patch. I'd like to avoid having a patch land with non-exercised code as much as possible :). If you plan on using it in another CL, could you consider folding the two of them together? Thanks!
LGTM, but I'm not in love with the code organization. Browser is kind of a dumping ground, and I'm always looking for ways to avoid adding functions to it. In this case, it's hard to get away from doing so if the access is coming through WebContentDelegate. Short-term, it seems kind of lame to add a global function that Browser is just going to call to -- why not inline the implementation into Browser? Is someone else going to be calling this directly, and not have a Browser instance to use? (The same goes for the existing CheckMediaAccessPermission().) Long-term, it would be nice to move this out of Browser entirely, e.g. by having a separate delegate that implements these that maybe Browser inherits from or maybe the caller on the content/ side obtains directly. https://codereview.chromium.org/2629383002/diff/1/chrome/browser/media/webrtc... File chrome/browser/media/webrtc/media_capture_devices_dispatcher.cc (right): https://codereview.chromium.org/2629383002/diff/1/chrome/browser/media/webrtc... chrome/browser/media/webrtc/media_capture_devices_dispatcher.cc:244: else if (type == content::MEDIA_DEVICE_VIDEO_CAPTURE) Drive-by nit: No else after return
https://codereview.chromium.org/2629383002/diff/1/chrome/browser/media/webrtc... File chrome/browser/media/webrtc/media_capture_devices_dispatcher.cc (right): https://codereview.chromium.org/2629383002/diff/1/chrome/browser/media/webrtc... chrome/browser/media/webrtc/media_capture_devices_dispatcher.cc:244: else if (type == content::MEDIA_DEVICE_VIDEO_CAPTURE) On 2017/01/17 21:02:46, Peter Kasting wrote: > Drive-by nit: No else after return Done. https://codereview.chromium.org/2629383002/diff/1/content/browser/frame_host/... File content/browser/frame_host/render_frame_host_delegate.h (right): https://codereview.chromium.org/2629383002/diff/1/content/browser/frame_host/... content/browser/frame_host/render_frame_host_delegate.h:164: virtual std::string GetDefaultMediaDeviceId(MediaStreamType type); On 2017/01/16 12:48:09, clamy wrote: > It seems to me that there are no instances of this function being used in the > patch. I'd like to avoid having a patch land with non-exercised code as much as > possible :). If you plan on using it in another CL, could you consider folding > the two of them together? Thanks! I added a test that exercises most of the code, although it still does not exercise the top-level function RenderFrameHostDelegate::GetDefaultMediaDeviceId. The reason is that it cannot know about the chrome namespace and viceversa, and the value for the default devices can only be set in the chrome namespace. The test uses WebContentsDelegate::GetDefaultMediaDeviceId which is accessible in chrome.
The CQ bit was checked by guidou@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/01/17 21:02:46, Peter Kasting wrote:
> LGTM, but I'm not in love with the code organization.
>
> Browser is kind of a dumping ground, and I'm always looking for ways to avoid
> adding functions to it. In this case, it's hard to get away from doing so if
> the access is coming through WebContentDelegate. Short-term, it seems kind of
> lame to add a global function that Browser is just going to call to -- why not
> inline the implementation into Browser? Is someone else going to be calling
> this directly, and not have a Browser instance to use? (The same goes for the
> existing CheckMediaAccessPermission().)
I agree with you and inlined the implementation into Browser.
I'll prepare a CL that inlines the existing global functions and removes
media_utils.{h|cc}
>
> Long-term, it would be nice to move this out of Browser entirely, e.g. by
having
> a separate delegate that implements these that maybe Browser inherits from or
> maybe the caller on the content/ side obtains directly.
>
Sounds like a good idea. I filed crbug.com/682255.
Thanks! https://codereview.chromium.org/2629383002/diff/1/content/browser/frame_host/... File content/browser/frame_host/render_frame_host_delegate.h (right): https://codereview.chromium.org/2629383002/diff/1/content/browser/frame_host/... content/browser/frame_host/render_frame_host_delegate.h:164: virtual std::string GetDefaultMediaDeviceId(MediaStreamType type); On 2017/01/18 15:03:59, Guido Urdaneta wrote: > On 2017/01/16 12:48:09, clamy wrote: > > It seems to me that there are no instances of this function being used in the > > patch. I'd like to avoid having a patch land with non-exercised code as much > as > > possible :). If you plan on using it in another CL, could you consider folding > > the two of them together? Thanks! > > I added a test that exercises most of the code, although it still does not > exercise the top-level function > RenderFrameHostDelegate::GetDefaultMediaDeviceId. > The reason is that it cannot know about the chrome namespace and viceversa, and > the value for the default devices can only be set in the chrome namespace. > The test uses WebContentsDelegate::GetDefaultMediaDeviceId which is accessible > in chrome. I understand, but why do you need to add this particular function that is not used? How do you intend to use it afterwards? Without example of usage it's a bit hard for me to review whether it makes sense for it to be here or not.
On 2017/01/18 15:15:50, clamy wrote: > Thanks! > > https://codereview.chromium.org/2629383002/diff/1/content/browser/frame_host/... > File content/browser/frame_host/render_frame_host_delegate.h (right): > > https://codereview.chromium.org/2629383002/diff/1/content/browser/frame_host/... > content/browser/frame_host/render_frame_host_delegate.h:164: virtual std::string > GetDefaultMediaDeviceId(MediaStreamType type); > On 2017/01/18 15:03:59, Guido Urdaneta wrote: > > On 2017/01/16 12:48:09, clamy wrote: > > > It seems to me that there are no instances of this function being used in > the > > > patch. I'd like to avoid having a patch land with non-exercised code as much > > as > > > possible :). If you plan on using it in another CL, could you consider > folding > > > the two of them together? Thanks! > > > > I added a test that exercises most of the code, although it still does not > > exercise the top-level function > > RenderFrameHostDelegate::GetDefaultMediaDeviceId. > > The reason is that it cannot know about the chrome namespace and viceversa, > and > > the value for the default devices can only be set in the chrome namespace. > > The test uses WebContentsDelegate::GetDefaultMediaDeviceId which is accessible > > in chrome. > > I understand, but why do you need to add this particular function that is not > used? How do you intend to use it afterwards? Without example of usage it's a > bit hard for me to review whether it makes sense for it to be here or not. I see. What if I move the parts that are not in content/public to the next CL? The part in content/public would be exercised by the test for now.
clamy@: rolled back the changes in content/browser. All the code is exercised by the test. An upcoming CL will reintroduce the plumbing so that everything is exercised.
The CQ bit was checked by guidou@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks! If you have the upcoming CL link, can you link to it in the description?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Add functionality to get default media device from user preferences. The motivation for this is that constraint algorithms for getUserMedia and related functions need this information. The existing mechanism to get the default device (requesting permission) is insufficient as some algorithms, such as device selection, need this information before requesting permission. BUG=623104 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Add functionality to get default media device from user preferences. The motivation for this is that constraint algorithms for getUserMedia and related functions need this information. The existing mechanism to get the default device (requesting permission) is insufficient as some algorithms, such as device selection, need this information before requesting permission. The WebContentsDelegate::GetDefaultMediaDeviceID is used in follow-up CL https://codereview.chromium.org/2629383002/. BUG=623104 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
clammy@: modified the description to include a link to the follow-up CL. pkasting@: incorporated some of your suggestions, a minor function rename and a test. Please let me know if it still LGTY
Thanks! Lgtm.
The CQ bit was checked by guidou@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
c/b/ui LGTM https://codereview.chromium.org/2629383002/diff/80001/chrome/browser/ui/brows... File chrome/browser/ui/browser_browsertest.cc (right): https://codereview.chromium.org/2629383002/diff/80001/chrome/browser/ui/brows... chrome/browser/ui/browser_browsertest.cc:2759: std::string default_audio_capture_1("test_default_audio_capture"); Nit: Prefer = to () for initializing strings; see https://www.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Variab... (several places) Consider using const and naming these kConstantStyle, since these are compile-time constants. (Can't use constexpr with std::string.) https://codereview.chromium.org/2629383002/diff/80001/chrome/browser/ui/brows... chrome/browser/ui/browser_browsertest.cc:2764: default_video_capture_1); Nit: Optional, but if you use a lambda here, you can reduce the boilerplate in these SetString() calls: auto SetString = [this](const std::string& path, const std::string& value) { browser()->profile()->GetPrefs()->SetString(path, value); } SetString(prefs::kDefaultAudioCaptureDevice, default_audio_capture_1); ... (overall this is one line shorter) https://codereview.chromium.org/2629383002/diff/80001/chrome/browser/ui/brows... chrome/browser/ui/browser_browsertest.cc:2770: web_contents, content::MEDIA_DEVICE_AUDIO_CAPTURE)); Nit: Optional, but if you use a lambda here, you can reduce the boilerplate in these EXPECTs: auto GetDeviceID = [web_contents](content::MediaStreamType type) { return web_contents->GetDelegate()->GetDefaultMediaDeviceID(web_contents, type); } EXPECT_EQ(default_audio_capture_1, GetDeviceID(content::MEDIA_DEVICE_AUDIO_CAPTURE)); ... (overall this is the same number of lines)
https://codereview.chromium.org/2629383002/diff/80001/chrome/browser/ui/brows... File chrome/browser/ui/browser_browsertest.cc (right): https://codereview.chromium.org/2629383002/diff/80001/chrome/browser/ui/brows... chrome/browser/ui/browser_browsertest.cc:2759: std::string default_audio_capture_1("test_default_audio_capture"); On 2017/01/24 00:39:17, Peter Kasting wrote: > Nit: Prefer = to () for initializing strings; see > https://www.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Variab... > (several places) > > Consider using const and naming these kConstantStyle, since these are > compile-time constants. (Can't use constexpr with std::string.) Done. https://codereview.chromium.org/2629383002/diff/80001/chrome/browser/ui/brows... chrome/browser/ui/browser_browsertest.cc:2764: default_video_capture_1); On 2017/01/24 00:39:17, Peter Kasting wrote: > Nit: Optional, but if you use a lambda here, you can reduce the boilerplate in > these SetString() calls: > > auto SetString = [this](const std::string& path, const std::string& value) { > browser()->profile()->GetPrefs()->SetString(path, value); > } > SetString(prefs::kDefaultAudioCaptureDevice, default_audio_capture_1); > ... > > (overall this is one line shorter) Done. https://codereview.chromium.org/2629383002/diff/80001/chrome/browser/ui/brows... chrome/browser/ui/browser_browsertest.cc:2770: web_contents, content::MEDIA_DEVICE_AUDIO_CAPTURE)); On 2017/01/24 00:39:17, Peter Kasting wrote: > Nit: Optional, but if you use a lambda here, you can reduce the boilerplate in > these EXPECTs: > > auto GetDeviceID = [web_contents](content::MediaStreamType type) { > return web_contents->GetDelegate()->GetDefaultMediaDeviceID(web_contents, > type); > } > EXPECT_EQ(default_audio_capture_1, > GetDeviceID(content::MEDIA_DEVICE_AUDIO_CAPTURE)); > ... > > (overall this is the same number of lines) Done.
The CQ bit was checked by guidou@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tommi@chromium.org, clamy@chromium.org, pkasting@chromium.org Link to the patchset: https://codereview.chromium.org/2629383002/#ps100001 (title: "address pkasting@'s comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 100001, "attempt_start_ts": 1485250335557560,
"parent_rev": "9da80076a5063f0f2d9c0dfb609a0948c841f4c3", "commit_rev":
"52a3de796ac586a2ab7d6123663b01415e34966f"}
Message was sent while issue was closed.
Description was changed from ========== Add functionality to get default media device from user preferences. The motivation for this is that constraint algorithms for getUserMedia and related functions need this information. The existing mechanism to get the default device (requesting permission) is insufficient as some algorithms, such as device selection, need this information before requesting permission. The WebContentsDelegate::GetDefaultMediaDeviceID is used in follow-up CL https://codereview.chromium.org/2629383002/. BUG=623104 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Add functionality to get default media device from user preferences. The motivation for this is that constraint algorithms for getUserMedia and related functions need this information. The existing mechanism to get the default device (requesting permission) is insufficient as some algorithms, such as device selection, need this information before requesting permission. The WebContentsDelegate::GetDefaultMediaDeviceID is used in follow-up CL https://codereview.chromium.org/2629383002/. BUG=623104 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation Review-Url: https://codereview.chromium.org/2629383002 Cr-Commit-Position: refs/heads/master@{#445692} Committed: https://chromium.googlesource.com/chromium/src/+/52a3de796ac586a2ab7d6123663b... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://chromium.googlesource.com/chromium/src/+/52a3de796ac586a2ab7d6123663b... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
