Ensure text nodes get an object if we're unsure that they will be collapsed

Ensure text nodes get an object if we're unsure that they will be collapsed

BUG=556733
Committed: https://crrev.com/2748c5cea72b053e64920d9bce3bd0440ba5d22d
Cr-Commit-Position: refs/heads/master@{#364765}

[rhogan, 2015-11-19 21:04:29]

robhogan@gmail.com changed reviewers:
+ wangxianzhu@chromium.org

[rhogan, 2015-11-20 18:59:13]

robhogan@gmail.com changed reviewers:
+ eae@chromium.org

[rhogan, 2015-11-20 18:59:14]

wangxianzhu@ - can you take a look?

[Xianzhu, 2015-11-20 19:16:43]

wangxianzhu@chromium.org changed reviewers:
+ leviw@chromium.org

[Xianzhu, 2015-11-20 19:16:44]

I'm afraid I don't have enough knowledge to review it.

leviw@?

[rhogan, 2015-11-20 19:33:14]

https://codereview.chromium.org/1461773002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/dom/Text.cpp (right):

https://codereview.chromium.org/1461773002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/Text.cpp:310: // If we're adding children to
this flow our previous siblings are not in the layout tree yet so we
More specifically, when we're doing an .innerHTML= or appendChild children get
added last to first. I haven't inquired into why.

[rhogan, 2015-11-20 19:50:25]

https://codereview.chromium.org/1461773002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/dom/Text.cpp (right):

https://codereview.chromium.org/1461773002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/Text.cpp:310: // If we're adding children to
this flow our previous siblings are not in the layout tree yet so we
On 2015/11/20 at 19:33:13, rhogan wrote:
> More specifically, when we're doing an .innerHTML= or appendChild children get
added last to first. I haven't inquired into why.

I knew as soon as I typed it that was daft. This actually happens because
recalcChildStyle goes from back to front - so later siblings get their layout
objects before previous ones.

[leviw_travelin_and_unemployed, 2015-11-20 21:18:34]

leviw@chromium.org changed reviewers:
+ esprehn@chromium.org

[leviw_travelin_and_unemployed, 2015-11-20 21:18:34]

https://codereview.chromium.org/1461773002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/dom/Text.cpp (right):

https://codereview.chromium.org/1461773002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/Text.cpp:312: Node* firstChild =
parent.node() ? LayoutTreeBuilderTraversal::firstChild(*parent.node()) :
nullptr;
Nit: I'd probably call this firstChildNode

https://codereview.chromium.org/1461773002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/Text.cpp:313: if (first && first ==
layoutObject() && firstChild && !firstChild->layoutObject() &&
firstChild->isTextNode())
Why are you checking firstChild->isTextNode() here?

[rhogan, 2015-11-21 16:55:42]

https://codereview.chromium.org/1461773002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/dom/Text.cpp (right):

https://codereview.chromium.org/1461773002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/Text.cpp:313: if (first && first ==
layoutObject() && firstChild && !firstChild->layoutObject() &&
firstChild->isTextNode())
On 2015/11/20 at 21:18:34, leviw wrote:
> Why are you checking firstChild->isTextNode() here?

Right, it doesn't matter does it?

[rhogan, 2015-11-24 19:26:20]

On 2015/11/21 at 16:55:42, rhogan wrote:
>
https://codereview.chromium.org/1461773002/diff/1/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/dom/Text.cpp (right):
> 
>
https://codereview.chromium.org/1461773002/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/dom/Text.cpp:313: if (first && first ==
layoutObject() && firstChild && !firstChild->layoutObject() &&
firstChild->isTextNode())
> On 2015/11/20 at 21:18:34, leviw wrote:
> > Why are you checking firstChild->isTextNode() here?
> 
> Right, it doesn't matter does it?

Updated per your comments!

[esprehn, 2015-12-03 07:03:32]

https://codereview.chromium.org/1461773002/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/text/whitespace-created-when-siblings-added-back-to-front.html
(right):

https://codereview.chromium.org/1461773002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/text/whitespace-created-when-siblings-added-back-to-front.html:14:
<p>crbug.com/556733: Ensure whitespace gets created when we attached siblings
from back to front. There should be a space between the t and the t.</p>
If we ever reversed the order again this test wouldn't work right? Do we need to
manually appendChild to make this test robust?

https://codereview.chromium.org/1461773002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/Text.cpp (right):

https://codereview.chromium.org/1461773002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Text.cpp:312: Node* firstChildNode =
parent.node() ? LayoutTreeBuilderTraversal::firstChild(*parent.node()) :
nullptr;
this doesn't work if your parent is anonymous, are you sure this is right? when
is node() null?

