Bug 238579

Summary:

Add a heuristic to identify and extract the prominent video element in element fullscreen

Product:

WebKit

Reporter:

Wenson Hsieh <wenson_hsieh>

Component:

Media

Assignee:

Wenson Hsieh <wenson_hsieh>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

akeerthi, calvaris, cdumez, changseok, cmarcelo, eric.carlson, esprehn+autocc, ews-watchlist, glenn, gyuyoung.kim, jer.noble, kangil.han, katherine_cheney, megan_gardner, philipj, sergio, thorton, webkit-bug-importer

Priority:

Keywords:

InRadar

Version:

WebKit Nightly Build

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

Bug Blocks:

238607

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	jer.noble: review+
Patch for landing	none

Wenson Hsieh

Reported 2022-03-30 16:10:03 PDT

Attachments
Patch (13.11 KB, patch) 2022-03-30 17:05 PDT, Wenson Hsieh	no flags	Details Formatted Diff Diff
Patch (10.42 KB, patch) 2022-03-30 21:41 PDT, Wenson Hsieh	no flags	Details Formatted Diff Diff
Patch (13.23 KB, patch) 2022-03-31 07:45 PDT, Wenson Hsieh	jer.noble: review+	Details Formatted Diff Diff
Patch for landing (13.40 KB, patch) 2022-03-31 14:41 PDT, Wenson Hsieh	no flags	Details Formatted Diff Diff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.

Wenson Hsieh

Comment 1 2022-03-30 17:05:40 PDT Comment hidden (obsolete)

Created attachment 456187 [details] Patch

Wenson Hsieh

Comment 2 2022-03-30 21:41:31 PDT Comment hidden (obsolete)

Created attachment 456202 [details] Patch

Wenson Hsieh

Comment 3 2022-03-31 07:45:54 PDT

Created attachment 456235 [details] Patch

Jer Noble

Comment 4 2022-03-31 14:16:04 PDT

Comment on attachment 456235 [details] Patch Nit: it appears this heuristic depends on the video element already existing in the page when fullscreen mode is entered. You may want consider a mutation observer, or listening for events bubbled up to the fullscreen element itself (like 'loadstart', 'play' or 'pause') to trigger recalculation of this heuristic.

Wenson Hsieh

Comment 5 2022-03-31 14:27:32 PDT

Thanks for the review! (In reply to Jer Noble from comment #4) > Comment on attachment 456235 [details] > Patch > > Nit: it appears this heuristic depends on the video element already existing > in the page when fullscreen mode is entered. You may want consider a > mutation observer, or listening for events bubbled up to the fullscreen > element itself (like 'loadstart', 'play' or 'pause') to trigger > recalculation of this heuristic. Indeed! That is an omission from this iteration of the heuristic (as well as the case where m_mainVideoElement is unparented while in element fullscreen mode, and replaced with a new video element). I was initially planning to address this in a future patch by overriding HTMLMediaElement's tree insertion/removal method hooks (as well as other media-related methods), but I think the approach of listening for media-related events bubbled up to the fullscreen element is a lot more elegant — I'll go with that approach, and add a FIXME as well.

Wenson Hsieh

Comment 6 2022-03-31 14:41:22 PDT

Created attachment 456284 [details] Patch for landing

EWS

Comment 7 2022-03-31 22:53:59 PDT

Committed r292205 (249108@main): <https://commits.webkit.org/249108@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 456284 [details].

Radar WebKit Bug Importer

Comment 8 2022-03-31 22:55:15 PDT

<rdar://problem/91146510>

Note You need to log in before you can comment on or make changes to this bug.