Bug 130552 – Document assumptions made in TrailingObjects::updateMidpointsForTrailingBoxes

• Component: New Bugs
• Status: RESOLVED DUPLICATE

[Myles C. Maxfield]

Document assumptions made in TrailingObjects::updateMidpointsForTrailingBoxes

[Myles C. Maxfield]

Created attachment 227366 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 227366 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=227366&action=review

> Source/WebCore/rendering/line/TrailingObjects.cpp:56
> +            // There is something subtle going on here. This function gets called in two cases:
> +            // 1) In BreakingContext::handleEndOfLine(). The offset below can be 0
> +            // in this case (meaning that the code below sets the offset to 4B), but that is okay
> +            // because BreakingContext::handleEndOfLine() immediately fixes up the offset by calling
> +            // increment() on the iterator, which detects offsets that are beyond the end of its node
> +            // and moves the iterator to the next node.
> +            // 2) When BreakingContext::handleText() detects multiple whitespace characters in a row. In
> +            // that case, the only way that the offset can be 0 is if the first two characters are
> +            // whitespace. However, LineBreaker::nextSegmentBreak() calls
> +            // LineBreaker::skipLeadingWhitespace() to skip over such whitespace before calling
> +            // BreakingContext::handleText().
> +            // Therefore, this -1 wrapping is benign (albeit brittle).

I think it would be better to make the code more self-documenting (and safe) than having to write a comment like this.

> Source/WebCore/rendering/line/TrailingObjects.cpp:57
>              lineMidpointState.midpoints()[trailingSpaceMidpoint].setOffset(lineMidpointState.midpoints()[trailingSpaceMidpoint].offset() -1);

You should add a space before the 1

[Myles C. Maxfield]

Comment on attachment 227366 [details]
Patch

Rather than documenting bad design, I should fix the bad design.

[Myles C. Maxfield]

*** This bug has been marked as a duplicate of bug 130540 ***