[webkit-reviews] review denied: [Bug 80583] [Meta] Audio/Video Elements bugs found by the IE test center : [Attachment 133477] Fixed tests #18 and #19

[bugzilla-daemon at webkit.org]

Eric Carlson <eric.carlson at apple.com> has denied Andrei Poenaru
<andrey_poenaru at yahoo.com>'s request for review:
Bug 80583: [Meta] Audio/Video Elements bugs found by the IE test center
https://bugs.webkit.org/show_bug.cgi?id=80583

Attachment 133477: Fixed tests #18 and #19
https://bugs.webkit.org/attachment.cgi?id=133477&action=review

------- Additional Comments from Eric Carlson <eric.carlson at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=133477&action=review


While this patch fixes the problem, I think it takes the wrong approach. The
TextTrackList is just a specialized container that doesn't really know anything
about the behavior of a TextTrack. The TextTrackList is owned and managed by an
HTMLMediaElement, which already knows about its HTMLTrackElement children, so I
think it makes much more sense to have the logic about enabling the default
track live there.

Also, I prefer to have the actual spec text in the sources as comments (as we
do in most of the media element code). This part of the spec is fairly complex
and it is still changing, so having the actual spec in the source makes it
easier to follow the logic and to see when we need to change something because
of a spec change.

Thanks for taking picking this up!

> Source/WebCore/html/HTMLTrackElement.cpp:84
> +	   if (!isDefault())
> +	       m_track->setMode(TextTrack::DISABLED, ec);

Are you certain that m_track can't be NULL here?
"ensureTrack()->setMode(TextTrack::DISABLED, ec)" would be much safer.

> Source/WebCore/html/track/TextTrackList.h:98
> +    LoadableTextTrack* m_defaultTrack; 

Hanging onto a raw pointer to an object whose lifetime is managed elsewhere is
fraught with peril.

> LayoutTests/media/track/track-appended-changes-mode.html:9
> +	       var videoElem1, videoElem2;
> +	       var trackElem1, trackElem2;

Nit: WebKit style is to declare each variable separately.

> LayoutTests/media/track/track-appended-changes-mode.html:19
> +		   testExpected("trackElem1.track.mode == TextTrack.SHOWING",
true);

You should also test adding a second track with the default attribute set to
make sure it is NOT showing.

> LayoutTests/media/track/track-appended-changes-mode.html:26
> +		   run("videoElem2.appendChild(trackElem2)");
> +		   testExpected("trackElem2.track.mode == TextTrack.DISABLED",
true);

And you might as well test adding a track with 'default' after adding one
without to make sure it is enabled.