Bug 215488 - The PiP button on the fullscreen youtube player disappears after starting a new video in a playlist
Summary: The PiP button on the fullscreen youtube player disappears after starting a n...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: WebKit Nightly Build
Hardware: iPhone / iPad Unspecified
: P2 Normal
Assignee: Peng Liu
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-08-13 20:25 PDT by Peng Liu
Modified: 2020-08-14 16:36 PDT (History)
8 users (show)

See Also:

Attachments
Patch (3.30 KB, patch)
2020-08-13 21:13 PDT, Peng Liu 		no flags Details | Formatted Diff | Diff
update the change log (3.34 KB, patch)
2020-08-14 00:15 PDT, Peng Liu 		no flags Details | Formatted Diff | Diff
Show Obsolete (1) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Peng Liu 2020-08-13 20:25:01 PDT 
The PiP button on the fullscreen youtube player disappears after starting a new video in a playlist
Comment 1 Radar WebKit Bug Importer 2020-08-13 20:25:51 PDT 
<rdar://problem/67039225>
Comment 2 Peng Liu 2020-08-13 21:13:57 PDT 
Created attachment 406570 [details]
Patch
Comment 3 Peng Liu 2020-08-14 00:15:54 PDT 
Created attachment 406577 [details]
update the change log
Comment 4 Eric Carlson 2020-08-14 11:33:28 PDT 
Comment on attachment 406577 [details]
update the change log

Great ChangeLog!
Comment 5 EWS 2020-08-14 11:57:30 PDT 
Committed r265690: <https://trac.webkit.org/changeset/265690>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 406577 [details].
Comment 6 Darin Adler 2020-08-14 15:37:18 PDT 
Comment on attachment 406577 [details]
update the change log

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

> Source/WebCore/platform/cocoa/PlaybackSessionModelMediaElement.mm:73
> -    if (m_mediaElement == mediaElement)
> +    if (m_mediaElement == mediaElement) {
> +        if (m_mediaElement) {
> +            for (auto client : m_clients)
> +                client->isPictureInPictureSupportedChanged(isPictureInPictureSupported());
> +        }
>          return;
> +    }

Adding this guaranteed side effect of this function seems like a difficult-to-maintain oblique way to fix this, especially since there is no regression test.

I’d like us to find a way to make a regression test.

I’d also like to make this not depend on side effects like this. Requiring that setting something to an existing value has a side effect means the function should probably be named differently and is not a simple setter. So one solution would be to rename the function.
Comment 7 Peng Liu 2020-08-14 16:36:07 PDT 
Comment on attachment 406577 [details]
update the change log

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

>> Source/WebCore/platform/cocoa/PlaybackSessionModelMediaElement.mm:73
>> +    }
> 
> Adding this guaranteed side effect of this function seems like a difficult-to-maintain oblique way to fix this, especially since there is no regression test.
> 
> I’d like us to find a way to make a regression test.
> 
> I’d also like to make this not depend on side effects like this. Requiring that setting something to an existing value has a side effect means the function should probably be named differently and is not a simple setter. So one solution would be to rename the function.

Agree. Filed bug 215526 to track the task.