Collapse whitespace away when creating whitespace layout objects.

Collapse whitespace away when creating whitespace layout objects.

BUG=686016

[emilio, 2017-02-27 17:21:24]

The CQ bit was checked by ecobos@igalia.com

[emilio, 2017-02-27 17:21:28]

The CQ bit was unchecked by ecobos@igalia.com

[emilio, 2017-02-27 17:21:36]

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

[commit-bot: I haz the power, 2017-02-27 17:21:51]

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

[emilio, 2017-02-27 18:22:53]

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

[emilio, 2017-02-27 18:22:54]

Hi rune, please take a look, this is also split from
https://codereview.chromium.org/2719713002/, and fixes crbug.com/686016.

[commit-bot: I haz the power, 2017-02-27 18:40:51]

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

[commit-bot: I haz the power, 2017-02-27 18:40:51]

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

Please add the changed expectations to the CL so that we can confirm they're
sound.

[emilio, 2017-02-27 23:29:47]

On 2017/02/27 22:51:46, rune wrote:
> Please add the changed expectations to the CL so that we can confirm they're
> sound.

You can see the changes at
https://storage.googleapis.com/chromium-layout-test-archives/win_chromium_rel...
(I will update the CL with them tomorrow since I'm not on my computer now).

There is a crashtest and a unit test that assert due to an empty text layout
object (I didn't know we could have those even). It doesn't seem even in editing
it seems, but well, the solution is checking for the empty case.

The rest of the changes seem ok, so I'll update the CL tomorrow.

[rune, 2017-02-28 08:34:35]

On 2017/02/27 23:29:47, emilio wrote:
> On 2017/02/27 22:51:46, rune wrote:
> > Please add the changed expectations to the CL so that we can confirm they're
> > sound.
> 
> You can see the changes at
>
https://storage.googleapis.com/chromium-layout-test-archives/win_chromium_rel...
> (I will update the CL with them tomorrow since I'm not on my computer now).

Yeah, but it's easier to see the actual diff here confirmed by a green dry run.
I'll wait for a new patch set.

> There is a crashtest and a unit test that assert due to an empty text layout
> object (I didn't know we could have those even). It doesn't seem even in
editing
> it seems, but well, the solution is checking for the empty case.

Yes, editing returns true for creating a layout object before checking the
length.

[emilio, 2017-02-28 09:18:12]

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

[commit-bot: I haz the power, 2017-02-28 09:18:19]

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-28 09:23:09]

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

[commit-bot: I haz the power, 2017-02-28 09:23:11]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[emilio, 2017-02-28 09:27:10]

On 2017/02/28 08:34:35, rune wrote:
> On 2017/02/27 23:29:47, emilio wrote:
> > On 2017/02/27 22:51:46, rune wrote:
> > > Please add the changed expectations to the CL so that we can confirm
they're
> > > sound.
> > 
> > You can see the changes at
> >
>
https://storage.googleapis.com/chromium-layout-test-archives/win_chromium_rel...
> > (I will update the CL with them tomorrow since I'm not on my computer now).
> 
> Yeah, but it's easier to see the actual diff here confirmed by a green dry
run.
> I'll wait for a new patch set.

Yeah, fair enough :). I just updated the test expectations, waiting on a dry run
now.

The most concerning expectation change is
paint/invalidation/absolute-display-block-to-none-expected.txt[1].

That doesn't regress with my other patch at
https://codereview.chromium.org/2719713002/, since we skip out of flow elements
in the first place, so we don't create that text node at all.

I can confirm that a loop like:

  while (prev && prev->isOutOfFlowPositioned() && --maxSiblingsToVisit) {
    prev = prev->previousSibling();
  }

  if (!maxSiblingsToVisit)
    return true;

in this patch fixes the issue as well. If you want I can add it.

[1]: https://codereview.chromium.org/2719993003/patch/20001/30046

[emilio, 2017-02-28 09:35:16]

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

[commit-bot: I haz the power, 2017-02-28 09:35:30]

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

[emilio, 2017-02-28 09:36:08]

On 2017/02/28 09:27:10, emilio wrote:
> On 2017/02/28 08:34:35, rune wrote:
> > On 2017/02/27 23:29:47, emilio wrote:
> > > On 2017/02/27 22:51:46, rune wrote:
> > > > Please add the changed expectations to the CL so that we can confirm
> they're
> > > > sound.
> > > 
> > > You can see the changes at
> > >
> >
>
https://storage.googleapis.com/chromium-layout-test-archives/win_chromium_rel...
> > > (I will update the CL with them tomorrow since I'm not on my computer
now).
> > 
> > Yeah, but it's easier to see the actual diff here confirmed by a green dry
> run.
> > I'll wait for a new patch set.
> 
> Yeah, fair enough :). I just updated the test expectations, waiting on a dry
run
> now.
> 
> The most concerning expectation change is
> paint/invalidation/absolute-display-block-to-none-expected.txt[1].
> 
> That doesn't regress with my other patch at
> https://codereview.chromium.org/2719713002/, since we skip out of flow
elements
> in the first place, so we don't create that text node at all.
> 
> I can confirm that a loop like:
> 
>   while (prev && prev->isOutOfFlowPositioned() && --maxSiblingsToVisit) {
>     prev = prev->previousSibling();
>   }
> 
>   if (!maxSiblingsToVisit)
>     return true;
> 
> in this patch fixes the issue as well. If you want I can add it.
> 
> [1]: https://codereview.chromium.org/2719993003/patch/20001/30046

I ended up doing this since the expectation adjustments were completely rotten
due to https://chromium-review.googlesource.com/444189, so I've had to rebase
anyway.

[rune, 2017-02-28 09:59:46]

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

https://codereview.chromium.org/2719993003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Text.cpp:315: if (prev && !prev->isInline()
&& !prev->isOutOfFlowPositioned())
prev cannot be out-of-flow at this point since prev is not modified after the
while-loop you added.

[emilio, 2017-02-28 11:12:07]

On 2017/02/28 09:59:46, rune wrote:
>
https://codereview.chromium.org/2719993003/diff/40001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/dom/Text.cpp (right):
> 
>
https://codereview.chromium.org/2719993003/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/dom/Text.cpp:315: if (prev && !prev->isInline()
> && !prev->isOutOfFlowPositioned())
> prev cannot be out-of-flow at this point since prev is not modified after the
> while-loop you added.

