Avoid creating consecutive whitespace renderers.

Avoid creating consecutive whitespace renderers.

In textLayoutObjectIsNeeded(), return false if we are a whitespace-only
text node, and our previous LayoutObject sibling is a whitespace
renderer. This avoids for instance creating a consecutive list of
whitespace renderers when we have multiple display:none sibling
elements.

This change makes [1] take ~900ms instead of ~2600ms on my computer.

[1] https://bugs.chromium.org/p/chromium/issues/attachmentText?aid=8037

BUG=399816
Committed: https://crrev.com/75a7b12e587c28ac101464935f5e724fab6b6ec5
Cr-Commit-Position: refs/heads/master@{#421794}

[rune, 2016-09-27 13:40:43]

Description was changed from

==========
Avoid creating consecutive whitespace renderers.

In textLayoutObjectIsNeeded(), return false if we are a whitespace-only
text node, and our previous LayoutObject sibling is a whitespace
renderer. This avoids for instance creating a consecutive list of
whitespace renderers when we have multiple display:none sibling
elements.

This change makes [1] take ~900ms instead of ~2600ms on my computer.

A lot of layout tree expectations will fail.

[1] https://bugs.chromium.org/p/chromium/issues/attachmentText?aid=8037

BUG=399816
==========

to

==========
Avoid creating consecutive whitespace renderers.

In textLayoutObjectIsNeeded(), return false if we are a whitespace-only
text node, and our previous LayoutObject sibling is a whitespace
renderer. This avoids for instance creating a consecutive list of
whitespace renderers when we have multiple display:none sibling
elements.

This change makes [1] take ~900ms instead of ~2600ms on my computer.

[1] https://bugs.chromium.org/p/chromium/issues/attachmentText?aid=8037

BUG=399816
==========

[rune, 2016-09-27 13:51:59]

rune@opera.com changed reviewers:
+ bugsnash@chromium.org, eae@chromium.org, mstensho@opera.com,
nainar@chromium.org

[rune, 2016-09-27 13:52:00]

I've been looking at whitespace renderer re-attachment problems, and found that
we create a LayoutObject for all consecutive whitespace renderers.

I have not adjusted expectation files, but it seems most, if not all, are simple
and plausible adjustments.

Does this look reasonable from a layout perspective?

See layout test results in PS2. The ones I'm unsure about are:

http/tests/loading/image-picture-download-after-shrink.html
fast/forms/select/select-initial-position.html
editing/execCommand/outdent-break-with-style.html
css2.1/20110323/replaced-min-max-001.htm

[rune, 2016-09-27 13:54:14]

https://codereview.chromium.org/2369963005/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutTextTest.cpp (right):

https://codereview.chromium.org/2369963005/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutTextTest.cpp:142: // <span
display:none>
This should be the comment <!-- -->

[eae, 2016-09-27 14:06:41]

Nice win!

LGTM

[eae, 2016-09-27 14:19:40]

The CQ bit was checked by eae@chromium.org

[commit-bot: I haz the power, 2016-09-27 14:20:00]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[rune, 2016-09-27 14:25:04]

The CQ bit was unchecked by rune@opera.com

[rune, 2016-09-27 14:26:19]

On 2016/09/27 at 14:06:41, eae wrote:
> Nice win!
> 
> LGTM

I cancelled the commit as I need to at least fix expectations.

[mstensho (USE GERRIT), 2016-09-28 11:19:09]

lgtm

https://codereview.chromium.org/2369963005/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutTextTest.cpp (right):

https://codereview.chromium.org/2369963005/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutTextTest.cpp:100: }
How about a test similar to this one, except that you insert the new whitespace
node BEFORE the existing whitespace node?

The expectation then would be that the old whitespace node would lose its
LayoutObject and have a LayoutObject created for the new node instead.

But you just explained to me f2f that it won't work like that, and that both
whitespace nodes would end up with a LayoutObject, because
Text::textLayoutObjectIsNeeded() only searches backwards. I don't know if
creating different layout trees with the same DOM, depending on the order things
are inserted in could be a potential problem, but it sounds reasonable to add a
test for it, with an explanation.

We're already inconsistent in textLayoutObjectIsNeeded(), anyway, because we
limit the search to 50 nodes, with a comment saying that it shouldn't be a
problem.

[rune, 2016-09-28 12:12:11]

On 2016/09/28 at 11:19:09, mstensho wrote:
> lgtm
> 
>
https://codereview.chromium.org/2369963005/diff/40001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/layout/LayoutTextTest.cpp (right):
> 
>
https://codereview.chromium.org/2369963005/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/layout/LayoutTextTest.cpp:100: }
> How about a test similar to this one, except that you insert the new
whitespace node BEFORE the existing whitespace node?
> 
> The expectation then would be that the old whitespace node would lose its
LayoutObject and have a LayoutObject created for the new node instead.
> 
> But you just explained to me f2f that it won't work like that, and that both
whitespace nodes would end up with a LayoutObject, because
Text::textLayoutObjectIsNeeded() only searches backwards. I don't know if
creating different layout trees with the same DOM, depending on the order things
are inserted in could be a potential problem, but it sounds reasonable to add a
test for it, with an explanation.
> 
> We're already inconsistent in textLayoutObjectIsNeeded(), anyway, because we
limit the search to 50 nodes, with a comment saying that it shouldn't be a
problem.

So, it turns out that it works. recalcTextStyle() calls
reattachWhitespaceSiblingsIfNeeded(). I could've sworn that wasn't the case
yesterday. I've added the test.

[rune, 2016-09-28 12:13:10]

https://codereview.chromium.org/2369963005/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutTextTest.cpp (right):

https://codereview.chromium.org/2369963005/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutTextTest.cpp:142: // <span
display:none>
On 2016/09/27 at 13:54:14, rune wrote:
> This should be the comment <!-- -->

Fixed.

[rune, 2016-09-28 12:27:43]

The CQ bit was checked by rune@opera.com

[rune, 2016-09-28 12:27:45]

The patchset sent to the CQ was uploaded after l-g-t-m from mstensho@opera.com,
eae@chromium.org
Link to the patchset: https://codereview.chromium.org/2369963005/#ps80001
(title: "Rebaseline")

[commit-bot: I haz the power, 2016-09-28 12:28:03]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-28 14:26:11]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-28 14:26:14]

Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[rune, 2016-09-28 21:31:42]

