Persist invalidation sets on detach root

Persist invalidation sets on detach root

This is a second attempt at not clearing sibling invalidation sets on
detached and SubtreeStyleChange nodes. The first[1] failed because
childNeedsStyleInvalidation() was still cleared on detach().

This approach keeps invalidation sets on the detach root, but clears
invalidation sets on descendants. However, when we detach a node to be
removed, we also clear the detach root as that will not have a chance of
being used on the next invalidation. Also, there are asserts checking
that we don't re-insert nodes into the tree with needsStyleInvalidation().

Removed scheduled sibling invalidation sets from nodes being removed from
the tree is not a problem as the checkForSiblingStyleChanges() method
will make sure sibling trees are invalidated.

The early return in StyleInvalidator::scheduleInvalidationSetsForElement
is removed to be able to schedule sibling invalidation sets although we
have a SubtreeStyleChange or ReattachStyleChange on the element itself.

The added test checks that the invalidation works properly, also after
the checkForChildrenAdjacentRuleChanges() removal.

[1] https://codereview.chromium.org/1507653002

R=esprehn@chromium.org,ericwilligers@chromium.org
BUG=557440
Committed: https://crrev.com/8559ce4ad0e473907604f6ecd51c80a06880cc29
Cr-Commit-Position: refs/heads/master@{#369021}

[rune, 2015-12-16 23:26:21]

The CQ bit was checked by rune@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2015-12-16 23:27:02]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1533683002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1533683002/1

[commit-bot: I haz the power, 2015-12-17 02:45:00]

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

[commit-bot: I haz the power, 2015-12-17 02:45:00]

Dry run: This issue passed the CQ dry run.

[rune, 2015-12-18 08:18:57]

ptal

[esprehn, 2015-12-19 03:35:51]

This doesn't feel right, it means we still do invalidation even if something
caused an eager detach. I think you just want to persist the invalidation sets
of the top level element when doing a detach. Perhaps inside ContainerNode when
we call ->detach?

[rune, 2015-12-22 10:13:54]

Description was changed from

==========
Clear (child)NeedsStyleInvalidation on DOM removal.

This is a second attempt at moving clearance of needsStyleInvalidation
from detach() to removedFrom(). The first[1] failed because
childNeedsStyleInvalidation() was still cleared on detach().

The reason why we cannot clear scheduled invalidations on detach() is
that there might be a sibling invalidation set, scheduled for the element
being detached, which should invalidate some element(s) in the sibling
forest of the detached element. It's not a problem as long as we have the
checkForChildrenAdjacentRuleChanges() method on ContainerNode which
causes a subtree recalc of the sibling forest. But when that method is
going away, we will rely on the sibling invalidation to be done
correctly.

The early return in StyleInvalidator::scheduleInvalidationSetsForElement
is removed to be able to schedule sibling invalidation sets although we
have a SubtreeStyleChange or ReattachStyleChange on the element itself.

The added test checks that the invalidation works properly, also after
the checkForChildrenAdjacentRuleChanges() removal.

[1] https://codereview.chromium.org/1507653002

R=esprehn@chromium.org,ericwilligers@chromium.org
BUG=557440
==========

to

==========
Clear (child)NeedsStyleInvalidation on DOM removal.

This is a second attempt at moving clearance of needsStyleInvalidation
from detach() to removedFrom(). The first[1] failed because
childNeedsStyleInvalidation() was still cleared on detach().

This approach keeps invalidation sets on the detach root, but clears
invalidation sets on descendants. However, when we detach a node to be
removed, we also clear the detach root as that will not have a chance of
being used on the next invalidation. Also, there are asserts checking
that we don't re-insert nodes into the tree with needsStyleInvalidation().

Removed scheduled sibling invalidation sets from nodes being removed from
the tree is not a problem as the checkForSiblingStyleChanges() method
will make sure sibling trees are invalidated.

The early return in StyleInvalidator::scheduleInvalidationSetsForElement
is removed to be able to schedule sibling invalidation sets although we
have a SubtreeStyleChange or ReattachStyleChange on the element itself.

The added test checks that the invalidation works properly, also after
the checkForChildrenAdjacentRuleChanges() removal.

[1] https://codereview.chromium.org/1507653002

R=esprehn@chromium.org,ericwilligers@chromium.org
BUG=557440
==========

[rune, 2015-12-22 10:24:46]