https://codereview.chromium.org/1461773002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Text.cpp:313: if (first && first ==
layoutObject() && firstChildNode && !firstChildNode->layoutObject())
This is saying "if we're the first in flow layout object, and there's a previous
sibling that doesn't have a layout object yet."

that'll always be true if the first child is display: none, is that correct?

[rhogan, 2015-12-03 19:45:32]

https://codereview.chromium.org/1461773002/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/text/whitespace-created-when-siblings-added-back-to-front.html
(right):

https://codereview.chromium.org/1461773002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/text/whitespace-created-when-siblings-added-back-to-front.html:14:
<p>crbug.com/556733: Ensure whitespace gets created when we attached siblings
from back to front. There should be a space between the t and the t.</p>
On 2015/12/03 at 07:03:32, esprehn wrote:
> If we ever reversed the order again this test wouldn't work right? Do we need
to manually appendChild to make this test robust?

Sure.

https://codereview.chromium.org/1461773002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/Text.cpp (right):

https://codereview.chromium.org/1461773002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Text.cpp:312: Node* firstChildNode =
parent.node() ? LayoutTreeBuilderTraversal::firstChild(*parent.node()) :
nullptr;
On 2015/12/03 at 07:03:32, esprehn wrote:
> this doesn't work if your parent is anonymous, 
Right enough, I hadn't thought of that but in the cases where we're deciding
whether to add an object we never end up in textLayoutObjectIsNeeded unless the
parent object has a node so this only happens when called from
updateTextLayoutObject. I think we're right to miss out on the optimization in
such situations so I need to account for that here.

> are you sure this is right? when is node() null?
LayoutTests/fast/css/image-set-value-not-removed-crash.html manages it when
calling from updateTextLayoutObject() during editing.

https://codereview.chromium.org/1461773002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Text.cpp:313: if (first && first ==
layoutObject() && firstChildNode && !firstChildNode->layoutObject())
On 2015/12/03 at 07:03:32, esprehn wrote:
> This is saying "if we're the first in flow layout object, and there's a
previous sibling that doesn't have a layout object yet."
> 
> that'll always be true if the first child is display: none, is that correct?

Sure, and in that situation we would create a text layout object that would get
collapsed away. The purpose here is to avoid that in the common cases. The bug
we're fixing is in the other direction - the text object isn't getting created
when it's needed.

[rhogan, 2015-12-03 19:48:17]

On 2015/12/03 at 19:45:32, rhogan wrote:
> Sure, and in that situation we would create a text layout object that would
get collapsed away. The purpose here is to avoid that in the common cases. The
bug we're fixing is in the other direction - the text object isn't getting
created when it's needed.

Updated CL attached.

[esprehn, 2015-12-11 10:48:54]

lgtm w/ nit, alright lets try it.

https://codereview.chromium.org/1461773002/diff/60001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/text/whitespace-created-when-siblings-added-back-to-front.html
(right):

https://codereview.chromium.org/1461773002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/text/whitespace-created-when-siblings-added-back-to-front.html:3:
.f {
.float-left

[rhogan, 2015-12-11 12:52:06]

The CQ bit was checked by robhogan@gmail.com

[rhogan, 2015-12-11 12:52:06]

The patchset sent to the CQ was uploaded after l-g-t-m from esprehn@chromium.org
Link to the patchset: https://codereview.chromium.org/1461773002/#ps80001
(title: "Update")

[commit-bot: I haz the power, 2015-12-11 12:52:51]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1461773002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1461773002/80001

[commit-bot: I haz the power, 2015-12-11 12:53:33]

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

[commit-bot: I haz the power, 2015-12-11 12:53:34]

Try jobs failed on following builders:
  android_arm64_dbg_recipe on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_arm64_d...)
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[rhogan, 2015-12-11 18:53:04]

The CQ bit was checked by robhogan@gmail.com

[rhogan, 2015-12-11 18:53:04]

The patchset sent to the CQ was uploaded after l-g-t-m from esprehn@chromium.org
Link to the patchset: https://codereview.chromium.org/1461773002/#ps120001
(title: "Updated")

[commit-bot: I haz the power, 2015-12-11 18:55:09]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1461773002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1461773002/120001

[commit-bot: I haz the power, 2015-12-11 19:12:10]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-12-11 19:13:44]

Description was changed from

==========
Ensure text nodes get an object if we're unsure that they will be collapsed

BUG=556733
==========

to

==========
Ensure text nodes get an object if we're unsure that they will be collapsed

BUG=556733

Committed: https://crrev.com/2748c5cea72b053e64920d9bce3bd0440ba5d22d
Cr-Commit-Position: refs/heads/master@{#364765}
==========

[commit-bot: I haz the power, 2015-12-11 19:13:46]

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