Bug 64791 - MediaStream API: Update the tracklists to the latest spec
Summary: MediaStream API: Update the tracklists to the latest spec
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Tommy Widenflycht
URL:
Keywords:
Depends on:
Blocks: 56459
  Show dependency treegraph
 
Reported: 2011-07-19 04:19 PDT by Tommy Widenflycht
Modified: 2011-07-20 07:55 PDT (History)
7 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Tommy Widenflycht 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
Comment 1 Tommy Widenflycht 2011-07-19 04:38:05 PDT
Created attachment 101300 [details]
Patch
Comment 2 Tommy Widenflycht 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!?
Comment 3 Tommy Widenflycht 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!?
Comment 4 Leandro Graciá Gil 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.
Comment 5 Tommy Widenflycht 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.
Comment 6 Tommy Widenflycht 2011-07-20 03:32:06 PDT
Created attachment 101445 [details]
Patch
Comment 7 Early Warning System Bot 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
Comment 8 WebKit Review Bot 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
Comment 9 Collabora GTK+ EWS bot 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
Comment 10 Tommy Widenflycht 2011-07-20 04:47:59 PDT
Created attachment 101450 [details]
Patch
Comment 11 Leandro Graciá Gil 2011-07-20 05:03:20 PDT
(In reply to comment #10)
> Created an attachment (id=101450) [details]
> Patch

LGTM.
Comment 12 Tony Gentilcore 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?
Comment 13 Tommy Widenflycht 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.
Comment 14 Tommy Widenflycht 2011-07-20 06:44:31 PDT
Created attachment 101463 [details]
Patch
Comment 15 WebKit Review Bot 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>
Comment 16 WebKit Review Bot 2011-07-20 07:55:22 PDT
All reviewed patches have been landed.  Closing bug.