That's right (tbh I didn't expect you to go ahead and start reviewing, I just
pushed that loop from class to see the expectation changes), but thanks for the
heads up, I updated it :)

[emilio, 2017-02-28 11:17:03]

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

[commit-bot: I haz the power, 2017-02-28 11:17:14]

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-28 13:30:48]

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

[commit-bot: I haz the power, 2017-02-28 13:30:49]

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-28 15:20:12]

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

[commit-bot: I haz the power, 2017-02-28 15:20:27]

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

[emilio, 2017-02-28 15:29:28]

On 2017/02/28 11:12:07, emilio wrote:
> On 2017/02/28 09:59:46, rune wrote:
> >
>
https://codereview.chromium.org/2719993003/diff/40001/third_party/WebKit/Sour...
> > File third_party/WebKit/Source/core/dom/Text.cpp (right):
> > 
> >
>
https://codereview.chromium.org/2719993003/diff/40001/third_party/WebKit/Sour...
> > third_party/WebKit/Source/core/dom/Text.cpp:315: if (prev &&
!prev->isInline()
> > && !prev->isOutOfFlowPositioned())
> > prev cannot be out-of-flow at this point since prev is not modified after
the
> > while-loop you added.
> 
> That's right (tbh I didn't expect you to go ahead and start reviewing, I just
> pushed that loop from class to see the expectation changes), but thanks for
the
> heads up, I updated it :)

I submitted the test expectations for linux (since I don't have any other
platform to test on). The changes in
fast/spatial-navigation/snav-multiple-select-focusring.html seem due to my
specific environment, given the result on the builder is just a removed
whitespace layout object (instead of a new "caret: ..." line).

The rest seems sane I think

[commit-bot: I haz the power, 2017-02-28 17:22:57]

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

[commit-bot: I haz the power, 2017-02-28 17:22:59]

Dry run: 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, 2017-02-28 21:56:38]

On 2017/02/28 15:29:28, emilio wrote:

> I submitted the test expectations for linux (since I don't have any other
> platform to test on).

You can try rebaselining using try-bots as described here:

https://chromium.googlesource.com/chromium/src/+/master/docs/testing/layout_t...

[emilio, 2017-03-01 08:14:11]

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

[commit-bot: I haz the power, 2017-03-01 08:14:29]

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-03-01 09:34:58]

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

[commit-bot: I haz the power, 2017-03-01 09:35:02]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[emilio, 2017-03-01 14:56:58]

On 2017/02/28 21:56:38, rune wrote:
> On 2017/02/28 15:29:28, emilio wrote:
> 
> > I submitted the test expectations for linux (since I don't have any other
> > platform to test on).
> 
> You can try rebaselining using try-bots as described here:
> 
>
https://chromium.googlesource.com/chromium/src/+/master/docs/testing/layout_t...

Ouch, thanks for the hint.

So I rebaselined, pushed, and there are some unit tests failing from editing,
with stuff like:

  static const char* bodyContent =
      "<a id=host><b id=one>one</b> not appeared <b id=two>two</b></a>";
  const char* shadowContent =
      "three <content select=#two></content> <content select=#one></content> "
      "zero";

I'm digging, this may have to do with reattachWhitespaceSiblings, or with the
editing iterators. In any case until that's resolved I don't think you should
review this.