[WebKitLegacy] Media playback pauses on scroll
<rdar://problem/46420245>
Created attachment 357610 [details] Patch
Attachment 357610 [details] did not pass style-queue: ERROR: Tools/TestWebKitAPI/Tests/WebKitLegacy/ios/ScrollingDoesNotPauseMedia.mm:48: This { should be at the end of the previous line [whitespace/braces] [4] Total errors found: 1 in 21 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 357615 [details] Patch
Attachment 357615 [details] did not pass style-queue: ERROR: Tools/TestWebKitAPI/Tests/WebKitLegacy/ios/ScrollingDoesNotPauseMedia.mm:48: This { should be at the end of the previous line [whitespace/braces] [4] Total errors found: 1 in 24 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 357615 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=357615&action=review > Source/WebCore/dom/Document.cpp:1727 > + platformMediaSessionManager->stopAllMediaPlaybackForDocument(this); Nit: you might as well update stopAllMediaPlaybackForDocument to take a Document reference. > Source/WebCore/html/HTMLMediaElement.cpp:834 > + INFO_LOG(LOGIDENTIFIER, "src = ", value); We shouldn't log urls in release builds. You can have this only log in a debug build by changing it to something like INFO_LOG(LOGIDENTIFIER, "src = ", URL { URL { }, value }); > Source/WebCore/page/Page.cpp:1738 > + for (Frame* frame = &mainFrame(); frame; frame = frame->tree().traverseNext()) { > + if (auto* document = frame->document()) > + document->suspendAllMediaPlayback(); > + } > + > + m_mediaPlaybackIsSuspended = true; Nit: it is probably slightly safer to set m_mediaPlaybackIsSuspended before suspending the documents, in case they do something that causes this to be called again.
(In reply to Eric Carlson from comment #6) > Comment on attachment 357615 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=357615&action=review > > > Source/WebCore/dom/Document.cpp:1727 > > + platformMediaSessionManager->stopAllMediaPlaybackForDocument(this); > > Nit: you might as well update stopAllMediaPlaybackForDocument to take a > Document reference. > > > Source/WebCore/html/HTMLMediaElement.cpp:834 > > + INFO_LOG(LOGIDENTIFIER, "src = ", value); > > We shouldn't log urls in release builds. You can have this only log in a > debug build by changing it to something like > > INFO_LOG(LOGIDENTIFIER, "src = ", URL { URL { }, value }); Yeah, this was a debugging statement that shouldn't have gone into this patch; I'll delete it. > > Source/WebCore/page/Page.cpp:1738 > > + for (Frame* frame = &mainFrame(); frame; frame = frame->tree().traverseNext()) { > > + if (auto* document = frame->document()) > > + document->suspendAllMediaPlayback(); > > + } > > + > > + m_mediaPlaybackIsSuspended = true; > > Nit: it is probably slightly safer to set m_mediaPlaybackIsSuspended before > suspending the documents, in case they do something that causes this to be > called again. No, it needs to happen afterwards, because pause() checks whether playback is allowed, which it won't be if playback is suspended.
Created attachment 358337 [details] Patch for landing
Attachment 358337 [details] did not pass style-queue: ERROR: Tools/TestWebKitAPI/Tests/WebKitLegacy/ios/ScrollingDoesNotPauseMedia.mm:48: This { should be at the end of the previous line [whitespace/braces] [4] Total errors found: 1 in 24 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 358337 [details] Patch for landing Clearing flags on attachment: 358337 Committed r239640: <https://trac.webkit.org/changeset/239640>
All reviewed patches have been landed. Closing bug.
Comment on attachment 358337 [details] Patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=358337&action=review > Source/WebCore/ChangeLog:15 > + Do not use suspendActiveDOMObjects(ReasonForSuspension::PageWillBeSuspended) to pause > + video. Roll back the changes to HTMLMediaElement, and introduce a new set of Page calls > + suspendAllMediaPlayback() & resumeAllMediaPlayback() which replaces the removed bahavior. What was the cause of the bug though?
(In reply to Simon Fraser (smfr) from comment #12) > Comment on attachment 358337 [details] > Patch for landing > > View in context: > https://bugs.webkit.org/attachment.cgi?id=358337&action=review > > > Source/WebCore/ChangeLog:15 > > + Do not use suspendActiveDOMObjects(ReasonForSuspension::PageWillBeSuspended) to pause > > + video. Roll back the changes to HTMLMediaElement, and introduce a new set of Page calls > > + suspendAllMediaPlayback() & resumeAllMediaPlayback() which replaces the removed bahavior. > > What was the cause of the bug though? WebKitLegacy uses suspendActiveDOMObjects(PageWillBeSuspended) during active scroll operations.
The ScrollingDoesNotPauseMedia API Test added in this patch seems to be failing consistently. It would be nice to fix that soon to keep the automation green/happy.