On 2015/12/19 03:35:51, esprehn wrote:
> This doesn't feel right, it means we still do invalidation even if something
> caused an eager detach. I think you just want to persist the invalidation sets
> of the top level element when doing a detach. Perhaps inside ContainerNode
when
> we call ->detach?

I've modified the patch to not clear invalidation sets on the detach root unless
the detach root is a node being removed from the tree.

Note, the needsAttach() condition on detach() in removeBetween() is gone because
it may have been detached with the invalidations set intact, but they have to go
when removed from the tree.

I noticed that if we removed a node which is !needsAttach() it would still
detach all descendants even if they all were needsAttach(). If you think this
unconditional detach is a problem, we could always add an else for needsAttach()
where we only clear the invalidation.

[rune, 2015-12-22 10:34:23]

Description was changed from

==========
Clear (child)NeedsStyleInvalidation on DOM removal.

This is a second attempt at moving clearance of needsStyleInvalidation
from detach() to removedFrom(). The first[1] failed because
childNeedsStyleInvalidation() was still cleared on detach().

This approach keeps invalidation sets on the detach root, but clears
invalidation sets on descendants. However, when we detach a node to be
removed, we also clear the detach root as that will not have a chance of
being used on the next invalidation. Also, there are asserts checking
that we don't re-insert nodes into the tree with needsStyleInvalidation().

Removed scheduled sibling invalidation sets from nodes being removed from
the tree is not a problem as the checkForSiblingStyleChanges() method
will make sure sibling trees are invalidated.

The early return in StyleInvalidator::scheduleInvalidationSetsForElement
is removed to be able to schedule sibling invalidation sets although we
have a SubtreeStyleChange or ReattachStyleChange on the element itself.

The added test checks that the invalidation works properly, also after
the checkForChildrenAdjacentRuleChanges() removal.

[1] https://codereview.chromium.org/1507653002

R=esprehn@chromium.org,ericwilligers@chromium.org
BUG=557440
==========

to

==========
Persist invalidation sets on detach root

This is a second attempt at not clearing sibling invalidation sets on
detached and SubtreeStyleChange nodes. The first[1] failed because
childNeedsStyleInvalidation() was still cleared on detach().

This approach keeps invalidation sets on the detach root, but clears
invalidation sets on descendants. However, when we detach a node to be
removed, we also clear the detach root as that will not have a chance of
being used on the next invalidation. Also, there are asserts checking
that we don't re-insert nodes into the tree with needsStyleInvalidation().

Removed scheduled sibling invalidation sets from nodes being removed from
the tree is not a problem as the checkForSiblingStyleChanges() method
will make sure sibling trees are invalidated.

The early return in StyleInvalidator::scheduleInvalidationSetsForElement
is removed to be able to schedule sibling invalidation sets although we
have a SubtreeStyleChange or ReattachStyleChange on the element itself.

The added test checks that the invalidation works properly, also after
the checkForChildrenAdjacentRuleChanges() removal.

[1] https://codereview.chromium.org/1507653002

R=esprehn@chromium.org,ericwilligers@chromium.org
BUG=557440
==========

[rune, 2016-01-05 08:47:55]

ptal

[rune, 2016-01-11 11:38:46]

ping

[esprehn, 2016-01-12 20:47:28]

Yeah this seems right. Lgtm

I wondered if there was an implicit way to detect the root but I can't think of
one.

[rune, 2016-01-12 21:03:02]

The CQ bit was checked by rune@opera.com

[commit-bot: I haz the power, 2016-01-12 21:05:10]

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

[commit-bot: I haz the power, 2016-01-12 23:00:35]

Description was changed from

==========
Persist invalidation sets on detach root

This is a second attempt at not clearing sibling invalidation sets on
detached and SubtreeStyleChange nodes. The first[1] failed because
childNeedsStyleInvalidation() was still cleared on detach().

This approach keeps invalidation sets on the detach root, but clears
invalidation sets on descendants. However, when we detach a node to be
removed, we also clear the detach root as that will not have a chance of
being used on the next invalidation. Also, there are asserts checking
that we don't re-insert nodes into the tree with needsStyleInvalidation().

Removed scheduled sibling invalidation sets from nodes being removed from
the tree is not a problem as the checkForSiblingStyleChanges() method
will make sure sibling trees are invalidated.

