Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(270)

Issue 1399493002: Column balancing refactoring. Don't propagate data during layout. (Closed)

Created:
5 years, 2 months ago by mstensho (USE GERRIT)
Modified:
5 years, 2 months ago
CC:
blink-reviews, blink-reviews-layout_chromium.org, chromium-reviews, eae+blinkwatch, jchaffraix+rendering, leviw+renderwatch, mstensho (USE GERRIT), pdr+renderingwatchlist_chromium.org, szager+layoutwatch_chromium.org, zoltan1
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Column balancing refactoring. Don't propagate data during layout. The column balancing machinery needs the following to calculate an optimal column height: 1. Flow thread height 2. Flow thread logical top position of all forced breaks 3. The total number of forced breaks 4. Minimum space shortage at any soft break The first two are needed to estimate an initial column height. The last two are needed if the initial column height estimate wasn't good enough and we need to stretch it. We used to propagate this information to the flow thread continuously during layout. Then the flow thread, after layout, would use this information for column balancing (either estimate an initial column height, or stretch columns based on minimum space shortage) and then re-lay out. This was problematic, because during layout, we sometimes go back, and re-lay out e.g. a block because the initial logical top was wrong (due to complex margin collapsing). This would confuse the column balancing machinery (forced breaks being reported and counted twice, space shortage being incorrectly reported, and so on). Instead, traverse the tree after flow thread layout, to collect the information that we need. This fixes an existing corner-case bug, allows for future optimizations. There is still one thing that the flow thread needs to be notified about during layout, if we have nested multicol: It needs to know where content is laid out (especially if the content is in a new column), in order to tell if we need to insert another fragmentainer group (i.e. column row). It would be nice to get rid of this as well in the future, but we need it for now. Renamed the poorly named setPageBreak() to the more call-back-like paginatedContentWasLaidOut(). The new class ColumnBalancer and its subclasses are now in charge of column balancing. Moved / refactored relevant code into those classes. Taken from LayoutMultiColumnFlowThread, LayoutMultiColumnSet, MultiColumnFramentainerGroup, and even LayoutBlockFlow. BUG=521964 R=jchaffraix@chromium.org,leviw@chromium.org Committed: https://crrev.com/ce77a26bc7b22cf511e91b336f40ae405712f66b Cr-Commit-Position: refs/heads/master@{#353463}

Patch Set 1 #

Total comments: 10

Patch Set 2 : code review #

Patch Set 3 : more code review #

Unified diffs Side-by-side diffs Delta from patch set Stats (+503 lines, -249 lines) Patch
A third_party/WebKit/LayoutTests/fast/multicol/auto-height-forced-break-complex-margin-collapsing.html View 1 chunk +21 lines, -0 lines 0 comments Download
A third_party/WebKit/LayoutTests/fast/multicol/auto-height-forced-break-complex-margin-collapsing-expected.txt View 1 chunk +10 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/core.gypi View 1 chunk +2 lines, -0 lines 0 comments Download
A third_party/WebKit/Source/core/layout/ColumnBalancer.h View 1 2 1 chunk +157 lines, -0 lines 0 comments Download
A third_party/WebKit/Source/core/layout/ColumnBalancer.cpp View 1 chunk +244 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/layout/LayoutBlock.h View 1 chunk +4 lines, -2 lines 0 comments Download
M third_party/WebKit/Source/core/layout/LayoutBlock.cpp View 1 chunk +2 lines, -2 lines 0 comments Download
M third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp View 7 chunks +5 lines, -39 lines 0 comments Download
M third_party/WebKit/Source/core/layout/LayoutFlowThread.h View 1 chunk +1 line, -3 lines 0 comments Download
M third_party/WebKit/Source/core/layout/LayoutMultiColumnFlowThread.h View 2 chunks +6 lines, -8 lines 0 comments Download
M third_party/WebKit/Source/core/layout/LayoutMultiColumnFlowThread.cpp View 3 chunks +27 lines, -20 lines 0 comments Download
M third_party/WebKit/Source/core/layout/LayoutMultiColumnSet.h View 1 chunk +0 lines, -12 lines 0 comments Download
M third_party/WebKit/Source/core/layout/LayoutMultiColumnSet.cpp View 2 chunks +0 lines, -28 lines 0 comments Download
M third_party/WebKit/Source/core/layout/MultiColumnFragmentainerGroup.h View 1 2 4 chunks +3 lines, -39 lines 0 comments Download
M third_party/WebKit/Source/core/layout/MultiColumnFragmentainerGroup.cpp View 9 chunks +21 lines, -96 lines 0 comments Download

Messages

