[Webkit-unassigned] [Bug 80583] [Meta] Audio/Video Elements bugs found by the IE test center

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=80583


Eric Carlson <eric.carlson at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #133477|review?                     |review-
               Flag|                            |




--- Comment #5 from Eric Carlson <eric.carlson at apple.com>  2012-03-23 09:50:59 PST ---
(From update of attachment 133477)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.