Created attachment 101300 [details] Patch

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

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 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 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.

Created attachment 101445 [details] Patch

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

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

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

Created attachment 101450 [details] Patch

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

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 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.

Created attachment 101463 [details] Patch

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

All reviewed patches have been landed. Closing bug.