[webkit-reviews] review granted: [Bug 106660] MediaStream API: Update the track accessors on MediaStream to match the latest specification : [Attachment 182343] Patch

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has granted Tommy Widenflycht
<tommyw at google.com>'s request for review:
Bug 106660: MediaStream API: Update the track accessors on MediaStream to match
the latest specification
https://bugs.webkit.org/show_bug.cgi?id=106660

Attachment 182343: Patch
https://bugs.webkit.org/attachment.cgi?id=182343&action=review

------- Additional Comments from Adam Barth <abarth at webkit.org>
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?