Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2961583003:
Element with columnCount > 1 should be classified as INDEPENDENT (Closed)
|
Created:
3 years, 5 months ago by cathiechentx Modified:
3 years, 5 months ago CC:
chromium-reviews, pdr+renderingwatchlist_chromium.org, zoltan1, blink-reviews-layout_chromium.org, szager+layoutwatch_chromium.org, eae+blinkwatch, leviw+renderwatch, jchaffraix+rendering, blink-reviews Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionElement with columnCount > 1 should be classified as INDEPENDENT
The LayoutWidth of LayoutMultiColumnFlowThread is one columnCountth of
its parent node. The width difference between them is large. So element
with columnCount > 1 should create cluster anyway.
BUG=731681
Review-Url: https://codereview.chromium.org/2961583003
Cr-Commit-Position: refs/heads/master@{#482927}
Committed: https://chromium.googlesource.com/chromium/src/+/aa7b8c388ea0b2af4eb1e14ba246c3e1b5893aee
Patch Set 1 #
Total comments: 4
Patch Set 2 : use the *used* column-count instead of style columnCount #
Total comments: 2
Patch Set 3 : nit #
Messages
Total messages: 21 (9 generated)
cathiechen@tencent.com changed reviewers: + jamesxwei@tencent.com, pdr@chromium.org, skobes@chromium.org
Hi pdr@ and skobes@,
Last CL(2817443003: font-size shaking) cause 45.2%-66.9% regression in
multicol_lots-of-text-balanced:(
Reason of regression:
Test case: multicol_lots-of-text-balanced tests the performance of multi-columns
balance. During balancing columns height, need to traverse each line of content.
If font-size enlarged and more line box generated, the performance would go
down.
Why font-size enlarged?
Here is the test case:
<html>
<body>
<div style="-webkit-columns:3; orphans:2; widows:2;">
lots of text......
</div>
</body>
</html>
There is only one cluster created by LayoutView in this page. <div> will use the
multiplier of LayoutView cluster. After last CL(2817443003: font-size shaking),
LayoutView cluster will use itself as width provider, so <div> will be
autosized.
Solution:
The layout width difference between multi-columns and its anonymous child is
large (anonymous-child-width * column-count = multi-columns-width).
multi-columns elements should create its own cluster and calculate own
multiplier.
Hi pdr@ and skobes@,
Last CL(2817443003: font-size shaking) cause 45.2%-66.9% regression in
multicol_lots-of-text-balanced:(
Reason of regression:
Test case: multicol_lots-of-text-balanced tests the performance of multi-columns
balance. During balancing columns height, need to traverse each line of content.
If font-size enlarged and more line box generated, the performance would go
down.
Why font-size enlarged?
Here is the test case:
<html>
<body>
<div style="-webkit-columns:3; orphans:2; widows:2;">
lots of text......
</div>
</body>
</html>
There is only one cluster created by LayoutView in this page. <div> will use the
multiplier of LayoutView cluster. After last CL(2817443003: font-size shaking),
LayoutView cluster will use itself as width provider, so <div> will be
autosized.
Solution:
The layout width difference between multi-columns and its anonymous child is
large (anonymous-child-width * column-count = multi-columns-width).
multi-columns elements should create its own cluster and calculate own
multiplier.
mstensho@opera.com changed reviewers: + mstensho@opera.com
https://codereview.chromium.org/2961583003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/TextAutosizer.cpp (right): https://codereview.chromium.org/2961583003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/TextAutosizer.cpp:680: block->Style()->ColumnCount() > 1)) I don't think this is the correct way to check. If you want to check whether it's a multicol container at all, it's better to use ComputedStyle::SpecifiesColumns() (even better to check if LayoutBlockFlow::MultiColumnFlowThread() is non-NULL). column-count:1 also establishes a multicol container. Additionally, one can use column-width instead of column-count. Or: if you want to check whether the *used* column-count is larger than 1 (i.e. what we get after resolving widths, column-count, column-width and column-gap etc. during layout), you cannot check with computed style, since this is determined during layout. LayoutMultiColumnFlowThread::ColumnCount() will return the used column-count. https://codereview.chromium.org/2961583003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/TextAutosizerTest.cpp (right): https://codereview.chromium.org/2961583003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/TextAutosizerTest.cpp:862: " #mc {-webkit-columns: 3; orphans: 2; widows: 2;}" You can skip the -webkit- prefix. No longer needed. Also, I'm not sure if you want to specify orphans and widows explicitly. They are 2 initially anyway, but it's probably not important in this test?
https://codereview.chromium.org/2961583003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/TextAutosizer.cpp (right): https://codereview.chromium.org/2961583003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/TextAutosizer.cpp:680: block->Style()->ColumnCount() > 1)) On 2017/06/26 16:49:59, mstensho wrote: > I don't think this is the correct way to check. > > If you want to check whether it's a multicol container at all, it's better to > use ComputedStyle::SpecifiesColumns() (even better to check if > LayoutBlockFlow::MultiColumnFlowThread() is non-NULL). column-count:1 also > establishes a multicol container. Additionally, one can use column-width instead > of column-count. > > Or: if you want to check whether the *used* column-count is larger than 1 (i.e. > what we get after resolving widths, column-count, column-width and column-gap > etc. during layout), you cannot check with computed style, since this is > determined during layout. LayoutMultiColumnFlowThread::ColumnCount() will return > the used column-count. Got it! Thanks a lot! https://codereview.chromium.org/2961583003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/TextAutosizerTest.cpp (right): https://codereview.chromium.org/2961583003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/TextAutosizerTest.cpp:862: " #mc {-webkit-columns: 3; orphans: 2; widows: 2;}" On 2017/06/26 16:49:59, mstensho wrote: > You can skip the -webkit- prefix. No longer needed. Also, I'm not sure if you > want to specify orphans and widows explicitly. They are 2 initially anyway, but > it's probably not important in this test? Done.
lgtm - please update the bug number, though - it's a duplicate. It should also be noted that I don't know much about this feature (TextAutoSizer). So maybe you want a second opinion from someone who does. :)
On 2017/06/27 08:26:41, mstensho wrote: > lgtm - please update the bug number, though - it's a duplicate. > > It should also be noted that I don't know much about this feature > (TextAutoSizer). So maybe you want a second opinion from someone who does. :) Thanks! This CL only fix BUG(731681), and I'll clarify it on bug description. Yeah, we need pdr@ or skobes@ take a look of this.
lgtm https://codereview.chromium.org/2961583003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/TextAutosizerTest.cpp (right): https://codereview.chromium.org/2961583003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/TextAutosizerTest.cpp:855: TEST_F(TextAutosizerTest, multiColumns) { Nit: capitalize test name (MultiColumns)
https://codereview.chromium.org/2961583003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/TextAutosizerTest.cpp (right): https://codereview.chromium.org/2961583003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/TextAutosizerTest.cpp:855: TEST_F(TextAutosizerTest, multiColumns) { On 2017/06/27 14:44:07, skobes wrote: > Nit: capitalize test name (MultiColumns) Done.
The CQ bit was checked by cathiechen@tencent.com
The patchset sent to the CQ was uploaded after l-g-t-m from mstensho@opera.com, skobes@chromium.org Link to the patchset: https://codereview.chromium.org/2961583003/#ps40001 (title: "nit")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
The CQ bit was checked by cathiechen@tencent.com
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1498639662595490,
"parent_rev": "41f6d1dad03160bbc855f7b4b0f1c42ad5d485a0", "commit_rev":
"aa7b8c388ea0b2af4eb1e14ba246c3e1b5893aee"}
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1498639662595490,
"parent_rev": "41f6d1dad03160bbc855f7b4b0f1c42ad5d485a0", "commit_rev":
"aa7b8c388ea0b2af4eb1e14ba246c3e1b5893aee"}
Message was sent while issue was closed.
Description was changed from ========== Element with columnCount > 1 should be classified as INDEPENDENT The LayoutWidth of LayoutMultiColumnFlowThread is one columnCountth of its parent node. The width difference between them is large. So element with columnCount > 1 should create cluster anyway. BUG=731681 ========== to ========== Element with columnCount > 1 should be classified as INDEPENDENT The LayoutWidth of LayoutMultiColumnFlowThread is one columnCountth of its parent node. The width difference between them is large. So element with columnCount > 1 should create cluster anyway. BUG=731681 Review-Url: https://codereview.chromium.org/2961583003 Cr-Commit-Position: refs/heads/master@{#482927} Committed: https://chromium.googlesource.com/chromium/src/+/aa7b8c388ea0b2af4eb1e14ba246... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/aa7b8c388ea0b2af4eb1e14ba246... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
