Bug 106660 - MediaStream API: Update the track accessors on MediaStream to match the latest specification
Summary: MediaStream API: Update the track accessors on MediaStream to match the lates...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Tommy Widenflycht
URL:
Keywords: WebExposed
Depends on:
Blocks: 56459
  Show dependency tree / graph
 
Reported: 2013-01-11 06:31 PST by Tommy Widenflycht
Modified: 2013-01-14 05:34 PST (History)
11 users (show)

See Also:

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 an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Tommy Widenflycht 2013-01-11 06:31:13 PST 
MediaStream API: Update the track accessors on MediaStream to match the latest specification
Comment 1 Tommy Widenflycht 2013-01-11 06:35:57 PST 
Created attachment 182333 [details]
Patch
Comment 2 Tommy Widenflycht 2013-01-11 06:38:18 PST 
FYI the patch size is mostly deleted files and edited tests.
Comment 3 kov's GTK+ EWS bot 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
Comment 4 Tommy Widenflycht 2013-01-11 07:30:06 PST 
Created attachment 182343 [details]
Patch
Comment 5 Adam Barth 2013-01-11 10:20:16 PST 
Regressions: Unexpected text-only failures (1)
  webaudio/mediastreamaudiosourcenode.html [ Failure ]
Comment 6 Adam Barth 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?
Comment 7 WebKit Review Bot 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
Comment 8 WebKit Review Bot 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
Comment 9 Tommy Widenflycht 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.
Comment 10 Tommy Widenflycht 2013-01-14 02:05:21 PST 
Created attachment 182528 [details]
Patch for landing
Comment 11 Tommy Widenflycht 2013-01-14 03:58:01 PST 
Created attachment 182543 [details]
Patch for landing
Comment 12 Tommy Widenflycht 2013-01-14 04:04:32 PST 
Patch updated for merge issues.
Comment 13 WebKit Review Bot 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>