Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1308693003:
Ensure the text autosizer creates a root cluster when needed (Closed)
|
Created:
5 years, 4 months ago by pdr. Modified:
5 years, 3 months ago CC:
blink-reviews, blink-reviews-rendering, eae+blinkwatch, jchaffraix+rendering, leviw+renderwatch, pdr+renderingwatchlist_chromium.org, szager+layoutwatch_chromium.org, zoltan1 Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Target Ref:
refs/remotes/origin/master Project:
blink Visibility:
Public. |
DescriptionEnsure the text autosizer creates a root cluster when needed
The text autosizer demarcates certain nodes as "clusters" that autosize
using a consistent font multiplier (see http://tinyurl.com/TextAutosizer
for a full description). Clusters can nest forming a tree, and a root
cluster is assumed to be available to all Nodes except the LayoutView.
This patch fixes a bug where no root cluster was created by a LayoutView
which contained LayoutObject children (in this case, a sole pagination
anonymous block) but no Node children. A similar situation can occur
with completely empty documents.
Asserts & comments have been added to clarify the root cluster concept.
BUG=521657
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=201172
Patch Set 1 #
Total comments: 9
Patch Set 2 : Address reviewer comments: cleanup test #Patch Set 3 : Force a layout in the test #
Total comments: 3
Messages
Total messages: 39 (14 generated)
The CQ bit was checked by pdr@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1308693003/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1308693003/1
pdr@chromium.org changed reviewers: + mstensho@opera.com, skobes@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
I'm not familiar with the text autosizer, but this lgtm. https://codereview.chromium.org/1308693003/diff/1/LayoutTests/fast/text-autos... File LayoutTests/fast/text-autosizing/anonymous-block-crash-expected.html (right): https://codereview.chromium.org/1308693003/diff/1/LayoutTests/fast/text-autos... LayoutTests/fast/text-autosizing/anonymous-block-crash-expected.html:1: Test for crbug.com/521657: this test passes if it does not crash. Do we need all this? Couldn't you just make a dumpAsText() test instead, which should generate an empty (or one blank line or whatever) -expected.txt for you? https://codereview.chromium.org/1308693003/diff/1/LayoutTests/fast/text-autos... File LayoutTests/fast/text-autosizing/anonymous-block-crash.html (right): https://codereview.chromium.org/1308693003/diff/1/LayoutTests/fast/text-autos... LayoutTests/fast/text-autosizing/anonymous-block-crash.html:8: document.documentElement.parentNode.removeChild(document.documentElement); How about a document.body.offsetTop; in front of this, to make sure that layout is up-to-date? https://codereview.chromium.org/1308693003/diff/1/Source/core/layout/TextAuto... File Source/core/layout/TextAutosizer.cpp (right): https://codereview.chromium.org/1308693003/diff/1/Source/core/layout/TextAuto... Source/core/layout/TextAutosizer.cpp:156: if (node && !node->hasChildren() && !layoutObject->isLayoutView()) Not really a problem with your change here, but I was wondering why we're checking for children on the DOM side, and not on the layout side. Why not just go "if (!layoutObject->slowFirstChild()) return false;". Then you wouldn't even need the LayoutView exception, because it has an anonymous flowthread child. Or?
https://codereview.chromium.org/1308693003/diff/1/LayoutTests/fast/text-autos... File LayoutTests/fast/text-autosizing/anonymous-block-crash-expected.html (right): https://codereview.chromium.org/1308693003/diff/1/LayoutTests/fast/text-autos... LayoutTests/fast/text-autosizing/anonymous-block-crash-expected.html:1: Test for crbug.com/521657: this test passes if it does not crash. On 2015/08/24 at 10:55:10, mstensho wrote: > Do we need all this? Couldn't you just make a dumpAsText() test instead, which should generate an empty (or one blank line or whatever) -expected.txt for you? Done https://codereview.chromium.org/1308693003/diff/1/LayoutTests/fast/text-autos... File LayoutTests/fast/text-autosizing/anonymous-block-crash.html (right): https://codereview.chromium.org/1308693003/diff/1/LayoutTests/fast/text-autos... LayoutTests/fast/text-autosizing/anonymous-block-crash.html:8: document.documentElement.parentNode.removeChild(document.documentElement); On 2015/08/24 at 10:55:10, mstensho wrote: > How about a document.body.offsetTop; in front of this, to make sure that layout is up-to-date? Done https://codereview.chromium.org/1308693003/diff/1/Source/core/layout/TextAuto... File Source/core/layout/TextAutosizer.cpp (right): https://codereview.chromium.org/1308693003/diff/1/Source/core/layout/TextAuto... Source/core/layout/TextAutosizer.cpp:156: if (node && !node->hasChildren() && !layoutObject->isLayoutView()) On 2015/08/24 at 10:55:10, mstensho wrote: > Not really a problem with your change here, but I was wondering why we're checking for children on the DOM side, and not on the layout side. > > Why not just go "if (!layoutObject->slowFirstChild()) return false;". Then you wouldn't even need the LayoutView exception, because it has an anonymous flowthread child. Or? That's a good question and it's non-obvious. The text autosizer does some tricks to avoid requiring two layouts. This function needs to be callable before we have layoutobject children.
The CQ bit was checked by pdr@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1308693003/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1308693003/40001
lgtm https://codereview.chromium.org/1308693003/diff/1/Source/core/layout/TextAuto... File Source/core/layout/TextAutosizer.cpp (right): https://codereview.chromium.org/1308693003/diff/1/Source/core/layout/TextAuto... Source/core/layout/TextAutosizer.cpp:156: if (node && !node->hasChildren() && !layoutObject->isLayoutView()) On 2015/08/24 10:55:10, mstensho wrote: > Not really a problem with your change here, but I was wondering why we're > checking for children on the DOM side, and not on the layout side. > > Why not just go "if (!layoutObject->slowFirstChild()) return false;". Then you > wouldn't even need the LayoutView exception, because it has an anonymous > flowthread child. Or? The LayoutView is special because it needs to be a cluster root regardless of whether it has children. I suppose there is a question, for blocks other than the LayoutView, of whether layoutObject->slowFirstChild() would be a better check than node->hasChildren(), which I am not certain about. But that's unrelated to this bug. https://codereview.chromium.org/1308693003/diff/40001/LayoutTests/fast/text-a... File LayoutTests/fast/text-autosizing/anonymous-block-crash.html (right): https://codereview.chromium.org/1308693003/diff/40001/LayoutTests/fast/text-a... LayoutTests/fast/text-autosizing/anonymous-block-crash.html:1: Test for crbug.com/521657: This test passes if it does not crash. I think your -expected.txt should have this text in it?
https://codereview.chromium.org/1308693003/diff/1/Source/core/layout/TextAuto... File Source/core/layout/TextAutosizer.cpp (right): https://codereview.chromium.org/1308693003/diff/1/Source/core/layout/TextAuto... Source/core/layout/TextAutosizer.cpp:156: if (node && !node->hasChildren() && !layoutObject->isLayoutView()) On 2015/08/24 17:43:28, pdr wrote: > On 2015/08/24 at 10:55:10, mstensho wrote: > > Not really a problem with your change here, but I was wondering why we're > checking for children on the DOM side, and not on the layout side. > > > > Why not just go "if (!layoutObject->slowFirstChild()) return false;". Then you > wouldn't even need the LayoutView exception, because it has an anonymous > flowthread child. Or? > > That's a good question and it's non-obvious. > > The text autosizer does some tricks to avoid requiring two layouts. This > function needs to be callable before we have layoutobject children. So it's actually called during layout tree building, in other words (since you do have layoutObject, but don't know if its children have been created yet)? I mean, we don't insert layout objects during layout, so that can't be it. https://codereview.chromium.org/1308693003/diff/40001/LayoutTests/fast/text-a... File LayoutTests/fast/text-autosizing/anonymous-block-crash.html (right): https://codereview.chromium.org/1308693003/diff/40001/LayoutTests/fast/text-a... LayoutTests/fast/text-autosizing/anonymous-block-crash.html:1: Test for crbug.com/521657: This test passes if it does not crash. On 2015/08/24 17:47:57, skobes wrote: > I think your -expected.txt should have this text in it? No, because the final result is a blank page. This text node will be gone.
https://codereview.chromium.org/1308693003/diff/1/Source/core/layout/TextAuto... File Source/core/layout/TextAutosizer.cpp (right): https://codereview.chromium.org/1308693003/diff/1/Source/core/layout/TextAuto... Source/core/layout/TextAutosizer.cpp:156: if (node && !node->hasChildren() && !layoutObject->isLayoutView()) On 2015/08/24 17:51:49, mstensho wrote: > So it's actually called during layout tree building, in other words (since you > do have layoutObject, but don't know if its children have been created yet)? I > mean, we don't insert layout objects during layout, so that can't be it. It's called during layout. If I'm reading the bug correctly, this is fixing an edge case where we layout a LayoutView with no children. In that case we still need to create a cluster root otherwise things blow up. https://codereview.chromium.org/1308693003/diff/40001/LayoutTests/fast/text-a... File LayoutTests/fast/text-autosizing/anonymous-block-crash.html (right): https://codereview.chromium.org/1308693003/diff/40001/LayoutTests/fast/text-a... LayoutTests/fast/text-autosizing/anonymous-block-crash.html:1: Test for crbug.com/521657: This test passes if it does not crash. On 2015/08/24 17:51:49, mstensho wrote: > On 2015/08/24 17:47:57, skobes wrote: > > I think your -expected.txt should have this text in it? > > No, because the final result is a blank page. This text node will be gone. Ah, right :)
Are folks okay with the latest patch?
lgtm
The CQ bit was unchecked by pdr@chromium.org
The CQ bit was checked by pdr@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1308693003/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1308693003/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...)
The CQ bit was checked by pdr@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1308693003/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1308693003/40001
The CQ bit was unchecked by commit-bot@chromium.org
No L-G-T-M from a valid reviewer yet. Only full committers are accepted. 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.
The CQ bit was checked by pdr@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1308693003/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1308693003/40001
On 2015/08/25 at 16:00:45, commit-bot wrote: > No L-G-T-M from a valid reviewer yet. Only full committers are accepted. > 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. Interesting. I think we overflowed the superstar committers with both skobes and mstensho.
On 2015/08/25 17:18:12, pdr wrote: > On 2015/08/25 at 16:00:45, commit-bot wrote: > > No L-G-T-M from a valid reviewer yet. Only full committers are accepted. > > 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. > > Interesting. I think we overflowed the superstar committers with both skobes and > mstensho. http://crbug.com/524505, supposedly fixed now
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
On 2015/08/25 18:18:32, commit-bot: I haz the power wrote: > Try jobs failed on following builders: > mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...) ^ http://crbug.com/524248, ongoing
On 2015/08/25 at 18:20:45, skobes wrote: > On 2015/08/25 18:18:32, commit-bot: I haz the power wrote: > > Try jobs failed on following builders: > > mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, > > http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...) > > ^ http://crbug.com/524248, ongoing This is not particularly time sensitive. I'll try the CQ again later today or later this week.
The CQ bit was checked by pdr@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1308693003/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1308693003/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by pdr@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1308693003/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1308693003/40001
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://src.chromium.org/viewvc/blink?view=rev&revision=201172 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
