Bug 60113 – Add a LineLayoutState object to hold global state during line layout

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Eric Seidel (no email)]

Add a LineLayoutState object to hold global state during line layout

[Eric Seidel (no email)]

Created attachment 92185 [details]
Patch

[Eric Seidel (no email)]

Created attachment 92506 [details]
wip patch, currently testing

[WebKit Review Bot]

Comment on attachment 92506 [details]
wip patch, currently testing

Attachment 92506 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8683001

New failing tests:
fast/multicol/vertical-lr/float-multicol.html
fast/multicol/vertical-rl/float-multicol.html
fast/repaint/line-flow-with-floats-9.html

[WebKit Review Bot]

Created attachment 92816 [details]
Archive of layout-test-results from ec2-cr-linux-01

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-01  Port: Chromium  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Eric Seidel (no email)]

Created attachment 92856 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 92856 [details]
Patch

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

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:820
> +// Like LayoutState for layout(), LineLayoutState keeps track of global information
> +// during an entire linebox tree layout pass (aka layoutInlineChildren).

Nit: This comment should go into the change log entry as well.

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:1004
> -                        if (useRepaintBounds) // This can only be a positive adjustment, so no need to update repaintTop.
> -                            repaintLogicalBottom = max(repaintLogicalBottom, lineBox->logicalBottomVisualOverflow());
> +                        if (useRepaintBounds)
> +                            layoutState.updateRepaintRangeFromBox(lineBox);

I don't think compiler is smart enough to optimize-away an extra assignment; also, can we assert that m_repaintLogicalTop didn't change? I'm afraid that despite of the comment here, the value might still change.

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:1081
>              // Delete all the remaining lines.

I don't think this comment adds any useful value here.  Could you delete this comment so that we can get rid of curly brackets?

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:1286
> +            bool encounteredNewFloat = checkFloatsInCleanLine(curr, floats, floatIndex, dirtiedByFloat);
> +            if (encounteredNewFloat)

I would have renamed checkFloatsInCleanLine to markFloatsDirtyInCleanLine and wouldn't have introduced 'encounteredNewFloat'.

[Eric Seidel (no email)]

Was that an r+ or r-?

[Eric Seidel (no email)]

This is going to conflict with bug 60729.... but since this is still waiting review, I just pressed onward and upward. :)

[Ryosuke Niwa]

(In reply to comment #7)
> Was that an r+ or r-?

I'm waiting for your answer for:
> Source/WebCore/rendering/RenderBlockLineLayout.cpp:1004
> -                        if (useRepaintBounds) // This can only be a positive adjustment, so no need to update repaintTop.
> -                            repaintLogicalBottom = max(repaintLogicalBottom, lineBox->logicalBottomVisualOverflow());
> +                        if (useRepaintBounds)
> +                            layoutState.updateRepaintRangeFromBox(lineBox);

I don't think compiler is smart enough to optimize-away an extra assignment; also, can we assert that m_repaintLogicalTop didn't change? I'm afraid that despite of the comment here, the value might still change.

Are you sure that this change won't regress any tests?

[Eric Seidel (no email)]

(In reply to comment #9)
> (In reply to comment #7)
> > Was that an r+ or r-?
> 
> I'm waiting for your answer for:
> > Source/WebCore/rendering/RenderBlockLineLayout.cpp:1004
> > -                        if (useRepaintBounds) // This can only be a positive adjustment, so no need to update repaintTop.
> > -                            repaintLogicalBottom = max(repaintLogicalBottom, lineBox->logicalBottomVisualOverflow());
> > +                        if (useRepaintBounds)
> > +                            layoutState.updateRepaintRangeFromBox(lineBox);
> 
> I don't think compiler is smart enough to optimize-away an extra assignment; also, can we assert that m_repaintLogicalTop didn't change? I'm afraid that despite of the comment here, the value might still change.
> 
> Are you sure that this change won't regress any tests?

Yes, I've run the tests.  No regressions. :)

[Ryosuke Niwa]

Comment on attachment 92856 [details]
Patch

(In reply to comment #10)
> Yes, I've run the tests.  No regressions. :)

Great!

[Eric Seidel (no email)]

(In reply to comment #6)
> (From update of attachment 92856 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=92856&action=review
> 
> > Source/WebCore/rendering/RenderBlockLineLayout.cpp:820
> > +// Like LayoutState for layout(), LineLayoutState keeps track of global information
> > +// during an entire linebox tree layout pass (aka layoutInlineChildren).
> 
> Nit: This comment should go into the change log entry as well.

OK, can do.

> > Source/WebCore/rendering/RenderBlockLineLayout.cpp:1004
> > -                        if (useRepaintBounds) // This can only be a positive adjustment, so no need to update repaintTop.
> > -                            repaintLogicalBottom = max(repaintLogicalBottom, lineBox->logicalBottomVisualOverflow());
> > +                        if (useRepaintBounds)
> > +                            layoutState.updateRepaintRangeFromBox(lineBox);
> 
> I don't think compiler is smart enough to optimize-away an extra assignment; also, can we assert that m_repaintLogicalTop didn't change? I'm afraid that despite of the comment here, the value might still change.
> 
> > Source/WebCore/rendering/RenderBlockLineLayout.cpp:1081
> >              // Delete all the remaining lines.
> 
> I don't think this comment adds any useful value here.  Could you delete this comment so that we can get rid of curly brackets?

This can just become deleteLineBoxTree (as I did in a separate patch).

> > Source/WebCore/rendering/RenderBlockLineLayout.cpp:1286
> > +            bool encounteredNewFloat = checkFloatsInCleanLine(curr, floats, floatIndex, dirtiedByFloat);
> > +            if (encounteredNewFloat)
> 
> I would have renamed checkFloatsInCleanLine to markFloatsDirtyInCleanLine and wouldn't have introduced 'encounteredNewFloat'.

OK.  I'll undo and look at doing the rename.  This patch can't be landed as-is, given that I've written bug 60729 since.  But I'd like to get it r+'d in some form and I'll update it on top of bug 60729 once that lands.

[Eric Seidel (no email)]

Created attachment 93792 [details]
Patch for landing

[Eric Seidel (no email)]

Created attachment 93795 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 93795 [details]
Patch for landing

Rejecting attachment 93795 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-8', 'apply-..." exit_code: 1

Last 500 characters of output:
autoinstalled/mechanize/_urllib2_fork.py", line 332, in _call_chain
    result = func(*args)
  File "/mnt/git/webkit-commit-queue/Tools/Scripts/webkitpy/thirdparty/autoinstalled/mechanize/_urllib2_fork.py", line 1170, in https_open
    return self.do_open(conn_factory, req)
  File "/mnt/git/webkit-commit-queue/Tools/Scripts/webkitpy/thirdparty/autoinstalled/mechanize/_urllib2_fork.py", line 1118, in do_open
    raise URLError(err)
urllib2.URLError: <urlopen error [Errno 60] Operation timed out>

Full output: http://queues.webkit.org/results/8704383

[WebKit Commit Bot]

Comment on attachment 93795 [details]
Patch for landing

Clearing flags on attachment: 93795

Committed r86698: <http://trac.webkit.org/changeset/86698>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.