Summary: | MediaStream API: Changing the device enumeration to be async | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Eric Carlson <eric.carlson> | ||||||||||||||
Component: | WebCore Misc. | Assignee: | Eric Carlson <eric.carlson> | ||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||
Severity: | Normal | CC: | buildbot, cdumez, commit-queue, esprehn+autocc, glenn, gyuyoung.kim, hta, jer.noble, kondapallykalyan, philn, rakuco, rego+ews, rniwa, tommyw, xan.lopez | ||||||||||||||
Priority: | P2 | Keywords: | BlinkMergeCandidate | ||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||
OS: | Unspecified | ||||||||||||||||
Bug Depends on: | |||||||||||||||||
Bug Blocks: | 121101 | ||||||||||||||||
Attachments: |
|
Description
Eric Carlson
2013-09-06 12:44:14 PDT
This will also require a merge of some parts of https://chromium.googlesource.com/chromium/blink/+/ff783a23bb1add588971a8187048a305cf485121 Created attachment 211596 [details]
Patch for the bots to chew on
Attachment 211596 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast/mediastream/MediaStreamTrack-getSources-expected.txt', u'LayoutTests/fast/mediastream/MediaStreamTrack-getSources.html', u'Source/WebCore/CMakeLists.txt', u'Source/WebCore/ChangeLog', u'Source/WebCore/DerivedSources.make', u'Source/WebCore/GNUmakefile.list.am', u'Source/WebCore/Modules/mediastream/MediaStreamTrack.cpp', u'Source/WebCore/Modules/mediastream/MediaStreamTrack.h', u'Source/WebCore/Modules/mediastream/MediaStreamTrack.idl', u'Source/WebCore/Modules/mediastream/MediaStreamTrackSourcesCallback.h', u'Source/WebCore/Modules/mediastream/MediaStreamTrackSourcesCallback.idl', u'Source/WebCore/Modules/mediastream/MediaStreamTrackSourcesRequest.cpp', u'Source/WebCore/Modules/mediastream/MediaStreamTrackSourcesRequest.h', u'Source/WebCore/Modules/mediastream/SourceInfo.cpp', u'Source/WebCore/Modules/mediastream/SourceInfo.h', u'Source/WebCore/Modules/mediastream/SourceInfo.idl', u'Source/WebCore/WebCore.xcodeproj/project.pbxproj', u'Source/WebCore/bindings/js/JSDOMBinding.h', u'Source/WebCore/platform/mediastream/MediaStreamCenter.h', u'Source/WebCore/platform/mediastream/MediaStreamTrackSourcesRequestClient.h', u'Source/WebCore/platform/mediastream/blackberry/MediaStreamCenterBlackBerry.cpp', u'Source/WebCore/platform/mediastream/blackberry/MediaStreamCenterBlackBerry.h', u'Source/WebCore/platform/mediastream/gstreamer/MediaStreamCenterGStreamer.cpp', u'Source/WebCore/platform/mediastream/gstreamer/MediaStreamCenterGStreamer.h', u'Source/WebCore/platform/mediastream/mac/MediaStreamCenterMac.cpp', u'Source/WebCore/platform/mediastream/mac/MediaStreamCenterMac.h']" exit_code: 1
Source/WebCore/bindings/js/JSDOMBinding.h:373: Extra space before ) in for [whitespace/parens] [5]
Source/WebCore/bindings/js/JSDOMBinding.h:384: Extra space before ) in for [whitespace/parens] [5]
Source/WebCore/Modules/mediastream/MediaStreamTrackSourcesRequest.h:32: wtf includes should be <wtf/file.h> instead of "wtf/file.h". [build/include] [4]
Source/WebCore/Modules/mediastream/MediaStreamTrackSourcesRequest.h:33: wtf includes should be <wtf/file.h> instead of "wtf/file.h". [build/include] [4]
Source/WebCore/Modules/mediastream/MediaStreamTrackSourcesRequest.h:34: wtf includes should be <wtf/file.h> instead of "wtf/file.h". [build/include] [4]
Source/WebCore/Modules/mediastream/MediaStreamTrackSourcesRequest.h:35: wtf includes should be <wtf/file.h> instead of "wtf/file.h". [build/include] [4]
Source/WebCore/Modules/mediastream/MediaStreamTrackSourcesRequest.cpp:33: Alphabetical sorting problem. [build/include_order] [4]
Source/WebCore/Modules/mediastream/MediaStreamTrack.cpp:31: Alphabetical sorting problem. [build/include_order] [4]
Source/WebCore/Modules/mediastream/SourceInfo.h:34: Alphabetical sorting problem. [build/include_order] [4]
Total errors found: 9 in 27 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 211596 [details] Patch for the bots to chew on Attachment 211596 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/1801286 Comment on attachment 211596 [details] Patch for the bots to chew on Attachment 211596 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/1817172 Created attachment 211600 [details]
Updated patch
Created attachment 211601 [details]
Updated patch
Created attachment 211604 [details]
Add blink revisions this is based on to ChangeLogs
Comment on attachment 211604 [details] Add blink revisions this is based on to ChangeLogs View in context: https://bugs.webkit.org/attachment.cgi?id=211604&action=review > Source/WebCore/Modules/mediastream/MediaStreamTrack.cpp:125 > + if (!MediaStreamCenter::instance().getMediaStreamTrackSources(request.release())) Not part of the patch, but is instance() the right name for the singleton? I think we typically use sharedBlah() or even just shared()? > Source/WebCore/Modules/mediastream/MediaStreamTrack.idl:42 > + [CallWith=ScriptExecutionContext, RaisesException] static void getSources(MediaStreamTrackSourcesCallback callback); Is this right? The more recent specs list a synchronous call called getSourceInfos(). If that's the case, I don't think the callback/request classes are necessary. > Source/WebCore/Modules/mediastream/MediaStreamTrackSourcesRequest.cpp:50 > +void MediaStreamTrackSourcesRequest::didCompleteRequest(const TrackSourceInfoVector& requstSourceInfos) Typo: requestSourceInfos > Source/WebCore/Modules/mediastream/MediaStreamTrackSourcesRequest.h:54 > + String origin() { return m_origin; } should be const. > Source/WebCore/Modules/mediastream/MediaStreamTrackSourcesRequest.h:56 > + PassRefPtr<ExtraData> extraData() const { return m_extraData; } This is strange. Shouldn't it just be ExtraData*? > Source/WebCore/Modules/mediastream/SourceInfo.cpp:56 > + return videoKind; This logic seems completely wrong. > Source/WebCore/bindings/js/JSDOMBinding.h:380 > + inline JSC::JSValue toJS(JSC::ExecState* exec, JSDOMGlobalObject* globalObject, Vector<RefPtr<T> > vector) Spaces between consecutive >'s is no longer needed, right? > Source/WebCore/platform/mediastream/MediaStreamTrackSourcesRequestClient.h:61 > + SourceKind kind() { return m_kind; } Why not const? > Source/WebCore/platform/mediastream/MediaStreamTrackSourcesRequestClient.h:62 > + VideoFacingMode facing() { return m_facing; } Ditto? > Source/WebCore/platform/mediastream/MediaStreamTrackSourcesRequestClient.h:79 > +typedef Vector<RefPtr<TrackSourceInfo> > TrackSourceInfoVector; Vector<RefPtr<TrackSourceInfo>> > LayoutTests/fast/mediastream/MediaStreamTrack-getSources.html:5 > +<script src="../js/resources/js-test-pre.js"></script> this is now ../../resources/ > LayoutTests/fast/mediastream/MediaStreamTrack-getSources.html:56 > + We should add a setTimeout to force the test to complete without waiting for the long test timeout. > LayoutTests/fast/mediastream/MediaStreamTrack-getSources.html:60 > +<script src="../js/resources/js-test-post.js"></script> ../../resources (In reply to comment #9) > (From update of attachment 211604 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=211604&action=review > > > Source/WebCore/Modules/mediastream/MediaStreamTrack.idl:42 > > + [CallWith=ScriptExecutionContext, RaisesException] static void getSources(MediaStreamTrackSourcesCallback callback); > > Is this right? The more recent specs list a synchronous call called getSourceInfos(). If that's the case, I don't think the callback/request classes are necessary. > Just FYI that the change from the synchronous getSourceInfos to the async getSources have been informally approved but not yet officially approved by the standardization committee nor pushed out. Chromium decided to jump the gun here because in some cases media device enum isn't instantaneous; especially when also querying for capabilities. Created attachment 211790 [details] Updated patch (In reply to comment #9) > (From update of attachment 211604 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=211604&action=review > > > Source/WebCore/Modules/mediastream/MediaStreamTrack.cpp:125 > > + if (!MediaStreamCenter::instance().getMediaStreamTrackSources(request.release())) > > Not part of the patch, but is instance() the right name for the singleton? I think we typically use sharedBlah() or even just shared()? > Sure. > > Source/WebCore/Modules/mediastream/MediaStreamTrack.idl:42 > > + [CallWith=ScriptExecutionContext, RaisesException] static void getSources(MediaStreamTrackSourcesCallback callback); > > Is this right? The more recent specs list a synchronous call called getSourceInfos(). If that's the case, I don't think the callback/request classes are necessary. > As Tommy notes, this change was agreed to informally on the mailing list in June. The spec bug is here: https://www.w3.org/Bugs/Public/show_bug.cgi?id=22269. > > Source/WebCore/Modules/mediastream/MediaStreamTrackSourcesRequest.cpp:50 > > +void MediaStreamTrackSourcesRequest::didCompleteRequest(const TrackSourceInfoVector& requstSourceInfos) > > Typo: requestSourceInfos > Fixed. > > Source/WebCore/Modules/mediastream/MediaStreamTrackSourcesRequest.h:54 > > + String origin() { return m_origin; } > > should be const. > Fixed. > > Source/WebCore/Modules/mediastream/MediaStreamTrackSourcesRequest.h:56 > > + PassRefPtr<ExtraData> extraData() const { return m_extraData; } > > This is strange. Shouldn't it just be ExtraData*? > Actually, we don't need this at all so I removed it. "ExtraData" is a chromium platform pattern we don't use). > > Source/WebCore/Modules/mediastream/SourceInfo.cpp:56 > > + return videoKind; > > This logic seems completely wrong. > Oops, fixed. > > Source/WebCore/bindings/js/JSDOMBinding.h:380 > > + inline JSC::JSValue toJS(JSC::ExecState* exec, JSDOMGlobalObject* globalObject, Vector<RefPtr<T> > vector) > > Spaces between consecutive >'s is no longer needed, right? > Fixed. > > Source/WebCore/platform/mediastream/MediaStreamTrackSourcesRequestClient.h:61 > > + SourceKind kind() { return m_kind; } > > Why not const? > Why not! > > Source/WebCore/platform/mediastream/MediaStreamTrackSourcesRequestClient.h:62 > > + VideoFacingMode facing() { return m_facing; } > > Ditto? > Ditto! > > Source/WebCore/platform/mediastream/MediaStreamTrackSourcesRequestClient.h:79 > > +typedef Vector<RefPtr<TrackSourceInfo> > TrackSourceInfoVector; > > Vector<RefPtr<TrackSourceInfo>> > Fixed. > > LayoutTests/fast/mediastream/MediaStreamTrack-getSources.html:5 > > +<script src="../js/resources/js-test-pre.js"></script> > > this is now ../../resources/ > Fixed, and removed the reference to "../js/resources/js-test-style.css". > > LayoutTests/fast/mediastream/MediaStreamTrack-getSources.html:56 > > + > > We should add a setTimeout to force the test to complete without waiting for the long test timeout. > Fixed. > > LayoutTests/fast/mediastream/MediaStreamTrack-getSources.html:60 > > +<script src="../js/resources/js-test-post.js"></script> > > ../../resources > Fixed. Comment on attachment 211790 [details]
Updated patch
Removing r?, this needs some rework.
Created attachment 211900 [details]
YAP
Comment on attachment 211900 [details] YAP View in context: https://bugs.webkit.org/attachment.cgi?id=211900&action=review OK to land this as is, but there are a lot of minor programming mistakes in this patch that you should fix. > Source/WebCore/Modules/mediastream/MediaStreamTrack.cpp:109 > + static NeverDestroyed<AtomicString> ended("ended", AtomicString::ConstructFromLiteral); > + static NeverDestroyed<AtomicString> live("live", AtomicString::ConstructFromLiteral); > + static NeverDestroyed<AtomicString> muted("muted", AtomicString::ConstructFromLiteral); I think that having these globals is probably excessive optimization. Is the speed of creating a new atomic string really prohibitive? > Source/WebCore/Modules/mediastream/MediaStreamTrackSourcesRequest.h:34 > +#include <wtf/PassRefPtr.h> Should not need to include this just to use PassRefPtr as an argument or return type. No header at all, or <wtf/Forward.h> should suffice. > Source/WebCore/Modules/mediastream/MediaStreamTrackSourcesRequest.h:35 > +#include <wtf/RefCounted.h> Not sure why you need to include this here. It seems you’d get it indirectly from some header. > Source/WebCore/Modules/mediastream/MediaStreamTrackSourcesRequest.h:44 > +class MediaStreamTrackSourcesRequest : public MediaStreamTrackSourcesRequestClient { Mark this class FINAL? > Source/WebCore/Modules/mediastream/MediaStreamTrackSourcesRequest.h:49 > + String origin() const { return m_origin; } Might consider const String& for this instead. > Source/WebCore/Modules/mediastream/MediaStreamTrackSourcesRequest.h:52 > + // MediaStreamTrackSourcesRequestClient > + void didCompleteRequest(const TrackSourceInfoVector&) OVERRIDE; Can this be private instead of public? > Source/WebCore/Modules/mediastream/SourceInfo.cpp:50 > + static NeverDestroyed<AtomicString> none("none", AtomicString::ConstructFromLiteral); > + static NeverDestroyed<AtomicString> audioKind("audio", AtomicString::ConstructFromLiteral); > + static NeverDestroyed<AtomicString> videoKind("video", AtomicString::ConstructFromLiteral); Is this optimization needed? > Source/WebCore/Modules/mediastream/SourceInfo.cpp:70 > + static NeverDestroyed<AtomicString> userFacing("user", AtomicString::ConstructFromLiteral); > + static NeverDestroyed<AtomicString> environmentFacing("environment", AtomicString::ConstructFromLiteral); > + static NeverDestroyed<AtomicString> leftFacing("left", AtomicString::ConstructFromLiteral); > + static NeverDestroyed<AtomicString> rightFacing("right", AtomicString::ConstructFromLiteral); Is this optimization needed? > Source/WebCore/Modules/mediastream/SourceInfo.h:34 > +#include <wtf/OwnPtr.h> Should not need to include this. > Source/WebCore/Modules/mediastream/SourceInfo.h:35 > +#include <wtf/PassRefPtr.h> Should not need to include this, maybe Forward.h instead. > Source/WebCore/Modules/mediastream/SourceInfo.h:45 > + virtual ~SourceInfo() { } This class isn’t polymorphic at all, so this just makes the code bigger for no reason. I suggest omitting it entirely. > Source/WebCore/Modules/mediastream/SourceInfo.h:50 > + AtomicString id() const { return m_trackSourceInfo->id(); } > + AtomicString label() const { return m_trackSourceInfo->label(); } > + AtomicString kind() const; > + AtomicString facing() const; Given the implementations of these, I think we can use const AtomicString& instead, perhaps. > Source/WebCore/Modules/mediastream/SourceInfo.h:53 > + SourceInfo(PassRefPtr<TrackSourceInfo>); Should mark this explicit. > Source/WebCore/Modules/mediastream/SourceInfo.h:58 > +typedef Vector<RefPtr<SourceInfo> > SourceInfoVector; Should not leave the space between "> >" -- that’s for older compilers that we no longer support. Also, I don’t think this typedef is a great idea. Generally Anders has been pointing out that such typedefs hide important details, like whether the elements are RefPtr or not, without making the code significantly more elegant. > Source/WebCore/platform/mediastream/MediaStreamTrackSourcesRequestClient.h:33 > +#include <wtf/text/WTFString.h> I don’t understand how this file can use AtomicString without including the relevant header. > Source/WebCore/platform/mediastream/MediaStreamTrackSourcesRequestClient.h:43 > + enum SourceKind { > + NoSource, > + Audio, > + Video > + }; I think that simple enums like this read better on a single line rather than splayed out over multiple lines like this. > Source/WebCore/platform/mediastream/MediaStreamTrackSourcesRequestClient.h:51 > + enum VideoFacingMode { > + None, > + User, > + Environment, > + Left, > + Right > + }; Ditto. > Source/WebCore/platform/mediastream/MediaStreamTrackSourcesRequestClient.h:57 > + virtual ~TrackSourceInfo() { } This should be omitted. There is no polymorphic use of this class, and this will just make the code bigger for no reason. > Source/WebCore/platform/mediastream/MediaStreamTrackSourcesRequestClient.h:60 > + AtomicString id() const { return m_id; } > + AtomicString label() const { return m_label; } Return type should be const AtomicString&. > Source/WebCore/platform/mediastream/MediaStreamTrackSourcesRequestClient.h:79 > +typedef Vector<RefPtr<TrackSourceInfo>> TrackSourceInfoVector; Same comment as elsewhere about these vector typedefs. > Source/WebCore/platform/mediastream/MediaStreamTrackSourcesRequestClient.h:85 > +public: > + > + virtual ~MediaStreamTrackSourcesRequestClient() { } Should remove this blank line. Committed in r155992: https://trac.webkit.org/r155992 |