[Webkit-unassigned] [Bug 114330] Refactor TextTrack and TextTrackList to make it easier to add audio and video tracks

[bugzilla-daemon at webkit.org]

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





--- Comment #20 from Eric Carlson <eric.carlson at apple.com>  2013-04-12 10:41:00 PST ---
(From update of attachment 197865)
View in context: https://bugs.webkit.org/attachment.cgi?id=197865&action=review

This is really close! 

Thanks for taking this on, refactoring isn't a lot of fun :-)

> Source/WebCore/ChangeLog:19
> +        (WebCore::HTMLMediaElement::mediaPlayerDidAddTextTrack):
> +        (WebCore::HTMLMediaElement::mediaPlayerDidRemoveTextTrack):
> +        (WebCore::HTMLMediaElement::addTextTrack):
> +        (WebCore::HTMLMediaElement::removeTextTrack):
> +        (WebCore::HTMLMediaElement::removeAllInbandTracks):
> +        (WebCore::HTMLMediaElement::didAddTextTrack):
> +        (WebCore::HTMLMediaElement::didRemoveTextTrack):

Nit: I think it is useful to have brief comments in the ChangeLog detailing what changed in each method.

> Source/WebCore/ChangeLog:21
> +        (WebCore):

Nit: all of these empty entries added by the script can be removed.

> Source/WebCore/html/track/TextTrack.h:87
> +    virtual const AtomicString& defaultKindKeyword() const { return subtitlesKeyword(); }

Nit: OVERRIDE.

> Source/WebCore/html/track/TextTrack.h:169
> +    virtual void refEventTarget() { ref(); }
> +    virtual void derefEventTarget() { deref(); }

OVERRIDE

> Source/WebCore/html/track/TextTrackList.cpp:58
> +    TextTrack* textTrack = static_cast<TextTrack*>(track);

This would be better if you had a toTextTrack() function that ASSERTs when passed the wrong type (see the various toXXX functions in Render classes).

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