Bug 206661

Summary: HTMLMediaElement should not remove the media session at DOM suspension time
Product: WebKit Reporter: youenn fablet <youennf>
Component: WebRTCAssignee: youenn fablet <youennf>
Status: RESOLVED FIXED    
Severity: Normal CC: calvaris, cdumez, commit-queue, eric.carlson, esprehn+autocc, ews-watchlist, glenn, gyuyoung.kim, jacob_uphoff, jer.noble, philipj, rniwa, sergio, webkit-bug-importer, youennf
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Description youenn fablet 2020-01-23 06:56:33 PST 
HTMLMediaElement should not remove the media session at DOM suspension time
Comment 1 youenn fablet 2020-01-23 06:56:43 PST 
<rdar://problem/58800787>
Comment 2 youenn fablet 2020-01-23 07:04:24 PST 
Created attachment 388539 [details]
Patch
Comment 3 Eric Carlson 2020-01-23 08:01:22 PST 
Comment on attachment 388539 [details]
Patch

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

> Source/WebCore/ChangeLog:16
> +        Partially revert the behavior by calling the same code as clientWiillPausePlayback but make sure updateNowPlayingInfo is calling asynchronously when suspending the media element.

s/clientWiillPausePlayback/clientWillPausePlayback/

Also, this line is really long.

> Source/WebCore/html/HTMLMediaElement.cpp:2331
> +        queueTaskKeepingObjectAlive(*this, TaskSource::Networking, [this] {

Why not TaskSource::MediaElement?

> Source/WebCore/platform/audio/PlatformMediaSession.cpp:253
> +    bool shouldDelayCallingUpdateNowPlaying = false;
> +    return processClientWillPausePlayback(shouldDelayCallingUpdateNowPlaying);
> +}
> +
> +void PlatformMediaSession::clientWillBeDOMSuspended()
> +{
> +    bool shouldDelayCallingUpdateNowPlaying = true;
> +    processClientWillPausePlayback(shouldDelayCallingUpdateNowPlaying);

Why not pass a PlatformMediaSessionManager::DelayCallingUpdateNowPlaying instead of a bool? Or maybe define the enum in this class and use it in PlatformMediaSessionManager
Comment 4 youenn fablet 2020-01-23 23:39:04 PST 
Created attachment 388656 [details]
Patch
Comment 5 youenn fablet 2020-01-24 08:50:00 PST 
Created attachment 388699 [details]
Patch
Comment 6 youenn fablet 2020-01-24 08:51:47 PST 
Thanks for the review Eric, I took it all!
As of iOS failure, this is due to the test simulating scrolling but waiting for an event while scrolling.
Given scrolling is suspending DOM objects, events cannot really happen during that time. I updated the test accordingly.
Comment 7 WebKit Commit Bot 2020-01-25 04:27:44 PST 
Comment on attachment 388699 [details]
Patch

Clearing flags on attachment: 388699

Committed r255116: <https://trac.webkit.org/changeset/255116>
Comment 8 WebKit Commit Bot 2020-01-25 04:27:46 PST 
All reviewed patches have been landed.  Closing bug.
Comment 9 Alexey Proskuryakov 2020-01-27 23:05:38 PST 
*** Bug 206760 has been marked as a duplicate of this bug. ***