Move layout tree reconstruction code for Text nodes into new function

Move layout tree reconstruction code for Text nodes into new function

This code moves the Layout Tree construction code for Text
nodes into Text::rebuildTextLayoutTree()

BUG=595137
Committed: https://crrev.com/a4805a287ac79834a932f44c87791a7b9322ede8
Cr-Commit-Position: refs/heads/master@{#427967}

[nainar, 2016-10-14 03:47:27]

The CQ bit was checked by nainar@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-14 03:47:40]

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

[nainar, 2016-10-14 03:49:18]

Description was changed from

==========
Move layout tree reconstruction code for Text nodes into new function

BUG=
==========

to

==========
Move layout tree reconstruction code for Text nodes into new function

This code moves the Layout Tree construction code for Text
nodes into Text::rebuildTextLayoutTree()

BUG=595137
==========

[nainar, 2016-10-14 03:49:18]

nainar@chromium.org changed reviewers:
+ bugsnash@chromium.org

[nainar, 2016-10-14 03:51:47]

bugsnash, 

PTAL? 

Thanks!

[Bugs Nash, 2016-10-14 03:58:18]

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

https://codereview.chromium.org/2421793002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/Text.cpp:405: if (needsStyleRecalc() ||
needsWhitespaceLayoutObject()) {
needsStyleRecalc() probably shouldn't be inside this method, as it's supposed to
be about layout. Might need some refactoring and use the new needsLayout flags?

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

https://codereview.chromium.org/2421793002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/Text.h:57: void rebuildTextLayoutTree(Text*
nextTextSibling);
I'm not sure about this name. There is no tree below a text node, they are
always leaves. Maybe just rebuildTextLayout? Not sure if that's clear/accurate.
On the other hand, this name is consistent with Element::rebuildLayoutTree...
I'm not sure about it. Maybe Elliott will have a better idea?

[commit-bot: I haz the power, 2016-10-14 05:06:10]

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

[commit-bot: I haz the power, 2016-10-14 05:06:11]

Dry run: This issue passed the CQ dry run.

[nainar, 2016-10-14 07:22:11]

The CQ bit was checked by nainar@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-14 07:22:21]

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

[nainar, 2016-10-14 07:22:41]

Made the changes you asked for. PTAL? Thanks!

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

https://codereview.chromium.org/2421793002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/Text.h:57: void rebuildTextLayoutTree(Text*
nextTextSibling);
Another name I had in mind was: 

rebuildTextLayoutNode() or rebuildLayoutText()

[commit-bot: I haz the power, 2016-10-14 10:13:53]

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

[commit-bot: I haz the power, 2016-10-14 10:13:53]

Dry run: This issue passed the CQ dry run.

[Bugs Nash, 2016-10-17 02:20:37]

On 2016/10/14 at 07:22:41, nainar wrote:
> Made the changes you asked for. PTAL? Thanks!
> 
>
https://codereview.chromium.org/2421793002/diff/1/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/dom/Text.h (right):
> 
>
https://codereview.chromium.org/2421793002/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/dom/Text.h:57: void rebuildTextLayoutTree(Text*
nextTextSibling);
> Another name I had in mind was: 
> 
> rebuildTextLayoutNode() or rebuildLayoutText()

rebuildLayoutText()?

[Bugs Nash, 2016-10-17 02:20:52]

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

https://codereview.chromium.org/2421793002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Text.cpp:401: setNeedsReattachLayoutTree();
probably don't need to implement this yet, not needed until 'The Reorg'

https://codereview.chromium.org/2421793002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Text.cpp:402: clearNeedsStyleRecalc();
why is this being changed in this patch?

https://codereview.chromium.org/2421793002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Text.cpp:409: if (needsReattachLayoutTree()
|| needsWhitespaceLayoutObject()) {
this check doesn't belong in here, the method shouldn't be called if this
doesn't evaluate to true. you can change it to a DCHECK if you like, but leave
the 'if (needsStyleRecalc() || needsWhitespaceLayoutObject())' check in the
calling method

[nainar, 2016-10-17 16:56:20]

The CQ bit was checked by nainar@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-17 16:56:57]

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

[nainar, 2016-10-17 16:57:31]

It seems that I am misunderstanding what you mean here so have added comments
inline explaining what I think you meant. PTAL? Thanks!

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

https://codereview.chromium.org/2421793002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Text.cpp:401: setNeedsReattachLayoutTree();
The flag setting is being done in the mirror patch here:
https://codereview.chromium.org/2398293003 and so should be done here. The next
patch (the reorg one) doesn't introduce the setting of the bits it just makes
sure that they are being set in the correct phases (style recalc and layout tree
construction).

https://codereview.chromium.org/2421793002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Text.cpp:409: if (needsReattachLayoutTree()
|| needsWhitespaceLayoutObject()) {
Seems like I misread your comment in the previous patch. Your comment intended
for this:

void Text::recalcTextStyle(StyleRecalcChange change, Text* nextTextSibling) {
  ...
  } else if (needsStyleRecalc() || needsWhitespaceLayoutObject()) {
    setNeedsReattachLayoutTree();
    rebuildTextLayoutTree(nextTextSibling);
  }
}

