Bug 120883

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 Flags
Patch for the bots to chew on
buildbot: commit-queue-
Updated patch
none
Updated patch
none
Add blink revisions this is based on to ChangeLogs
none
Updated patch
none
YAP none

Description Eric Carlson 2013-09-06 12:44:14 PDT
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.
Comment 1 Eric Carlson 2013-09-13 13:24:37 PDT
This will also require a merge of some parts of https://chromium.googlesource.com/chromium/blink/+/ff783a23bb1add588971a8187048a305cf485121
Comment 2 Eric Carlson 2013-09-13 15:12:16 PDT
Created attachment 211596 [details]
Patch for the bots to chew on
Comment 3 WebKit Commit Bot 2013-09-13 15:14:48 PDT
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 4 Build Bot 2013-09-13 15:24:04 PDT
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 5 Build Bot 2013-09-13 15:54:22 PDT
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
Comment 6 Eric Carlson 2013-09-13 16:15:04 PDT
Created attachment 211600 [details]
Updated patch
Comment 7 Eric Carlson 2013-09-13 16:24:44 PDT
Created attachment 211601 [details]
Updated patch
Comment 8 Eric Carlson 2013-09-13 16:52:51 PDT
Created attachment 211604 [details]
Add blink revisions this is based on to ChangeLogs
Comment 9 Jon Lee 2013-09-16 00:56:17 PDT
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
Comment 10 Tommy Widenflycht 2013-09-16 05:12:22 PDT
(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.
Comment 11 Eric Carlson 2013-09-16 08:39:06 PDT
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 12 Eric Carlson 2013-09-16 13:43:52 PDT
Comment on attachment 211790 [details]
Updated patch

Removing r?, this needs some rework.
Comment 13 Eric Carlson 2013-09-17 07:07:34 PDT
Created attachment 211900 [details]
YAP
Comment 14 Darin Adler 2013-09-17 08:53:51 PDT
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.
Comment 15 Eric Carlson 2013-09-17 13:35:49 PDT
Committed in r155992: https://trac.webkit.org/r155992