[webkit-reviews] review denied: [Bug 62881] Loading out-of-band text track files from <track> : [Attachment 97928] Patch

[bugzilla-daemon at webkit.org]

Sam Weinig <sam at webkit.org> has denied Anna Cavender <annacc at chromium.org>'s
request for review:
Bug 62881: Loading out-of-band text track files from <track>
https://bugs.webkit.org/show_bug.cgi?id=62881

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

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

> Source/WebCore/html/HTMLTrackElement.cpp:45
> +    , m_track(0)

RefPtr's are initialized to null, so this is not necessary.

> Source/WebCore/html/HTMLTrackElement.cpp:54
> +    document()->unregisterForDocumentActivationCallbacks(this);

You probably need to add calls to this and
registerForDocumentActivationCallbacks when moving to a different document. 
See HTMLMediaElement::willMoveToNewOwnerDocument and
HTMLMediaElement::didMoveToNewOwnerDocument.

> Source/WebCore/html/HTMLTrackElement.cpp:143
> +	   Node* node;
> +	   for (node = firstChild(); node; node = node->nextSibling()) {

There is no reason to declare the Node* out side of the for-loop syntax.

> Source/WebCore/platform/track/CueParser.h:70
>      void fetchParsedCues(Vector<PassRefPtr<TextTrackCue> >&);

Not new code, but this is a highly unusual thing to do.  This Vector should
probably be a Vector<RefPtr<foo> >.