Don't use absolute bounding boxes in LayoutVTTCue

third_party/WebKit/Source/core/layout/LayoutVTTCue.cpp

Index: third_party/WebKit/Source/core/layout/LayoutVTTCue.cpp
diff --git a/third_party/WebKit/Source/core/layout/LayoutVTTCue.cpp b/third_party/WebKit/Source/core/layout/LayoutVTTCue.cpp
index 2374b983fea50906bd4975e33deb0d8cb3c098a9..36dfef80c77b7a490a231532c85c936ee276c325 100644
--- a/third_party/WebKit/Source/core/layout/LayoutVTTCue.cpp
+++ b/third_party/WebKit/Source/core/layout/LayoutVTTCue.cpp
@@ -33,11 +33,7 @@
 
 namespace blink {
 
-LayoutVTTCue::LayoutVTTCue(ContainerNode* node, float snapToLinesPosition)
-    : LayoutBlockFlow(node)
-    , m_snapToLinesPosition(snapToLinesPosition)
-{
-}
+namespace {
 
 class SnapToLinesLayouter {
     STACK_ALLOCATED();
@@ -82,7 +78,7 @@ InlineFlowBox* SnapToLinesLayouter::findFirstLineBox() const
 LayoutUnit SnapToLinesLayouter::computeInitialPositionAdjustment(LayoutUnit& step, LayoutUnit maxDimension,
     LayoutUnit margin) const
 {
-    ASSERT(std::isfinite(m_cueBox.snapToLinesPosition()));
+    DCHECK(std::isfinite(m_cueBox.snapToLinesPosition()));
 
     // 6. Let line be cue's computed line.
     // 7. Round line to an integer by adding 0.5 and then flooring it.
@@ -117,25 +113,32 @@ LayoutUnit SnapToLinesLayouter::computeInitialPositionAdjustment(LayoutUnit& ste
     return position;
 }
 
+IntRect contentBoxRelativeToAncestor(
+    const LayoutBox& box, const LayoutBoxModelObject& ancestor)
+{
+    FloatRect cueContentBox(box.contentBoxRect());
+    FloatQuad mappedContentQuad = box.localToAncestorQuad(cueContentBox, &ancestor, UseTransforms);

[foolip, 2016/09/29 19:25:19]

Here or somewhere in this file it might be worth calling out why it's important
that a local coordinate system is used. The test will fail if someone changes
it, but without context I couldn't guess if "UseTransforms" here meant that
transforms are applied like with getBoundingClientRect or ignored like with
clientLeft and friends.

[fs, 2016/09/29 19:40:08]

The reason we need UseTransforms here is because we use transforms (ha!) to
layout non-line-snapped cues. Will add a comment.

[fs, 2016/09/30 08:05:32]

Added.

+    return mappedContentQuad.enclosingBoundingBox();
+}
+
+IntRect cueBoundingBox(const LayoutBox& cueBox)
+{
+    return contentBoxRelativeToAncestor(cueBox, *cueBox.containingBlock());
+}
+
 bool SnapToLinesLayouter::isOutside(const IntRect& titleArea) const
 {
-    return !titleArea.contains(m_cueBox.absoluteContentBox());
+    return !titleArea.contains(cueBoundingBox(m_cueBox));
 }
 
 bool SnapToLinesLayouter::isOverlapping() const
 {
-    IntRect cueBoxRect = m_cueBox.absoluteBoundingBoxRect();
-    for (LayoutObject* box = m_cueBox.previousSibling(); box; box = box->previousSibling()) {
-        IntRect boxRect = box->absoluteBoundingBoxRect();
-
-        if (cueBoxRect.intersects(boxRect))
+    IntRect cueBoxRect = cueBoundingBox(m_cueBox);
+    for (LayoutBox* box = m_cueBox.previousSiblingBox(); box; box = box->previousSiblingBox()) {
+        if (cueBoxRect.intersects(cueBoundingBox(*box)))
             return true;
     }
-
-    if (cueBoxRect.intersects(m_controlsRect))
-        return true;
-
-    return false;
+    return cueBoxRect.intersects(m_controlsRect);
 }
 
 bool SnapToLinesLayouter::shouldSwitchDirection(InlineFlowBox* firstLineBox, LayoutUnit step, LayoutUnit margin) const
@@ -211,7 +214,7 @@ void SnapToLinesLayouter::layout()
     // Vertical: Let title area be a box that covers all of the video’s
     // rendering area except for a width of margin at the left of the rendering
     // area and a width of margin at the right of the rendering area.
-    IntRect titleArea = m_cueBox.containingBlock()->absoluteBoundingBoxRect();
+    IntRect titleArea = enclosingIntRect(m_cueBox.containingBlock()->contentBoxRect());
     if (blink::isHorizontalWritingMode(writingMode)) {
         titleArea.move(0, margin.toInt());
         titleArea.contract(0, (2 * margin).toInt());
@@ -264,6 +267,14 @@ void SnapToLinesLayouter::layout()
     }
 }
 
+} // unnamed namespace
+
+LayoutVTTCue::LayoutVTTCue(ContainerNode* node, float snapToLinesPosition)
+    : LayoutBlockFlow(node)
+    , m_snapToLinesPosition(snapToLinesPosition)
+{
+}
+
 void LayoutVTTCue::repositionCueSnapToLinesNotSet()
 {
     // FIXME: Implement overlapping detection when snap-to-lines is not set. http://wkb.ug/84296
@@ -308,30 +319,41 @@ void LayoutVTTCue::repositionCueSnapToLinesNotSet()
     // boxes will unfortunately overlap.)
 }
 
+IntRect LayoutVTTCue::computeControlsRect() const
+{
+    // Determine the area covered by the media controls, if any. If the controls
+    // are present, they are the next sibling of the text track container, which
+    // is our parent. (LayoutMedia ensures that the media controls are laid out
+    // before text tracks, so that the layout is up to date here.)
+    DCHECK(parent()->node()->isTextTrackContainer());
+    LayoutObject* controlsContainer = parent()->nextSibling();
+    if (!controlsContainer)
+        return IntRect();
+    // Only a part of the media controls is used for overlap avoidance.
+    MediaControls* controls = toMediaControls(controlsContainer->node());
+    LayoutObject* controlsLayout = controls->layoutObjectForTextTrackLayout();
+    // The (second part of) following is mostly defensive - in general there
+    // should be a LayoutBox representing the part of the controls that are

[foolip, 2016/09/29 19:25:19]

And I guess it can actually happen, like if you play with
video::-webkit-media-text-track-*

[fs, 2016/09/29 19:40:08]

Right, "unlikely, but not impossible" (=> mostly). IIRC this is the layout box
corresponding to -webkit-media-controls-panel ATM.

[fs, 2016/09/30 08:05:32]

Added additional note mentioning the potential pseudo element issue.

+    // relevant for overlap avoidance.
+    if (!controlsLayout || !controlsLayout->isBox())
+        return IntRect();
+    // Assume that the controls container are positioned in the same relative
+    // position as the text track container. (LayoutMedia::layout ensures this.)
+    return contentBoxRelativeToAncestor(
+        toLayoutBox(*controlsLayout), toLayoutBox(*controlsContainer));
+}
+
 void LayoutVTTCue::layout()
 {
     LayoutBlockFlow::layout();
 
-    ASSERT(firstChild());
+    DCHECK(firstChild());
 
     LayoutState state(*this, locationOffset());
 
-    // Determine the area covered by the media controls, if any. If the controls
-    // are present, they are the next sibling of the text track container, which
-    // is our parent. (LayoutMedia ensures that the media controls are laid out
-    // before text tracks, so that the layout is up to date here.)
-    ASSERT(parent()->node()->isTextTrackContainer());
-    IntRect controlsRect;
-    if (LayoutObject* parentSibling = parent()->nextSibling()) {
-        // Only a part of the media controls is used for overlap avoidance.
-        MediaControls* controls = toMediaControls(parentSibling->node());
-        if (LayoutObject* controlsLayout = controls->layoutObjectForTextTrackLayout())
-            controlsRect = controlsLayout->absoluteBoundingBoxRect();
-    }
-
     // http://dev.w3.org/html5/webvtt/#dfn-apply-webvtt-cue-settings - step 13.
     if (!std::isnan(m_snapToLinesPosition))
-        SnapToLinesLayouter(*this, controlsRect).layout();
+        SnapToLinesLayouter(*this, computeControlsRect()).layout();
     else
         repositionCueSnapToLinesNotSet();
 }