Bug 120883 – MediaStream API: Changing the device enumeration to be async

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Eric Carlson]

Consider merging https://chromium.googlesource.com/chromium/blink/+/40a96080a1531e50de4eb84571c7dc9fb321ece5 

When we stared to implement this in chrome we discovered that sometimes
device enumeration could take some time and therefore the API needed
to be adjusted. This has been discussed on the W3C email list without
objections but not yet reached the editors draft.
The old name getSourceInfos got changed to getSources as well.

[Eric Carlson]

This will also require a merge of some parts of https://chromium.googlesource.com/chromium/blink/+/ff783a23bb1add588971a8187048a305cf485121

[Eric Carlson]

Created attachment 211596 [details]
Patch for the bots to chew on

[WebKit Commit Bot]

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.

[Build Bot]

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

[Build Bot]

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

[Eric Carlson]

Created attachment 211600 [details]
Updated patch

[Eric Carlson]

Created attachment 211601 [details]
Updated patch

[Eric Carlson]

Created attachment 211604 [details]
Add blink revisions this is based on to ChangeLogs

[Jon Lee]

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

[Tommy Widenflycht]

(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.

[Eric Carlson]

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.

[Eric Carlson]

Comment on attachment 211790 [details]
Updated patch

Removing r?, this needs some rework.

[Eric Carlson]

Created attachment 211900 [details]
YAP

[Darin Adler]

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.

[Eric Carlson]

Committed in r155992: https://trac.webkit.org/r155992