Set NeedsReattachLayoutTree and ChildNeedsReattachLayoutTree flags on Node

Set NeedsReattachLayoutTree and ChildNeedsReattachLayoutTree flags on Node

This patch sets the NeedsReattachLayoutTree and 
ChildNeedsReattachLayoutTree flags on Node correctly in 
Element::recalcOwnStyle. 
BUG=595137

[nainar, 2016-09-29 01:20:34]

Description was changed from

==========
set dirty bits

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

to

==========
Set NeedsReattachLayoutTree and ChildNeedsReattachLayoutTree flags on Node

This patch sets the NeedsReattachLayoutTree and 
ChildNeedsReattachLayoutTree flags on Node correctly in 
Element::recalcOwnStyle. 
BUG=595137
==========

[nainar, 2016-09-29 02:48:57]

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

[commit-bot: I haz the power, 2016-09-29 02:49:06]

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

[nainar, 2016-09-29 02:49:58]

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

[nainar, 2016-09-29 02:49:59]

PTAL? Thanks!

[commit-bot: I haz the power, 2016-09-29 03:03:44]

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

[commit-bot: I haz the power, 2016-09-29 03:03:45]

Dry run: Try jobs failed on following builders:
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Bugs Nash, 2016-09-29 03:15:24]

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

https://codereview.chromium.org/2375293002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Element.cpp:1832: // ComputedStyle will now
be stored on Node and accessed in buildLayoutTree()
This comment needs to be updated to reflect the new plan

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

https://codereview.chromium.org/2375293002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Node.cpp:699: void
Node::markNodeAndAncestorsWithChildNeedsReattachLayoutTree()
This name implies that you're marking the current node with ChildNeedsReattach.
I actually think this logic would be better moved to the
setNeedsReattachLayoutTree method, as marking your ancestors with
ChildNeedsReattach comes hand in hand with marking yourself needing reattach.

[nainar, 2016-09-29 04:04:48]

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

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

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

[nainar, 2016-09-29 04:05:15]

Made changes you asked for.

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

https://codereview.chromium.org/2375293002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Element.cpp:1832: // ComputedStyle will now
be stored on Node and accessed in buildLayoutTree()
Done.

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

https://codereview.chromium.org/2375293002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Node.cpp:699: void
Node::markNodeAndAncestorsWithChildNeedsReattachLayoutTree()
Done.

[Bugs Nash, 2016-09-29 04:11:47]

lgtm

[nainar, 2016-09-29 04:26:47]

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

[nainar, 2016-09-29 04:26:47]

To esprehn for OWNERS.

[esprehn, 2016-09-29 04:31:14]

I'd probably go about setting the bits in the same patch you start processing
them. Right now this is dead code since nothing clears the bits. You'll want
tree correctness asserts then too, you'll want to update assertLayoutTreeUpdated
to DCHECK that both of these bits are false.

[esprehn, 2016-09-29 04:32:48]

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

https://codereview.chromium.org/2375293002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Node.cpp:695: // TODO(nainar): Move this to
Node::markAncestorsWithChildNeedsReattachLayoutTree()
Hmm, why do you want to move this?

[nainar, 2016-09-29 04:39:28]

On 2016/09/29 at 04:32:48, esprehn wrote:
>
https://codereview.chromium.org/2375293002/diff/60001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/dom/Node.cpp (right):
> 
>
https://codereview.chromium.org/2375293002/diff/60001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/dom/Node.cpp:695: // TODO(nainar): Move this to
Node::markAncestorsWithChildNeedsReattachLayoutTree()
> Hmm, why do you want to move this?

It makes sense to move all layout scheduling to after all the relevant reattach
bits have been marked as opposed to after the stylerecalc bits have been sit.  

> I'd probably go about setting the bits in the same patch you start processing
them. Right now this is dead code since nothing clears the bits. You'll want
tree correctness asserts then too, you'll want to 
> update assertLayoutTreeUpdated to DCHECK that both of these bits are false.

Sure, I was attempting to break up patches into smaller pieces but will combine
if needed.

[Bugs Nash, 2016-09-29 04:45:32]

On 2016/09/29 at 04:39:28, nainar wrote:
> On 2016/09/29 at 04:32:48, esprehn wrote:
> >
https://codereview.chromium.org/2375293002/diff/60001/third_party/WebKit/Sour...
> > File third_party/WebKit/Source/core/dom/Node.cpp (right):
> > 
> >
https://codereview.chromium.org/2375293002/diff/60001/third_party/WebKit/Sour...
> > third_party/WebKit/Source/core/dom/Node.cpp:695: // TODO(nainar): Move this
to Node::markAncestorsWithChildNeedsReattachLayoutTree()
> > Hmm, why do you want to move this?
> 
> It makes sense to move all layout scheduling to after all the relevant
reattach bits have been marked as opposed to after the stylerecalc bits have
been sit.  
> 
> > I'd probably go about setting the bits in the same patch you start
processing them. Right now this is dead code since nothing clears the bits.
You'll want tree correctness asserts then too, you'll want to 
> > update assertLayoutTreeUpdated to DCHECK that both of these bits are false.
> 
> Sure, I was attempting to break up patches into smaller pieces but will
combine if needed.


