[webkit-reviews] review granted: [Bug 125270] [MSE] Bring end-of-stream algorithm section up to current spec. : [Attachment 218575] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Jer Noble <jer.noble at apple.com>'s
request for review:
Bug 125270: [MSE] Bring end-of-stream algorithm section up to current spec.
https://bugs.webkit.org/show_bug.cgi?id=125270

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

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=218575&action=review


This looks good. The patch did not apply, so EWS did not process it. I can’t
tell how well the test covers all the code paths.

> Source/WebCore/Modules/mediasource/MediaSource.cpp:344
> +    if (error.isNull() || error.isEmpty()) {

Null strings return true for isEmpty so there is no need to check both. Can
just check isEmpty.

> Source/WebCore/Modules/mediasource/MediaSource.cpp:368
> +	   if (m_mediaElement->readyState() == HTMLMediaElement::HAVE_NOTHING)
{
> +	       //  ↪︎ If the HTMLMediaElement.readyState attribute equals
HAVE_NOTHING
> +	       //    Run the "If the media data cannot be fetched at all, due
to network errors, causing
> +	       //    the user agent to give up trying to fetch the resource"
steps of the resource fetch algorithm.
> +	       m_mediaElement->mediaLoadingFailed(MediaPlayer::NetworkError);
> +	   } else {
> +	       //  ↪︎ If the HTMLMediaElement.readyState attribute is
greater than HAVE_NOTHING
> +	       //    Run the "If the connection is interrupted after some media
data has been received, causing the
> +	       //    user agent to give up trying to fetch the resource" steps
of the resource fetch algorithm.
> +	      
m_mediaElement->mediaLoadingFailedFatally(MediaPlayer::NetworkError);
> +	   }

The comments here do not make it clear why one case calls “failed fatally” and
the other does not. I guess the problem is that literally quoting this piece of
the spec ends up meaning we don’t say enough.

> Source/WebCore/Modules/mediasource/MediaSource.cpp:384
> +	   return;

Seems unnecessary to have a return just before the end of the function.

> Source/WebCore/Modules/mediasource/MediaSource.h:83
>      void endOfStream(const AtomicString& error, ExceptionCode&);
> +    void endOfStreamAlgorithm(const AtomicString& error, ExceptionCode&);

I don’t think the names here are clear enough. It’s not like “end of stream” is
not already an algorithm. I know there is some term of art or concept in the
spec we are referring to here, but I would suggest making the names more
distinct, probably also including a verb in the name of the new function. And
also I would suggest not paragraphing this internal function right next to
public functions from the DOM API.