Don't early return on SubtreeStyleChange for scheduling invalidations.

Don't early return on SubtreeStyleChange for scheduling invalidations.

Sibling invalidation sets still need to be scheduled for elements with
SubtreeStyleChange when SubtreeStyleChange is for strict subtree.

R=dstockwell@chromium.org,ericwilligers@chromium.org
BUG=557440
Committed: https://crrev.com/8050c33e5d32973efecfff584d055666b5361e8c
Cr-Commit-Position: refs/heads/master@{#364092}

[rune, 2015-12-08 09:55:43]

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

[commit-bot: I haz the power, 2015-12-08 09:56:23]

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

[rune, 2015-12-08 09:56:44]

The CQ bit was unchecked by rune@opera.com

[Eric Willigers, 2015-12-09 12:36:30]

lgtm

[rune, 2015-12-09 13:16:15]

The CQ bit was checked by rune@opera.com

[commit-bot: I haz the power, 2015-12-09 13:16:24]

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

[commit-bot: I haz the power, 2015-12-09 13:16:27]

This CL has an open dependency (Issue 1507653002 Patch 1). Please resolve the
dependency and try again.

[rune, 2015-12-09 15:07:30]

The CQ bit was checked by rune@opera.com

[rune, 2015-12-09 15:07:31]

The patchset sent to the CQ was uploaded after l-g-t-m from
ericwilligers@chromium.org
Link to the patchset: https://codereview.chromium.org/1503993002/#ps20001
(title: "Rebased")

[commit-bot: I haz the power, 2015-12-09 15:09:46]

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

[commit-bot: I haz the power, 2015-12-09 17:10:49]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-12-09 17:11:26]

Description was changed from

==========
Don't early return on SubtreeStyleChange for scheduling invalidations.

Sibling invalidation sets still need to be scheduled for elements with
SubtreeStyleChange when SubtreeStyleChange is for strict subtree.

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

to

==========
Don't early return on SubtreeStyleChange for scheduling invalidations.

Sibling invalidation sets still need to be scheduled for elements with
SubtreeStyleChange when SubtreeStyleChange is for strict subtree.

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

Committed: https://crrev.com/8050c33e5d32973efecfff584d055666b5361e8c
Cr-Commit-Position: refs/heads/master@{#364092}
==========

[commit-bot: I haz the power, 2015-12-09 17:11:27]

Patchset 2 (id:??) landed as
https://crrev.com/8050c33e5d32973efecfff584d055666b5361e8c
Cr-Commit-Position: refs/heads/master@{#364092}

[esprehn, 2015-12-09 17:55:51]

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

[esprehn, 2015-12-09 17:55:51]

Hmm this means we schedule invalidation sets for brand new elements that haven't
been attached yet? That's unfortunate, previously if you appended a bunch of
DOM, and set properties it wouldn't do any extra invalidation work.

For example this is going to make polymer slower since the elements modify
themselves in attached callbacks. I also worry it will make the HashMap in the
StyleInvalidator approach the size of the whole DOM since setting a class after
appendChild like appendChild().className schedules invalidation sets now.

We need some early outs for when there's no sibling sets to schedule.

[rune, 2015-12-09 18:37:31]

On 2015/12/09 17:55:51, esprehn wrote:
> Hmm this means we schedule invalidation sets for brand new elements that
haven't
> been attached yet? That's unfortunate, previously if you appended a bunch of
> DOM, and set properties it wouldn't do any extra invalidation work.

It's centralized to scheduleInvalidationSetsForElement() now. There is an added
"element.styleChangeType() < SubtreeStyleChange" guard for descendant sets
there. That means we'll collect invalidation sets before we skip scheduling the
invalidation, so it's a bit more costly.

> For example this is going to make polymer slower since the elements modify
> themselves in attached callbacks. I also worry it will make the HashMap in the
> StyleInvalidator approach the size of the whole DOM since setting a class
after
> appendChild like appendChild().className schedules invalidation sets now.
> 
> We need some early outs for when there's no sibling sets to schedule.

I think we can return earlier by checking Node::inDocument()? Also, we could
check Node::nextSibling() and parentNode() < SubtreeStyleChange before
scheduling any sibling invalidation sets. I can prepare another CL.

[rune, 2015-12-09 18:59:20]

On 2015/12/09 18:37:31, rune wrote:

> I think we can return earlier by checking Node::inDocument()? Also, we could
> check Node::nextSibling() and parentNode() < SubtreeStyleChange before
> scheduling any sibling invalidation sets. I can prepare another CL.

There are inActiveDocument() checks already. Need to go through and check.

[esprehn, 2015-12-09 19:05:21]

These elements are often already in the document, imagine
document.body.appendChild().className, there's template systems that do
that but thousands of times in a row building up content. The parent check
sounds good, can we also check if the new invalidation set contains any
sibling sets? If not we can skip it if >= Subtree
On Dec 9, 2015 10:59 AM, <rune@opera.com> wrote:

> On 2015/12/09 18:37:31, rune wrote:
>
> I think we can return earlier by checking Node::inDocument()? Also, we
>> could
>> check Node::nextSibling() and parentNode() < SubtreeStyleChange before
>> scheduling any sibling invalidation sets. I can prepare another CL.
>>
>
> There are inActiveDocument() checks already. Need to go through and check.
>
>
>
> https://codereview.chromium.org/1503993002/
>
> --
> You received this message because you are subscribed to the Google Groups
> "Blink Reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to blink-reviews+unsubscribe@chromium.org.
>
>

-- 
You received this message because you are subscribed to the Google Groups "Blink
Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[esprehn, 2015-12-09 19:05:21]

These elements are often already in the document, imagine
document.body.appendChild().className, there's template systems that do
that but thousands of times in a row building up content. The parent check
sounds good, can we also check if the new invalidation set contains any
sibling sets? If not we can skip it if >= Subtree
On Dec 9, 2015 10:59 AM, <rune@opera.com> wrote:

> On 2015/12/09 18:37:31, rune wrote:
>
> I think we can return earlier by checking Node::inDocument()? Also, we
>> could
>> check Node::nextSibling() and parentNode() < SubtreeStyleChange before
>> scheduling any sibling invalidation sets. I can prepare another CL.
>>
>
> There are inActiveDocument() checks already. Need to go through and check.
>
>
>
> https://codereview.chromium.org/1503993002/
>
> --
> You received this message because you are subscribed to the Google Groups
> "Blink Reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to blink-reviews+unsubscribe@chromium.org.
>
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[rune, 2015-12-10 00:32:20]

On 2015/12/09 19:05:21, esprehn wrote:

> These elements are often already in the document, imagine
> document.body.appendChild().className, there's template systems that do
> that but thousands of times in a row building up content. The parent check
> sounds good, can we also check if the new invalidation set contains any
> sibling sets? If not we can skip it if >= Subtree

The check for >= Subtree is already done for descendant invalidation sets in
scheduleInvalidationSetsForElement. I've added a parent check and a nextSibling
check for sibling sets in https://codereview.chromium.org/1514733002/