Yeah Elliott is right. Small patches are good, but they should still be a
functional unit. Setting the bits in and of itself isn't a useful unit of code -
it goes hand in hand with using the values.

[esprehn, 2016-09-29 05:07:57]

On 2016/09/29 at 04:39:28, nainar wrote:
> On 2016/09/29 at 04:32:48, esprehn wrote:
> >
https://codereview.chromium.org/2375293002/diff/60001/third_party/WebKit/Sour...
> > File third_party/WebKit/Source/core/dom/Node.cpp (right):
> > 
> >
https://codereview.chromium.org/2375293002/diff/60001/third_party/WebKit/Sour...
> > third_party/WebKit/Source/core/dom/Node.cpp:695: // TODO(nainar): Move this
to Node::markAncestorsWithChildNeedsReattachLayoutTree()
> > Hmm, why do you want to move this?
> 
> It makes sense to move all layout scheduling to after all the relevant
reattach bits have been marked as opposed to after the stylerecalc bits have
been sit.  
> 

Ah, I think that's a misunderstanding. These reattach bits are set *inside*
style recalc. Per the pseudo code:

Document::updateStyle
  recalcStyle (step 1)
  rebuildLayoutTree (step 2)

The rebuild bits are only needed inside step 1, inside updateStyle. You
shouldn't need to schedule a frame unless we're setting these bits outside of
style recalc.

The setNeedsStyleRecalc method is called *outside* style recalc, for example if
I append a new child or change a class. It must schedule a frame so the pipeline
will run and update the style to clear out the dirty bits.

We might need a call to scheduleLayoutTreeUpdateIfNeeded in both code paths if
we start calling setNeedsReattachLayoutTree outside style recalc, but the
current plan is to always call it inside. So you probably want
DCHECK(document().inStyleRecalc()) instead.

[nainar, 2016-09-29 05:14:32]

On 2016/09/29 at 05:07:57, esprehn wrote:
> On 2016/09/29 at 04:39:28, nainar wrote:
> > On 2016/09/29 at 04:32:48, esprehn wrote:
> > >
https://codereview.chromium.org/2375293002/diff/60001/third_party/WebKit/Sour...
> > > File third_party/WebKit/Source/core/dom/Node.cpp (right):
> > > 
> > >
https://codereview.chromium.org/2375293002/diff/60001/third_party/WebKit/Sour...
> > > third_party/WebKit/Source/core/dom/Node.cpp:695: // TODO(nainar): Move
this to Node::markAncestorsWithChildNeedsReattachLayoutTree()
> > > Hmm, why do you want to move this?
> > 
> > It makes sense to move all layout scheduling to after all the relevant
reattach bits have been marked as opposed to after the stylerecalc bits have
been sit.  
> > 
> 
> Ah, I think that's a misunderstanding. These reattach bits are set *inside*
style recalc. Per the pseudo code:
> 
> Document::updateStyle
>   recalcStyle (step 1)
>   rebuildLayoutTree (step 2)
> 
> The rebuild bits are only needed inside step 1, inside updateStyle. You
shouldn't need to schedule a frame unless we're setting these bits outside of
style recalc.
> 
> The setNeedsStyleRecalc method is called *outside* style recalc, for example
if I append a new child or change a class. It must schedule a frame so the
pipeline will run and update the style to clear out the dirty bits.

I didn't factor in the case where setNeedsStyleRecalc might be called from other
functions. Retaining the call to scheduleLayoutTreeUpdateIfNeeded() in that case
does make sense. Thanks for the explanation!

> We might need a call to scheduleLayoutTreeUpdateIfNeeded in both code paths if
we start calling setNeedsReattachLayoutTree outside style recalc, but the
current plan is to always call it inside. So you probably want
DCHECK(document().inStyleRecalc()) instead.

Will add the DCHECK  in next round of review.

[commit-bot: I haz the power, 2016-09-29 05:27:54]

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

[commit-bot: I haz the power, 2016-09-29 05:27:55]

Dry run: This issue passed the CQ dry run.

[nainar, 2016-10-05 12:27:39]

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

[commit-bot: I haz the power, 2016-10-05 12:27:53]

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-05 14:22:57]

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

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

Dry run: Exceeded global retry quota

[esprehn, 2016-10-08 03:34:46]

Is this ready for review now? Could you update the asserts in
assertLayoutTreeUpdated? Otherwise this patch lgtm

[nainar, 2016-10-09 23:17:19]

Since we all agreed that just setting this bits without start processing them.
So I merged in this change into the code here:
https://codereview.chromium.org/2398293003. I've updated
assertLayoutTreeUpdated() there as well as made sure we are clearing the bits
where needed.

[nainar, 2016-10-09 23:18:26]

On 2016/10/09 at 23:17:19, nainar wrote:
> Since we all agreed that just setting this bits without start processing them.
So I merged in this change into the code here:
https://codereview.chromium.org/2398293003. I've updated
assertLayoutTreeUpdated() there as well as made sure we are clearing the bits
where needed.

Also going to close this CL - so that there are fewer confusions here. Sorry
about that