[webkit-reviews] review denied: [Bug 72279] media/track/track-webvtt-tc004-magic-header.html flakily times out : [Attachment 134522] Patch

[bugzilla-daemon at webkit.org]

Eric Carlson <eric.carlson at apple.com> has denied Zan Dobersek
<zandobersek at gmail.com>'s request for review:
Bug 72279: media/track/track-webvtt-tc004-magic-header.html flakily times out
https://bugs.webkit.org/show_bug.cgi?id=72279

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

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


This is close, but I would prefer to have the cleanup noted. Thanks for
identifying the issue and taking it on!

> Source/WebCore/html/track/WebVTTParser.cpp:141
>	   case Initial:
>	       // 4-12 - Collect the first line and check for "WEBVTT".
> -	       if (!hasRequiredFileIdentifier(data, length))
> +	       if (!hasRequiredFileIdentifier(data, length)) {
> +		   if (m_client)
> +		       m_client->fileUnsuccessfullyParsed();
>		   return;
> +	       }

"fileUnsuccessfullyParsed" is awkward, how about something like
"fileFailedToParse", or "invalidFile" instead?

As long as you are fixing this, I think it would be a good idea to move *all*
of the logic about the file identifier into the parser. IOW, allow the client
to call parseBytes() immediately instead of waiting until it knows that there
is enough data loaded to check the file identifier. This will require the
parser stay in the "Initial" state until can check for the identifier, but that
will only require us to buffer up to 6 or 9 bytes before checking for the
identifier.

> Source/WebCore/loader/TextTrackLoader.cpp:195
> +

Please include the error LOG(...) here.