CLOSED WONTFIX209126
Simplify the mechanism to paint captions in video fullscreen and picture-in-picture 
https://bugs.webkit.org/show_bug.cgi?id=209126
Summary Simplify the mechanism to paint captions in video fullscreen and picture-in-p...
Peng Liu
Reported 2020-03-15 11:27:16 PDT
Simply the mechanism to paint captions in video fullscreen and picture-in-picture
Attachments
Patch (14.24 KB, patch)
2020-03-15 11:43 PDT, Peng Liu 		simon.fraser: review-
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
Radar WebKit Bug Importer
Comment 1 2020-03-15 11:28:40 PDT
<rdar://problem/60471459>
Peng Liu
Comment 2 2020-03-15 11:43:21 PDT
Created attachment 393621 [details] Patch
Eric Carlson
Comment 3 2020-03-15 16:30:16 PDT
Comment on attachment 393621 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=393621&action=review > Source/WebCore/html/shadow/MediaControlElements.cpp:1331 > + if (document().page()) { Nit: "if (auto* page = document().page())" would be slightly more efficient. > Source/WebCore/page/Page.cpp:1368 > + for (auto& callback : m_updateRenderingCallbacks.values()) A callback could mutate m_updateRenderingCallbacks so you should iterate over a copy. It is also possible for an existing callback to be removed during a client callback, so you should probably copy the keys to a vector and make a callback only if an identifier is still in m_updateRenderingCallbacks. > Source/WebCore/page/Page.h:952 > + uint64_t m_nextUpdateRenderingCallbackID { 0 }; An ObjectIdentifier would be better than a uint64_t.
Simon Fraser (smfr)
Comment 4 2020-03-16 09:42:52 PDT
Comment on attachment 393621 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=393621&action=review Seems OK but let's see another patch. > Source/WebCore/page/Page.cpp:1367 > #if ENABLE(VIDEO_TRACK) This shouldn't be inside the #ifdef any more.
Peng Liu
Comment 5 2020-03-16 13:44:19 PDT
Because of concerns about arbitrary callbacks running post-updateRendering, e.g., it will encourage people to run code that will dirty layout. Let's close this bug without landing the patch.
Note You need to log in before you can comment on or make changes to this bug.