Bug 122458 – Simple line layout

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Antti Koivisto]

Line box based inline layout is powerful but also rather slow and memory intensive. We might be able to cover many common cases with simpler data structures and algorithms.

[Antti Koivisto]

Created attachment 213609 [details]
some work in progress

No hit testing or selections yet. Does pretty well in static CSS tests.

[EFL EWS Bot]

Comment on attachment 213609 [details]
some work in progress

Attachment 213609 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/3354124

[EFL EWS Bot]

Comment on attachment 213609 [details]
some work in progress

Attachment 213609 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/3733002

[Build Bot]

Comment on attachment 213609 [details]
some work in progress

Attachment 213609 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/3717007

[Build Bot]

Comment on attachment 213609 [details]
some work in progress

Attachment 213609 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/3599003

New failing tests:
editing/execCommand/indent-div-inside-list.html
fast/block/float/float-not-removed-from-first-letter.html
fast/lists/dynamic-marker-crash.html
editing/pasteboard/merge-end-list-2.html
tables/mozilla/bugs/bug2479-3.html
fast/block/margin-collapse/self-collapsing-block-with-float-children.html
accessibility/table-cells.html
editing/execCommand/justify.html
editing/deleting/delete-listitem-001.html
editing/execCommand/query-text-alignment.html
editing/execCommand/indent-block-in-list.html
fast/table/022.html
platform/mac/accessibility/list-items-ignored.html
fast/dom/inner-text-001.html
fast/css/getComputedStyle/getComputedStyle-zoom-and-background-size.html
accessibility/table-attributes.html
scrollbars/rtl/div-horizontal.html
fast/css/list-item-pseudo-nocrash.html
tables/mozilla_expected_failures/bugs/bug1725.html
fast/selectors/style-sharing-last-child.html
tables/mozilla/marvin/x_th_align_justify.xml
accessibility/box-styled-lists.html
editing/style/5046875-2.html
platform/mac/accessibility/table-multi-bodies.html
editing/execCommand/break-out-of-empty-list-item.html

[Build Bot]

Created attachment 213614 [details]
Archive of layout-test-results from webkit-ews-10 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-10  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 213609 [details]
some work in progress

Attachment 213609 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/3731009

New failing tests:
editing/execCommand/indent-div-inside-list.html
fast/block/float/float-not-removed-from-first-letter.html
fast/lists/dynamic-marker-crash.html
editing/pasteboard/merge-end-list-2.html
tables/mozilla/bugs/bug2479-3.html
fast/block/margin-collapse/self-collapsing-block-with-float-children.html
accessibility/table-cells.html
editing/execCommand/justify.html
editing/deleting/delete-listitem-001.html
editing/execCommand/query-text-alignment.html
editing/execCommand/indent-block-in-list.html
fast/table/022.html
platform/mac/accessibility/list-items-ignored.html
fast/dom/inner-text-001.html
fast/css/getComputedStyle/getComputedStyle-zoom-and-background-size.html
accessibility/table-attributes.html
scrollbars/rtl/div-horizontal.html
fast/css/list-item-pseudo-nocrash.html
tables/mozilla_expected_failures/bugs/bug1725.html
fast/selectors/style-sharing-last-child.html
tables/mozilla/marvin/x_th_align_justify.xml
accessibility/box-styled-lists.html
editing/style/5046875-2.html
platform/mac/accessibility/table-multi-bodies.html
editing/execCommand/break-out-of-empty-list-item.html

[Build Bot]

