Bug 238579 - Add a heuristic to identify and extract the prominent video element in element fullscreen
Summary: Add a heuristic to identify and extract the prominent video element in elemen...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Wenson Hsieh
URL:
Keywords: InRadar
Depends on:
Blocks: 238607
  Show dependency treegraph
 
Reported: 2022-03-30 16:10 PDT by Wenson Hsieh
Modified: 2022-03-31 22:55 PDT (History)
18 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Wenson Hsieh 2022-03-30 16:10:03 PDT
.
Comment 1 Wenson Hsieh 2022-03-30 17:05:40 PDT Comment hidden (obsolete)
Comment 2 Wenson Hsieh 2022-03-30 21:41:31 PDT Comment hidden (obsolete)
Comment 3 Wenson Hsieh 2022-03-31 07:45:54 PDT
Created attachment 456235 [details]
Patch
Comment 4 Jer Noble 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.
Comment 5 Wenson Hsieh 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.
Comment 6 Wenson Hsieh 2022-03-31 14:41:22 PDT
Created attachment 456284 [details]
Patch for landing
Comment 7 EWS 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].
Comment 8 Radar WebKit Bug Importer 2022-03-31 22:55:15 PDT
<rdar://problem/91146510>