Bug 64791

Summary:

MediaStream API: Update the tracklists to the latest spec

Product:

WebKit

Reporter:

Tommy Widenflycht <tommyw>

Component:

DOM

Assignee:

Tommy Widenflycht <tommyw>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

dglazkov, gustavo.noronha, gustavo, leandrogracia, tonyg, webkit.review.bot, xan.lopez

Priority:

Version:

528+ (Nightly build)

Hardware:

All

OS:

All

Bug Depends on:

Bug Blocks:

56459

Attachments:

Description	Flags
Patch	none
Patch	webkit-ews: commit-queue-
Patch	tonyg: review+
Patch	none

Tommy Widenflycht

Reported 2011-07-19 04:19:46 PDT

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

Attachments
Patch (123.06 KB, patch) 2011-07-19 04:38 PDT, Tommy Widenflycht	no flags	Details Formatted Diff Diff
Patch (100.29 KB, patch) 2011-07-20 03:32 PDT, Tommy Widenflycht	webkit-ews: commit-queue-	Details Formatted Diff Diff
Patch (122.96 KB, patch) 2011-07-20 04:47 PDT, Tommy Widenflycht	tonyg: review+	Details Formatted Diff Diff
Patch (122.92 KB, patch) 2011-07-20 06:44 PDT, Tommy Widenflycht	no flags	Details Formatted Diff Diff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.

Tommy Widenflycht

Comment 1 2011-07-19 04:38:05 PDT

Created attachment 101300 [details] Patch

Tommy Widenflycht

Comment 2 2011-07-19 04:40:03 PDT

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

Tommy Widenflycht

Comment 3 2011-07-19 06:16:49 PDT

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 4 2011-07-19 08:10:24 PDT

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 5 2011-07-20 03:31:41 PDT

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

Comment 6 2011-07-20 03:32:06 PDT

Created attachment 101445 [details] Patch

Early Warning System Bot

Comment 7 2011-07-20 03:43:02 PDT

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 8 2011-07-20 03:49:48 PDT

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 9 2011-07-20 04:00:28 PDT

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

Tommy Widenflycht

Comment 10 2011-07-20 04:47:59 PDT

Created attachment 101450 [details] Patch

Leandro Graciá Gil

Comment 11 2011-07-20 05:03:20 PDT

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

Tony Gentilcore

Comment 12 2011-07-20 06:29:33 PDT

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 13 2011-07-20 06:42:56 PDT

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

Comment 14 2011-07-20 06:44:31 PDT

Created attachment 101463 [details] Patch

WebKit Review Bot

Comment 15 2011-07-20 07:55:16 PDT

Comment on attachment 101463 [details] Patch Clearing flags on attachment: 101463 Committed r91364: <http://trac.webkit.org/changeset/91364>

WebKit Review Bot

Comment 16 2011-07-20 07:55:22 PDT

All reviewed patches have been landed. Closing bug.

Note You need to log in before you can comment on or make changes to this bug.