Media continues loading after rendered invisible (removed from DOM; scrolled off screen)
<rdar://problem/38897810>
Created attachment 340016 [details] Patch
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
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
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.
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
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
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.
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.
Created attachment 340045 [details] Patch for landing
Created attachment 340326 [details] Patch for landing
Created attachment 340332 [details] Patch for landing
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.
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
Created attachment 340376 [details] Patch for landing
Comment on attachment 340376 [details] Patch for landing Clearing flags on attachment: 340376 Committed r231817: <https://trac.webkit.org/changeset/231817>
All reviewed patches have been landed. Closing bug.