The early return in StyleInvalidator::scheduleInvalidationSetsForElement
is removed to be able to schedule sibling invalidation sets although we
have a SubtreeStyleChange or ReattachStyleChange on the element itself.

The added test checks that the invalidation works properly, also after
the checkForChildrenAdjacentRuleChanges() removal.

[1] https://codereview.chromium.org/1507653002

R=esprehn@chromium.org,ericwilligers@chromium.org
BUG=557440
==========

to

==========
Persist invalidation sets on detach root

This is a second attempt at not clearing sibling invalidation sets on
detached and SubtreeStyleChange nodes. The first[1] failed because
childNeedsStyleInvalidation() was still cleared on detach().

This approach keeps invalidation sets on the detach root, but clears
invalidation sets on descendants. However, when we detach a node to be
removed, we also clear the detach root as that will not have a chance of
being used on the next invalidation. Also, there are asserts checking
that we don't re-insert nodes into the tree with needsStyleInvalidation().

Removed scheduled sibling invalidation sets from nodes being removed from
the tree is not a problem as the checkForSiblingStyleChanges() method
will make sure sibling trees are invalidated.

The early return in StyleInvalidator::scheduleInvalidationSetsForElement
is removed to be able to schedule sibling invalidation sets although we
have a SubtreeStyleChange or ReattachStyleChange on the element itself.

The added test checks that the invalidation works properly, also after
the checkForChildrenAdjacentRuleChanges() removal.

[1] https://codereview.chromium.org/1507653002

R=esprehn@chromium.org,ericwilligers@chromium.org
BUG=557440
==========

[commit-bot: I haz the power, 2016-01-12 23:00:36]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-01-12 23:01:37]

Description was changed from

==========
Persist invalidation sets on detach root

This is a second attempt at not clearing sibling invalidation sets on
detached and SubtreeStyleChange nodes. The first[1] failed because
childNeedsStyleInvalidation() was still cleared on detach().

This approach keeps invalidation sets on the detach root, but clears
invalidation sets on descendants. However, when we detach a node to be
removed, we also clear the detach root as that will not have a chance of
being used on the next invalidation. Also, there are asserts checking
that we don't re-insert nodes into the tree with needsStyleInvalidation().

Removed scheduled sibling invalidation sets from nodes being removed from
the tree is not a problem as the checkForSiblingStyleChanges() method
will make sure sibling trees are invalidated.

The early return in StyleInvalidator::scheduleInvalidationSetsForElement
is removed to be able to schedule sibling invalidation sets although we
have a SubtreeStyleChange or ReattachStyleChange on the element itself.

The added test checks that the invalidation works properly, also after
the checkForChildrenAdjacentRuleChanges() removal.

[1] https://codereview.chromium.org/1507653002

R=esprehn@chromium.org,ericwilligers@chromium.org
BUG=557440
==========

to

==========
Persist invalidation sets on detach root

This is a second attempt at not clearing sibling invalidation sets on
detached and SubtreeStyleChange nodes. The first[1] failed because
childNeedsStyleInvalidation() was still cleared on detach().

This approach keeps invalidation sets on the detach root, but clears
invalidation sets on descendants. However, when we detach a node to be
removed, we also clear the detach root as that will not have a chance of
being used on the next invalidation. Also, there are asserts checking
that we don't re-insert nodes into the tree with needsStyleInvalidation().

Removed scheduled sibling invalidation sets from nodes being removed from
the tree is not a problem as the checkForSiblingStyleChanges() method
will make sure sibling trees are invalidated.

The early return in StyleInvalidator::scheduleInvalidationSetsForElement
is removed to be able to schedule sibling invalidation sets although we
have a SubtreeStyleChange or ReattachStyleChange on the element itself.

The added test checks that the invalidation works properly, also after
the checkForChildrenAdjacentRuleChanges() removal.

[1] https://codereview.chromium.org/1507653002

R=esprehn@chromium.org,ericwilligers@chromium.org
BUG=557440

Committed: https://crrev.com/8559ce4ad0e473907604f6ecd51c80a06880cc29
Cr-Commit-Position: refs/heads/master@{#369021}
==========

[commit-bot: I haz the power, 2016-01-12 23:01:38]

Patchset 3 (id:??) landed as
https://crrev.com/8559ce4ad0e473907604f6ecd51c80a06880cc29
Cr-Commit-Position: refs/heads/master@{#369021}