Bug 220887 - Twitter PiP video pauses when scrolling
Summary: Twitter PiP video pauses when scrolling
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: 2021-01-22 20:18 PST by Peng Liu
Modified: 2021-01-25 16:58 PST (History)
13 users (show)

See Also:

Attachments
WIP patch (9.45 KB, patch)
2021-01-22 22:45 PST, Peng Liu 		no flags Details | Formatted Diff | Diff
WIP patch (9.46 KB, patch)
2021-01-22 22:58 PST, Peng Liu 		no flags Details | Formatted Diff | Diff
WIP patch (7.90 KB, patch)
2021-01-22 23:33 PST, Peng Liu 		no flags Details | Formatted Diff | Diff
Patch (9.45 KB, patch)
2021-01-25 11:02 PST, Peng Liu 		eric.carlson: review+
Details | Formatted Diff | Diff
Revise the patch based on Eric's comments (9.55 KB, patch)
2021-01-25 12:16 PST, Peng Liu 		no flags Details | Formatted Diff | Diff
Show Obsolete (3) 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 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].