Add a LineLayoutState object to hold global state during line layout
Created attachment 92185 [details] Patch
Created attachment 92506 [details] wip patch, currently testing
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
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
Created attachment 92856 [details] Patch
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'.
Was that an r+ or r-?
This is going to conflict with bug 60729.... but since this is still waiting review, I just pressed onward and upward. :)
(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?
(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. :)
Comment on attachment 92856 [details] Patch (In reply to comment #10) > Yes, I've run the tests. No regressions. :) Great!
(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.
Created attachment 93792 [details] Patch for landing
Created attachment 93795 [details] Patch for landing
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
Comment on attachment 93795 [details] Patch for landing Clearing flags on attachment: 93795 Committed r86698: <http://trac.webkit.org/changeset/86698>
All reviewed patches have been landed. Closing bug.