Don't schedule invalidations when attributes changed if not needed or incorrect.

Don't schedule invalidations when attributes changed if not needed or incorrect.

In particular, don't do it  when attributes changed if there
is no style resolver or the style change type is subtree or
greater. In both of these cases, it will either have no
additional effect or the code is incorrect.

It could be incorrect if the element was not yet attached.
It could be not needed because if there is no style resolver,
re-making it will recalc the whole document's style. Also,
style invalidation cannot trigger a style that is greater than
subtree.

Also clear style invalidation bits unconditionally in Node::detach.

BUG=366788
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=173665

[esprehn, 2014-05-08 05:19:05]

https://codereview.chromium.org/273783003/diff/20001/Source/core/css/invalida...
File Source/core/css/invalidation/StyleInvalidator.cpp (right):

https://codereview.chromium.org/273783003/diff/20001/Source/core/css/invalida...
Source/core/css/invalidation/StyleInvalidator.cpp:31:
ASSERT(element.inActiveDocument() && element.styleChangeType() <
SubtreeStyleChange);
Lets split this into two ASSERTs, one per line. When you combine asserts of
totally different properties like this it's hard to know which one is hit in a
stack trace.

https://codereview.chromium.org/273783003/diff/20001/Source/core/dom/Element.h
File Source/core/dom/Element.h (right):

https://codereview.chromium.org/273783003/diff/20001/Source/core/dom/Element....
Source/core/dom/Element.h:819: ASSERT(!childNeedsStyleInvalidation() &&
!needsStyleInvalidation());
ditto

https://codereview.chromium.org/273783003/diff/20001/Source/core/dom/Node.cpp
File Source/core/dom/Node.cpp (right):

https://codereview.chromium.org/273783003/diff/20001/Source/core/dom/Node.cpp...
Source/core/dom/Node.cpp:1026: clearNeedsStyleInvalidation();
You don't need to clear these bits inside clearInvalidation now.

[chrishtr, 2014-05-08 05:29:18]

https://codereview.chromium.org/273783003/diff/20001/Source/core/css/invalida...
File Source/core/css/invalidation/StyleInvalidator.cpp (right):

https://codereview.chromium.org/273783003/diff/20001/Source/core/css/invalida...
Source/core/css/invalidation/StyleInvalidator.cpp:31:
ASSERT(element.inActiveDocument() && element.styleChangeType() <
SubtreeStyleChange);
On 2014/05/08 05:19:06, esprehn wrote:
> Lets split this into two ASSERTs, one per line. When you combine asserts of
> totally different properties like this it's hard to know which one is hit in a
> stack trace.

Done.

https://codereview.chromium.org/273783003/diff/20001/Source/core/dom/Element.h
File Source/core/dom/Element.h (right):

https://codereview.chromium.org/273783003/diff/20001/Source/core/dom/Element....
Source/core/dom/Element.h:819: ASSERT(!childNeedsStyleInvalidation() &&
!needsStyleInvalidation());
On 2014/05/08 05:19:06, esprehn wrote:
> ditto

Done.

https://codereview.chromium.org/273783003/diff/20001/Source/core/dom/Node.cpp
File Source/core/dom/Node.cpp (right):

https://codereview.chromium.org/273783003/diff/20001/Source/core/dom/Node.cpp...
Source/core/dom/Node.cpp:1026: clearNeedsStyleInvalidation();
On 2014/05/08 05:19:06, esprehn wrote:
> You don't need to clear these bits inside clearInvalidation now.

Done. Forgot to upload that patch set earlier.

[esprehn, 2014-05-08 07:24:37]

lgtm

[commit-bot: I haz the power, 2014-05-08 07:25:18]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/chrishtr@chromium.org/273783003/40001

[commit-bot: I haz the power, 2014-05-08 08:53:58]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  linux_blink_dbg on tryserver.blink

[commit-bot: I haz the power, 2014-05-08 10:22:10]

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

[commit-bot: I haz the power, 2014-05-08 10:22:10]

Try jobs failed on following builders:
  linux_blink_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/6913)

[chrishtr, 2014-05-08 17:40:26]

Added some styleChangeType() < SubtreeStyleChange
conditionals in ContainerNode for the case of scheduling
style recalc for focus/active change.

[chrishtr, 2014-05-08 17:40:30]

The CQ bit was checked by chrishtr@chromium.org

[commit-bot: I haz the power, 2014-05-08 17:41:39]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/chrishtr@chromium.org/273783003/50007

[commit-bot: I haz the power, 2014-05-08 19:00:04]

Change committed as 173665

[ojan, 2014-05-08 19:14:56]

https://codereview.chromium.org/273783003/diff/50007/Source/core/dom/Element.cpp
File Source/core/dom/Element.cpp (right):

https://codereview.chromium.org/273783003/diff/50007/Source/core/dom/Element....
Source/core/dom/Element.cpp:2804: if (inActiveDocument() &&
document().styleResolver() && styleChangeType() < SubtreeStyleChange)
We chatted about this in person. I'm not sure the styleResolver null-check here
is correct. Re-creating the style resolver does not AFAIK cause a subtree
recalc.

[esprehn, 2014-05-08 19:17:55]

https://codereview.chromium.org/273783003/diff/50007/Source/core/dom/Element.cpp
File Source/core/dom/Element.cpp (right):

https://codereview.chromium.org/273783003/diff/50007/Source/core/dom/Element....
Source/core/dom/Element.cpp:2804: if (inActiveDocument() &&
document().styleResolver() && styleChangeType() < SubtreeStyleChange)
On 2014/05/08 19:14:56, ojan wrote:
> We chatted about this in person. I'm not sure the styleResolver null-check
here
> is correct. Re-creating the style resolver does not AFAIK cause a subtree
> recalc.

Yeah it doesn't seem like we do that anymore, want to post a patch that removes
all three of them?

[chrishtr, 2014-05-08 19:54:10]

I'll post a patch.