[webkit-reviews] review granted: [Bug 122458] Simple line layout : [Attachment 215008] patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Antti Koivisto <koivisto at iki.fi>'s
request for review:
Bug 122458: Simple line layout
https://bugs.webkit.org/show_bug.cgi?id=122458

Attachment 215008: patch
https://bugs.webkit.org/attachment.cgi?id=215008&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
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.