RESOLVED FIXED 106660
MediaStream API: Update the track accessors on MediaStream to match the latest specification
https://bugs.webkit.org/show_bug.cgi?id=106660
Summary MediaStream API: Update the track accessors on MediaStream to match the lates...
Tommy Widenflycht
Reported 2013-01-11 06:31:13 PST
MediaStream API: Update the track accessors on MediaStream to match the latest specification
Attachments
Patch (50.23 KB, patch)
2013-01-11 06:35 PST, Tommy Widenflycht
no flags
Patch (51.23 KB, patch)
2013-01-11 07:30 PST, Tommy Widenflycht
abarth: review+
webkit.review.bot: commit-queue-
Patch for landing (52.62 KB, patch)
2013-01-14 02:05 PST, Tommy Widenflycht
no flags
Patch for landing (52.11 KB, patch)
2013-01-14 03:58 PST, Tommy Widenflycht
no flags
Tommy Widenflycht
Comment 1 2013-01-11 06:35:57 PST
Tommy Widenflycht
Comment 2 2013-01-11 06:38:18 PST
FYI the patch size is mostly deleted files and edited tests.
kov's GTK+ EWS bot
Comment 3 2013-01-11 07:04:11 PST
Tommy Widenflycht
Comment 4 2013-01-11 07:30:06 PST
Adam Barth
Comment 5 2013-01-11 10:20:16 PST
Regressions: Unexpected text-only failures (1) webaudio/mediastreamaudiosourcenode.html [ Failure ]
Adam Barth
Comment 6 2013-01-11 10:24:07 PST
Comment on attachment 182343 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=182343&action=review Looks great. I wonder if there's one more test you need to update (based on the partial EWS results). > Source/WebCore/ChangeLog:13 > + In short: the attributes audioTrack/videoTrack that returned special MediaStreamTrackLists have been > + replaced by the functions getAudioTracks()/getVideoTracks that return standard sequences of > + MediaStreamTracks. Great! > Source/WebCore/Modules/mediastream/MediaStream.cpp:166 > + default: > + ASSERT_NOT_REACHED(); We generally like to leave off the default and let the compiler tell us when we forget an enum value. > Source/WebCore/Modules/mediastream/MediaStream.h:71 > + DEFINE_ATTRIBUTE_EVENT_LISTENER(addtrack); > + DEFINE_ATTRIBUTE_EVENT_LISTENER(removetrack); Should these be "trackadded" and "trackremoved" ? Those names sound more like notifications. (I'm sure you're just matching the spec here---this just jumped out at me.) > Source/WebCore/Modules/mediastream/MediaStream.h:95 > + // ContextDestructionObserver > + virtual void contextDestroyed(); OVERRIDE?
WebKit Review Bot
Comment 7 2013-01-11 11:09:10 PST
Comment on attachment 182343 [details] Patch Attachment 182343 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/15811373 New failing tests: webaudio/mediastreamaudiosourcenode.html
WebKit Review Bot
Comment 8 2013-01-11 12:02:57 PST
Comment on attachment 182343 [details] Patch Attachment 182343 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/15811386 New failing tests: webaudio/mediastreamaudiosourcenode.html
Tommy Widenflycht
Comment 9 2013-01-14 01:47:34 PST
Comment on attachment 182343 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=182343&action=review >> Source/WebCore/ChangeLog:13 >> + MediaStreamTracks. > > Great! Indeed! >> Source/WebCore/Modules/mediastream/MediaStream.cpp:166 >> + ASSERT_NOT_REACHED(); > > We generally like to leave off the default and let the compiler tell us when we forget an enum value. Fixed. >> Source/WebCore/Modules/mediastream/MediaStream.h:71 >> + DEFINE_ATTRIBUTE_EVENT_LISTENER(removetrack); > > Should these be "trackadded" and "trackremoved" ? Those names sound more like notifications. (I'm sure you're just matching the spec here---this just jumped out at me.) You are right in both cases; the names is not 100% ok and I am matching the spec. >> Source/WebCore/Modules/mediastream/MediaStream.h:95 >> + virtual void contextDestroyed(); > > OVERRIDE? Fixed.
Tommy Widenflycht
Comment 10 2013-01-14 02:05:21 PST
Created attachment 182528 [details] Patch for landing
Tommy Widenflycht
Comment 11 2013-01-14 03:58:01 PST
Created attachment 182543 [details] Patch for landing
Tommy Widenflycht
Comment 12 2013-01-14 04:04:32 PST
Patch updated for merge issues.
WebKit Review Bot
Comment 13 2013-01-14 05:32:27 PST
Comment on attachment 182543 [details] Patch for landing Clearing flags on attachment: 182543 Committed r139611: <http://trac.webkit.org/changeset/139611>
Note You need to log in before you can comment on or make changes to this bug.