The CQ bit was checked by rune@opera.com

[rune, 2016-09-28 21:31:44]

The patchset sent to the CQ was uploaded after l-g-t-m from mstensho@opera.com,
eae@chromium.org
Link to the patchset: https://codereview.chromium.org/2369963005/#ps100001
(title: "Readded deleted expectations")

[commit-bot: I haz the power, 2016-09-28 21:32:43]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-28 22:42:59]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-28 22:43:02]

Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Bugs Nash, 2016-09-29 05:01:42]

lgtm

[Bugs Nash, 2016-09-29 05:06:54]

On 2016/09/29 at 05:01:42, Bugs Nash wrote:
> lgtm

This seems to change the white space that ends up being rendered - does it still
follow spec?

[rune, 2016-09-29 07:07:10]

On 2016/09/29 05:06:54, Bugs Nash wrote:
> On 2016/09/29 at 05:01:42, Bugs Nash wrote:
> > lgtm
> 
> This seems to change the white space that ends up being rendered - does it
still
> follow spec?

Yes. There should be no visual changes, and the existence of a LayoutText object
should not be observable in any other way.

[rune, 2016-09-29 09:13:48]

The CQ bit was checked by rune@opera.com

[rune, 2016-09-29 09:13:50]

The patchset sent to the CQ was uploaded after l-g-t-m from mstensho@opera.com,
bugsnash@chromium.org, eae@chromium.org
Link to the patchset: https://codereview.chromium.org/2369963005/#ps120001
(title: "Rebased and manually fixed expectations")

[commit-bot: I haz the power, 2016-09-29 09:14:04]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-29 10:40:38]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-09-29 10:42:49]

Description was changed from

==========
Avoid creating consecutive whitespace renderers.

In textLayoutObjectIsNeeded(), return false if we are a whitespace-only
text node, and our previous LayoutObject sibling is a whitespace
renderer. This avoids for instance creating a consecutive list of
whitespace renderers when we have multiple display:none sibling
elements.

This change makes [1] take ~900ms instead of ~2600ms on my computer.

[1] https://bugs.chromium.org/p/chromium/issues/attachmentText?aid=8037

BUG=399816
==========

to

==========
Avoid creating consecutive whitespace renderers.

In textLayoutObjectIsNeeded(), return false if we are a whitespace-only
text node, and our previous LayoutObject sibling is a whitespace
renderer. This avoids for instance creating a consecutive list of
whitespace renderers when we have multiple display:none sibling
elements.

This change makes [1] take ~900ms instead of ~2600ms on my computer.

[1] https://bugs.chromium.org/p/chromium/issues/attachmentText?aid=8037

BUG=399816

Committed: https://crrev.com/75a7b12e587c28ac101464935f5e724fab6b6ec5
Cr-Commit-Position: refs/heads/master@{#421794}
==========

[commit-bot: I haz the power, 2016-09-29 10:42:52]

Patchset 7 (id:??) landed as
https://crrev.com/75a7b12e587c28ac101464935f5e724fab6b6ec5
Cr-Commit-Position: refs/heads/master@{#421794}

[jwd, 2016-09-29 14:37:56]

On 2016/09/29 10:42:52, commit-bot: I haz the power wrote:
> Patchset 7 (id:??) landed as
> https://crrev.com/75a7b12e587c28ac101464935f5e724fab6b6ec5
> Cr-Commit-Position: refs/heads/master@{#421794}

Reverted in https://codereview.chromium.org/2374983004, it appeared to be
failing webkit_tests.