Bug 220887

Summary:

Twitter PiP video pauses when scrolling

Product:

WebKit

Reporter:

Peng Liu <peng.liu6>

Component:

Media

Assignee:

Peng Liu <peng.liu6>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

calvaris, cdumez, changseok, eric.carlson, esprehn+autocc, ews-watchlist, glenn, gyuyoung.kim, jer.noble, kondapallykalyan, philipj, sergio, webkit-bug-importer

Priority:

Keywords:

InRadar

Version:

WebKit Nightly Build

Hardware:

iPhone / iPad

OS:

Unspecified

See Also:

https://bugs.webkit.org/show_bug.cgi?id=220939

Attachments:

Description	Flags
WIP patch	none
WIP patch	none
WIP patch	none
Patch	eric.carlson: review+
Revise the patch based on Eric's comments	none

Description Peng Liu 2021-01-22 20:18:29 PST

Twitter PiP video pauses when scrolling

Comment 1 Peng Liu 2021-01-22 20:19:37 PST

<rdar://73369869>

Comment 2 Peng Liu 2021-01-22 22:45:24 PST

Created attachment 418214 [details]
WIP patch

Comment 3 Peng Liu 2021-01-22 22:58:23 PST

Created attachment 418215 [details]
WIP patch

Comment 4 Peng Liu 2021-01-22 23:33:00 PST

Created attachment 418218 [details]
WIP patch

Comment 5 Peng Liu 2021-01-25 11:02:02 PST

Created attachment 418309 [details]
Patch

Comment 6 Eric Carlson 2021-01-25 11:49:18 PST

Comment on attachment 418309 [details]
Patch

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

> Source/WebCore/html/MediaElementSession.cpp:279
> +SuccessOr<MediaPlaybackDenialReason> MediaElementSession::playbackPermitted(MediaPlaybackOperation operation) const

As we discussed, I'm not happy with this enum. Please add a FIXME here and note the url of the bug you filed to fix this.

> Source/WebCore/page/Quirks.cpp:1269
> +    // Facebook and twitter will naively pause a <video> element that has scrolled out of the viewport,

s/twitter/Twitter/

> Source/WebCore/page/Quirks.cpp:1276
>          auto domain = RegistrableDomain(m_document->topDocument().url());

auto domain = RegistrableDomain(m_document->topDocument().url()).string();
m_requiresUserGestureToPauseInPictureInPicture = domain == "facebook.com"_s || domain == "twitter.com"_s;

Comment 7 Peng Liu 2021-01-25 12:16:45 PST

Created attachment 418318 [details]
Revise the patch based on Eric's comments

Comment 8 Peng Liu 2021-01-25 12:18:47 PST

Comment on attachment 418309 [details]
Patch

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

>> Source/WebCore/html/MediaElementSession.cpp:279
>> +SuccessOr<MediaPlaybackDenialReason> MediaElementSession::playbackPermitted(MediaPlaybackOperation operation) const
> 
> As we discussed, I'm not happy with this enum. Please add a FIXME here and note the url of the bug you filed to fix this.

Filed bug 220939 to track the refactoring.

>> Source/WebCore/page/Quirks.cpp:1269
>> +    // Facebook and twitter will naively pause a <video> element that has scrolled out of the viewport,
> 
> s/twitter/Twitter/

Fixed.

>> Source/WebCore/page/Quirks.cpp:1276
>>          auto domain = RegistrableDomain(m_document->topDocument().url());
> 
> auto domain = RegistrableDomain(m_document->topDocument().url()).string();
> m_requiresUserGestureToPauseInPictureInPicture = domain == "facebook.com"_s || domain == "twitter.com"_s;

Good point! Fixed.

Comment 9 EWS 2021-01-25 16:58:09 PST

Committed r271870: <https://trac.webkit.org/changeset/271870>

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