Bug 122458

Summary: Simple line layout
Product: WebKit Reporter: Antti Koivisto <koivisto>
Component: Layout and RenderingAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: buildbot, commit-queue, darin, eflews.bot, esprehn+autocc, glenn, gtk-ews, gyuyoung.kim, hyatt, jinwoo7.song, kling, kondapallykalyan, mitz, ossy, philn, rego+ews, rniwa, roger_fong, sam, simon.fraser, WebkitBugTracker, xan.lopez
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=216914
Bug Depends on: 123328    
Bug Blocks:    
Attachments:
Description Flags
some work in progress
eflews.bot: commit-queue-
Archive of layout-test-results from webkit-ews-10 for mac-mountainlion-wk2
none
Archive of layout-test-results from webkit-ews-07 for mac-mountainlion
none
for bots
eflews.bot: commit-queue-
Archive of layout-test-results from webkit-ews-11 for mac-mountainlion-wk2
none
Archive of layout-test-results from webkit-ews-07 for mac-mountainlion
none
another
eflews.bot: commit-queue-
Archive of layout-test-results from webkit-ews-15 for mac-mountainlion-wk2
none
Archive of layout-test-results from webkit-ews-08 for mac-mountainlion
none
Archive of layout-test-results from webkit-ews-06 for mac-mountainlion
none
another
none
and another
buildbot: commit-queue-
Archive of layout-test-results from webkit-ews-12 for mac-mountainlion-wk2
none
Archive of layout-test-results from webkit-ews-05 for mac-mountainlion
none
Archive of layout-test-results from webkit-ews-02 for mac-mountainlion
none
yet another
buildbot: commit-queue-
patch darin: review+, buildbot: commit-queue-

Description Antti Koivisto 2013-10-07 12:02:36 PDT
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.
Comment 1 Antti Koivisto 2013-10-07 12:05:43 PDT
Created attachment 213609 [details]
some work in progress

No hit testing or selections yet. Does pretty well in static CSS tests.
Comment 2 EFL EWS Bot 2013-10-07 12:29:12 PDT
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
Comment 3 EFL EWS Bot 2013-10-07 12:30:18 PDT
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
Comment 4 Build Bot 2013-10-07 12:47:10 PDT
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
Comment 5 Build Bot 2013-10-07 13:04:33 PDT
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
Comment 6 Build Bot 2013-10-07 13:04:36 PDT
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
Comment 7 Build Bot 2013-10-07 13:11:48 PDT
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
Comment 8 Build Bot 2013-10-07 13:11:52 PDT
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
Comment 9 kov's GTK+ EWS bot 2013-10-07 15:23:02 PDT
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
Comment 10 Antti Koivisto 2013-10-18 06:11:50 PDT
Created attachment 214564 [details]
for bots
Comment 11 EFL EWS Bot 2013-10-18 06:24:31 PDT
Comment on attachment 214564 [details]
for bots

Attachment 214564 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/5378030
Comment 12 Build Bot 2013-10-18 06:53:55 PDT
Comment on attachment 214564 [details]
for bots

Attachment 214564 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/5378034
Comment 13 Build Bot 2013-10-18 06:57:00 PDT
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
Comment 14 Build Bot 2013-10-18 06:57:03 PDT
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
Comment 15 kov's GTK+ EWS bot 2013-10-18 07:09:02 PDT
Comment on attachment 214564 [details]
for bots

Attachment 214564 [details] did not pass gtk-ews (gtk):
Output: http://webkit-queues.appspot.com/results/4798052
Comment 16 Build Bot 2013-10-18 07:22:51 PDT
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
Comment 17 Build Bot 2013-10-18 07:22:55 PDT
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
Comment 18 Antti Koivisto 2013-10-18 08:14:17 PDT
Created attachment 214574 [details]
another
Comment 19 EFL EWS Bot 2013-10-18 08:19:11 PDT
Comment on attachment 214574 [details]
another

