Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2299213003:
Fix the inconsistent problem while the content of textNodes is changed (Closed)
|
Created:
4 years, 3 months ago by cathiechentx Modified:
4 years ago CC:
blink-reviews, blink-reviews-layout_chromium.org, chromium-reviews, eae+blinkwatch, jchaffraix+rendering, leviw+renderwatch, 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. |
DescriptionFix the inconsistent problem while the content of textNodes is changed.
If the text content of one node who share fingerPrint with others changed,
the multipiler of this node might be different from others.
This's the reason of this inconsistent problem.
The solution:
1. Make superclusters across layouts
2. Mark superclusters if needed
2.1 new roots are added to superclusters
2.2 old roots add some new contents
3. Recalculate the multiplier of the marked superclusters during the first beginLayout().
BUG=612119
Committed: https://crrev.com/bf9901feb0c46cefbd5e7af40fe252a6a92d52cb
Cr-Commit-Position: refs/heads/master@{#439093}
Patch Set 1 #Patch Set 2 : fixed the merge issues #Patch Set 3 : add layout test cases and fix format issues #
Total comments: 14
Patch Set 4 : 1. change 'FingerPrint' to 'Fingerprint' 2. remove const from BlockSet 3. change 'textAutosizingNew… #
Total comments: 6
Patch Set 5 : using everHadLayout to determine if nodes are new #
Total comments: 16
Patch Set 6 : 1.hashset<fingerprint> to hashset<supercluster*> 2. fixing test cases problem 3. move m_supercluste… #Patch Set 7 : remove unused fingerprint #
Total comments: 26
Patch Set 8 : 1. fix cr issues 2.fix the nested inherit supercluster inssue #
Total comments: 2
Patch Set 9 : typos #
Total comments: 8
Patch Set 10 : fix cr issues #Patch Set 11 : need to remove supercluster from m_potentiallyInconsistentSuperclusters when this supercluster is d… #Patch Set 12 : git rebase... #
Messages
Total messages: 70 (29 generated)
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 by full committers or 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. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
Description was changed from ========== Fix the inconsistent problem while the content of textNodes is changed If the text content of one node who share the common fingerPrint with others changed, the multipiler of this node might be different from others. It's the reason to cause the inconsistent problem. 'm_multiplierOfFingerprint' is saving the multipilers of figerPrints, if the multipilers have been changed, then the changed fingerPrints will be saved at 'm_fingerprintInconsistent'. At the end of endlayout a task will be sent out to checkConsistency(). If 'm_fingerprintInconsistent' isn't empty, it will trigger for relayout. BUG=612119 ========== to ========== Fix the inconsistent problem while the content of textNodes is changed If the text content of one node who share the common fingerPrint with others changed, the multipiler of this node might be different from others. This's the reason to cause the inconsistent problem. 'm_multiplierOfFingerprint' is saving the multipilers of figerPrints, if the multipilers have been changed, then the changed fingerPrints will be saved at 'm_fingerprintInconsistent'. At the end of endlayout a task will be sent out to checkConsistency(). If 'm_fingerprintInconsistent' isn't empty, it will trigger for relayout. BUG=612119 ==========
Description was changed from ========== Fix the inconsistent problem while the content of textNodes is changed If the text content of one node who share the common fingerPrint with others changed, the multipiler of this node might be different from others. This's the reason to cause the inconsistent problem. 'm_multiplierOfFingerprint' is saving the multipilers of figerPrints, if the multipilers have been changed, then the changed fingerPrints will be saved at 'm_fingerprintInconsistent'. At the end of endlayout a task will be sent out to checkConsistency(). If 'm_fingerprintInconsistent' isn't empty, it will trigger for relayout. BUG=612119 ========== to ========== Fix the inconsistent problem while the content of textNodes is changed. If the text content of one node who share the common fingerPrint with others changed, the multipiler of this node might be different from others. This's the reason of the inconsistent problem. 'm_multiplierOfFingerprint' is saving the multipilers of figerPrints, if the multipilers have been changed, then the changed fingerPrints will be saved at 'm_fingerprintInconsistent'. At the end of endlayout a task will be sent out to checkConsistency(). If 'm_fingerprintInconsistent' isn't empty, it will trigger for relayout. BUG=612119 ==========
Description was changed from ========== Fix the inconsistent problem while the content of textNodes is changed. If the text content of one node who share the common fingerPrint with others changed, the multipiler of this node might be different from others. This's the reason of the inconsistent problem. 'm_multiplierOfFingerprint' is saving the multipilers of figerPrints, if the multipilers have been changed, then the changed fingerPrints will be saved at 'm_fingerprintInconsistent'. At the end of endlayout a task will be sent out to checkConsistency(). If 'm_fingerprintInconsistent' isn't empty, it will trigger for relayout. BUG=612119 ========== to ========== Fix the inconsistent problem while the content of textNodes is changed. If the text content of one node who share the common fingerPrint with others changed, the multipiler of this node might be different from others. This's the reason of the inconsistent problem. 'm_multiplierOfFingerprint' is saving the multipilers of figerPrints, if the multipilers have been changed, then the changed fingerPrints will be saved at 'm_fingerprintInconsistent'. At the end of endlayout a task will be sent out to checkConsistency(). If 'm_fingerprintInconsistent' isn't empty, it will trigger for relayout. BUG=612119 ==========
Description was changed from ========== Fix the inconsistent problem while the content of textNodes is changed. If the text content of one node who share the common fingerPrint with others changed, the multipiler of this node might be different from others. This's the reason of the inconsistent problem. 'm_multiplierOfFingerprint' is saving the multipilers of figerPrints, if the multipilers have been changed, then the changed fingerPrints will be saved at 'm_fingerprintInconsistent'. At the end of endlayout a task will be sent out to checkConsistency(). If 'm_fingerprintInconsistent' isn't empty, it will trigger for relayout. BUG=612119 ========== to ========== Fix the inconsistent problem while the content of textNodes is changed. If the text content of one node who share the common fingerPrint with others changed, the multipiler of this node might be different from others. This's the reason of the inconsistent problem. 'm_multiplierOfFingerprint' is saving the multipilers of figerPrints, if the multipilers have been changed, then the changed fingerPrints will be saved at 'm_fingerprintInconsistent'. At the end of endlayout a task will be sent out to checkConsistency(). If 'm_fingerprintInconsistent' isn't empty, it will trigger for relayout. BUG=612119 ==========
Description was changed from ========== Fix the inconsistent problem while the content of textNodes is changed. If the text content of one node who share the common fingerPrint with others changed, the multipiler of this node might be different from others. This's the reason of the inconsistent problem. 'm_multiplierOfFingerprint' is saving the multipilers of figerPrints, if the multipilers have been changed, then the changed fingerPrints will be saved at 'm_fingerprintInconsistent'. At the end of endlayout a task will be sent out to checkConsistency(). If 'm_fingerprintInconsistent' isn't empty, it will trigger for relayout. BUG=612119 ========== to ========== Fix the inconsistent problem while the content of textNodes is changed. If the text content of one node who share fingerPrint with others changed, the multipiler of this node might be different from others. This's the reason of the inconsistent problem. 'm_multiplierOfFingerprint' is saving the multipilers of figerPrints, if the multipilers have been changed, then the changed fingerPrints will be saved at 'm_fingerprintInconsistent'. At the end of endlayout a task will be sent out to checkConsistency(). If 'm_fingerprintInconsistent' isn't empty, it will trigger for relayout. BUG=612119 ==========
Description was changed from ========== Fix the inconsistent problem while the content of textNodes is changed. If the text content of one node who share fingerPrint with others changed, the multipiler of this node might be different from others. This's the reason of the inconsistent problem. 'm_multiplierOfFingerprint' is saving the multipilers of figerPrints, if the multipilers have been changed, then the changed fingerPrints will be saved at 'm_fingerprintInconsistent'. At the end of endlayout a task will be sent out to checkConsistency(). If 'm_fingerprintInconsistent' isn't empty, it will trigger for relayout. BUG=612119 ========== to ========== Fix the inconsistent problem while the content of textNodes is changed. If the text content of one node who share fingerPrint with others changed, the multipiler of this node might be different from others. This's the reason of the inconsistent problem. In this solution 'm_multiplierOfFingerprint' is saving the multipilers of figerPrints, if the multipilers have been changed, then the changed fingerPrints will be saved at 'm_fingerprintInconsistent'. At the end of layout a task will be sent out to checkConsistency(). If 'm_fingerprintInconsistent' isn't empty, it will trigger for relayout. BUG=612119 ==========
cathiechen@tencent.com changed reviewers: + jamesxwei@tencent.com, pdr@chromium.org, skobes@chromium.org, skyostil@chromium.org
Description was changed from ========== Fix the inconsistent problem while the content of textNodes is changed. If the text content of one node who share fingerPrint with others changed, the multipiler of this node might be different from others. This's the reason of the inconsistent problem. In this solution 'm_multiplierOfFingerprint' is saving the multipilers of figerPrints, if the multipilers have been changed, then the changed fingerPrints will be saved at 'm_fingerprintInconsistent'. At the end of layout a task will be sent out to checkConsistency(). If 'm_fingerprintInconsistent' isn't empty, it will trigger for relayout. BUG=612119 ========== to ========== Fix the inconsistent problem while the content of textNodes is changed. If the text content of one node who share fingerPrint with others changed, the multipiler of this node might be different from others. This's the reason of this inconsistent problem. In this solution 'm_multiplierOfFingerprint' is saving the multipilers of figerPrints, if the multipilers have been changed, then the changed fingerPrints will be saved at 'm_fingerprintInconsistent'. At the end of layout a task will be sent out to checkConsistency(). If 'm_fingerprintInconsistent' isn't empty, it will trigger for relayout. BUG=612119 ==========
Description was changed from ========== Fix the inconsistent problem while the content of textNodes is changed. If the text content of one node who share fingerPrint with others changed, the multipiler of this node might be different from others. This's the reason of this inconsistent problem. In this solution 'm_multiplierOfFingerprint' is saving the multipilers of figerPrints, if the multipilers have been changed, then the changed fingerPrints will be saved at 'm_fingerprintInconsistent'. At the end of layout a task will be sent out to checkConsistency(). If 'm_fingerprintInconsistent' isn't empty, it will trigger for relayout. BUG=612119 ========== to ========== Fix the inconsistent problem while the content of textNodes is changed. If the text content of one node who share fingerPrint with others changed, the multipiler of this node might be different from others. This's the reason of this inconsistent problem. In this solution, 'm_multiplierOfFingerprint' saved the multipilers of figerPrints, if the multipilers have been changed, then this fingerPrints will be saved at 'm_fingerprintInconsistent'. At the end of layout a task will be sent out to checkConsistency(). If 'm_fingerprintInconsistent' isn't empty, it will trigger for relayout. BUG=612119 ==========
hey guys, I'm trying to solve issue 612119, but it seems to be a big change. Let me know if there're any mistakes. Thanks!
Thanks for looking at this. If I understand correctly, this patch uses Document::postTask during layout to schedule a "consistency check", which then schedules a new layout pass to resolve the inconsistency. I'm not sure that design is a good solution to the supercluster consistency problem. It seems like we may paint a frame and/or have other side effects of the "inconsistent" state before the multipliers receive their final values. Furthermore one of TextAutosizer's design goals is to do all work during a single layout pass. On crbug.com/612119 I see that pdr mentions retaining superclusters across layouts, which might be a better path?
On 2016/09/02 19:36:58, skobes wrote: > Thanks for looking at this. > > If I understand correctly, this patch uses Document::postTask during layout to > schedule a "consistency check", which then schedules a new layout pass to > resolve the inconsistency. > > I'm not sure that design is a good solution to the supercluster consistency > problem. It seems like we may paint a frame and/or have other side effects of > the "inconsistent" state before the multipliers receive their final values. > Furthermore one of TextAutosizer's design goals is to do all work during a > single layout pass. > > On crbug.com/612119 I see that pdr mentions retaining superclusters across > layouts, which might be a better path? Thanks for your reply, skobes. I hesitated about the new layout pass too, and it do paint the "inconsistent" state frame. As to retaining superclusters across layouts, if we use the old multiplier of superclusters, the text won't be inflated, and it might not be what is expected. And if we update the multiplier of superclusters, it might face the same situation of a new layout pass. Maybe using the old multiplier of superclusters is better.
On 2016/09/04 01:28:31, cathiechentx wrote: > On 2016/09/02 19:36:58, skobes wrote: > > Thanks for looking at this. > > > > If I understand correctly, this patch uses Document::postTask during layout to > > schedule a "consistency check", which then schedules a new layout pass to > > resolve the inconsistency. > > > > I'm not sure that design is a good solution to the supercluster consistency > > problem. It seems like we may paint a frame and/or have other side effects of > > the "inconsistent" state before the multipliers receive their final values. > > Furthermore one of TextAutosizer's design goals is to do all work during a > > single layout pass. > > > > On crbug.com/612119 I see that pdr mentions retaining superclusters across > > layouts, which might be a better path? > > Thanks for your reply, skobes. > > I hesitated about the new layout pass too, and it do paint the "inconsistent" > state > frame. > > As to retaining superclusters across layouts, if we use the old multiplier of > superclusters, the text won't be inflated, and it might not be what is expected. > And if we update the multiplier of superclusters, it might face the same > situation > of a new layout pass. Maybe using the old multiplier of superclusters is better. hi guys, I read the code again. Maybe "retaining superclusters across layouts" would work. How about this: 1. Make superclusters across layouts 2. Mark superclusters if there are new roots added during record() 3. Recalculate the multiplier of the marked superclusters during the first beginLayout(). If multiplier changed, set all text nodes of the superclusters needs layout. Then we can apply the right multipliers during a single layout pass.
On 2016/09/06 10:33:44, cathiechentx wrote: > I read the code again. Maybe "retaining superclusters across layouts" would > work. How about this: > 1. Make superclusters across layouts > 2. Mark superclusters if there are new roots added during record() > 3. Recalculate the multiplier of the marked superclusters during the first > beginLayout(). If multiplier changed, set all text nodes of the superclusters > needs layout. > Then we can apply the right multipliers during a single layout pass. I think something like this could work, but be careful in step 3 that you only set needsLayout on nodes that the current layout pass hasn't reached yet. FrameView::layout asserts that no node needs layout when it returns.
On 2016/09/06 18:03:44, skobes wrote:
> On 2016/09/06 10:33:44, cathiechentx wrote:
> > I read the code again. Maybe "retaining superclusters across layouts" would
> > work. How about this:
> > 1. Make superclusters across layouts
> > 2. Mark superclusters if there are new roots added during record()
> > 3. Recalculate the multiplier of the marked superclusters during the first
> > beginLayout(). If multiplier changed, set all text nodes of the
superclusters
> > needs layout.
> > Then we can apply the right multipliers during a single layout pass.
>
> I think something like this could work, but be careful in step 3 that you only
> set needsLayout on nodes that the current layout pass hasn't reached yet.
> FrameView::layout asserts that no node needs layout when it returns.
Hey guys,
I'm sorry for not updating this issue for such a long time.
During the implementation, I found I missed something up there. Actually we
should mark superclusters to recalculate the multiplier not only when new roots
is added, but also when the old roots add new contents. The second one is much
complicated. I've tried many ways to achieve it, and didn't work so well.
Finally I found this way:
Issue: Mark the supercluster when old roots add new contents.
Solution:
1. add fingerprint as a new member in StyleInheritedData
2. update the block's StyleInheritedData's fingerprint when record
3. when new LayoutText created, using StyleInheritedData's fingerprint to
find the supercluster which will count the length of the text
4. if the length is long enough, mark the supercluster to recalculate
multiplier
so the whole solution became to this:
1. Make superclusters across layouts
2. Mark superclusters if needed
2.1 new roots are added to superclusters
2.2 old roots add some new contents
3. Recalculate the multiplier of the marked superclusters during the first
beginLayout().
so what do you think about this solution?
The CQ bit was checked by wistoch.tencent@gmail.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: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
Hi skobes, As this patch changes a lot, I like to clarify something here. Main changes outside TextAutoSizer: 1. add fingerprint to StyleInheritedData 2. when new text added in LayoutText, call textAutosizingNewTextAdded() to keep a count of new text Main changes inside TextAutoSizer 1. make superclusters across layouts 2. use m_fingerprintsNeedCheckConsistent to cache the fingerprints that need checkConsistent() 3. provide public function textAutosizingNewTextAdded() which determine if fingerprints need to add to m_fingerprintsNeedCheckConsistent 4. checkConsistent() to check if the fingerprints in m_fingerprintsNeedCheckConsistent is consistent Anyway, if there's any problem, let me know. > During the implementation, I found I missed something up there. Actually we > should mark superclusters to recalculate the multiplier not only when new roots > is added, but also when the old roots add new contents. The second one is much > complicated. I've tried many ways to achieve it, and didn't work so well. > Finally I found this way: > Issue: Mark the supercluster when old roots add new contents. > Solution: > 1. add fingerprint as a new member in StyleInheritedData > 2. update the block's StyleInheritedData's fingerprint when record > 3. when new LayoutText created, using StyleInheritedData's fingerprint to > find the supercluster which will count the length of the text > 4. if the length is long enough, mark the supercluster to recalculate > multiplier > > so the whole solution became to this: > 1. Make superclusters across layouts > 2. Mark superclusters if needed > 2.1 new roots are added to superclusters > 2.2 old roots add some new contents > 3. Recalculate the multiplier of the marked superclusters during the first > beginLayout().
Description was changed from
==========
Fix the inconsistent problem while the content of textNodes is changed.
If the text content of one node who share fingerPrint with others changed,
the multipiler of this node might be different from others.
This's the reason of this inconsistent problem.
In this solution, 'm_multiplierOfFingerprint' saved the multipilers of
figerPrints,
if the multipilers have been changed, then this fingerPrints will
be saved at 'm_fingerprintInconsistent'. At the end of layout a task
will be sent out to checkConsistency(). If 'm_fingerprintInconsistent'
isn't empty, it will trigger for relayout.
BUG=612119
==========
to
==========
Fix the inconsistent problem while the content of textNodes is changed.
If the text content of one node who share fingerPrint with others changed,
the multipiler of this node might be different from others.
This's the reason of this inconsistent problem.
The solution:
1. Make superclusters across layouts
2. Mark superclusters if needed
2.1 new roots are added to superclusters
2.2 old roots add some new contents
3. Recalculate the multiplier of the marked superclusters during the first
beginLayout().
BUG=612119
==========
Sorry for the delay. Overall approach seems reasonable but would like pdr@ to have a look too. https://codereview.chromium.org/2299213003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutText.cpp (right): https://codereview.chromium.org/2299213003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutText.cpp:216: if (!oldStyle && textAutosizer && newAddedTextLength > 0 && Why do we check !oldStyle here? https://codereview.chromium.org/2299213003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutText.cpp:219: newStyle.textAutosizingFingerPrint(), newAddedTextLength); Can we just pass the LayoutText* and have TextAutosizer retrieve these? https://codereview.chromium.org/2299213003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/TextAutosizer.cpp (right): https://codereview.chromium.org/2299213003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/TextAutosizer.cpp:962: maxWidth * 4 / result->style()->specifiedFontSize(); This is duplicating logic in TextAutosizer::clusterHasEnoughTextToAutosize, can we factor it out into a common helper? https://codereview.chromium.org/2299213003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/TextAutosizer.cpp:1410: block = const_cast<LayoutBlock*>(root); Remove const from the BlockSet typedef instead of using const_cast. https://codereview.chromium.org/2299213003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/TextAutosizer.h (right): https://codereview.chromium.org/2299213003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/TextAutosizer.h:77: void textAutosizingNewTextAdded(unsigned, unsigned); "textAutosizing" is redundant here, just call it newTextAdded https://codereview.chromium.org/2299213003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/TextAutosizer.h:250: // cluster root. Clusters whose roots share the same fingerprint use the It looks like the map key is now the Fingerprint. https://codereview.chromium.org/2299213003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/style/ComputedStyle.h (right): https://codereview.chromium.org/2299213003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/style/ComputedStyle.h:2842: unsigned textAutosizingFingerPrint() const { capitalize as "textAutosizingFingerprint" (since fingerprint is one word)
Thanks for review, skobes. That is really inspiring:) Looking forward to the review of pdr. https://codereview.chromium.org/2299213003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutText.cpp (right): https://codereview.chromium.org/2299213003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutText.cpp:216: if (!oldStyle && textAutosizer && newAddedTextLength > 0 && On 2016/11/09 03:11:39, skobes wrote: > Why do we check !oldStyle here? We only keep a count of new created/added text. "!oldStyle" represents that this layouttext is new created. https://codereview.chromium.org/2299213003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutText.cpp:219: newStyle.textAutosizingFingerPrint(), newAddedTextLength); On 2016/11/09 03:11:39, skobes wrote: > Can we just pass the LayoutText* and have TextAutosizer retrieve these? yes, passing the LayoutText* is more elegant, and it could work in styleDidChange(), but in setText() we like to count the increase part of the text. So newAddedTextLength is more precise. https://codereview.chromium.org/2299213003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/TextAutosizer.cpp (right): https://codereview.chromium.org/2299213003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/TextAutosizer.cpp:962: maxWidth * 4 / result->style()->specifiedFontSize(); On 2016/11/09 03:11:39, skobes wrote: > This is duplicating logic in TextAutosizer::clusterHasEnoughTextToAutosize, can > we factor it out into a common helper? There is a subtle distinction between them. m_minTextLengthToAutosize is a rough estimate， and in TextAutosizer::clusterHasEnoughTextToAutosize it is more precise. clusterHasEnoughTextToAutosize use the exact specifiedFontSize() of text, but m_minTextLengthToAutosize use widthProvider's font size instead. https://codereview.chromium.org/2299213003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/TextAutosizer.cpp:1410: block = const_cast<LayoutBlock*>(root); On 2016/11/09 03:11:39, skobes wrote: > Remove const from the BlockSet typedef instead of using const_cast. Done. https://codereview.chromium.org/2299213003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/TextAutosizer.h (right): https://codereview.chromium.org/2299213003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/TextAutosizer.h:77: void textAutosizingNewTextAdded(unsigned, unsigned); On 2016/11/09 03:11:39, skobes wrote: > "textAutosizing" is redundant here, just call it newTextAdded Done. https://codereview.chromium.org/2299213003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/TextAutosizer.h:250: // cluster root. Clusters whose roots share the same fingerprint use the On 2016/11/09 03:11:39, skobes wrote: > It looks like the map key is now the Fingerprint. Done. https://codereview.chromium.org/2299213003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/style/ComputedStyle.h (right): https://codereview.chromium.org/2299213003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/style/ComputedStyle.h:2842: unsigned textAutosizingFingerPrint() const { On 2016/11/09 03:11:39, skobes wrote: > capitalize as "textAutosizingFingerprint" (since fingerprint is one word) Done.
Thanks for taking the time to work on this tough area of code. This is one of the harder areas for someone new to the codebase :'( I think your overall design of storing the superclusters and recalculating in prepareForLayout is a good one. I wonder if we could tweak your design a little to require fewer changes in StyleInheritedData/styleDidChange/setText, at the expense of recomputing text lengths in some cases? I'm thinking something along these lines: * If LayoutText's text changes, make sure the TextAutosizer is informed that the nearest block has some changes. We may be able to rely on the nearest block already needing layout. * In TextAutosizer::prepareForLayout, examine all superclusters roots. * If any root has changes (maybe needsLayout(), or alternatively a hashset of dirty roots), ensure the entire supercluster is re-examined for having enough text to autosize. https://codereview.chromium.org/2299213003/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/text-autosizing/js-change-short-text-to-long.html (right): https://codereview.chromium.org/2299213003/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/text-autosizing/js-change-short-text-to-long.html:58: setTimeout("changeText()", 100); When the text autosizer was first written, we only had primitive layouttest technology. For new code, prefer unit tests because they are much faster to run on the bots. Do you mind rewriting these tests in TextAutosizerTest.cpp? I think you can use a similar pattern to the DeviceScaleAdjustmentWithViewport test. https://codereview.chromium.org/2299213003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutText.cpp (right): https://codereview.chromium.org/2299213003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutText.cpp:216: if (!oldStyle && textAutosizer && newAddedTextLength > 0 && I'm worried about the performance impact of this since we need to calculate the text length and do a somewhat cache-unfriendly hashmap lookup in newTextAdded. Do you think we could avoid adding code here? https://codereview.chromium.org/2299213003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/style/StyleInheritedData.h (right): https://codereview.chromium.org/2299213003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/style/StyleInheritedData.h:64: unsigned textAutosizingFingerprint; Echoing Steve's comment a bit, I think we should avoid storing this in StyleInheritedData, if possible, because it isn't really style. textAutosizingMultiplier already violates this a bit :/
On 2016/11/10 at 06:04:09, pdr. wrote: > I'm thinking something along these lines: Ah, realized this wouldn't handle the new node case... Maybe we could just store a hashset of nodes needing to be re-examined when a new text node is added? This would avoid the performance cost of calculating text length in styleDidChange.
Thanks for review, pdr:) The point you worried about is the hardest part I faced... So the uncertain point is how to let Autosize know which root's text is changed. I summed up all the approaches that I could image in a form. I'll send it to you by email, cause we could just add text here:) https://codereview.chromium.org/2299213003/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/text-autosizing/js-change-short-text-to-long.html (right): https://codereview.chromium.org/2299213003/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/text-autosizing/js-change-short-text-to-long.html:58: setTimeout("changeText()", 100); On 2016/11/10 06:04:09, pdr. wrote: > When the text autosizer was first written, we only had primitive layouttest > technology. For new code, prefer unit tests because they are much faster to run > on the bots. Do you mind rewriting these tests in TextAutosizerTest.cpp? I think > you can use a similar pattern to the DeviceScaleAdjustmentWithViewport test. Good to know! Will do... https://codereview.chromium.org/2299213003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutText.cpp (right): https://codereview.chromium.org/2299213003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutText.cpp:216: if (!oldStyle && textAutosizer && newAddedTextLength > 0 && On 2016/11/10 06:04:09, pdr. wrote: > I'm worried about the performance impact of this since we need to calculate the > text length and do a somewhat cache-unfriendly hashmap lookup in newTextAdded. > Do you think we could avoid adding code here? I do have the same worry as yours. So I tried best to avoid the unnecessary calling of newTextAdded(). As to textlength, it's a rough estimate here. So maybe we could change stripWhiteSpace() to length()? https://codereview.chromium.org/2299213003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/style/StyleInheritedData.h (right): https://codereview.chromium.org/2299213003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/style/StyleInheritedData.h:64: unsigned textAutosizingFingerprint; On 2016/11/10 06:04:09, pdr. wrote: > Echoing Steve's comment a bit, I think we should avoid storing this in > StyleInheritedData, if possible, because it isn't really style. > textAutosizingMultiplier already violates this a bit :/ Lol, here is the shortcut to pass things across the layout tree:) You're right, and we should avoid this. Trying to figure out a way to not use this shortcut...
solution trigger cost Bottom up ( LayoutText to root) StyleInheritedData 1. When new text added, find if there is supercluster with the fingerprint cached in StyleInheritedData. 2. Count the new added text, add the fingerprint to a hashset if supercluster has enough text. 3. beginLayout(), check out needsLayout roots related with fingerprints in hashset. New text added 1. Add fingerprint in StyleInheritedData 2. The cost of calculate text length LayoutObjectBitfields 1. Add a new bitfield into LayoutObjectBitfields, like, textchanged. 2. Add nearest supercluster root into hashset 3. beginLayout(), check out the roots in hashset New text added 1. New bitfield in LayoutObjectBitfields Traverse up every time 1. When new text added, traverse up to find supercluster root 2. Add root to hashset 3. beginLayout(), check out roots in hashset New text added 1. traverse up to find supercluster root, every time new text added Top down(root) beginLayout(), check out all supercluster needsLayout roots beginLayout 1. needsLayout==true and no text added ________________________________ cathiechen(陈明琴) From: cathiechen@tencent.com<mailto:cathiechen@tencent.com> Date: 2016-11-11 17:34 To: pdr@chromium.org<mailto:pdr@chromium.org>; jamesxwei(魏晓海)<mailto:jamesxwei@tencent.com>; skobes@chromium.org<mailto:skobes@chromium.org>; skyostil@chromium.org<mailto:skyostil@chromium.org>; wistoch.tencent@gmail.com<mailto:wistoch.tencent@gmail.com> CC: blink-reviews@chromium.org<mailto:blink-reviews@chromium.org>; blink-reviews-layout@chromium.org<mailto:blink-reviews-layout@chromium.org>; chromium-reviews@chromium.org<mailto:chromium-reviews@chromium.org>; eae+blinkwatch@chromium.org<mailto:eae+blinkwatch@chromium.org>; jchaffraix+rendering@chromium.org<mailto:jchaffraix+rendering@chromium.org>; leviw+renderwatch@chromium.org<mailto:leviw+renderwatch@chromium.org>; pdr+renderingwatchlist@chromium.org<mailto:pdr+renderingwatchlist@chromium.org>; szager+layoutwatch@chromium.org<mailto:szager+layoutwatch@chromium.org>; zoltan@webkit.org<mailto:zoltan@webkit.org> Subject: Re: Fix the inconsistent problem while the content of textNodes ischanged (issue 2299213003 by cathiechen@tencent.com)(Internet mail) Thanks for review, pdr:) The point you worried about is the hardest part I faced... So the uncertain point is how to let Autosize know which root's text is changed. I summed up all the approaches that I could image in a form. I'll send it to you by email, cause we could just add text here:) https://codereview.chromium.org/2299213003/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/text-autosizing/js-change-short-text-to-long.html (right): https://codereview.chromium.org/2299213003/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/text-autosizing/js-change-short-text-to-long.html:58: setTimeout("changeText()", 100); On 2016/11/10 06:04:09, pdr. wrote: > When the text autosizer was first written, we only had primitive layouttest > technology. For new code, prefer unit tests because they are much faster to run > on the bots. Do you mind rewriting these tests in TextAutosizerTest.cpp? I think > you can use a similar pattern to the DeviceScaleAdjustmentWithViewport test. Good to know! Will do... https://codereview.chromium.org/2299213003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutText.cpp (right): https://codereview.chromium.org/2299213003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutText.cpp:216: if (!oldStyle && textAutosizer && newAddedTextLength > 0 && On 2016/11/10 06:04:09, pdr. wrote: > I'm worried about the performance impact of this since we need to calculate the > text length and do a somewhat cache-unfriendly hashmap lookup in newTextAdded. > Do you think we could avoid adding code here? I do have the same worry as yours. So I tried best to avoid the unnecessary calling of newTextAdded(). As to textlength, it's a rough estimate here. So maybe we could change stripWhiteSpace() to length()? https://codereview.chromium.org/2299213003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/style/StyleInheritedData.h (right): https://codereview.chromium.org/2299213003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/style/StyleInheritedData.h:64: unsigned textAutosizingFingerprint; On 2016/11/10 06:04:09, pdr. wrote: > Echoing Steve's comment a bit, I think we should avoid storing this in > StyleInheritedData, if possible, because it isn't really style. > textAutosizingMultiplier already violates this a bit :/ Lol, here is the shortcut to pass things across the layout tree:) You're right, and we should avoid this. Trying to figure out a way to not use this shortcut... https://codereview.chromium.org/2299213003/ -- You received this message because you are subscribed to the Google Groups "Blink Reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to blink-reviews+unsubscribe@chromium.org.
solution trigger cost Bottom up ( LayoutText to root) StyleInheritedData 1. When new text added, find if there is supercluster with the fingerprint cached in StyleInheritedData. 2. Count the new added text, add the fingerprint to a hashset if supercluster has enough text. 3. beginLayout(), check out needsLayout roots related with fingerprints in hashset. New text added 1. Add fingerprint in StyleInheritedData 2. The cost of calculate text length LayoutObjectBitfields 1. Add a new bitfield into LayoutObjectBitfields, like, textchanged. 2. Add nearest supercluster root into hashset 3. beginLayout(), check out the roots in hashset New text added 1. New bitfield in LayoutObjectBitfields Traverse up every time 1. When new text added, traverse up to find supercluster root 2. Add root to hashset 3. beginLayout(), check out roots in hashset New text added 1. traverse up to find supercluster root, every time new text added Top down(root) beginLayout(), check out all supercluster needsLayout roots beginLayout 1. needsLayout==true and no text added ________________________________ cathiechen(陈明琴) From: cathiechen@tencent.com<mailto:cathiechen@tencent.com> Date: 2016-11-11 17:34 To: pdr@chromium.org<mailto:pdr@chromium.org>; jamesxwei(魏晓海)<mailto:jamesxwei@tencent.com>; skobes@chromium.org<mailto:skobes@chromium.org>; skyostil@chromium.org<mailto:skyostil@chromium.org>; wistoch.tencent@gmail.com<mailto:wistoch.tencent@gmail.com> CC: blink-reviews@chromium.org<mailto:blink-reviews@chromium.org>; blink-reviews-layout@chromium.org<mailto:blink-reviews-layout@chromium.org>; chromium-reviews@chromium.org<mailto:chromium-reviews@chromium.org>; eae+blinkwatch@chromium.org<mailto:eae+blinkwatch@chromium.org>; jchaffraix+rendering@chromium.org<mailto:jchaffraix+rendering@chromium.org>; leviw+renderwatch@chromium.org<mailto:leviw+renderwatch@chromium.org>; pdr+renderingwatchlist@chromium.org<mailto:pdr+renderingwatchlist@chromium.org>; szager+layoutwatch@chromium.org<mailto:szager+layoutwatch@chromium.org>; zoltan@webkit.org<mailto:zoltan@webkit.org> Subject: Re: Fix the inconsistent problem while the content of textNodes ischanged (issue 2299213003 by cathiechen@tencent.com)(Internet mail) Thanks for review, pdr:) The point you worried about is the hardest part I faced... So the uncertain point is how to let Autosize know which root's text is changed. I summed up all the approaches that I could image in a form. I'll send it to you by email, cause we could just add text here:) https://codereview.chromium.org/2299213003/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/text-autosizing/js-change-short-text-to-long.html (right): https://codereview.chromium.org/2299213003/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/text-autosizing/js-change-short-text-to-long.html:58: setTimeout("changeText()", 100); On 2016/11/10 06:04:09, pdr. wrote: > When the text autosizer was first written, we only had primitive layouttest > technology. For new code, prefer unit tests because they are much faster to run > on the bots. Do you mind rewriting these tests in TextAutosizerTest.cpp? I think > you can use a similar pattern to the DeviceScaleAdjustmentWithViewport test. Good to know! Will do... https://codereview.chromium.org/2299213003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutText.cpp (right): https://codereview.chromium.org/2299213003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutText.cpp:216: if (!oldStyle && textAutosizer && newAddedTextLength > 0 && On 2016/11/10 06:04:09, pdr. wrote: > I'm worried about the performance impact of this since we need to calculate the > text length and do a somewhat cache-unfriendly hashmap lookup in newTextAdded. > Do you think we could avoid adding code here? I do have the same worry as yours. So I tried best to avoid the unnecessary calling of newTextAdded(). As to textlength, it's a rough estimate here. So maybe we could change stripWhiteSpace() to length()? https://codereview.chromium.org/2299213003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/style/StyleInheritedData.h (right): https://codereview.chromium.org/2299213003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/style/StyleInheritedData.h:64: unsigned textAutosizingFingerprint; On 2016/11/10 06:04:09, pdr. wrote: > Echoing Steve's comment a bit, I think we should avoid storing this in > StyleInheritedData, if possible, because it isn't really style. > textAutosizingMultiplier already violates this a bit :/ Lol, here is the shortcut to pass things across the layout tree:) You're right, and we should avoid this. Trying to figure out a way to not use this shortcut... https://codereview.chromium.org/2299213003/ -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2016/11/11 09:34:13, cathiechentx wrote: > Thanks for review, pdr:) The point you worried about is the hardest part I > faced... > > So the uncertain point is how to let Autosize know which root's text is changed. > I summed up all the approaches that I could image in a form. I'll send it to you > by email, cause we could just add text here:) Did you get it? Do we have any test tool to test the performance of these approaches? Or is there any better way to get this done?
https://docs.google.com/document/d/14WpGRZd4UTGAyOVW9dvIG7Rn5tiY9JtfK2ochHuRv... Just found an easy way to share document;) I think maybe LayoutObjectBitfields is a good one...
In writing a response, I tried creating a proof-of-concept and found my suggested approach will not work in general. One difficulty is in your testcase where we don't create superclusters at all, so saving them across frames is not sufficient. Sadly, I think this fix will require a larger redesign than I had expected :( I wonder if it would be best to fix some smaller bugs in this area (either text autosizing or layout) and return to this difficult bug later?
On 2016/11/15 04:00:58, pdr. wrote: > In writing a response, I tried creating a proof-of-concept and found my > suggested approach will not work in general. One difficulty is in your testcase > where we don't create superclusters at all, so saving them across frames is not > sufficient. Sadly, I think this fix will require a larger redesign than I had > expected :( > > I wonder if it would be best to fix some smaller bugs in this area (either text > autosizing or layout) and return to this difficult bug later? yeah, in most case new added nodes are the child node of superclusters' roots which mentioned in 2.2. See the solution below. So the hardest part of this solution is 2.2. Would you mind to check 'EverHadLayout' solution in the below doucument. I think it could resolve this issue. https://docs.google.com/document/d/14WpGRZd4UTGAyOVW9dvIG7Rn5tiY9JtfK2ochHuRv... The solution: 1. Make superclusters across layouts 2. Mark superclusters if needed 2.1 new roots are added to superclusters 2.2 old roots add some new contents 3. Recalculate the multiplier of the marked superclusters during the first beginLayout().
On 2016/11/15 at 04:30:23, cathiechen wrote: > On 2016/11/15 04:00:58, pdr. wrote: > > In writing a response, I tried creating a proof-of-concept and found my > > suggested approach will not work in general. One difficulty is in your testcase > > where we don't create superclusters at all, so saving them across frames is not > > sufficient. Sadly, I think this fix will require a larger redesign than I had > > expected :( > > > > I wonder if it would be best to fix some smaller bugs in this area (either text > > autosizing or layout) and return to this difficult bug later? > > yeah, in most case new added nodes are the child node of superclusters' roots which mentioned in 2.2. See the solution below. So the hardest part of this solution is 2.2. > Would you mind to check 'EverHadLayout' solution in the below doucument. I think it could resolve this issue. > https://docs.google.com/document/d/14WpGRZd4UTGAyOVW9dvIG7Rn5tiY9JtfK2ochHuRv... > > The solution: > 1. Make superclusters across layouts > 2. Mark superclusters if needed > 2.1 new roots are added to superclusters > 2.2 old roots add some new contents > 3. Recalculate the multiplier of the marked superclusters during the first beginLayout(). Question to make sure I understand your patch: you've modified Supercluster so that the amount of text is tracked in m_containingTextlength. To ensure m_containingTextlength is always correct, all ancestors (not just the nearest Supercluster) need to be updated when text changes. Is that correct? I think your solution works in the cases where existing superclusters exist and new text is added, such as the following: <div><span><div>[lots of text]</div></span></div> <div><span><div>[lots of text]</div></span></div> <div><span><div>[a little text]</div></span></div> <div><span><div>[lots of text]</div></span></div> During loading the following is appended: <div><span><div>[a And then this is appended: little text]</div></span></div> In this case, you will have existing superclusters and can ensure the new content matches up to the existing superclusters and all text will have the same size. I think this part of your patch will work. The case I am worried about is when there are no existing roots/clusters/superclusters: <div><span><div>[only a little text]</div></span></div> <div><span><div>[only a little text]</div></span></div> During loading the following is appended: <div><span><div>[lots of text]</div></span></div> In this case, "little text" did not create a supercluster. "lots of text" creates a supercluster and the text is very big compared to "little text". I don't think our current designs can solve this. Do you think they can?
> Question to make sure I understand your patch: you've modified Supercluster so > that the amount of text is tracked in m_containingTextlength. To ensure > m_containingTextlength is always correct, all ancestors (not just the nearest > Supercluster) need to be updated when text changes. Is that correct? > > > I think your solution works in the cases where existing superclusters exist and > new text is added, such as the following: > <div><span><div>[lots of text]</div></span></div> > <div><span><div>[lots of text]</div></span></div> > <div><span><div>[a little text]</div></span></div> > <div><span><div>[lots of text]</div></span></div> > During loading the following is appended: > <div><span><div>[a > And then this is appended: > little text]</div></span></div> > > In this case, you will have existing superclusters and can ensure the new > content matches up to the existing superclusters and all text will have the same > size. I think this part of your patch will work. > > The case I am worried about is when there are no existing > roots/clusters/superclusters: > <div><span><div>[only a little text]</div></span></div> > <div><span><div>[only a little text]</div></span></div> > During loading the following is appended: > <div><span><div>[lots of text]</div></span></div> > > In this case, "little text" did not create a supercluster. "lots of text" > creates a supercluster and the text is very big compared to "little text". I > don't think our current designs can solve this. Do you think they can? I'm sorry, pdr. I realized I didn't fully understand your meaning before. Thanks for your patient explanation. Things about the old patch: 1. Actually I didn't update all ancestors, and I just update the nearest Supercluster's m_containingTextlength when text changes. Then add this Supercluster to a hashset if m_containingTextlength is big enough. (ps: m_containingTextlength is abolished in new patch) 2. Yes, the old design couldn't solve it if the superclusters didn't be created. (The new patch could;)) I think the new patch is better than the old one. Things about the new patch: 1.record: Using everHadLayout() in m_bitfields to determine if the node is new. When the node is new, traversal up the layout tree to find the nearest supercluser, add this supercluser to hashset. The descendants of this node do nothing is their parent is new. All nodes should be checked, except the inline nodes. 2.beginLayout: check supterclusters in hashset. 3.endLayout: clear hashset. What if the supercluster hasn't be created? Create supercluster first, then add the supercluster to hashset. Best case: Add subtree to the dom tree every time. Worst case: Add leaves to the dom tree every time. Finally, I like to say it's glad to discuss this issue with you, and I totally understand your concerns. Maybe I could fixed a smaller bug next time;)
(Sorry for the slow review, got caught up with some other work. I won't be able to review this until tomorrow :/)
On 2016/11/17 22:23:36, pdr. wrote: > (Sorry for the slow review, got caught up with some other work. I won't be able > to review this until tomorrow :/) Got it. It's OK. Take your time:)
https://codereview.chromium.org/2299213003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutText.cpp (right): https://codereview.chromium.org/2299213003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutText.cpp:215: if (!oldStyle && textAutosizer) This approach of calling newNodeAdded for each node could be a little slow because each node independently finds the nearest cluster. I wonder if we could simplify this by tracking a set of blocks in the autosizer (HashSet<LayoutBlock*> m_blocksWithNewText), and adding the nearest block for each text node? In prepareForLayout, we could then do the expensive nearest-cluster lookup for each of the blocks. https://codereview.chromium.org/2299213003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/TextAutosizer.cpp (right): https://codereview.chromium.org/2299213003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/TextAutosizer.cpp:314: newNodeAdded(block); Are these calls (newNodeAdded and needCheckConsistent) needed? Would the new newNodeAdded calls in LayoutText handle these cases already? https://codereview.chromium.org/2299213003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/TextAutosizer.cpp:348: if (block->isLayoutView()) Is this isLayoutView check needed? https://codereview.chromium.org/2299213003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/TextAutosizer.cpp:575: m_fingerprintsNeedCheckConsistent.add(fingerprint); Instead of storing a set of fingerprints, could we just store raw pointers to the superclusters (HashSet<Supercluster*> m_superclustersWithNewText)? The consistency check could then just iterate over the superclusters with: for (Supercluster* supercluster : m_superclustersWithNewText) { ... } https://codereview.chromium.org/2299213003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/TextAutosizerTest.cpp (right): https://codereview.chromium.org/2299213003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/TextAutosizerTest.cpp:507: TEST_F(TextAutosizerTest, ChangingSuperClusterText) { Can you add a second test where shortText comes first and longText comes second? https://codereview.chromium.org/2299213003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/TextAutosizerTest.cpp:521: Can you check the computed font size here for both shortText and longText? Something like: EXPECT_FLOAT_EQ(16.f, longText->layoutObject()->style()->computedFontSize()); EXPECT_FLOAT_EQ(16.f, shortText->layoutObject()->style()->computedFontSize()); Similarly, as the last step in the test, can you assert the actual pixel sizes? https://codereview.chromium.org/2299213003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/TextAutosizerTest.cpp:522: LayoutObject* longText = Nit: these tests have some compile errors. I think you may have switched this from Element*.
https://codereview.chromium.org/2299213003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutText.cpp (right): https://codereview.chromium.org/2299213003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutText.cpp:215: if (!oldStyle && textAutosizer) On 2016/11/20 06:38:13, pdr. wrote: > This approach of calling newNodeAdded for each node could be a little slow > because each node independently finds the nearest cluster. I wonder if we could > simplify this by tracking a set of blocks in the autosizer > (HashSet<LayoutBlock*> m_blocksWithNewText), and adding the nearest block for > each text node? In prepareForLayout, we could then do the expensive > nearest-cluster lookup for each of the blocks. Actually this approach isn't as slow as it looks like. It will find the nearest-clusters for all new nodes(block nodes and text nodes). So if a node's parent is new(!everHadLayout()), there's no need for this node to travel up the tree to find nearest-cluster, cause its parent already found it and added it to the hashset. If we add a subtree to the dom tree, the costs are: the root of subtree finding nearest-cluster, adding it to hashset and all descendants checking if parent are new. https://codereview.chromium.org/2299213003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/TextAutosizer.cpp (right): https://codereview.chromium.org/2299213003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/TextAutosizer.cpp:314: newNodeAdded(block); On 2016/11/20 06:38:13, pdr. wrote: > Are these calls (newNodeAdded and needCheckConsistent) needed? Would the new > newNodeAdded calls in LayoutText handle these cases already? Yes, it's necessary. Here is to find the nearest-cluster of block, so that block's children don't have to travel up the tree. https://codereview.chromium.org/2299213003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/TextAutosizer.cpp:321: needCheckConsistent(block); Add block to hashset. So block's children don't have to travel up the tree to find nearest-cluster. https://codereview.chromium.org/2299213003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/TextAutosizer.cpp:348: if (block->isLayoutView()) On 2016/11/20 06:38:13, pdr. wrote: > Is this isLayoutView check needed? Yes, it is necessary. CheckConsistent might change needslayout of objects which are outside of block, if bock isn't LayoutView. In that case, needslayout checking after layout won't be passed. https://codereview.chromium.org/2299213003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/TextAutosizer.cpp:531: if (!parent || !parent->everHadLayout()) The descendants of a new node will return from here. https://codereview.chromium.org/2299213003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/TextAutosizer.cpp:575: m_fingerprintsNeedCheckConsistent.add(fingerprint); On 2016/11/20 06:38:13, pdr. wrote: > Instead of storing a set of fingerprints, could we just store raw pointers to > the superclusters (HashSet<Supercluster*> m_superclustersWithNewText)? > The consistency check could then just iterate over the superclusters with: > for (Supercluster* supercluster : m_superclustersWithNewText) { > ... > } Done. https://codereview.chromium.org/2299213003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/TextAutosizerTest.cpp (right): https://codereview.chromium.org/2299213003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/TextAutosizerTest.cpp:507: TEST_F(TextAutosizerTest, ChangingSuperClusterText) { On 2016/11/20 06:38:13, pdr. wrote: > Can you add a second test where shortText comes first and longText comes second? Done. https://codereview.chromium.org/2299213003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/TextAutosizerTest.cpp:521: On 2016/11/20 06:38:13, pdr. wrote: > Can you check the computed font size here for both shortText and longText? > Something like: > EXPECT_FLOAT_EQ(16.f, longText->layoutObject()->style()->computedFontSize()); > EXPECT_FLOAT_EQ(16.f, shortText->layoutObject()->style()->computedFontSize()); > > Similarly, as the last step in the test, can you assert the actual pixel sizes? Done. https://codereview.chromium.org/2299213003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/TextAutosizerTest.cpp:522: LayoutObject* longText = On 2016/11/20 06:38:13, pdr. wrote: > Nit: these tests have some compile errors. I think you may have switched this > from Element*. Done.
I think this looks good at a high level. There are some smaller layout-related issues. @skobes, would you be willing to finish up this review?
https://codereview.chromium.org/2299213003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/TextAutosizer.cpp (right): https://codereview.chromium.org/2299213003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/TextAutosizer.cpp:543: block = parent->containingBlock(); containingBlock() is a bit different from "nearest ancestor that is a LayoutBlock". You probably just want to walk up the tree calling parent() at each step. https://codereview.chromium.org/2299213003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/TextAutosizer.cpp:548: // Find the nearest block which choud be a cluster root typo: chould -> could https://codereview.chromium.org/2299213003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/TextAutosizer.cpp:897: // Cause cluster is changed, m_supercluster's "NotEnoughText" is not reliable I had trouble understanding isParentClusterReliable. Doesn't checkConsistent ensure that m_hasEnoughTextToAutosize is set correctly by the time clusterMultiplier is called? https://codereview.chromium.org/2299213003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/TextAutosizer.cpp:941: if (skipLayoutedNodes && !root->normalChildNeedsLayout()) What's the reason for skipping nodes that don't need layout here? https://codereview.chromium.org/2299213003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/TextAutosizer.cpp:1395: void setAllTextInsideContainerNeedsLayout(LayoutBlock* container) { Can this be combined with setAllTextNeedsLayout? The container param can default to nullptr. https://codereview.chromium.org/2299213003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/TextAutosizer.cpp:1427: if (root->normalChildNeedsLayout()) Should this be root->needsLayout()? I think any layout invalidation bit is sufficient. https://codereview.chromium.org/2299213003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/TextAutosizer.h (right): https://codereview.chromium.org/2299213003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/TextAutosizer.h:76: void newNodeAdded(LayoutObject*); This method is confusing from an API standpoint. It's called from record() and LayoutText::setText(), so it's not indicating that a node has been added to the DOM as the name seems to imply. Can we perhaps fold it into record()? It seems like it's just adding new behavior to style recalc which we already have record() for. Since we only care about LayoutText and LayoutBlock, I suggest an overload: void record(LayoutBlock*); void record(LayoutText*); https://codereview.chromium.org/2299213003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/TextAutosizer.h:229: Supercluster* getSupercluster(LayoutBlock*); It looks like this method creates the Supercluster if it doesn't exist (and if the block should have one). Can we name it createSuperclusterIfNeeded? https://codereview.chromium.org/2299213003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/TextAutosizer.h:230: Supercluster* getSuperclusterByFingerprint(const Fingerprint); This doesn't seem to be called anywhere. Can it be removed? https://codereview.chromium.org/2299213003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/TextAutosizer.h:241: // Make superclusters across layouts. I'd suggest rearranging the comments as follows: Put "// Clusters are created and destroyed during layout" above m_clusterStack, not here. Put "Clusters whose roots share the same fingerprint use the same multiplier" at the end of the comments above "class FingerprintMapper {". Above m_superclusters, put "// Maps fingerprints to superclusters. Superclusters persist across layouts." https://codereview.chromium.org/2299213003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/TextAutosizer.h:333: void checkConsistent(); Let's call this checkSuperclusterConsistency and add a comment "// Must be called at the start of layout." https://codereview.chromium.org/2299213003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/TextAutosizer.h:335: void needCheckConsistent(LayoutBlock*); Let's call this "markSuperclusterForConsistencyCheck". https://codereview.chromium.org/2299213003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/TextAutosizer.h:348: HashSet<Supercluster*> m_superclustersWithNewText; Let's call this m_potentiallyInconsistentSuperclusters and add a comment like "// Superclusters that need to be checked for consistency at the start of the next layout." (Correct me if I've misunderstood anything here.)
Hey guys, Thanks very very much for your time and patient:) There's one more thing to clarify: As skobes mentioned, checkSuperclusterConsistency() is a little confusing here. checkSuperclusterConsistency() is to solve the nested supercluster issues, like "ChangingInheritedClusterTextInsideSuperCluster" in TextAutoSizerTest.cpp. I removed checkSuperclusterConsistency() in this patch. And add the nearest non-inheritance supercluster to m_potentiallyInconsistentSuperclusters, not just the nearest cluster. This way could fix the nested supercluster issues too. https://codereview.chromium.org/2299213003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/TextAutosizer.cpp (right): https://codereview.chromium.org/2299213003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/TextAutosizer.cpp:543: block = parent->containingBlock(); On 2016/12/06 01:00:39, skobes wrote: > containingBlock() is a bit different from "nearest ancestor that is a > LayoutBlock". You probably just want to walk up the tree calling parent() at > each step. Done. https://codereview.chromium.org/2299213003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/TextAutosizer.cpp:548: // Find the nearest block which choud be a cluster root On 2016/12/06 01:00:39, skobes wrote: > typo: chould -> could Done. https://codereview.chromium.org/2299213003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/TextAutosizer.cpp:897: // Cause cluster is changed, m_supercluster's "NotEnoughText" is not reliable On 2016/12/06 01:00:39, skobes wrote: > I had trouble understanding isParentClusterReliable. Doesn't checkConsistent > ensure that m_hasEnoughTextToAutosize is set correctly by the time > clusterMultiplier is called? Actually, only the nearest supercluster's m_hasEnoughTextToAutosize is correct. If this supercluster inherits multiplier from its parent cluster, this supercluster won't recalculate its own multiplier. And if the parent cluster is a supercluster, parent's old multiplier will be applied. Ps: Test case "ChangingInheritedClusterTextInsideSuperCluster" will test this issue. Maybe "isParentClusterReliable" is not a good solution to this issue. I think it's more reasonable to make the nearest supercluster with non-inheritance multiplier checkConsistent. "isParentClusterReliable" will be removed and "markNearestSuperclusterForConsistencyCheck" will find out a reasonable supercluster. https://codereview.chromium.org/2299213003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/TextAutosizer.cpp:941: if (skipLayoutedNodes && !root->normalChildNeedsLayout()) On 2016/12/06 01:00:39, skobes wrote: > What's the reason for skipping nodes that don't need layout here? In the case of called by "checkSuperclusterConsistency()", if the root doesn't need layout, this means that the text count in this root doesn't change. So this root won't affect the result of "superclusterHasEnoughTextToAutosize()". And it is more effective to skip this root. https://codereview.chromium.org/2299213003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/TextAutosizer.cpp:1395: void setAllTextInsideContainerNeedsLayout(LayoutBlock* container) { On 2016/12/06 01:00:39, skobes wrote: > Can this be combined with setAllTextNeedsLayout? The container param can > default to nullptr. Done. https://codereview.chromium.org/2299213003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/TextAutosizer.cpp:1427: if (root->normalChildNeedsLayout()) On 2016/12/06 01:00:39, skobes wrote: > Should this be root->needsLayout()? I think any layout invalidation bit is > sufficient. Yes, you're right. If no cluster inside this supercluster, root->needsLayout() is enough. Since I changed nearest cluster to nearest non-inheritance supercluster, so we should use !root->everHadLayout() here. And I add everHadLayout() check in setAllTextNeedsLayout too. https://codereview.chromium.org/2299213003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/TextAutosizer.h (right): https://codereview.chromium.org/2299213003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/TextAutosizer.h:76: void newNodeAdded(LayoutObject*); On 2016/12/06 01:00:39, skobes wrote: > This method is confusing from an API standpoint. It's called from record() and > LayoutText::setText(), so it's not indicating that a node has been added to the > DOM as the name seems to imply. > > Can we perhaps fold it into record()? It seems like it's just adding new > behavior to style recalc which we already have record() for. > > Since we only care about LayoutText and LayoutBlock, I suggest an overload: > > void record(LayoutBlock*); > void record(LayoutText*); Done. https://codereview.chromium.org/2299213003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/TextAutosizer.h:229: Supercluster* getSupercluster(LayoutBlock*); On 2016/12/06 01:00:39, skobes wrote: > It looks like this method creates the Supercluster if it doesn't exist (and if > the block should have one). Can we name it createSuperclusterIfNeeded? Done. https://codereview.chromium.org/2299213003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/TextAutosizer.h:230: Supercluster* getSuperclusterByFingerprint(const Fingerprint); On 2016/12/06 01:00:39, skobes wrote: > This doesn't seem to be called anywhere. Can it be removed? Done. https://codereview.chromium.org/2299213003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/TextAutosizer.h:241: // Make superclusters across layouts. On 2016/12/06 01:00:39, skobes wrote: > I'd suggest rearranging the comments as follows: > > Put "// Clusters are created and destroyed during layout" above m_clusterStack, > not here. > > Put "Clusters whose roots share the same fingerprint use the same multiplier" at > the end of the comments above "class FingerprintMapper {". > > Above m_superclusters, put "// Maps fingerprints to superclusters. Superclusters > persist across layouts." Done. https://codereview.chromium.org/2299213003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/TextAutosizer.h:333: void checkConsistent(); On 2016/12/06 01:00:39, skobes wrote: > Let's call this checkSuperclusterConsistency and add a comment "// Must be > called at the start of layout." Done. https://codereview.chromium.org/2299213003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/TextAutosizer.h:335: void needCheckConsistent(LayoutBlock*); On 2016/12/06 01:00:39, skobes wrote: > Let's call this "markSuperclusterForConsistencyCheck". Done. We only mark the nearest non-inheritance Supercluster here. https://codereview.chromium.org/2299213003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/TextAutosizer.h:348: HashSet<Supercluster*> m_superclustersWithNewText; On 2016/12/06 01:00:39, skobes wrote: > Let's call this m_potentiallyInconsistentSuperclusters and add a comment like > "// Superclusters that need to be checked for consistency at the start of the > next layout." > > (Correct me if I've misunderstood anything here.) Done. https://codereview.chromium.org/2299213003/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/TextAutosizer.cpp (right): https://codereview.chromium.org/2299213003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/TextAutosizer.cpp:555: supercluster->m_inheritParentMultiplier == NotInheritMultiplier) { Add the nearest non_inheritance supercluster to m_potentiallyInconsistentSuperclusters https://codereview.chromium.org/2299213003/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/TextAutosizer.h (right): https://codereview.chromium.org/2299213003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/TextAutosizer.h:170: InheritParentMultiplier m_inheritParentMultiplier; m_inheritParentMultiplier is used to representative if this supercluster inherited multiplier from parent cluster.
Thanks, just a few nits left. https://codereview.chromium.org/2299213003/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/TextAutosizer.cpp (right): https://codereview.chromium.org/2299213003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/TextAutosizer.cpp:544: Supercluster* lastSupercluser = nullptr; typo: lastSupercluster https://codereview.chromium.org/2299213003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/TextAutosizer.cpp:551: // If supercluster hasn't be created yet, create one. "hasn't been" https://codereview.chromium.org/2299213003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/TextAutosizer.cpp:1400: superclusterHasEnoughTextToAutosize(supercluster, widthProvider, true); Use the return value in the next line: if (superclusterHasEnoughTextToAutosize(...) == HasEnoughText) { https://codereview.chromium.org/2299213003/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/TextAutosizer.h (right): https://codereview.chromium.org/2299213003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/TextAutosizer.h:148: NotInheritMultiplier DontInheritMultiplier
https://codereview.chromium.org/2299213003/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/TextAutosizer.cpp (right): https://codereview.chromium.org/2299213003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/TextAutosizer.cpp:544: Supercluster* lastSupercluser = nullptr; On 2016/12/14 21:40:36, skobes wrote: > typo: lastSupercluster Done. https://codereview.chromium.org/2299213003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/TextAutosizer.cpp:551: // If supercluster hasn't be created yet, create one. On 2016/12/14 21:40:36, skobes wrote: > "hasn't been" Done. https://codereview.chromium.org/2299213003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/TextAutosizer.cpp:1400: superclusterHasEnoughTextToAutosize(supercluster, widthProvider, true); On 2016/12/14 21:40:36, skobes wrote: > Use the return value in the next line: > > if (superclusterHasEnoughTextToAutosize(...) == HasEnoughText) { Done. https://codereview.chromium.org/2299213003/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/TextAutosizer.h (right): https://codereview.chromium.org/2299213003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/TextAutosizer.h:148: NotInheritMultiplier On 2016/12/14 21:40:36, skobes wrote: > DontInheritMultiplier Done.
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...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by wistoch.tencent@gmail.com
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: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
On 2016/12/15 06:46:12, commit-bot: I haz the power wrote: > Try jobs failed on following builders: > linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, > https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) Need to remove supercluster from m_potentiallyInconsistentSuperclusters when this supercluster is destoryed. Move m_potentiallyInconsistentSuperclusters to class FingerprintMapper, cause only in FingerprintMapper::remove() where we could know if one supercluster is destoryed.
still lgtm
The CQ bit was checked by wistoch.tencent@gmail.com
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
Failed to apply patch for third_party/WebKit/Source/core/layout/TextAutosizer.h:
While running git apply --index -p1;
error: patch failed: third_party/WebKit/Source/core/layout/TextAutosizer.h:49
error: third_party/WebKit/Source/core/layout/TextAutosizer.h: patch does not
apply
Patch: third_party/WebKit/Source/core/layout/TextAutosizer.h
Index: third_party/WebKit/Source/core/layout/TextAutosizer.h
diff --git a/third_party/WebKit/Source/core/layout/TextAutosizer.h
b/third_party/WebKit/Source/core/layout/TextAutosizer.h
index
35d93fcbc9c3c103440f25cbe88a7d84388262f8..1f28b217789d64fde5c59ea0d1eb5c3bc70f1129
100644
--- a/third_party/WebKit/Source/core/layout/TextAutosizer.h
+++ b/third_party/WebKit/Source/core/layout/TextAutosizer.h
@@ -49,6 +49,7 @@ class LayoutListItem;
class LayoutListMarker;
class LayoutObject;
class LayoutTable;
+class LayoutText;
class LocalFrame;
class Page;
class SubtreeLayoutScope;
@@ -69,8 +70,9 @@ class CORE_EXPORT TextAutosizer final
void updatePageInfoInAllFrames();
void updatePageInfo();
- void record(const LayoutBlock*);
- void destroy(const LayoutBlock*);
+ void record(LayoutBlock*);
+ void record(LayoutText*);
+ void destroy(LayoutBlock*);
bool pageNeedsAutosizing() const;
@@ -107,7 +109,8 @@ class CORE_EXPORT TextAutosizer final
};
private:
- typedef HashSet<const LayoutBlock*> BlockSet;
+ typedef HashSet<LayoutBlock*> BlockSet;
+ typedef HashSet<const LayoutBlock*> ConstBlockSet;
enum HasEnoughTextToAutosize {
UnknownAmountOfText,
@@ -139,6 +142,12 @@ class CORE_EXPORT TextAutosizer final
SUPPRESSING = 1 << 4
};
+ enum InheritParentMultiplier {
+ Unknown,
+ InheritMultiplier,
+ DontInheritMultiplier
+ };
+
typedef unsigned BlockFlags;
// A supercluster represents autosizing information about a set of two or
@@ -152,11 +161,13 @@ class CORE_EXPORT TextAutosizer final
explicit Supercluster(const BlockSet* roots)
: m_roots(roots),
m_hasEnoughTextToAutosize(UnknownAmountOfText),
- m_multiplier(0) {}
+ m_multiplier(0),
+ m_inheritParentMultiplier(Unknown) {}
- const BlockSet* const m_roots;
+ const BlockSet* m_roots;
HasEnoughTextToAutosize m_hasEnoughTextToAutosize;
float m_multiplier;
+ InheritParentMultiplier m_inheritParentMultiplier;
};
struct Cluster {
@@ -210,30 +221,40 @@ class CORE_EXPORT TextAutosizer final
"sizeof(FingerprintSourceData) must be a multiple of UChar");
typedef unsigned Fingerprint;
- typedef HashMap<Fingerprint, std::unique_ptr<Supercluster>> SuperclusterMap;
typedef Vector<std::unique_ptr<Cluster>> ClusterStack;
// Fingerprints are computed during style recalc, for (some subset of)
// blocks that will become cluster roots.
+ // Clusters whose roots share the same fingerprint use the same multiplier
class FingerprintMapper {
DISALLOW_NEW();
public:
- void add(const LayoutObject*, Fingerprint);
- void addTentativeClusterRoot(const LayoutBlock*, Fingerprint);
+ void add(LayoutObject*, Fingerprint);
+ void addTentativeClusterRoot(LayoutBlock*, Fingerprint);
// Returns true if any BlockSet was modified or freed by the removal.
- bool remove(const LayoutObject*);
+ bool remove(LayoutObject*);
Fingerprint get(const LayoutObject*);
BlockSet* getTentativeClusterRoots(Fingerprint);
+ Supercluster* createSuperclusterIfNeeded(LayoutBlock*);
bool hasFingerprints() const { return !m_fingerprints.isEmpty(); }
+ HashSet<Supercluster*>& getPotentiallyInconsistentSuperclusters() {
+ return m_potentiallyInconsistentSuperclusters;
+ }
private:
typedef HashMap<const LayoutObject*, Fingerprint> FingerprintMap;
typedef HashMap<Fingerprint, std::unique_ptr<BlockSet>>
ReverseFingerprintMap;
+ typedef HashMap<Fingerprint, std::unique_ptr<Supercluster>>
SuperclusterMap;
FingerprintMap m_fingerprints;
ReverseFingerprintMap m_blocksForFingerprint;
+ // Maps fingerprints to superclusters. Superclusters persist across
layouts.
+ SuperclusterMap m_superclusters;
+ // Superclusters that need to be checked for consistency at the start of
the
+ // next layout.
+ HashSet<Supercluster*> m_potentiallyInconsistentSuperclusters;
#if ENABLE(ASSERT)
void assertMapsAreConsistent();
#endif
@@ -270,23 +291,23 @@ class CORE_EXPORT TextAutosizer final
float multiplier = 0);
bool shouldHandleLayout() const;
IntSize windowSize() const;
- void setAllTextNeedsLayout();
+ void setAllTextNeedsLayout(LayoutBlock* container = nullptr);
void resetMultipliers();
- BeginLayoutBehavior prepareForLayout(const LayoutBlock*);
- void prepareClusterStack(const LayoutObject*);
+ BeginLayoutBehavior prepareForLayout(LayoutBlock*);
+ void prepareClusterStack(LayoutObject*);
bool clusterHasEnoughTextToAutosize(
Cluster*,
const LayoutBlock* widthProvider = nullptr);
bool superclusterHasEnoughTextToAutosize(
Supercluster*,
- const LayoutBlock* widthProvider = nullptr);
+ const LayoutBlock* widthProvider = nullptr,
+ bool skipLayoutedNodes = false);
bool clusterWouldHaveEnoughTextToAutosize(
const LayoutBlock* root,
const LayoutBlock* widthProvider = nullptr);
- Fingerprint getFingerprint(const LayoutObject*);
+ Fingerprint getFingerprint(LayoutObject*);
Fingerprint computeFingerprint(const LayoutObject*);
- Cluster* maybeCreateCluster(const LayoutBlock*);
- Supercluster* getSupercluster(const LayoutBlock*);
+ Cluster* maybeCreateCluster(LayoutBlock*);
float clusterMultiplier(Cluster*);
float superclusterMultiplier(Cluster*);
// A cluster's width provider is typically the deepest block containing all
@@ -294,7 +315,7 @@ class CORE_EXPORT TextAutosizer final
// table itself for width.
const LayoutBlock* clusterWidthProvider(const LayoutBlock*) const;
const LayoutBlock* maxClusterWidthProvider(
- const Supercluster*,
+ Supercluster*,
const LayoutBlock* currentRoot) const;
// Typically this returns a block's computed width. In the case of tables
// layout, this width is not yet known so the fixed width is used if it's
@@ -321,19 +342,20 @@ class CORE_EXPORT TextAutosizer final
#ifdef AUTOSIZING_DOM_DEBUG_INFO
void writeClusterDebugInfo(Cluster*);
#endif
+ // Must be called at the start of layout.
+ void checkSuperclusterConsistency();
+ // Mark the nearest non-inheritance supercluser
+ void markSuperclusterForConsistencyCheck(LayoutObject*);
Member<const Document> m_document;
const LayoutBlock* m_firstBlockToBeginLayout;
#if ENABLE(ASSERT)
// Used to ensure we don't compute properties of a block before beginLayout()
// is called on it.
- BlockSet m_blocksThatHaveBegunLayout;
+ ConstBlockSet m_blocksThatHaveBegunLayout;
#endif
- // Clusters are created and destroyed during layout. The map key is the
- // cluster root. Clusters whose roots share the same fingerprint use the
- // same multiplier.
- SuperclusterMap m_superclusters;
+ // Clusters are created and destroyed during layout
ClusterStack m_clusterStack;
FingerprintMapper m_fingerprintMapper;
Vector<RefPtr<ComputedStyle>> m_stylesRetainedDuringLayout;
The CQ bit was checked by cathiechen@tencent.com
The patchset sent to the CQ was uploaded after l-g-t-m from skobes@chromium.org Link to the patchset: https://codereview.chromium.org/2299213003/#ps220001 (title: "git rebase...")
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": 220001, "attempt_start_ts": 1481883290824810,
"parent_rev": "6d38137142d6ec25b03a2b9ce5d594d917cc9a44", "commit_rev":
"f100a4c9933c4f945be4d4ed2270675a83ada87d"}
Message was sent while issue was closed.
Description was changed from
==========
Fix the inconsistent problem while the content of textNodes is changed.
If the text content of one node who share fingerPrint with others changed,
the multipiler of this node might be different from others.
This's the reason of this inconsistent problem.
The solution:
1. Make superclusters across layouts
2. Mark superclusters if needed
2.1 new roots are added to superclusters
2.2 old roots add some new contents
3. Recalculate the multiplier of the marked superclusters during the first
beginLayout().
BUG=612119
==========
to
==========
Fix the inconsistent problem while the content of textNodes is changed.
If the text content of one node who share fingerPrint with others changed,
the multipiler of this node might be different from others.
This's the reason of this inconsistent problem.
The solution:
1. Make superclusters across layouts
2. Mark superclusters if needed
2.1 new roots are added to superclusters
2.2 old roots add some new contents
3. Recalculate the multiplier of the marked superclusters during the first
beginLayout().
BUG=612119
Review-Url: https://codereview.chromium.org/2299213003
==========
Message was sent while issue was closed.
Committed patchset #12 (id:220001)
Message was sent while issue was closed.
Description was changed from
==========
Fix the inconsistent problem while the content of textNodes is changed.
If the text content of one node who share fingerPrint with others changed,
the multipiler of this node might be different from others.
This's the reason of this inconsistent problem.
The solution:
1. Make superclusters across layouts
2. Mark superclusters if needed
2.1 new roots are added to superclusters
2.2 old roots add some new contents
3. Recalculate the multiplier of the marked superclusters during the first
beginLayout().
BUG=612119
Review-Url: https://codereview.chromium.org/2299213003
==========
to
==========
Fix the inconsistent problem while the content of textNodes is changed.
If the text content of one node who share fingerPrint with others changed,
the multipiler of this node might be different from others.
This's the reason of this inconsistent problem.
The solution:
1. Make superclusters across layouts
2. Mark superclusters if needed
2.1 new roots are added to superclusters
2.2 old roots add some new contents
3. Recalculate the multiplier of the marked superclusters during the first
beginLayout().
BUG=612119
Committed: https://crrev.com/bf9901feb0c46cefbd5e7af40fe252a6a92d52cb
Cr-Commit-Position: refs/heads/master@{#439093}
==========
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/bf9901feb0c46cefbd5e7af40fe252a6a92d52cb Cr-Commit-Position: refs/heads/master@{#439093} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