void Text::rebuildTextLayoutTree(Text* nextTextSibling) {
  reattachLayoutTree();
  if (this->layoutObject())
    reattachWhitespaceSiblingsIfNeeded(nextTextSibling);
  clearNeedsReattachLayoutTree();
}

Implemented this. Let me know if I still seem to be misunderstanding you.

[commit-bot: I haz the power, 2016-10-17 18:26:13]

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

[commit-bot: I haz the power, 2016-10-17 18:26:14]

Dry run: This issue passed the CQ dry run.

[Bugs Nash, 2016-10-18 23:19:14]

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

https://codereview.chromium.org/2421793002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Text.cpp:400: setNeedsReattachLayoutTree();
I understand that you want to set flags correctly, but I don't see this as
setting the flag correctly. This sets a flag that is never checked, then calls a
method that clears the flag. I wouldn't include it until it is used.

https://codereview.chromium.org/2421793002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Text.cpp:405: void
Text::rebuildTextLayoutTree(Text* nextTextSibling) {
yup this is (y)

[nainar, 2016-10-26 05:02:21]

The CQ bit was checked by nainar@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-26 05:02:53]

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

[nainar, 2016-10-26 05:08:55]

PTAL? Thanks!

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

https://codereview.chromium.org/2421793002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Text.cpp:400: setNeedsReattachLayoutTree();
Yup I understand. Made the change.

https://codereview.chromium.org/2421793002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Text.cpp:405: void
Text::rebuildTextLayoutTree(Text* nextTextSibling) {
Yeah!

[commit-bot: I haz the power, 2016-10-26 06:32:29]

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

[commit-bot: I haz the power, 2016-10-26 06:32:30]

Dry run: This issue passed the CQ dry run.

[Bugs Nash, 2016-10-27 00:16:56]

lgtm

[nainar, 2016-10-27 00:18:20]

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

[nainar, 2016-10-27 00:18:21]

To Elliott for OWNERS

[esprehn, 2016-10-27 02:06:07]

lgtm, btw you should start adding DCHECKs about the dirty bits whenever possible
in the various functions. It catches accidentally screwing things up. You'll see
I droppped asserts all over the top of ::recalcStyle and ::recalcOwnStyle to
make sure things made sense, and continued to make sense as the code was
refactored.

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

https://codereview.chromium.org/2421793002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Text.cpp:404: void
Text::rebuildTextLayoutTree(Text* nextTextSibling) {
DCHECK(needsReattachLayoutTree()) at the top of this?

https://codereview.chromium.org/2421793002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Text.cpp:406: if (this->layoutObject())
remove this->, there's no name conflict now that you refactored

[nainar, 2016-10-27 03:31:51]

The CQ bit was checked by nainar@chromium.org to run a CQ dry run

[nainar, 2016-10-27 03:32:14]

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

https://codereview.chromium.org/2421793002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Text.cpp:404: void
Text::rebuildTextLayoutTree(Text* nextTextSibling) {
We can't assert that here as we aren't setting needsReattachLayoutTree(). We
could assert if we set it in Text::recalcTextStyle().

https://codereview.chromium.org/2421793002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Text.cpp:406: if (this->layoutObject())
Done.

[commit-bot: I haz the power, 2016-10-27 03:32: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, 2016-10-27 04:49:42]

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

[commit-bot: I haz the power, 2016-10-27 04:49:43]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[nainar, 2016-10-27 06:49:40]

The CQ bit was checked by nainar@chromium.org

[nainar, 2016-10-27 06:49:41]

The patchset sent to the CQ was uploaded after l-g-t-m from
esprehn@chromium.org, bugsnash@chromium.org
Link to the patchset: https://codereview.chromium.org/2421793002/#ps80001
(title: "Edits post Elliott's lgtm")

[commit-bot: I haz the power, 2016-10-27 06:49:57]

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-10-27 08:49:24]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-10-27 08:54:18]

Description was changed from

==========
Move layout tree reconstruction code for Text nodes into new function

This code moves the Layout Tree construction code for Text
nodes into Text::rebuildTextLayoutTree()

BUG=595137
==========

to

==========
Move layout tree reconstruction code for Text nodes into new function

This code moves the Layout Tree construction code for Text
nodes into Text::rebuildTextLayoutTree()

BUG=595137

Committed: https://crrev.com/a4805a287ac79834a932f44c87791a7b9322ede8
Cr-Commit-Position: refs/heads/master@{#427967}
==========

[commit-bot: I haz the power, 2016-10-27 08:54:19]

Patchset 5 (id:??) landed as
https://crrev.com/a4805a287ac79834a932f44c87791a7b9322ede8
Cr-Commit-Position: refs/heads/master@{#427967}