Bug 215488 – The PiP button on the fullscreen youtube player disappears after starting a new video in a playlist

• Status: RESOLVED
• Resolution: FIXED
• Component: Media

[Peng Liu]

The PiP button on the fullscreen youtube player disappears after starting a new video in a playlist

[Radar WebKit Bug Importer]

<rdar://problem/67039225>

[Peng Liu]

Created attachment 406570 [details]
Patch

[Peng Liu]

Created attachment 406577 [details]
update the change log

[Eric Carlson]

Comment on attachment 406577 [details]
update the change log

Great ChangeLog!

[EWS]

Committed r265690: <https://trac.webkit.org/changeset/265690>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 406577 [details].

[Darin Adler]

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.

[Peng Liu]

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.