Simplify whitespace layout object creation.

Simplify whitespace layout object creation.

This makes whitespace layout object creation not look at the DOM tree, reducing
the chances of bugs with display: contents and shadow DOM.

BUG=686016, 657748

[emilio, 2017-02-25 21:06:51]

The CQ bit was checked by ecobos@igalia.com to run a CQ dry run

[commit-bot: I haz the power, 2017-02-25 21:06:57]

Dry run: 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, 2017-02-25 22:43:46]

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

[commit-bot: I haz the power, 2017-02-25 22:43:47]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[emilio, 2017-02-26 00:24:23]

ecobos@igalia.com changed reviewers:
+ rune@opera.com

[emilio, 2017-02-26 00:24:23]

Hi rune, you might be a good reviewer for this one?

I checked in the test case from the bug that introduced the firstChild lookup
just to ensure I don't regress anything.

I have to update the test expectations, but I've only hit a test case that
creates more layout objects, and it's because we now check for 49 oof siblings
instead of 50 (I can change that, but I don't like to check for underflows).

[rune, 2017-02-26 21:45:12]

The description didn't give me much clue about what you're changing or why it
would be better. Could you try to explain?

[emilio, 2017-02-26 22:06:54]

On 2017/02/26 21:45:12, rune wrote:
> The description didn't give me much clue about what you're changing or why it
> would be better. Could you try to explain?

Well, upon reflection the previous code was using LayoutTreeBuilderTraversal,
that's why I wasn't able to construct a testcase where it failed, so I'll adjust
the description to reflect that it's not fixing any source of bugs.

Still, this fixes https://bugs.chromium.org/p/chromium/issues/detail?id=686016
and simplifies the code in general, which I think is a win.

[rune, 2017-02-27 10:38:06]

https://codereview.chromium.org/2719713002/diff/1/third_party/WebKit/LayoutTe...
File third_party/WebKit/LayoutTests/fast/text/whitespace-text-nodes-float.html
(right):

https://codereview.chromium.org/2719713002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/text/whitespace-text-nodes-float.html:11:
p.innerHTML = 'a<span class="f">x</span> <span class="f">y</span>b';
I don't think you're guaranteed that you have generated a layout tree on onload?
Are you trying to test (re-)attachment of the p descendants given that p already
has a box?

You should have a better pass condition. For instance: "SS<span>P</span>
<span>A</span>PASS"
 with a preceding: "<p>You should see the word PASS two times below.</p>".

Also, what would typically break is the case where you don't re-attach all
siblings, for instance going from block to float where you didn't have a
whitespace layout object, but needs one after changing the display type to
float.

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

https://codereview.chromium.org/2719713002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/Text.cpp:293: uint32_t maxSiblingsToVisit =
50;
Looks unnecessary to have a size-specific (32-bit) type for this code.

https://codereview.chromium.org/2719713002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/Text.cpp:297: *this, maxSiblingsToVisit,
&ranOutOfSiblings);
Can't you send maxSiblingsToVisit as a non-const reference instead of adding
ranOutOfSiblings?

Both before and after this CL you can visit nearly a 100 siblings if
previousSiblingLayoutObject visits 49 and you have 50 out-of-flow boxes before
that.

https://codereview.chromium.org/2719713002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/Text.cpp:310:
emptyOrEndsWithWhitespace(toLayoutText(prev)->text())) {
Is checking the previous layout object for white-space all that changes test
expectations here?

https://codereview.chromium.org/2719713002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/Text.cpp:327: return !!prev;
We still re-attach right-to-left? That means we'll always return false first for
white-space nodes while attaching inline siblings inside a block, but it will be
rectified correctly by reattachWhitespaceSiblings() after attaching the previous
inline?

It seems to me this code is changing the behavior for that case, at least.

[emilio, 2017-02-27 12:09:32]

https://codereview.chromium.org/2719713002/diff/1/third_party/WebKit/LayoutTe...
File third_party/WebKit/LayoutTests/fast/text/whitespace-text-nodes-float.html
(right):

https://codereview.chromium.org/2719713002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/text/whitespace-text-nodes-float.html:11:
p.innerHTML = 'a<span class="f">x</span> <span class="f">y</span>b';
On 2017/02/27 10:38:06, rune wrote:
> I don't think you're guaranteed that you have generated a layout tree on
onload?
> Are you trying to test (re-)attachment of the p descendants given that p
already
> has a box?
> 
> You should have a better pass condition. For instance: "SS<span>P</span>
> <span>A</span>PASS"
>  with a preceding: "<p>You should see the word PASS two times below.</p>".
> 
> Also, what would typically break is the case where you don't re-attach all
> siblings, for instance going from block to float where you didn't have a
> whitespace layout object, but needs one after changing the display type to
> float.

This is literally a copy-pasted test case from
https://bugs.chromium.org/p/chromium/issues/detail?id=556733, which is what
introduced
https://chromium.googlesource.com/chromium/src.git/+/2748c5cea72b053e64920d9b...,
but yeah, I'll add more test cases with better passing conditions.

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

https://codereview.chromium.org/2719713002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/Text.cpp:297: *this, maxSiblingsToVisit,
&ranOutOfSiblings);
On 2017/02/27 10:38:06, rune wrote:
> Can't you send maxSiblingsToVisit as a non-const reference instead of adding
> ranOutOfSiblings?
> 
> Both before and after this CL you can visit nearly a 100 siblings if
> previousSiblingLayoutObject visits 49 and you have 50 out-of-flow boxes before
> that.

That's right. I can, but that'd be somewhat annoying for other callers that
don't want to limit it. I can pass it by pointer though, I guess.

https://codereview.chromium.org/2719713002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/Text.cpp:310:
emptyOrEndsWithWhitespace(toLayoutText(prev)->text())) {
On 2017/02/27 10:38:06, rune wrote:
> Is checking the previous layout object for white-space all that changes test
> expectations here?

Not totally sure about this, but it's likely. Actually, let me submit a CL with
this change, and another with the LayoutTreeBuilderTraversal limit fixes
separately before trying to land this.

https://codereview.chromium.org/2719713002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/Text.cpp:327: return !!prev;
On 2017/02/27 10:38:06, rune wrote:
> We still re-attach right-to-left? That means we'll always return false first
for
> white-space nodes while attaching inline siblings inside a block, but it will
be
> rectified correctly by reattachWhitespaceSiblings() after attaching the
previous
> inline?
> 
> It seems to me this code is changing the behavior for that case, at least.

Yes, I believe that's right. I don't think it matters a lot as long as
reattachWhitespaceSiblings does the right thing, though?