MediaStream API: Split the MediaStream track list into audio/video specific ones.

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Tommy Widenflycht]

The latest draft of the WebRTC standard have split the MediaStream combined track list into audio/video specific ones.

[Tommy Widenflycht]

Created attachment 123091 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail] before submitting because this patch contains changes to the Chromium public API.

[WebKit Review Bot]

Comment on attachment 123091 [details]
Patch

Attachment 123091 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11299055

New failing tests:
platform/chromium/media/video-capture-preview.html

[Tommy Widenflycht]

Created attachment 123108 [details]
Patch

[Darin Fisher (:fishd, Google)]

Comment on attachment 123108 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=123108&action=review

> Source/WebCore/mediastream/MediaStream.cpp:41
> +void processTrackList(PassRefPtr<MediaStreamTrackList> prpTracks, String kind, MediaStreamSourceVector& sources, ExceptionCode& ec)

nit: this function should be marked static or placed in an anonymous namespace.
nit: String should be passed as |const String&|

> Source/WebCore/mediastream/MediaStream.cpp:44
> +    if (tracks.get()) {

nit: consider an early return if !tracks.get()

what is the reason for storing prpTracks as tracks?

> Source/WebKit/chromium/public/WebUserMediaClient.h:46
> +    virtual void requestUserMedia(const WebUserMediaRequest&, const WebVector<WebMediaStreamSource>&, const WebVector<WebMediaStreamSource>&) { }

nit: add parameter names for the new function?  it isn't always obvious which is the set of the audio tracks and which is the set of video tracks.

shouldn't the comment just be about declaring the first function as DEPRECATED?  the second function is the new one, right?

> Source/WebKit/chromium/public/WebUserMediaRequest.h:74
> +    WEBKIT_EXPORT void requestSucceeded(const WebVector<WebMediaStreamSource>&, const WebVector<WebMediaStreamSource>&);

same nit:  please name the parameters.  it is not clear otherwise which one is for audio and which one is for video.

> Source/WebKit/chromium/public/platform/WebMediaStreamDescriptor.h:59
> +    WEBKIT_EXPORT void initialize(const WebString& label, const WebVector<WebMediaStreamSource>&, const WebVector<WebMediaStreamSource>&);

same nit.

[Tommy Widenflycht]

Comment on attachment 123108 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=123108&action=review

>> Source/WebCore/mediastream/MediaStream.cpp:41
>> +void processTrackList(PassRefPtr<MediaStreamTrackList> prpTracks, String kind, MediaStreamSourceVector& sources, ExceptionCode& ec)
> 
> nit: this function should be marked static or placed in an anonymous namespace.
> nit: String should be passed as |const String&|

Fixed * 2

>> Source/WebCore/mediastream/MediaStream.cpp:44
>> +    if (tracks.get()) {
> 
> nit: consider an early return if !tracks.get()
> 
> what is the reason for storing prpTracks as tracks?

Fixed nit.

Don't think I understand your question. I need to store the PassRefPtr into a RefPtr so I can use it more than once.

>> Source/WebKit/chromium/public/WebUserMediaClient.h:46
>> +    virtual void requestUserMedia(const WebUserMediaRequest&, const WebVector<WebMediaStreamSource>&, const WebVector<WebMediaStreamSource>&) { }
> 
> nit: add parameter names for the new function?  it isn't always obvious which is the set of the audio tracks and which is the set of video tracks.
> 
> shouldn't the comment just be about declaring the first function as DEPRECATED?  the second function is the new one, right?

Fixed

>> Source/WebKit/chromium/public/WebUserMediaRequest.h:74
>> +    WEBKIT_EXPORT void requestSucceeded(const WebVector<WebMediaStreamSource>&, const WebVector<WebMediaStreamSource>&);
> 
> same nit:  please name the parameters.  it is not clear otherwise which one is for audio and which one is for video.

Fixed.

>> Source/WebKit/chromium/public/platform/WebMediaStreamDescriptor.h:59
>> +    WEBKIT_EXPORT void initialize(const WebString& label, const WebVector<WebMediaStreamSource>&, const WebVector<WebMediaStreamSource>&);
> 
> same nit.

Fixed.

[Tommy Widenflycht]

Created attachment 123539 [details]
Patch

[WebKit Review Bot]

Comment on attachment 123539 [details]
Patch

Clearing flags on attachment: 123539

Committed r105774: <http://trac.webkit.org/changeset/105774>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.