Bug 185487 – Media continues loading after rendered invisible (removed from DOM; scrolled off screen)

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Jer Noble]

Media continues loading after rendered invisible (removed from DOM; scrolled off screen)

[Jer Noble]

<rdar://problem/38897810>

[Jer Noble]

Created attachment 340016 [details]
Patch

[EWS Watchlist]

Comment on attachment 340016 [details]
Patch

Attachment 340016 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/7627983

New failing tests:
media/video-restricted-invisible-autoplay-allowed-when-visible.html
media/media-source/only-bcp47-language-tags-accepted-as-valid.html

[EWS Watchlist]

Created attachment 340027 [details]
Archive of layout-test-results from ews100 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-sierra  Platform: Mac OS X 10.12.6

[Eric Carlson]

Comment on attachment 340016 [details]
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.

[EWS Watchlist]

Comment on attachment 340016 [details]
Patch

Attachment 340016 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/7628133

New failing tests:
media/video-restricted-invisible-autoplay-allowed-when-visible.html
media/media-source/only-bcp47-language-tags-accepted-as-valid.html

[EWS Watchlist]

Created attachment 340036 [details]
Archive of layout-test-results from ews106 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6

[Jer Noble]

Comment on attachment 340016 [details]
Patch

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

>> Source/WebCore/html/MediaElementSession.cpp:133
>> +    PlatformMediaSession::clientWillPausePlayback();
> 
> Don't you want to call PlatformMediaSession::clientWillBeginAutoplaying() instead?

Oof, yes.

>> Source/WebCore/html/MediaElementSession.cpp:134
>> +    m_elementWasRemovedFromDOM = false;
> 
> Why does playback clear m_elementWasRemovedFromDOM?

We only want to block loading until the media element tries to play.  If a video element was removed from the DOM, and then someone sets a src= and calls load(), we should unblock loading at this point.

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

Ditto above.

>> Source/WebCore/html/MediaElementSession.cpp:162
>> +        m_elementIsHiddenUntilVisibleInViewport = true;
> 
> Should this also check m_element.isFullscreen as inisVisibleInViewportChanged?

So this is confusing. elementIsHidden() == document().hidden() && m_videoFullscreenMode != VideoFullscreenModePictureInPicture.  So it's already checking whether it's PiP.  I don't /think/ that the Document will be hidden() when the video element is in standard fullscreen mode, and it definitely won't when in element fullscreen mode.  But just for sake of clarity, maybe we should explicitly check isFullscreen() here.

>> LayoutTests/media/video-buffering-allowed.html:15
>> +        }
> 
> Nit: why not put this in video-test.js?

Mostly just because of the risk of name collisions, and not wanting to make all the waitForEventAnd...() variants. :)

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

will remove.

[Eric Carlson]

Comment on attachment 340016 [details]
Patch

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

>>> Source/WebCore/html/MediaElementSession.cpp:134
>>> +    m_elementWasRemovedFromDOM = false;
>> 
>> Why does playback clear m_elementWasRemovedFromDOM?
> 
> We only want to block loading until the media element tries to play.  If a video element was removed from the DOM, and then someone sets a src= and calls load(), we should unblock loading at this point.

Sure, but m_elementWasRemovedFromDOM is probably not the right name for a variable that really means "allowed to load data".

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

Ditto.

[Jer Noble]

Created attachment 340045 [details]
Patch for landing

[Jer Noble]

Created attachment 340326 [details]
Patch for landing

[Jer Noble]

Created attachment 340332 [details]
Patch for landing

[EWS Watchlist]

Comment on attachment 340332 [details]
Patch for landing

Attachment 340332 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/7680059

Number of test failures exceeded the failure limit.

[EWS Watchlist]

Created attachment 340347 [details]
Archive of layout-test-results from ews117 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-sierra  Platform: Mac OS X 10.12.6

[Jer Noble]

Created attachment 340376 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 340376 [details]
Patch for landing

Clearing flags on attachment: 340376

Committed r231817: <https://trac.webkit.org/changeset/231817>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.