WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
Details
Formatted Diff
Diff
Patch
(51.23 KB, patch)
2013-01-11 07:30 PST
,
Tommy Widenflycht
abarth
: review+
webkit.review.bot
: commit-queue-
Details
Formatted Diff
Diff
Patch for landing
(52.62 KB, patch)
2013-01-14 02:05 PST
,
Tommy Widenflycht
no flags
Details
Formatted Diff
Diff
Patch for landing
(52.11 KB, patch)
2013-01-14 03:58 PST
,
Tommy Widenflycht
no flags
Details
Formatted Diff
Diff
Show Obsolete
(2)
View All
Add attachment
proposed patch, testcase, etc.
Tommy Widenflycht
Comment 1
2013-01-11 06:35:57 PST
Created
attachment 182333
[details]
Patch
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
Comment on
attachment 182333
[details]
Patch
Attachment 182333
[details]
did not pass gtk-ews (gtk): Output:
http://queues.webkit.org/results/15803433
Tommy Widenflycht
Comment 4
2013-01-11 07:30:06 PST
Created
attachment 182343
[details]
Patch
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.
Top of Page
Format For Printing
XML
Clone This Bug