Bug 64791 – MediaStream API: Update the tracklists to the latest spec

• Component: DOM
• Status: RESOLVED FIXED

[Tommy Widenflycht]

ExclusiveTrackList, MultipleTrackList and TrackList are now history and are replaced by MediaStreamTrackList / MediaStreamTrack.

http://www.whatwg.org/specs/web-apps/current-work/multipage/video-conferencing-and-peer-to-peer-communication.html

[Tommy Widenflycht]

Created attachment 101300 [details]
Patch

[Tommy Widenflycht]

Had to upload a manual patch again even though I explicitly deleted the old files first before created the new ones!?

[Tommy Widenflycht]

Doh, that's how git works (automatic rename detection)


(In reply to comment #2)
> Had to upload a manual patch again even though I explicitly deleted the old files first before created the new ones!?

[Leandro Graciá Gil]

Comment on attachment 101300 [details]
Patch

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

LGTM with a few comments and previously checking if the old TrackList/ExclusiveTrackList/MultipleTrackList objects are not required by the implementers of the Media Elements.

> Source/WebCore/dom/LocalMediaStream.h:50
> +    virtual bool isLocalMediaStream() const { return true; }

I think this may be introducing a regression. The reason of why a boolean was passed to the MediaStream constructor instead of using a virtual method here is because this value may be required by methods triggered during destruction. Please check that it's not the case.

> Source/WebCore/dom/MediaStreamTrack.cpp:28
> +#if ENABLE(MEDIA_STREAM) || ENABLE(VIDEO_TRACK)

VIDEO_TRACK is not required anymore since the spec proposes a different kind of track lists for them now: http://www.whatwg.org/specs/web-apps/current-work/multipage/the-video-element.html#audiotracklist
Because of this reason and since now the MediaStreamTracks are MediaStream API-specific, we should probably put the entire file inside a single ENABLE(MEDIA_STREAM).

> Source/WebCore/dom/MediaStreamTrack.cpp:68
> +#if ENABLE(MEDIA_STREAM)

Remove as commented before.

> Source/WebCore/dom/MediaStreamTrack.h:28
> +#if ENABLE(MEDIA_STREAM) || ENABLE(VIDEO_TRACK)

Same as before.

> Source/WebCore/dom/MediaStreamTrack.h:63
> +#endif // ENABLE(MEDIA_STREAM) || ENABLE(VIDEO_TRACK)

Remove VIDEO_TRACK also here in the comments.

> Source/WebCore/dom/MediaStreamTrack.idl:28
> +        Conditional=MEDIA_STREAM|VIDEO_TRACK,

VIDEO_TRACK not needed anymore.

> Source/WebCore/dom/MediaStreamTrackList.h:46
> +    void associateStream(const String& label) { m_associatedStreamLabel = label; }

No problem here, but we should be careful about the implications of this track -> media stream association when this forking-inheritance thing is introduced.

> Source/WebCore/dom/MediaStreamTrackList.idl:29
> +        Conditional=MEDIA_STREAM|VIDEO_TRACK,

VIDEO_TRACK not required anymore.

[Tommy Widenflycht]

Comment on attachment 101300 [details]
Patch

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

>> Source/WebCore/dom/LocalMediaStream.h:50
>> +    virtual bool isLocalMediaStream() const { return true; }
> 
> I think this may be introducing a regression. The reason of why a boolean was passed to the MediaStream constructor instead of using a virtual method here is because this value may be required by methods triggered during destruction. Please check that it's not the case.

Reintroduced.

>> Source/WebCore/dom/MediaStreamTrack.cpp:28
>> +#if ENABLE(MEDIA_STREAM) || ENABLE(VIDEO_TRACK)
> 
> VIDEO_TRACK is not required anymore since the spec proposes a different kind of track lists for them now: http://www.whatwg.org/specs/web-apps/current-work/multipage/the-video-element.html#audiotracklist
> Because of this reason and since now the MediaStreamTracks are MediaStream API-specific, we should probably put the entire file inside a single ENABLE(MEDIA_STREAM).

Done.

>> Source/WebCore/dom/MediaStreamTrack.cpp:68
>> +#if ENABLE(MEDIA_STREAM)
> 
> Remove as commented before.

Done.

>> Source/WebCore/dom/MediaStreamTrack.h:28
>> +#if ENABLE(MEDIA_STREAM) || ENABLE(VIDEO_TRACK)
> 
> Same as before.

Done.

>> Source/WebCore/dom/MediaStreamTrack.h:63
>> +#endif // ENABLE(MEDIA_STREAM) || ENABLE(VIDEO_TRACK)
> 
> Remove VIDEO_TRACK also here in the comments.

Done.

>> Source/WebCore/dom/MediaStreamTrack.idl:28
>> +        Conditional=MEDIA_STREAM|VIDEO_TRACK,
> 
> VIDEO_TRACK not needed anymore.

Done.

>> Source/WebCore/dom/MediaStreamTrackList.h:46
>> +    void associateStream(const String& label) { m_associatedStreamLabel = label; }
> 
> No problem here, but we should be careful about the implications of this track -> media stream association when this forking-inheritance thing is introduced.

Done.

>> Source/WebCore/dom/MediaStreamTrackList.idl:29
>> +        Conditional=MEDIA_STREAM|VIDEO_TRACK,
> 
> VIDEO_TRACK not required anymore.

Done.

[Tommy Widenflycht]

Created attachment 101445 [details]
Patch

[Early Warning System Bot]

Comment on attachment 101445 [details]
Patch

Attachment 101445 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/9201236

[WebKit Review Bot]

Comment on attachment 101445 [details]
Patch

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

[Collabora GTK+ EWS bot]

Comment on attachment 101445 [details]
Patch

Attachment 101445 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/9208180

[Tommy Widenflycht]

Created attachment 101450 [details]
Patch

[Leandro Graciá Gil]

(In reply to comment #10)
> Created an attachment (id=101450) [details]
> Patch

LGTM.

[Tony Gentilcore]

Comment on attachment 101450 [details]
Patch

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

> Source/WebCore/dom/MediaStreamTrackList.cpp:28
> +#if ENABLE(MEDIA_STREAM) || ENABLE(VIDEO_TRACK)

Is ENABLE(VIDEO_TRACK) still necessary here?

[Tommy Widenflycht]

Comment on attachment 101450 [details]
Patch

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

>> Source/WebCore/dom/MediaStreamTrackList.cpp:28
>> +#if ENABLE(MEDIA_STREAM) || ENABLE(VIDEO_TRACK)
> 
> Is ENABLE(VIDEO_TRACK) still necessary here?

No, removed.

[Tommy Widenflycht]

Created attachment 101463 [details]
Patch

[WebKit Review Bot]

Comment on attachment 101463 [details]
Patch

Clearing flags on attachment: 101463

Committed r91364: <http://trac.webkit.org/changeset/91364>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.