Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1202553002:
Implement navigator.mediaDevices.getUserMedia() (Closed)
|
Created:
5 years, 6 months ago by hta - Chromium Modified:
5 years, 6 months ago CC:
blink-reviews, tommyw+watchlist_chromium.org Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Target Ref:
refs/heads/master Project:
blink Visibility:
Public. |
DescriptionImplement navigator.mediaDevices.getUserMedia()
This makes a promise-based API for acquiring media available.
The implementation is still hidden behind the flag that guards MediaDevices (EnumerateDevices), so is not immediately available to users.
R=tommi
BUG=503227
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=197920
Patch Set 1 #Patch Set 2 : Review comments (cleanup) #
Total comments: 18
Patch Set 3 : Review comments #
Total comments: 4
Patch Set 4 : HTML style issues in test #
Messages
Total messages: 17 (4 generated)
hta@chromium.org changed reviewers: + guidou@chromium.org, peter@chromium.org
Early review....
https://codereview.chromium.org/1202553002/diff/20001/Source/modules/mediastr... File Source/modules/mediastream/MediaDevices.cpp (right): https://codereview.chromium.org/1202553002/diff/20001/Source/modules/mediastr... Source/modules/mediastream/MediaDevices.cpp:70: NavigatorUserMediaErrorCallback* errorCallback = new PromiseErrorCallback(resolver); Are you deleting these pointers somewhere?
Mostly nits, thanks :) In your CLs description, it mentions that it makes a Promise-based gUM() API available, but this is still guarded behind the EnumerateDevices runtime flag. Perhaps clarify the sentence to include that it's not making it unconditionally available? https://codereview.chromium.org/1202553002/diff/20001/LayoutTests/fast/medias... File LayoutTests/fast/mediastream/getusermedia-promise.html (right): https://codereview.chromium.org/1202553002/diff/20001/LayoutTests/fast/medias... LayoutTests/fast/mediastream/getusermedia-promise.html:17: assert_equals(1, s.getTracks().length); nit: when using testharness.js, the expected result should be in the second argument rather than the first. assert_equals(actual, expected[, description]); https://codereview.chromium.org/1202553002/diff/20001/LayoutTests/fast/medias... LayoutTests/fast/mediastream/getusermedia-promise.html:22: <!-- Surely you don't mean to comment the rest of this file out? :) https://codereview.chromium.org/1202553002/diff/20001/Source/modules/mediastr... File Source/modules/mediastream/MediaDevices.cpp (right): https://codereview.chromium.org/1202553002/diff/20001/Source/modules/mediastr... Source/modules/mediastream/MediaDevices.cpp:35: class PromiseSuccessCallback: public NavigatorUserMediaSuccessCallback { micro nit: empty space before the colon. For PromiseErrorCallback too. https://codereview.chromium.org/1202553002/diff/20001/Source/modules/mediastr... Source/modules/mediastream/MediaDevices.cpp:35: class PromiseSuccessCallback: public NavigatorUserMediaSuccessCallback { You should override the destructor here, to make sure m_resolver will be removed. Dito for PromiseErrorCallback. https://codereview.chromium.org/1202553002/diff/20001/Source/modules/mediastr... Source/modules/mediastream/MediaDevices.cpp:70: NavigatorUserMediaErrorCallback* errorCallback = new PromiseErrorCallback(resolver); On 2015/06/23 10:57:02, guidou wrote: > Are you deleting these pointers somewhere? These will be stored as Member<>s in UserMediaRequest (note that NavigatorUserMediaErrorCallback is GarbageCollectedFinalized) - they live on the Oilpan heap. This effectively means that the lifetime of these objects is being managed by garbage collection. There's a lot of conditions to this, but the general overview is documented in Handle.h/Heap.h and on the following page: https://www.chromium.org/blink/blink-gc https://codereview.chromium.org/1202553002/diff/20001/Source/modules/mediastr... Source/modules/mediastream/MediaDevices.cpp:80: return ScriptPromise::rejectWithDOMException(scriptState, DOMException::create(NotSupportedError, "Creating the request failed - internal error?")); Why do you create a new exception here, rather than forwarding the one held in |exceptionState| (which you ASSERT for)? https://codereview.chromium.org/1202553002/diff/20001/Source/modules/mediastr... File Source/modules/mediastream/MediaDevices.h (right): https://codereview.chromium.org/1202553002/diff/20001/Source/modules/mediastr... Source/modules/mediastream/MediaDevices.h:25: ScriptPromise getUserMedia(ScriptState*, const Dictionary&, ExceptionState&); At some point it would be interesting to see if we can change this from being a random Dictionary to being a defined one, see for example NotificationOptions.idl. That'd be out of scope for this patch, however. https://codereview.chromium.org/1202553002/diff/20001/Source/modules/mediastr... File Source/modules/mediastream/MediaDevices.idl (right): https://codereview.chromium.org/1202553002/diff/20001/Source/modules/mediastr... Source/modules/mediastream/MediaDevices.idl:13: [CallWith=ScriptState, RaisesException] Promise<MediaStream> getUserMedia(Dictionary options); You mentioned something about an Intent to Implement - this doesn't seem to be part of the media capture specification mentioned on line 5. Is the inclusion of getUserMedia() on the MediaDevices object part of a proposal somewhere?
All issues addressed, I think. Some questions (verifications) on memory management-related issues. https://codereview.chromium.org/1202553002/diff/20001/LayoutTests/fast/medias... File LayoutTests/fast/mediastream/getusermedia-promise.html (right): https://codereview.chromium.org/1202553002/diff/20001/LayoutTests/fast/medias... LayoutTests/fast/mediastream/getusermedia-promise.html:17: assert_equals(1, s.getTracks().length); On 2015/06/23 12:44:16, Peter Beverloo wrote: > nit: when using testharness.js, the expected result should be in the second > argument rather than the first. > > assert_equals(actual, expected[, description]); Test harnesses seem to flip randomly between the two, thanks! https://codereview.chromium.org/1202553002/diff/20001/LayoutTests/fast/medias... LayoutTests/fast/mediastream/getusermedia-promise.html:22: <!-- On 2015/06/23 12:44:16, Peter Beverloo wrote: > Surely you don't mean to comment the rest of this file out? :) No, I meant to convert the rest of the tests to testharness and promises, but forgot .... will convert some more and delete the rest. https://codereview.chromium.org/1202553002/diff/20001/Source/modules/mediastr... File Source/modules/mediastream/MediaDevices.cpp (right): https://codereview.chromium.org/1202553002/diff/20001/Source/modules/mediastr... Source/modules/mediastream/MediaDevices.cpp:35: class PromiseSuccessCallback: public NavigatorUserMediaSuccessCallback { On 2015/06/23 12:44:16, Peter Beverloo wrote: > You should override the destructor here, to make sure m_resolver will be > removed. Dito for PromiseErrorCallback. Just an empty destructor? https://www.chromium.org/blink/blink-gc was a little unclear about which destructor gets called when you're inheriting from a GarbageCollectedFinalized class and have an empty destructor. https://codereview.chromium.org/1202553002/diff/20001/Source/modules/mediastr... Source/modules/mediastream/MediaDevices.cpp:70: NavigatorUserMediaErrorCallback* errorCallback = new PromiseErrorCallback(resolver); On 2015/06/23 12:44:16, Peter Beverloo wrote: > On 2015/06/23 10:57:02, guidou wrote: > > Are you deleting these pointers somewhere? > > These will be stored as Member<>s in UserMediaRequest (note that > NavigatorUserMediaErrorCallback is GarbageCollectedFinalized) - they live on the > Oilpan heap. > > This effectively means that the lifetime of these objects is being managed by > garbage collection. There's a lot of conditions to this, but the general > overview is documented in Handle.h/Heap.h and on the following page: > > https://www.chromium.org/blink/blink-gc Just checking .. there's no problem with keeping the pointers as just naked pointers between here and where they're adopted by UserMediaRequest::create() below, right? I assume Oilpan will see the raw pointers, and reference counting won't deallocate them until the reference count is incremented in UserMediaRequest::create()? https://codereview.chromium.org/1202553002/diff/20001/Source/modules/mediastr... Source/modules/mediastream/MediaDevices.cpp:80: return ScriptPromise::rejectWithDOMException(scriptState, DOMException::create(NotSupportedError, "Creating the request failed - internal error?")); On 2015/06/23 12:44:16, Peter Beverloo wrote: > Why do you create a new exception here, rather than forwarding the one held in > |exceptionState| (which you ASSERT for)? Returning the passed-up exception works better, thanks! https://codereview.chromium.org/1202553002/diff/20001/Source/modules/mediastr... File Source/modules/mediastream/MediaDevices.h (right): https://codereview.chromium.org/1202553002/diff/20001/Source/modules/mediastr... Source/modules/mediastream/MediaDevices.h:25: ScriptPromise getUserMedia(ScriptState*, const Dictionary&, ExceptionState&); On 2015/06/23 12:44:16, Peter Beverloo wrote: > At some point it would be interesting to see if we can change this from being a > random Dictionary to being a defined one, see for example > NotificationOptions.idl. > > That'd be out of scope for this patch, however. We have support for non-random dictionaries now? Wonderful! But yes, out of scope for this CL. https://codereview.chromium.org/1202553002/diff/20001/Source/modules/mediastr... File Source/modules/mediastream/MediaDevices.idl (right): https://codereview.chromium.org/1202553002/diff/20001/Source/modules/mediastr... Source/modules/mediastream/MediaDevices.idl:13: [CallWith=ScriptState, RaisesException] Promise<MediaStream> getUserMedia(Dictionary options); On 2015/06/23 12:44:16, Peter Beverloo wrote: > You mentioned something about an Intent to Implement - this doesn't seem to be > part of the media capture specification mentioned on line 5. Is the inclusion of > getUserMedia() on the MediaDevices object part of a proposal somewhere? The other half of MediaDevices is a partial interface further down in the spec; I added pointers to both.
Somehow my changes in patchset 2 to getusermedia-promise.html (the test) aren't visible in Rietveld. If you don't see them either, email me.
On 2015/06/26 09:46:26, hta - Chromium wrote: > Somehow my changes in patchset 2 to getusermedia-promise.html (the test) aren't > visible in Rietveld. If you don't see them either, email me. Found the issue. Now correct patchset is uploaded (patchset 3).
lgtm, thanks! Is there a plan to switch existing gUM() tests over to the Promise-based API, to get more functional coverage? https://codereview.chromium.org/1202553002/diff/20001/Source/modules/mediastr... File Source/modules/mediastream/MediaDevices.cpp (right): https://codereview.chromium.org/1202553002/diff/20001/Source/modules/mediastr... Source/modules/mediastream/MediaDevices.cpp:35: class PromiseSuccessCallback: public NavigatorUserMediaSuccessCallback { On 2015/06/26 09:28:59, hta - Chromium wrote: > On 2015/06/23 12:44:16, Peter Beverloo wrote: > > You should override the destructor here, to make sure m_resolver will be > > removed. Dito for PromiseErrorCallback. > > Just an empty destructor? > > https://www.chromium.org/blink/blink-gc was a little unclear about which > destructor gets called when you're inheriting from a GarbageCollectedFinalized > class and have an empty destructor. Yeah, an empty one would work (the interesting bits will be in compiler-generated code). You're right that the "delete" operator will call the wrong destructor (as the base class' one is not declared virtual), but GarbageCollectedFinalized::finalizeGarbageCollectedObject() will manually call the expected one for you. https://codereview.chromium.org/1202553002/diff/20001/Source/modules/mediastr... Source/modules/mediastream/MediaDevices.cpp:70: NavigatorUserMediaErrorCallback* errorCallback = new PromiseErrorCallback(resolver); On 2015/06/26 09:28:59, hta - Chromium wrote: > On 2015/06/23 12:44:16, Peter Beverloo wrote: > > On 2015/06/23 10:57:02, guidou wrote: > > > Are you deleting these pointers somewhere? > > > > These will be stored as Member<>s in UserMediaRequest (note that > > NavigatorUserMediaErrorCallback is GarbageCollectedFinalized) - they live on > the > > Oilpan heap. > > > > This effectively means that the lifetime of these objects is being managed by > > garbage collection. There's a lot of conditions to this, but the general > > overview is documented in Handle.h/Heap.h and on the following page: > > > > https://www.chromium.org/blink/blink-gc > > Just checking .. there's no problem with keeping the pointers as just naked > pointers between here and where they're adopted by UserMediaRequest::create() > below, right? I assume Oilpan will see the raw pointers, and reference counting > won't deallocate them until the reference count is incremented in > UserMediaRequest::create()? > Having the raw pointers here is fine. In the Oilpan world, you'll see a lot more of those. When they're created like this (allocated on line 69), Oilpan can see the object, but would find (during a GC) that they're not referred to from anywhere, and would thus free them. That's why this is fine for the early-return case on line 75. However, when you create the UserMediaRequest object on line 77, the constructor will store them in the Member<>s, thus referencing them. The lifetime of UserMediaRequest itself is somewhat more interesting. It lives on the Oilpan heap, but actually depends on reference counting by the WebUserMediaRequest objects (that store a WebPrivatePtr<> to it) when padding it to the embedder - Chromium. I don't think it has to be a GarbageCollectedFinalized class, and could simply be refcounted. That said, it's already being used today and changing that is out of scope of this CL. It works fine, as long as you call UserMediaRequest::start() on it. https://codereview.chromium.org/1202553002/diff/40001/LayoutTests/fast/medias... File LayoutTests/fast/mediastream/getusermedia-promise.html (right): https://codereview.chromium.org/1202553002/diff/40001/LayoutTests/fast/medias... LayoutTests/fast/mediastream/getusermedia-promise.html:1: <!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN"> micro nit: could use the modern doctype: <!DOCTYPE HTML> https://codereview.chromium.org/1202553002/diff/40001/LayoutTests/fast/medias... LayoutTests/fast/mediastream/getusermedia-promise.html:8: <p id="description"></p> micro nit: description/console aren't necessary for test harness tests. In fact, if they would get used, the tests would fail due to absence of an -expected.txt file.
lgtm
The CQ bit was checked by hta@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1202553002/40001
lgtm
Fixed the style issues in HTML. There's no specific plan to convert (=copy functionality of) the other tests on getusermedia, but we should have one. I'll file a bug. https://codereview.chromium.org/1202553002/diff/40001/LayoutTests/fast/medias... File LayoutTests/fast/mediastream/getusermedia-promise.html (right): https://codereview.chromium.org/1202553002/diff/40001/LayoutTests/fast/medias... LayoutTests/fast/mediastream/getusermedia-promise.html:1: <!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN"> On 2015/06/26 12:36:55, Peter Beverloo wrote: > micro nit: could use the modern doctype: > > <!DOCTYPE HTML> Done. https://codereview.chromium.org/1202553002/diff/40001/LayoutTests/fast/medias... LayoutTests/fast/mediastream/getusermedia-promise.html:8: <p id="description"></p> On 2015/06/26 12:36:55, Peter Beverloo wrote: > micro nit: description/console aren't necessary for test harness tests. In fact, > if they would get used, the tests would fail due to absence of an -expected.txt > file. Done.
The CQ bit was checked by hta@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from peter@chromium.org, guidou@chromium.org, tommi@chromium.org Link to the patchset: https://codereview.chromium.org/1202553002/#ps60001 (title: "HTML style issues in test")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1202553002/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://src.chromium.org/viewvc/blink?view=rev&revision=197920 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