Attachment 214574 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/5388043
Comment 20 kov's GTK+ EWS bot 2013-10-18 08:21:29 PDT
Comment on attachment 214574 [details]
another

Attachment 214574 [details] did not pass gtk-ews (gtk):
Output: http://webkit-queues.appspot.com/results/5458043
Comment 21 Build Bot 2013-10-18 08:45:58 PDT
Comment on attachment 214574 [details]
another

Attachment 214574 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/5368044
Comment 22 kov's GTK+ EWS bot 2013-10-18 08:59:27 PDT
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
Comment 23 Build Bot 2013-10-18 09:03:39 PDT
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
Comment 24 Build Bot 2013-10-18 09:03:43 PDT
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
Comment 25 Simon Fraser (smfr) 2013-10-18 09:22:03 PDT
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).
Comment 26 Build Bot 2013-10-18 09:27:04 PDT
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
Comment 27 Build Bot 2013-10-18 09:27:07 PDT
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
Comment 28 Build Bot 2013-10-18 10:25:40 PDT
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
Comment 29 Build Bot 2013-10-18 10:25:44 PDT
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
Comment 30 Antti Koivisto 2013-10-18 10:57:03 PDT
> > 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.
Comment 31 Antti Koivisto 2013-10-18 11:12:28 PDT
> 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.
Comment 32 Andreas Kling 2013-10-19 06:52:46 PDT
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 :)
Comment 33 EFL EWS Bot 2013-10-20 09:21:20 PDT
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
Comment 34 Antti Koivisto 2013-10-23 06:13:39 PDT
Created attachment 214949 [details]
another
Comment 35 Antti Koivisto 2013-10-23 06:30:15 PDT
Created attachment 214952 [details]
and another
Comment 36 Build Bot 2013-10-23 07:16:05 PDT
Comment on attachment 214952 [details]
and another

Attachment 214952 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/9688008
Comment 37 EFL EWS Bot 2013-10-23 07:22:18 PDT
Comment on attachment 214952 [details]
and another

Attachment 214952 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/9678011
Comment 38 Build Bot 2013-10-23 07:26:38 PDT
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
Comment 39 Build Bot 2013-10-23 07:26:44 PDT
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
Comment 40 Build Bot 2013-10-23 07:48:48 PDT
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
Comment 41 Build Bot 2013-10-23 07:48:53 PDT
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
Comment 42 EFL EWS Bot 2013-10-23 08:00:03 PDT
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
Comment 43 kov's GTK+ EWS bot 2013-10-23 08:38:31 PDT
Comment on attachment 214952 [details]
and another

Attachment 214952 [details] did not pass gtk-ews (gtk):
Output: http://webkit-queues.appspot.com/results/6078066
Comment 44 Build Bot 2013-10-23 08:49:52 PDT
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
Comment 45 Build Bot 2013-10-23 08:49:57 PDT
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
Comment 46 Antti Koivisto 2013-10-23 11:38:59 PDT
Created attachment 214975 [details]
yet another
Comment 47 Build Bot 2013-10-23 12:40:18 PDT
Comment on attachment 214975 [details]
yet another

Attachment 214975 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/10148009
Comment 48 EFL EWS Bot 2013-10-23 13:01:41 PDT
Comment on attachment 214975 [details]
yet another

Attachment 214975 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/10068018
Comment 49 EFL EWS Bot 2013-10-23 15:08:43 PDT
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
Comment 50 Antti Koivisto 2013-10-23 16:15:39 PDT
Created attachment 215008 [details]
patch
Comment 51 Build Bot 2013-10-23 17:02:29 PDT
Comment on attachment 215008 [details]
patch

Attachment 215008 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/10528013
Comment 52 Darin Adler 2013-10-23 23:40:25 PDT
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.
Comment 53 Antti Koivisto 2013-10-24 13:30:04 PDT
https://trac.webkit.org/r157950
Comment 54 Antti Koivisto 2013-10-24 13:40:32 PDT
(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!
Comment 56 Csaba Osztrogonác 2013-10-25 03:41:00 PDT
(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.