Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(228)

Issue 2377193003: Don't use absolute bounding boxes in LayoutVTTCue (Closed)

Created:
4 years, 2 months ago by fs
Modified:
4 years, 2 months ago
Reviewers:
foolip
CC:
blink-reviews, blink-reviews-layout_chromium.org, chromium-reviews, eae+blinkwatch, jchaffraix+rendering, leviw+renderwatch, pdr+renderingwatchlist_chromium.org, szager+layoutwatch_chromium.org, zoltan1
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Don't use absolute bounding boxes in LayoutVTTCue LayoutVTTCue was using absoluteContentBox()/absoluteBoundingBoxRect() during overlap resolution. This would mean that boxes were computed relative to the containing frame. The former also doesn't take transforms into account, which would mean that the basic overlap check against the title area would fail if a transform was present. Instead compute the various bounding boxes relative to a common ancestor, namely the text track container (which is also the containing block of the cues.) Adjust the controls rect similarly to get it into the same coordinate space. BUG=647253 Committed: https://crrev.com/753ec4b0cfb34e187a485c9412ddeaca1604da3a Cr-Commit-Position: refs/heads/master@{#422072}

Patch Set 1 #

Patch Set 2 : Add test #

Total comments: 11

Patch Set 3 : Extend comments; adjust test #

Total comments: 1
Unified diffs Side-by-side diffs Delta from patch set Stats (+122 lines, -35 lines) Patch
A third_party/WebKit/LayoutTests/media/track/track-cue-rendering-transformed-video.html View 1 2 1 chunk +23 lines, -0 lines 0 comments Download
A third_party/WebKit/LayoutTests/media/track/track-cue-rendering-transformed-video-expected.html View 1 1 chunk +34 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/layout/LayoutVTTCue.h View 2 chunks +2 lines, -2 lines 0 comments Download
M third_party/WebKit/Source/core/layout/LayoutVTTCue.cpp View 1 2 6 chunks +63 lines, -33 lines 1 comment Download

Messages

Total messages: 24 (15 generated)
fs
4 years, 2 months ago (2016-09-29 18:33:46 UTC) #10
foolip
Great comments, lgtm % nits https://codereview.chromium.org/2377193003/diff/20001/third_party/WebKit/LayoutTests/media/track/track-cue-rendering-transformed-video.html File third_party/WebKit/LayoutTests/media/track/track-cue-rendering-transformed-video.html (right): https://codereview.chromium.org/2377193003/diff/20001/third_party/WebKit/LayoutTests/media/track/track-cue-rendering-transformed-video.html#newcode5 third_party/WebKit/LayoutTests/media/track/track-cue-rendering-transformed-video.html:5: transform: translate(1px, 0px); Haha, ...
4 years, 2 months ago (2016-09-29 19:25:19 UTC) #11
fs
https://codereview.chromium.org/2377193003/diff/20001/third_party/WebKit/LayoutTests/media/track/track-cue-rendering-transformed-video.html File third_party/WebKit/LayoutTests/media/track/track-cue-rendering-transformed-video.html (right): https://codereview.chromium.org/2377193003/diff/20001/third_party/WebKit/LayoutTests/media/track/track-cue-rendering-transformed-video.html#newcode15 third_party/WebKit/LayoutTests/media/track/track-cue-rendering-transformed-video.html:15: var video = document.currentScript.parentNode; On 2016/09/29 at 19:25:19, foolip ...
4 years, 2 months ago (2016-09-29 19:40:08 UTC) #12
foolip
https://codereview.chromium.org/2377193003/diff/20001/third_party/WebKit/LayoutTests/media/track/track-cue-rendering-transformed-video.html File third_party/WebKit/LayoutTests/media/track/track-cue-rendering-transformed-video.html (right): https://codereview.chromium.org/2377193003/diff/20001/third_party/WebKit/LayoutTests/media/track/track-cue-rendering-transformed-video.html#newcode15 third_party/WebKit/LayoutTests/media/track/track-cue-rendering-transformed-video.html:15: var video = document.currentScript.parentNode; On 2016/09/29 19:40:08, fs wrote: ...
4 years, 2 months ago (2016-09-29 19:49:34 UTC) #13
fs
https://codereview.chromium.org/2377193003/diff/20001/third_party/WebKit/LayoutTests/media/track/track-cue-rendering-transformed-video.html File third_party/WebKit/LayoutTests/media/track/track-cue-rendering-transformed-video.html (right): https://codereview.chromium.org/2377193003/diff/20001/third_party/WebKit/LayoutTests/media/track/track-cue-rendering-transformed-video.html#newcode15 third_party/WebKit/LayoutTests/media/track/track-cue-rendering-transformed-video.html:15: var video = document.currentScript.parentNode; On 2016/09/29 at 19:49:33, foolip ...
4 years, 2 months ago (2016-09-30 08:05:32 UTC) #16
foolip
https://codereview.chromium.org/2377193003/diff/40001/third_party/WebKit/Source/core/layout/LayoutVTTCue.cpp File third_party/WebKit/Source/core/layout/LayoutVTTCue.cpp (right): https://codereview.chromium.org/2377193003/diff/40001/third_party/WebKit/Source/core/layout/LayoutVTTCue.cpp#newcode124 third_party/WebKit/Source/core/layout/LayoutVTTCue.cpp:124: // We pass UseTransforms here primarily because we use ...
4 years, 2 months ago (2016-09-30 08:12:09 UTC) #17
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2377193003/40001
4 years, 2 months ago (2016-09-30 09:31:42 UTC) #21
commit-bot: I haz the power
Committed patchset #3 (id:40001)
4 years, 2 months ago (2016-09-30 09:36:22 UTC) #22
commit-bot: I haz the power
4 years, 2 months ago (2016-09-30 09:38:25 UTC) #24
Message was sent while issue was closed.
Patchset 3 (id:??) landed as
https://crrev.com/753ec4b0cfb34e187a485c9412ddeaca1604da3a
Cr-Commit-Position: refs/heads/master@{#422072}

Powered by Google App Engine
This is Rietveld 408576698