Refactor RenderVTTCue/SnapToLinesLayouter

Source/core/rendering/RenderVTTCue.cpp

 /*
  * Copyright (C) 2012 Victor Carbune (victor@rosedu.org)
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
  * are met:
  * 1. Redistributions of source code must retain the above copyright
  *    notice, this list of conditions and the following disclaimer.
  * 2. Redistributions in binary form must reproduce the above copyright
  *    notice, this list of conditions and the following disclaimer in the
@@ skipping 41 matching lines @@
         , m_cueWritingDirection(writingDirection)
         , m_linePosition(linePosition)
     {
     }
 
     void layout();
 
 private:
     bool isOutside() const;
     bool isOverlapping() const;
+    LayoutUnit computeInitialPositionAdjustment(LayoutUnit&) const;
+    void initializeFallbackPositioningState();
     bool shouldSwitchDirection(InlineFlowBox*, LayoutUnit) const;
 
-    void moveBoxesByStep(LayoutUnit);
-    bool switchDirection(bool&, LayoutUnit&);
+    void moveBoxesBy(LayoutUnit distance)
+    {
+        if (m_cueWritingDirection == VTTCue::Horizontal)
+            m_cueBox.setY(m_cueBox.location().y() + distance);
+        else
+            m_cueBox.setX(m_cueBox.location().x() + distance);
+    }
 
-    bool findFirstLineBox(InlineFlowBox*&);
-    bool initializeLayoutParameters(InlineFlowBox*, LayoutUnit&, LayoutUnit&);
-    void placeBoxInDefaultPosition(LayoutUnit, bool&);
+    InlineFlowBox* findFirstLineBox() const;
 
     LayoutPoint m_specifiedPosition;
     RenderVTTCue& m_cueBox;
     VTTCue::WritingDirection m_cueWritingDirection;
     float m_linePosition;
 };
 
-bool SnapToLinesLayouter::findFirstLineBox(InlineFlowBox*& firstLineBox)
+InlineFlowBox* SnapToLinesLayouter::findFirstLineBox() const
 {
-    if (m_cueBox.firstChild()->isRenderInline())
-        firstLineBox = toRenderInline(m_cueBox.firstChild())->firstLineBox();
-    else
-        return false;
-
-    return true;
+    ASSERT(m_cueBox.firstChild());

[philipj_slow, 2015/01/28 16:35:58]

This was moved from elsewhere and now it's basically ASSERT(x);x->foo() which is
a bit excessive IMHO. Not sure what the majority style on this matter is and
it's not easy to grep for. You know Soure/core/rendering/ better than I do so
you pick :)

[fs, 2015/01/28 17:03:05]

Yes... maybe RenderVTTCue::layout would be a better place for it... the old
place (just below here) was after the first dereference, so this location the
"last" one that can make any sense (IMHO).

+    if (!m_cueBox.firstChild()->isRenderInline())
+        return nullptr;
+    return toRenderInline(m_cueBox.firstChild())->firstLineBox();
 }
 
-bool SnapToLinesLayouter::initializeLayoutParameters(InlineFlowBox* firstLineBox, LayoutUnit& step, LayoutUnit& position)
+LayoutUnit SnapToLinesLayouter::computeInitialPositionAdjustment(LayoutUnit& step) const
 {
-    ASSERT(m_cueBox.firstChild());
-
-    // 4. Horizontal: Let step be the height of the first line box in boxes.
-    //    Vertical: Let step be the width of the first line box in boxes.
-    step = m_cueWritingDirection == VTTCue::Horizontal ? firstLineBox->size().height() : firstLineBox->size().width();
-
-    // 5. If step is zero, then jump to the step labeled done positioning below.
-    if (!step)
-        return false;
-
     // 6. Let line position be the text track cue computed line position.
     // 7. Round line position to an integer by adding 0.5 and then flooring it.
     LayoutUnit linePosition = floorf(m_linePosition + 0.5f);
 
     // 8. Vertical Growing Left: Add one to line position then negate it.
     if (m_cueWritingDirection == VTTCue::VerticalGrowingLeft)
         linePosition = -(linePosition + 1);
 
     // 9. Let position be the result of multiplying step and line position.
-    position = step * linePosition;
+    LayoutUnit position = step * linePosition;
 
     // 10. Vertical Growing Left: Decrease position by the width of the
     // bounding box of the boxes in boxes, then increase position by step.
     if (m_cueWritingDirection == VTTCue::VerticalGrowingLeft) {
         position -= m_cueBox.size().width();

[philipj_slow, 2015/01/28 16:35:58]

Off-topic: shouldn't this be firstLineBox->size().width()? Or is m_cueBox
somehow shrink-wrapping?

[fs, 2015/01/28 17:03:05]

'height' (i.e. width in this case because it's vertical) is 'auto', so I guess
you could call it shrink-wrapping (extent determined by content).

[philipj_slow, 2015/01/28 17:21:07]

Ah, right.

         position += step;
     }
 
     // 11. If line position is less than zero...
     if (linePosition < 0) {
         RenderBlock* parentBlock = m_cueBox.containingBlock();
 
         // Horizontal / Vertical: ... then increase position by the
         // height / width of the video's rendering area ...
         position += m_cueWritingDirection == VTTCue::Horizontal ? parentBlock->size().height() : parentBlock->size().width();
 
         // ... and negate step.
         step = -step;
     }
-    return true;
+    return position;
 }
 
-void SnapToLinesLayouter::placeBoxInDefaultPosition(LayoutUnit position, bool& switched)
+void SnapToLinesLayouter::initializeFallbackPositioningState()
 {
-    // 12. Move all boxes in boxes ...
-    if (m_cueWritingDirection == VTTCue::Horizontal) {
-        // Horizontal: ... down by the distance given by position
-        m_cueBox.setY(m_cueBox.location().y() + position);
-    } else {
-        // Vertical: ... right by the distance given by position
-        m_cueBox.setX(m_cueBox.location().x() + position);
-    }
-
     // 13. Remember the position of all the boxes in boxes as their specified position.
     m_specifiedPosition = m_cueBox.location();
 
     // 14. Let best position be null. It will hold a position for boxes, much like specified position in the previous step.
     // 15. Let best position score be null.
-
-    // 16. Let switched be false.
-    switched = false;
 }
 
 bool SnapToLinesLayouter::isOutside() const
 {
     return !m_cueBox.containingBlock()->absoluteBoundingBoxRect().contains(m_cueBox.absoluteContentBox());
 }
 
 bool SnapToLinesLayouter::isOverlapping() const
 {
     for (RenderObject* box = m_cueBox.previousSibling(); box; box = box->previousSibling()) {
@@ skipping 26 matching lines @@
     // if step is positive and the right edge of the first line box in boxes is
     // now to the right of the right edge of the title area, jump to the step
     // labeled switch direction.
     LayoutUnit parentWidth = m_cueBox.containingBlock()->size().width();
     if (m_cueWritingDirection != VTTCue::Horizontal && ((step < 0 && left < 0) || (step > 0 && right > parentWidth)))
         return true;
 
     return false;
 }
 
-void SnapToLinesLayouter::moveBoxesByStep(LayoutUnit step)
-{
-    // 22. Horizontal: Move all the boxes in boxes down by the distance
-    // given by step. (If step is negative, then this will actually
-    // result in an upwards movement of the boxes in absolute terms.)
-    if (m_cueWritingDirection == VTTCue::Horizontal)
-        m_cueBox.setY(m_cueBox.location().y() + step);
-
-    // 22. Vertical: Move all the boxes in boxes right by the distance
-    // given by step. (If step is negative, then this will actually
-    // result in a leftwards movement of the boxes in absolute terms.)
-    else
-        m_cueBox.setX(m_cueBox.location().x() + step);
-}
-
-bool SnapToLinesLayouter::switchDirection(bool& switched, LayoutUnit& step)
-{
-    // 24. Switch direction: If switched is true, then move all the boxes in
-    // boxes back to their best position, and jump to the step labeled done
-    // positioning below.
-
-    // 25. Otherwise, move all the boxes in boxes back to their specified
-    // position as determined in the earlier step.
-    m_cueBox.setLocation(m_specifiedPosition);
-
-    // XX. If switched is true, jump to the step labeled done
-    // positioning below.
-    if (switched)
-        return false;
-
-    // 26. Negate step.
-    step = -step;
-
-    // 27. Set switched to true.
-    switched = true;
-    return true;
-}
-
 void SnapToLinesLayouter::layout()
 {
     // http://dev.w3.org/html5/webvtt/#dfn-apply-webvtt-cue-settings
     // Step 13, "If cue's text track cue snap-to-lines flag is set".
 
-    InlineFlowBox* firstLineBox;
-    if (!findFirstLineBox(firstLineBox))
+    InlineFlowBox* firstLineBox = findFirstLineBox();
+    if (!firstLineBox)
         return;
 
-    // Step 1 skipped.
+    // Steps 1-3 skipped.
+    // 4. Horizontal: Let step be the height of the first line box in boxes.
+    //    Vertical: Let step be the width of the first line box in boxes.
+    LayoutUnit step = m_cueWritingDirection == VTTCue::Horizontal ? firstLineBox->size().height() : firstLineBox->size().width();
 
-    LayoutUnit step;
-    LayoutUnit position;
-    if (!initializeLayoutParameters(firstLineBox, step, position))
+    // 5. If step is zero, then jump to the step labeled done positioning below.
+    if (!step)
         return;
 
-    bool switched;
-    placeBoxInDefaultPosition(position, switched);
+    // Steps 6-11.
+    LayoutUnit positionAdjustment = computeInitialPositionAdjustment(step);
+
+    // 12. Move all boxes in boxes ...
+    // Horizontal: ... down by the distance given by position
+    // Vertical: ... right by the distance given by position
+    moveBoxesBy(positionAdjustment);
+
+    // Steps 13-15.
+    initializeFallbackPositioningState();

[philipj_slow, 2015/01/28 16:35:58]

I don't know, this is only called here and contains only one statement, might
just as well inline it?

[fs, 2015/01/28 17:03:05]

This is sort of dependent on how we end up resolving the 'best position' issue.
If 'best position' disappears inlining is absolutely reasonable. (This CL was
originally written as being on top of the 'best position' CL - if that explains
anything...)

[philipj_slow, 2015/01/28 17:21:07]

I see. Let's move it around some more in that CL :)

+
+    // 16. Let switched be false.
+    bool switched = false;
 
     // Step 17 skipped. (margin == 0; title area == video area)
 
     // 18. Step loop: If none of the boxes in boxes would overlap any of the
     // boxes in output, and all of the boxes in output are entirely within the
     // title area box, then jump to the step labeled done positioning below.
     while (isOutside() || isOverlapping()) {
         // 19. Let current position score be the percentage of the area of the
         // bounding box of the boxes in boxes that is outside the title area
         // box.
         // 20. If best position is null (i.e. this is the first run through
         // this loop, switched is still false, the boxes in boxes are at their
         // specified position, and best position score is still null), or if
         // current position score is a lower percentage than that in best
         // position score, then remember the position of all the boxes in boxes
         // as their best position, and set best position score to current
         // position score.
         if (!shouldSwitchDirection(firstLineBox, step)) {
-            // 22. Move all the boxes in boxes ...
-            moveBoxesByStep(step);
+            // 22. Horizontal: Move all the boxes in boxes down by the distance
+            // given by step. (If step is negative, then this will actually
+            // result in an upwards movement of the boxes in absolute terms.)
+            // Vertical: Move all the boxes in boxes right by the distance
+            // given by step. (If step is negative, then this will actually
+            // result in a leftwards movement of the boxes in absolute terms.)
+            moveBoxesBy(step);
+
             // 23. Jump back to the step labeled step loop.
-        } else if (!switchDirection(switched, step)) {
+            continue;
+        }
+
+        // 24. Switch direction: If switched is true, then move all the boxes in
+        // boxes back to their best position, and jump to the step labeled done
+        // positioning below.
+
+        // 25. Otherwise, move all the boxes in boxes back to their specified
+        // position as determined in the earlier step.
+        m_cueBox.setLocation(m_specifiedPosition);
+
+        // XX. If switched is true, jump to the step labeled done
+        // positioning below.
+        if (switched)
             break;
-        }
+
+        // 26. Negate step.
+        step = -step;
+
+        // 27. Set switched to true.
+        switched = true;
 
         // 28. Jump back to the step labeled step loop.
     }
 }
 
 void RenderVTTCue::repositionCueSnapToLinesNotSet()
 {
     // FIXME: Implement overlapping detection when snap-to-lines is not set. http://wkb.ug/84296
 
     // http://dev.w3.org/html5/webvtt/#dfn-apply-webvtt-cue-settings
@@ skipping 78 matching lines @@
         SnapToLinesLayouter(*this, m_cue->getWritingDirection(), m_cue->calculateComputedLinePosition()).layout();
 
         adjustForTopAndBottomMarginBorderAndPadding();
     } else {
         repositionCueSnapToLinesNotSet();
     }
 }
 
 } // namespace blink