[webkit-reviews] review canceled: [Bug 62881] Loading out-of-band text track files from <track> : [Attachment 98121] Adding "Source/WebCore/platform/track \" to GNUmakefile.am

[bugzilla-daemon at webkit.org]

Eric Carlson <eric.carlson at apple.com> has canceled 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 98121: Adding "Source/WebCore/platform/track \" to GNUmakefile.am
https://bugs.webkit.org/attachment.cgi?id=98121&action=review

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

Clearing r? so you can respond to comments.


> Source/WebCore/html/HTMLMediaElement.h:306
> +#if ENABLE(VIDEO_TRACK)
> +    // loading tracks
> +    void loadTextTracks();
> +#endif

Comments should be complete sentences (first word capitalized, followed by a
period, etc). However in this case I don't think this comment adds anything
because the function name says what it does.

> Source/WebCore/html/HTMLTrackElement.cpp:146
> +    if (hasAttribute(srcAttr))
> +	   m_track->load(getNonEmptyURLAttribute(srcAttr), context);
> +    else
> +	   for (Node* node = firstChild(); node; node = node->nextSibling()) {
> +	       if (node->hasTagName(sourceTag)) {
> +		   HTMLSourceElement* source =
static_cast<HTMLSourceElement*>(node);
> +		   KURL url = source->getNonEmptyURLAttribute(srcAttr);
> +		   if (!url.isEmpty() && m_track->supportsType(url))
> +		       m_track->load(url, context);
> +	       }
> +	   }
> +

While I agree that <track> should support <source>, it is a void element in
both specs at the moment so I think you shouldn't land support for it yet.

> Source/WebCore/html/LoadableTextTrack.cpp:41
>  LoadableTextTrack::~LoadableTextTrack()
>  {
> -    // FIXME(62881): Implement.
>  }

Aren't you leaking "m_private" here? Is there any reason not to use an OwnPtr?

As discussed in IRC, please consider just getting rid of LoadableTextTrackImpl
by combining it with this class.