Created attachment 213616 [details]
Archive of layout-test-results from webkit-ews-07 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-07  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[kov's GTK+ EWS bot]

Comment on attachment 213609 [details]
some work in progress

Attachment 213609 [details] did not pass gtk-ews (gtk):
Output: http://webkit-queues.appspot.com/results/3645027

[Antti Koivisto]

Created attachment 214564 [details]
for bots

[EFL EWS Bot]

Comment on attachment 214564 [details]
for bots

Attachment 214564 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/5378030

[Build Bot]

Comment on attachment 214564 [details]
for bots

Attachment 214564 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/5378034

[Build Bot]

Comment on attachment 214564 [details]
for bots

Attachment 214564 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/5438030

New failing tests:
fast/regions/shape-inside/shape-inside-on-second-region-block-content.html
accessibility/table-with-rules.html
fast/text/zero-font-size.html
accessibility/onclick-handlers.html
accessibility/div-within-anchors-causes-crash.html
accessibility/deleting-iframe-destroys-axcache.html
accessibility/table-cell-spans.html
fast/regions/selection/selection-direction.html
platform/mac/accessibility/id-class-attributes.html
accessibility/table-sections.html
accessibility/table-detection.html
platform/mac/accessibility/heading-clickpoint.html
platform/mac/accessibility/aria-expanded-standard-items.html
platform/mac/accessibility/element-for-text-marker.html
accessibility/lists.html
accessibility/table-attributes.html
editing/selection/anchor-focus3.html
accessibility/table-with-aria-role.html
platform/mac/accessibility/element-level.html
accessibility/aria-hidden-updates-alldescendants.html
platform/mac/accessibility/div-containing-div-with-aria.html
fast/table/click-near-anonymous-table.html
platform/mac/accessibility/footer-roledescription.html
platform/mac/accessibility/css-speech-speak.html
platform/mac/accessibility/html-section-elements.html
accessibility/fieldset-element.html
accessibility/adjacent-continuations-cause-assertion-failure.html
accessibility/language-attribute.html
compositing/background-color/background-color-change-to-text.html
platform/mac/accessibility/definition-list-term.html

[Build Bot]

Created attachment 214566 [details]
Archive of layout-test-results from webkit-ews-11 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-11  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[kov's GTK+ EWS bot]

Comment on attachment 214564 [details]
for bots

Attachment 214564 [details] did not pass gtk-ews (gtk):
Output: http://webkit-queues.appspot.com/results/4798052

[Build Bot]

Comment on attachment 214564 [details]
for bots

Attachment 214564 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5218010

New failing tests:
fast/regions/shape-inside/shape-inside-on-second-region-block-content.html
accessibility/table-with-rules.html
fast/text/zero-font-size.html
accessibility/onclick-handlers.html
accessibility/div-within-anchors-causes-crash.html
accessibility/deleting-iframe-destroys-axcache.html
accessibility/table-cell-spans.html
fast/regions/selection/selection-direction.html
platform/mac/accessibility/id-class-attributes.html
accessibility/table-sections.html
accessibility/table-detection.html
platform/mac/accessibility/heading-clickpoint.html
platform/mac/accessibility/aria-expanded-standard-items.html
platform/mac/accessibility/element-for-text-marker.html
accessibility/lists.html
accessibility/table-attributes.html
editing/selection/anchor-focus3.html
accessibility/table-with-aria-role.html
platform/mac/accessibility/element-level.html
accessibility/aria-hidden-updates-alldescendants.html
platform/mac/accessibility/div-containing-div-with-aria.html
fast/table/click-near-anonymous-table.html
platform/mac/accessibility/footer-roledescription.html
platform/mac/accessibility/css-speech-speak.html
platform/mac/accessibility/html-section-elements.html
accessibility/fieldset-element.html
accessibility/adjacent-continuations-cause-assertion-failure.html
accessibility/language-attribute.html
compositing/background-color/background-color-change-to-text.html
platform/mac/accessibility/definition-list-term.html

[Build Bot]

Created attachment 214568 [details]
Archive of layout-test-results from webkit-ews-07 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-07  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Antti Koivisto]

Created attachment 214574 [details]
another

[EFL EWS Bot]

Comment on attachment 214574 [details]
another

Attachment 214574 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/5388043

[kov's GTK+ EWS bot]

Comment on attachment 214574 [details]
another

Attachment 214574 [details] did not pass gtk-ews (gtk):
Output: http://webkit-queues.appspot.com/results/5458043

[Build Bot]

Comment on attachment 214574 [details]
another

Attachment 214574 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/5368044

[kov's GTK+ EWS bot]

Comment on attachment 214574 [details]
another

Attachment 214574 [details] did not pass gtk-wk2-ews (gtk-wk2):
Output: http://webkit-queues.appspot.com/results/5388052

[Build Bot]

Comment on attachment 214574 [details]
another

Attachment 214574 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/5478047

New failing tests:
fast/regions/shape-inside/shape-inside-on-second-region-block-content.html
fast/table/click-near-anonymous-table.html
editing/selection/anchor-focus3.html
fast/text/zero-font-size.html
accessibility/deleting-iframe-destroys-axcache.html
fast/regions/selection/selection-direction.html
compositing/background-color/background-color-change-to-text.html

[Build Bot]

Created attachment 214577 [details]
Archive of layout-test-results from webkit-ews-15 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-15  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Simon Fraser (smfr)]

Comment on attachment 214574 [details]
another

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

> Source/WebCore/dom/Position.cpp:650
> +            textRenderer->ensureInlineBoxes();

It's unclear how callers can know that they should call ensureInlineBoxes()

> Source/WebCore/editing/TextIterator.cpp:524
> +    if (renderer->simpleLineLayout()) {

Might be easier to read as usesSimpleLineLayout() or hasSimpleLineLayout(). It's a bit odd for an object to be called SimpleLineLayout.

> Source/WebCore/rendering/RenderBlock.h:793
>      unsigned m_hasMarginBeforeQuirk : 1; // Note these quirk values can't be put in RenderBlockRareData since they are set too frequently.
>      unsigned m_hasMarginAfterQuirk : 1;
>      unsigned m_beingDestroyed : 1;
>      unsigned m_hasMarkupTruncation : 1;
>      unsigned m_hasBorderOrPaddingLogicalWidthChanged : 1;
> +    unsigned m_forceLineboxInlineLayout : 1;

These can be bools, I think.

> Source/WebCore/rendering/RenderBlockFlow.cpp:2448
> +    m_forceLineboxInlineLayout = true;
> +
> +    if (!m_simpleLineLayout)
> +        return;
> +    m_simpleLineLayout = nullptr;

It's odd for something called ensureFoo to blow away existing state and recompute it. This function should be called updateFoo or recomputeFoo or something.

> Source/WebCore/rendering/RenderText.cpp:283
> +    // FIXME: These will go away when simple layout can do everything.

But then it will no longer be simple?

> Source/WebCore/rendering/RenderText.cpp:308
> +    const_cast<RenderText&>(*this).ensureInlineBoxes();

Again, not sure how I'd know, when writing new code, that I had to call ensureInlineBoxes().

> Source/WebCore/rendering/RenderText.cpp:1200
> +    if (auto layout = simpleLineLayout())
> +        return layout->caretMinOffset();
>      return m_lineBoxes.caretMinOffset();

Shame we can't turn m_lineBoxes into some virtual thing that does both.

> Source/WebCore/rendering/ResolvedSimpleLines.h:38
> +class ResolvedSimpleLines {

The "resolved" doesn't really communicate anything I think.

> Source/WebCore/rendering/SimpleLineLayout.cpp:298
> +    if (paintInfo.phase != PaintPhaseForeground)
> +        return;

You can probably bail if painting is disabled (updateControlTints).

[Build Bot]

Comment on attachment 214574 [details]
another

Attachment 214574 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5438047

New failing tests:
fast/regions/shape-inside/shape-inside-on-second-region-block-content.html
fast/table/click-near-anonymous-table.html
editing/selection/anchor-focus3.html
fast/text/zero-font-size.html
accessibility/deleting-iframe-destroys-axcache.html
fast/regions/selection/selection-direction.html
compositing/background-color/background-color-change-to-text.html

[Build Bot]

Created attachment 214580 [details]
Archive of layout-test-results from webkit-ews-08 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-08  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 214574 [details]
another

Attachment 214574 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5348013

New failing tests:
fast/regions/shape-inside/shape-inside-on-second-region-block-content.html
fast/table/click-near-anonymous-table.html
editing/selection/anchor-focus3.html
fast/text/zero-font-size.html
accessibility/deleting-iframe-destroys-axcache.html
fast/regions/selection/selection-direction.html
compositing/background-color/background-color-change-to-text.html

[Build Bot]

Created attachment 214585 [details]
Archive of layout-test-results from webkit-ews-06 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-06  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Antti Koivisto]

> > Source/WebCore/dom/Position.cpp:650
> > +            textRenderer->ensureInlineBoxes();
> 
> It's unclear how callers can know that they should call ensureInlineBoxes()

They won't eventually. It just signals that I haven't fixed the code to work with simple layout yet.

> > Source/WebCore/rendering/RenderText.cpp:283
> > +    // FIXME: These will go away when simple layout can do everything.
> 
> But then it will no longer be simple?

It will still be simple. "Simple" refers to the fact that it does not support full rendering model. For the parts that it does support it should do everything. Currently the missing piece is selection painting and some editing details.

> Shame we can't turn m_lineBoxes into some virtual thing that does both.

Are you planning to add another line layout mode?

> > Source/WebCore/rendering/ResolvedSimpleLines.h:38
> > +class ResolvedSimpleLines {
> 
> The "resolved" doesn't really communicate anything I think.

It communicates that these are resolved lines. Data structures (SimpleLineLayout::Line) are kept unresolved to save space. Line rects and baselines are resolved on demand.

[Antti Koivisto]

> It communicates that these are resolved lines. Data structures (SimpleLineLayout::Line) are kept unresolved to save space. Line rects and baselines are resolved on demand.

Whether it is worth communicating is another matter.

[Andreas Kling]

Comment on attachment 214574 [details]
another

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

> Source/WebCore/editing/TextIterator.cpp:531
> +        while (runEnd < str.length() && (isCollapsibleWhitespace(str[runEnd]) || str[runEnd] == '\t'))
> +            ++runEnd;

You might be able to tighten this loop by putting the StringImpl in a local, since both length() and operator[] will do null checks.

> Source/WebCore/editing/TextIterator.cpp:547
> +        while (runEnd < str.length() && !isCollapsibleWhitespace(str[runEnd]))
> +            ++runEnd;

This loop would benefit as well.

> Source/WebCore/rendering/RenderBlock.cpp:3519
> +        if (!child->hasSelfPaintingLayer() && !child->isFloating() && child->nodeAtPoint(request, result, locationInContainer, childPoint, childHitTest))

I would do the !isFloating() check first, since that's just a bit test.

> Source/WebCore/rendering/RenderFullScreen.cpp:38
> -using namespace WebCore;
> +namespace WebCore {

wat. We should fix this separately :)

> Source/WebCore/rendering/RenderTreeAsText.cpp:253
> +            if (text.containingBlock()->isTableCell())
> +                y -= toRenderTableCell(o.containingBlock())->intrinsicPaddingBefore();

Should put containingBlock() in a local since it's nontrivial.

> Source/WebCore/rendering/RenderTreeAsText.cpp:545
> +    if (o.containingBlock()->isTableCell())
> +        y -= toRenderTableCell(o.containingBlock())->intrinsicPaddingBefore();

Ditto.

> Source/WebCore/rendering/RenderTreeAsText.cpp:598
> +            for (InlineTextBox* box = text.firstTextBox(); box; box = box->nextTextBox()) {

auto?

> Source/WebCore/rendering/RenderTreeAsText.cpp:604
> +        const SimpleLineLayout* simpleLineLayout = text.parent()->isRenderBlockFlow() ? toRenderBlockFlow(text.parent())->simpleLineLayout() : nullptr;
> +        if (simpleLineLayout) {

if (auto simpleLineLayout = ...)?

> Source/WebCore/rendering/ResolvedSimpleLines.h:131
> +    , m_string(toRenderText(*flow.firstChild()).text())

Does ResolvedSimpleLines really need to hold a strong ref on the text?

> Source/WebCore/rendering/SimpleLineLayout.cpp:270
> +        Line line;
> +        line.textOffset = lineStartOffset;
> +        line.textLength = lineEndOffset - lineStartOffset;
> +        line.width = lineWidth.committedWidth();
> +
> +        m_lines.append(line);

You could C++11'ize this:

m_lines.append({lineStartOffset, lineEndOffset - lineStartOffset, lineWidth.committedWidth()});

If m_lines is only going to be built once, the member should be something with less overhead than Vector.
You could even go all 2012 and make it a variable-sized object :)

[EFL EWS Bot]

Comment on attachment 214574 [details]
another

Attachment 214574 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/7758010

[Antti Koivisto]

Created attachment 214949 [details]
another

[Antti Koivisto]

Created attachment 214952 [details]
and another

[Build Bot]

Comment on attachment 214952 [details]
and another

Attachment 214952 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/9688008

[EFL EWS Bot]

Comment on attachment 214952 [details]
and another

Attachment 214952 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/9678011

[Build Bot]

Comment on attachment 214952 [details]
and another

Attachment 214952 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/9678010

New failing tests:
compositing/regions/floated-region-with-transformed-child.html
compositing/regions/propagate-region-box-shadow-border-padding-for-video.html

[Build Bot]

Created attachment 214957 [details]
Archive of layout-test-results from webkit-ews-12 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-12  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 214952 [details]
and another

Attachment 214952 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/9658015

New failing tests:
compositing/regions/floated-region-with-transformed-child.html
compositing/regions/propagate-region-box-shadow-border-padding-for-video.html

[Build Bot]

Created attachment 214960 [details]
Archive of layout-test-results from webkit-ews-05 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-05  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[EFL EWS Bot]

Comment on attachment 214952 [details]
and another

Attachment 214952 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/9878014

[kov's GTK+ EWS bot]

Comment on attachment 214952 [details]
and another

Attachment 214952 [details] did not pass gtk-ews (gtk):
Output: http://webkit-queues.appspot.com/results/6078066

[Build Bot]

Comment on attachment 214952 [details]
and another

Attachment 214952 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/9918009

New failing tests:
compositing/regions/floated-region-with-transformed-child.html
compositing/regions/propagate-region-box-shadow-border-padding-for-video.html

[Build Bot]

Created attachment 214967 [details]
Archive of layout-test-results from webkit-ews-02 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-02  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Antti Koivisto]

Created attachment 214975 [details]
yet another

[Build Bot]

Comment on attachment 214975 [details]
yet another

Attachment 214975 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/10148009

[EFL EWS Bot]

Comment on attachment 214975 [details]
yet another

Attachment 214975 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/10068018

[EFL EWS Bot]

Comment on attachment 214975 [details]
yet another

Attachment 214975 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/10298030

[Antti Koivisto]

Created attachment 215008 [details]
patch

[Build Bot]

Comment on attachment 215008 [details]
patch

Attachment 215008 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/10528013

[Darin Adler]

Comment on attachment 215008 [details]
patch

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.

[Antti Koivisto]

https://trac.webkit.org/r157950

[Antti Koivisto]

(In reply to comment #52)
> The verb lay out is two separate words, so it would be best if this function had a capital O in its name.

It is a verb in WebKit!

[Roger Fong]

Caused many failures on Windows:http://build.webkit.org/builders/Apple%20Win%207%20Debug%20%28Tests%29/builds/55613/steps/layout-test/logs/stdio

for which I filed a bug: https://bugs.webkit.org/show_bug.cgi?id=123309

[Csaba Osztrogonác]

(In reply to comment #55)
> Caused many failures on Windows:http://build.webkit.org/builders/Apple%20Win%207%20Debug%20%28Tests%29/builds/55613/steps/layout-test/logs/stdio
> 
> for which I filed a bug: https://bugs.webkit.org/show_bug.cgi?id=123309

It broke everything on GTK and EFL too, it isn't Windows specific bug at all.