[Webkit-unassigned] [Bug 122458] Simple line layout

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=122458


Darin Adler <darin at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #215008|review?                     |review+
               Flag|                            |




--- Comment #52 from Darin Adler <darin at apple.com>  2013-10-23 23:40:25 PST ---
(From update of attachment 215008)
View in context: https://bugs.webkit.org/attachment.cgi?id=215008&action=review

Looks good.

> Source/WebCore/dom/Position.cpp:648
> +            auto textRenderer = toRenderText(renderer);

I would have written:

    auto& textRenderer = toRenderText(*renderer);

> Source/WebCore/rendering/RenderBlockFlow.cpp:3006
> +void RenderBlockFlow::layoutSimpleLines(LayoutUnit& repaintLogicalTop, LayoutUnit& repaintLogicalBottom)

The verb lay out is two separate words, so it would be best if this function had a capital O in its name.

> Source/WebCore/rendering/RenderBlockFlow.cpp:3028
> +void RenderBlockFlow::ensureLineBoxes()

I’m not a big fan of this use of the word ensure. Would it really be so bad to call this createLineBoxes?

> Source/WebCore/rendering/RenderText.cpp:282
> +    // FIXME: These will go away when simple layout can do everything.
> +    const_cast<RenderText&>(*this).ensureLineBoxes();

It’s a little unclear what “these” means here. I would prefer a comment at each call site, saying something more like “Remove this once simple layout can handle it.”

> Source/WebCore/rendering/RenderText.cpp:518
> +    if (preferredLogicalWidthsDirty())
> +        const_cast<RenderText*>(this)->computePreferredLogicalWidths(0);
> +
> +    return m_knownToHaveNoOverflowAndNoFallbackFonts;

Lame how non-obvious that we have to call computePreferredLogicalWidths for its side effect of setting m_knownToHaveNoOverflowAndNoFallbackFonts.

> Source/WebCore/rendering/RenderText.cpp:891
> +    if (simpleLines())
> +        m_linesDirty = true;
> +    else
> +        m_linesDirty = m_lineBoxes.dirtyRange(*this, offset, end, delta);

Could write this with || instead. I think it would read pretty well that way.

> Source/WebCore/rendering/SimpleLineLayout.cpp:139
> +        // This rejects anything with more than one concecutive whitespace, except at the beginning or end.

Typo: concecutive

> Source/WebCore/rendering/SimpleLineLayout.cpp:142
> +        if (character == ' ' || character == '\n' || character == '\t')

Could use the isWhitespace function from below if you defined it earlier.

> Source/WebCore/rendering/SimpleLineLayout.cpp:156
> +        // All RTL characters are above this.
> +        static const UChar hebrewRangeStart = 0x590;

Might call this firstRTLCharacter or lowestRTLCharacter instead of hebrewRangeStart, then you could omit the comment.

> Source/WebCore/rendering/SimpleLineLayout.cpp:175
> +    while (offset < length) {

Would read nicer as a for loop rather than a while loop:

    for (; offset < length; ++offset)

> Source/WebCore/rendering/SimpleLineLayout.h:46
> +typedef Vector<Line> Lines;

If we never resize these vectors after creating them, then we might want to consider using std::unique_ptr<Lines[]> instead to save some overhead, but I guess we’d need to store the length somewhere.

> Source/WebCore/rendering/SimpleLineLayoutFunctions.cpp:55
> +    String string = textRenderer.text();

Looks like this isn’t used.

> Source/WebCore/rendering/SimpleLineLayoutFunctions.cpp:76
> +//    context.setStrokeColor(Color(0, 255, 0), ColorSpaceDeviceRGB);
> +//    context.setStrokeThickness(4.0f);
> +//    context.setFillColor(Color::transparent, ColorSpaceDeviceRGB);
> +//    auto box = computeBoundingBox(layout, textRenderer);
> +//    box.moveBy(flooredIntPoint(paintOffset));
> +//    context.drawRect(box);

Oops, left this commented-out code in here.

> Source/WebCore/rendering/SimpleLineLayoutFunctions.h:100
> +    for (unsigned lineIndex = 0; lineIndex < lines.size(); ++lineIndex) {

I’d suggest just using “i” here instead of lineIndex.

> Source/WebCore/rendering/SimpleLineLayoutFunctions.h:112
> +    for (unsigned lineIndex = 0; lineIndex < lines.size(); ++lineIndex) {

I’d suggest just using “i” here instead of lineIndex.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.