Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2906033002:
Fix narrow child cluster inherit multiplier from wide parent cluster (Closed)
|
Created:
3 years, 6 months ago by cathiechentx Modified:
3 years, 6 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. |
DescriptionFix narrow child cluster inherit multiplier from wide parent cluster
A narrow child cluster(eg. width:200px) shouldn't inherit multiplier
from parent cluster(eg. width:800px).
BUG=
Review-Url: https://codereview.chromium.org/2906033002
Cr-Commit-Position: refs/heads/master@{#476587}
Committed: https://chromium.googlesource.com/chromium/src/+/091d7b4b9942e20b459194f57c5e6ac3d18082bb
Patch Set 1 #
Total comments: 3
Patch Set 2 : using DeepestBlockContainingAllText to compare #Patch Set 3 : format #
Messages
Total messages: 16 (9 generated)
Description was changed from ========== Fix narrow child cluster inherit multiplier from wide parent cluster A narrow child cluster(eg. width:200px) shouldn't inherit multiplier from parent cluster(eg. width:800px). This inherit issue could be reproduced when parent cluster has second child which have wide content width. BUG= ========== to ========== Fix narrow child cluster inherit multiplier from wide parent cluster A narrow child cluster(eg. width:200px) shouldn't inherit multiplier from parent cluster(eg. width:800px). BUG= ==========
cathiechen@tencent.com changed reviewers: + jamesxwei@tencent.com, pdr@chromium.org, skobes@chromium.org
To fix inherit issue:) https://codereview.chromium.org/2906033002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/TextAutosizer.cpp (right): https://codereview.chromium.org/2906033002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/TextAutosizer.cpp:1160: cluster->root_->ContentLogicalWidth().ToFloat()); This inherit issue could be reproduced when parent cluster has second child with wide content width. E.g: <div id="parent" style="width:800px"> <div id="firstchild" style="width:800px"> <div id="narrowContent" style="width:200px"> content long enough to autosize...</div> </div> <div id="secondchild">xxx</div> </div> Old procedure： Step 1: "narrowContent" inherit multiplier from "firstchild"( DeepestBlockContainingAllText of "firstchild" is "narrowContent"). Step 2: "firstchild" inherit multiplier from "parent"( "parent" width == "firstchild" width ) Step 3: So "narrowContent" inherit multiplier from "parent". With "secondChild" multiplier of "parent" is 2.5. While multiplier of "narrowContent" with width 200px should be 1.0f. After change: Step 2: "firstchild" won't inherit multiplier from "parent". ( "parent" width != "firstchild" DeepestBlockContainingAllText width ) Consider of performance: DeepestBlockContainingAllText of a cluster is cached. This approach won't cause extra process of DeepestBlockContainingAllText.
To fix inherit issue:)
https://codereview.chromium.org/2906033002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/TextAutosizer.cpp (right): https://codereview.chromium.org/2906033002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/TextAutosizer.cpp:1160: cluster->root_->ContentLogicalWidth().ToFloat()); On 2017/05/26 09:58:59, cathiechentx wrote: > This inherit issue could be reproduced when parent cluster has second child with > wide content width. E.g: > > <div id="parent" style="width:800px"> > <div id="firstchild" style="width:800px"> > <div id="narrowContent" style="width:200px"> content long enough to > autosize...</div> > </div> > <div id="secondchild">xxx</div> > </div> > > Old procedure： > Step 1: "narrowContent" inherit multiplier from "firstchild"( > DeepestBlockContainingAllText of "firstchild" is "narrowContent"). > Step 2: "firstchild" inherit multiplier from "parent"( "parent" width == > "firstchild" width ) > Step 3: So "narrowContent" inherit multiplier from "parent". With "secondChild" > multiplier of "parent" is 2.5. While multiplier of "narrowContent" with width > 200px should be 1.0f. > > After change: > Step 2: "firstchild" won't inherit multiplier from "parent". ( "parent" width != > "firstchild" DeepestBlockContainingAllText width ) > > Consider of performance: > DeepestBlockContainingAllText of a cluster is cached. This approach won't cause > extra process of DeepestBlockContainingAllText. I think the reason we didn't do this originally was that the cluster's DeepestBlockContainingAllText may not have had layout yet. But I guess this is safe since it's only used during multiplier computation which means we've already encountered text in Inflate? Do we still need to check cluster->root_, or should we just set content_width using only the DeepestBlockContainingAllText?
https://codereview.chromium.org/2906033002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/TextAutosizer.cpp (right): https://codereview.chromium.org/2906033002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/TextAutosizer.cpp:1160: cluster->root_->ContentLogicalWidth().ToFloat()); On 2017/05/31 02:25:28, skobes wrote: > On 2017/05/26 09:58:59, cathiechentx wrote: > > This inherit issue could be reproduced when parent cluster has second child > with > > wide content width. E.g: > > > > <div id="parent" style="width:800px"> > > <div id="firstchild" style="width:800px"> > > <div id="narrowContent" style="width:200px"> content long enough to > > autosize...</div> > > </div> > > <div id="secondchild">xxx</div> > > </div> > > > > Old procedure： > > Step 1: "narrowContent" inherit multiplier from "firstchild"( > > DeepestBlockContainingAllText of "firstchild" is "narrowContent"). > > Step 2: "firstchild" inherit multiplier from "parent"( "parent" width == > > "firstchild" width ) > > Step 3: So "narrowContent" inherit multiplier from "parent". With > "secondChild" > > multiplier of "parent" is 2.5. While multiplier of "narrowContent" with width > > 200px should be 1.0f. > > > > After change: > > Step 2: "firstchild" won't inherit multiplier from "parent". ( "parent" width > != > > "firstchild" DeepestBlockContainingAllText width ) > > > > Consider of performance: > > DeepestBlockContainingAllText of a cluster is cached. This approach won't > cause > > extra process of DeepestBlockContainingAllText. > > I think the reason we didn't do this originally was that the cluster's > DeepestBlockContainingAllText may not have had layout yet. But I guess this is > safe since it's only used during multiplier computation which means we've > already encountered text in Inflate? Yes, I check this too. After encountered text, parent blocks should have layout. It's safe here. > Do we still need to check cluster->root_, or should we just set content_width > using only the DeepestBlockContainingAllText? Yes, compare DeepestBlockContainingAllText with DeepestBlockContainingAllText is more reasonable.
The CQ bit was checked by cathiechen@tencent.com to run a CQ dry run
Dry run: 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
Dry run: No L-G-T-M from a valid reviewer yet. CQ run can only be started once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer,_not_ a full super star committer. Committers are members of the group "project-chromium-committers". Note that this has nothing to do with OWNERS files.
lgtm
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": 1496374175836980,
"parent_rev": "e8b52c26834fbeba662c6cf0073d88aefb7907a8", "commit_rev":
"091d7b4b9942e20b459194f57c5e6ac3d18082bb"}
Message was sent while issue was closed.
Description was changed from ========== Fix narrow child cluster inherit multiplier from wide parent cluster A narrow child cluster(eg. width:200px) shouldn't inherit multiplier from parent cluster(eg. width:800px). BUG= ========== to ========== Fix narrow child cluster inherit multiplier from wide parent cluster A narrow child cluster(eg. width:200px) shouldn't inherit multiplier from parent cluster(eg. width:800px). BUG= Review-Url: https://codereview.chromium.org/2906033002 Cr-Commit-Position: refs/heads/master@{#476587} Committed: https://chromium.googlesource.com/chromium/src/+/091d7b4b9942e20b459194f57c5e... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/091d7b4b9942e20b459194f57c5e... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
