[webkit-reviews] review denied: [Bug 71123] Implement TextTrackList and the textTracks attribute of HTMLMediaElement : [Attachment 114278] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Wed Nov 9 10:05:15 PST 2011


Sam Weinig <sam at webkit.org> has denied Eric Carlson <eric.carlson at apple.com>'s
request for review:
Bug 71123: Implement TextTrackList and the textTracks attribute of
HTMLMediaElement
https://bugs.webkit.org/show_bug.cgi?id=71123

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

------- Additional Comments from Sam Weinig <sam at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=114278&action=review


> Source/WebCore/html/HTMLMediaElement.cpp:2046
> +PassRefPtr<TextTrackList> HTMLMediaElement::textTracks() const 

This probably wants to be a raw pointer return type.

> Source/WebCore/html/HTMLMediaElement.h:249
> +    enum InvalidUrlAction { DoNothing, Complain };

Url should be spelled URL.

> Source/WebCore/html/HTMLTrackElement.cpp:157
> +PassRefPtr<TextTrack> HTMLTrackElement::track()

This should probably return a raw pointer.

> Source/WebCore/html/HTMLTrackElement.cpp:160
> +    RefPtr<TextTrack> track = loadableTrack();
> +    return track.release();

This can just be return loadableTrack();

> Source/WebCore/html/HTMLTrackElement.h:77
> +    // TextTrackLoadingClient

I think this is just a TextTrackClient now.

> Source/WebCore/html/HTMLTrackElement.h:88
> +    RefPtr<ScriptExecutionContext> m_context;

I am not sure you need to store this.  The Document associate with the element
should be the ScriptExecutionContext to use.

> Source/WebCore/html/LoadableTextTrack.h:47
> +    virtual void loadingCompleted(LoadableTextTrack*, bool /* loadingFailed
*/) { }

Maybe didCompleteLoad()?

> Source/WebCore/html/LoadableTextTrack.h:75
> +    RefPtr<ScriptExecutionContext> m_context;

You should probably be able to get to this through the m_trackElement.

> Source/WebCore/html/track/TextTrackList.h:31
> +#include "ActiveDOMObject.h"

I don't think you use this anymore.


More information about the webkit-reviews mailing list