[webkit-reviews] review granted: [Bug 185487] Media continues loading after rendered invisible (removed from DOM; scrolled off screen) : [Attachment 340016] Patch

[bugzilla-daemon at webkit.org]

Eric Carlson <eric.carlson at apple.com> has granted Jer Noble
<jer.noble at apple.com>'s request for review:
Bug 185487: Media continues loading after rendered invisible (removed from DOM;
scrolled off screen)
https://bugs.webkit.org/show_bug.cgi?id=185487

Attachment 340016: Patch

https://bugs.webkit.org/attachment.cgi?id=340016&action=review




--- Comment #5 from Eric Carlson <eric.carlson at apple.com> ---
Comment on attachment 340016
  --> https://bugs.webkit.org/attachment.cgi?id=340016
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=340016&action=review

r=me once the bots are also satisfied with it.

> Source/WebCore/html/HTMLMediaElement.h:1072
> +    bool m_elementWasRemovedFromDOM : 1;

As you pointed out, this isn't used.

> Source/WebCore/html/MediaElementSession.cpp:133
> +    PlatformMediaSession::clientWillPausePlayback();

Don't you want to call PlatformMediaSession::clientWillBeginAutoplaying()
instead?

> Source/WebCore/html/MediaElementSession.cpp:134
> +    m_elementWasRemovedFromDOM = false;

Why does playback clear m_elementWasRemovedFromDOM?

> Source/WebCore/html/MediaElementSession.cpp:143
> +    m_elementWasRemovedFromDOM = false;

Ditto.

> Source/WebCore/html/MediaElementSession.cpp:162
> +    if (m_element.elementIsHidden())
> +	   m_elementIsHiddenUntilVisibleInViewport = true;

Should this also check m_element.isFullscreen as inisVisibleInViewportChanged?

> LayoutTests/media/video-buffering-allowed.html:15
> +	   function waitFor(element, event) {
> +	       return new Promise(resolve => {
> +		   element.addEventListener(event, event => {
> +		       consoleWrite(`EVENT(${event.type})`);
> +		       resolve(event);
> +		   }, { once: true });
> +	       });
> +	   }

Nit: why not put this in video-test.js?

> LayoutTests/media/video-buffering-allowed.html:30
> +	       run ('video.play()');

Nit: extra space after "run"

> LayoutTests/media/video-test.js:106
> +    return new Promise(async (resolve, reject) => {

Nit: reject is not used.