Total messages: 16 (3 generated)
mstensho (USE GERRIT)
5 years, 2 months ago (2015-10-08 09:10:39 UTC) #1
leviw_travelin_and_unemployed
https://codereview.chromium.org/1399493002/diff/1/third_party/WebKit/Source/core/layout/ColumnBalancer.h File third_party/WebKit/Source/core/layout/ColumnBalancer.h (right): https://codereview.chromium.org/1399493002/diff/1/third_party/WebKit/Source/core/layout/ColumnBalancer.h#newcode14 third_party/WebKit/Source/core/layout/ColumnBalancer.h:14: ColumnBalancer(const MultiColumnFragmentainerGroup&); Does any of this need to be ...
5 years, 2 months ago (2015-10-08 09:49:29 UTC) #2
leviw_travelin_and_unemployed
https://codereview.chromium.org/1399493002/diff/1/third_party/WebKit/Source/core/layout/MultiColumnFragmentainerGroup.h File third_party/WebKit/Source/core/layout/MultiColumnFragmentainerGroup.h (right): https://codereview.chromium.org/1399493002/diff/1/third_party/WebKit/Source/core/layout/MultiColumnFragmentainerGroup.h#newcode75 third_party/WebKit/Source/core/layout/MultiColumnFragmentainerGroup.h:75: enum ColumnIndexCalculationMode { Is this change related? It doesn't ...
5 years, 2 months ago (2015-10-08 09:53:47 UTC) #3
mstensho (USE GERRIT)
https://codereview.chromium.org/1399493002/diff/1/third_party/WebKit/Source/core/layout/ColumnBalancer.h File third_party/WebKit/Source/core/layout/ColumnBalancer.h (right): https://codereview.chromium.org/1399493002/diff/1/third_party/WebKit/Source/core/layout/ColumnBalancer.h#newcode14 third_party/WebKit/Source/core/layout/ColumnBalancer.h:14: ColumnBalancer(const MultiColumnFragmentainerGroup&); On 2015/10/08 09:49:29, leviw wrote: > Does ...
5 years, 2 months ago (2015-10-08 10:38:05 UTC) #4
leviw_travelin_and_unemployed
On 2015/10/08 at 10:38:05, mstensho wrote: > https://codereview.chromium.org/1399493002/diff/1/third_party/WebKit/Source/core/layout/ColumnBalancer.h > File third_party/WebKit/Source/core/layout/ColumnBalancer.h (right): > > https://codereview.chromium.org/1399493002/diff/1/third_party/WebKit/Source/core/layout/ColumnBalancer.h#newcode14 ...
5 years, 2 months ago (2015-10-08 10:44:39 UTC) #5
mstensho (USE GERRIT)
Changing the initial vector size from 0x32 to 32. https://codereview.chromium.org/1399493002/diff/1/third_party/WebKit/Source/core/layout/MultiColumnFragmentainerGroup.h File third_party/WebKit/Source/core/layout/MultiColumnFragmentainerGroup.h (right): https://codereview.chromium.org/1399493002/diff/1/third_party/WebKit/Source/core/layout/MultiColumnFragmentainerGroup.h#newcode75 third_party/WebKit/Source/core/layout/MultiColumnFragmentainerGroup.h:75: ...
5 years, 2 months ago (2015-10-08 10:49:41 UTC) #6
leviw_travelin_and_unemployed
On 2015/10/08 at 10:49:41, mstensho wrote: > Changing the initial vector size from 0x32 to ...
5 years, 2 months ago (2015-10-08 10:51:37 UTC) #7
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1399493002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1399493002/40001
5 years, 2 months ago (2015-10-09 07:42:55 UTC) #9
commit-bot: I haz the power
Try jobs failed on following builders: linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_rel_ng/builds/124467)
5 years, 2 months ago (2015-10-09 09:10:04 UTC) #11
mstensho (USE GERRIT)
That's a real failure. Need to land https://codereview.chromium.org/1395753004/ before landing this.
5 years, 2 months ago (2015-10-09 13:08:03 UTC) #12
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1399493002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1399493002/40001
5 years, 2 months ago (2015-10-10 08:19:05 UTC) #14
commit-bot: I haz the power
Committed patchset #3 (id:40001)
5 years, 2 months ago (2015-10-10 10:00:50 UTC) #15
commit-bot: I haz the power
5 years, 2 months ago (2015-10-10 10:01:46 UTC) #16
Message was sent while issue was closed.
Patchset 3 (id:??) landed as
https://crrev.com/ce77a26bc7b22cf511e91b336f40ae405712f66b
Cr-Commit-Position: refs/heads/master@{#353463}

Powered by Google App Engine
This is Rietveld 408576698