| Summary: | [VTT] Fix various issues with complicated rendering of VTT cues | ||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Jer Noble <jer.noble> | ||||||||||||||||
| Component: | Media | Assignee: | Jer Noble <jer.noble> | ||||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||||
| Severity: | Normal | CC: | calvaris, cdumez, changseok, eric.carlson, esprehn+autocc, ews-watchlist, glenn, gyuyoung.kim, jean-yves.avenard, kondapallykalyan, pdr, philipj, sergio, webkit-bug-importer | ||||||||||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||||||||||
| Version: | WebKit Nightly Build | ||||||||||||||||||
| Hardware: | Unspecified | ||||||||||||||||||
| OS: | Unspecified | ||||||||||||||||||
| See Also: | https://bugs.webkit.org/show_bug.cgi?id=234090 | ||||||||||||||||||
| Attachments: |
|
||||||||||||||||||
|
Description
Jer Noble
2021-12-06 16:15:07 PST
Created attachment 446099 [details]
Patch
Created attachment 446101 [details]
Patch
Comment on attachment 446101 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=446101&action=review > Source/WebCore/html/track/VTTCue.h:248 > + std::pair<float, float> m_displayPosition { undefinedPosition, undefinedPosition }; I think this should be std::optional<std::pair<...>> ; having particular values doing a particular behaviour is often a later source of error. > Source/WebCore/rendering/RenderVTTCue.cpp:173 > + return overlappingObjectForRect(backdropBox().absoluteBoundingBoxRect()); const RenderVTTCue* > Source/WebCore/rendering/RenderVTTCue.cpp:359 > if (downcast<TextTrackCueGeneric>(*m_cue).useDefaultPosition() && firstLineBox) { could be testing for firstLineBox first > Source/WebCore/rendering/RenderVTTCue.cpp:392 > +RenderBlockFlow& RenderVTTCue::backdropBox() const const RenderBlockFlow& > Source/WebCore/rendering/RenderVTTCue.h:53 > + RenderVTTCue* overlappingObjectForRect(const IntRect&) const; both should return `const RenderVTTCue*` > Source/WebCore/rendering/RenderVTTCue.h:68 > + RenderInline& cueBox() const; this should return a const reference; particularly as the method is const. Created attachment 446176 [details]
Patch
Comment on attachment 446176 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=446176&action=review r=me > Source/WebCore/rendering/RenderVTTCue.cpp:157 > + return !rectIsWithinContainer(backdropBox().absoluteBoundingBoxRect()); It would be nice to have a convenience method on RenderVTTCue to replace all of the `backdropBox().absoluteBoundingBoxRect()`. > LayoutTests/media/track/track-webvtt-no-snap-to-lines-overlap.html:36 > + <title>track-webvtt-no-snap-to-lines-overlap</title> > + <style> > + html { overflow:hidden } > + body { margin:0 } > + ::cue { > + font-family: Ahem, sans-serif; > + font-size: 12px; > + background-color: black; > + color: green; > + } > + </style> > + <script>var requirePixelDump = true;</script> > + <script src="../video-test.js"></script> > + <script src="../media-file.js"></script> > + <script> > + window.addEventListener('load', async event => { > + findMediaElement(); > + > + run(`video.querySelector("track").src = "captions-webvtt/no-snap-to-lines-overlap.vtt"`); > + run(`video.textTracks[0].mode = "showing"`); > + await waitFor(video.querySelector('track'), 'load'); > + > + run(`video.src = findMediaFile("video", "w3c/track/webvtt/media/white")`); > + await waitFor(video, 'canplaythrough'); > + > + endTest(); > + }); > + </script> > +</head> > +<body> > + <video> > + <track id="testTrack" kind="captions" default> > + </video> Mixed tabs and spaces! (In reply to Jean-Yves Avenard [:jya] from comment #3) > Comment on attachment 446101 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=446101&action=review > > > Source/WebCore/html/track/VTTCue.h:248 > > + std::pair<float, float> m_displayPosition { undefinedPosition, undefinedPosition }; > > I think this should be std::optional<std::pair<...>> ; having particular > values doing a particular behaviour is often a later source of error. Agreed! > > Source/WebCore/rendering/RenderVTTCue.cpp:173 > > + return overlappingObjectForRect(backdropBox().absoluteBoundingBoxRect()); > > const RenderVTTCue* I don't think that's necessary. The object returned doesn't belong to this RenderVTTCue at all. > > Source/WebCore/rendering/RenderVTTCue.cpp:359 > > if (downcast<TextTrackCueGeneric>(*m_cue).useDefaultPosition() && firstLineBox) { > > could be testing for firstLineBox first Ok. > > Source/WebCore/rendering/RenderVTTCue.cpp:392 > > +RenderBlockFlow& RenderVTTCue::backdropBox() const > > const RenderBlockFlow& Since RenderElement::firstChild() is const and returns a non-const pointer, and this is just a wrapper around firstChild(), I don't actually think it's necessary here. > > Source/WebCore/rendering/RenderVTTCue.h:53 > > + RenderVTTCue* overlappingObjectForRect(const IntRect&) const; > > both should return `const RenderVTTCue*` Disagree for overlappingObjectForRect(); it's not owned by this RenderVTTCue object. > > Source/WebCore/rendering/RenderVTTCue.h:68 > > + RenderInline& cueBox() const; > > this should return a const reference; particularly as the method is const. Ditto for above. Created attachment 446246 [details]
Patch for landing
Created attachment 446254 [details]
Patch for landing
Created attachment 446312 [details]
Patch for landing
Comment on attachment 446101 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=446101&action=review >>> Source/WebCore/rendering/RenderVTTCue.h:53 >>> + RenderVTTCue* overlappingObjectForRect(const IntRect&) const; >> >> both should return `const RenderVTTCue*` > > Disagree for overlappingObjectForRect(); it's not owned by this RenderVTTCue object. I don't think that this should prevent a pointer or reference from being made const. It's not just about ownership IMHO, but also to indicate that this is a read-only object and that only read-only methods can be used with it. Created attachment 446385 [details]
Patch for landing
(In reply to Jean-Yves Avenard [:jya] from comment #10) > Comment on attachment 446101 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=446101&action=review > > >>> Source/WebCore/rendering/RenderVTTCue.h:53 > >>> + RenderVTTCue* overlappingObjectForRect(const IntRect&) const; > >> > >> both should return `const RenderVTTCue*` > > > > Disagree for overlappingObjectForRect(); it's not owned by this RenderVTTCue object. > > I don't think that this should prevent a pointer or reference from being > made const. > > It's not just about ownership IMHO, but also to indicate that this is a > read-only object and that only read-only methods can be used with it. It (the return value) is not a read-only object though. Const isn't _necessarily_ transitive. All const says is that the call will not mutate the state of the underlying object. Policy can be made that const means more than that, but that's policy, not const, and WebKit does not have that policy, AFAIK. Committed r286743 (244990@main): <https://commits.webkit.org/244990@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 446385 [details]. |