Implement style invalidation tree walk for targeted style recalc upon class change.

Implement style invalidation tree walk for targeted style recalc upon class change.

"Style invalidation" is defined as the process of deciding
whether elements need style recalculation, based on the
input of a particular DOM mutation.

When the class changes for an element, a style invalidation
bit is recorded on it. Just before style is subsequently
recomputed on the document, walks the tree to determine
which elements have style invalidation needed, and for those
elements, calls setNeedsStyleRecalc() on the elements of the
element subtree that need it. If we can't figure out which
elements exactly need style recalculation, we fall back on
the whole subtree.

All layout tests pass with and without the runtime flag for
this feature.

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

[esprehn, 2014-01-27 23:51:57]

Your change description isn't right. :)

[chrishtr, 2014-01-28 00:01:52]

Updated & elaborated, sorry about that.


On Mon, Jan 27, 2014 at 3:51 PM, <esprehn@chromium.org> wrote:

> Your change description isn't right. :)
>
> https://codereview.chromium.org/143873016/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[esprehn, 2014-01-28 00:11:33]

We need a perf test to go with this change. You also want to add a layout test
that uses internals.updateStyleAndReturnAffectedElementCount() to show that if
you add and remove a bunch of classes the number of affected elements is what
you expect.

https://codereview.chromium.org/143873016/diff/600001/Source/core/css/RuleFea...
File Source/core/css/RuleFeature.cpp (right):

https://codereview.chromium.org/143873016/diff/600001/Source/core/css/RuleFea...
Source/core/css/RuleFeature.cpp:251: &&
!RuntimeEnabledFeatures::targetedStyleRecalcEnabled())
Your flag checks need to always be first, otherwise we run code even with the
flag disabled. We usually keep it separate too.

if (!RuntimeEnabledFeatures::targetedStyleRecalcEnabled())
  return;

if (scheduleStyleInvalidationForClassChangeInternal(changedClasses, element))
  element->setNeedsStyleRecalc();

https://codereview.chromium.org/143873016/diff/600001/Source/core/css/RuleFea...
Source/core/css/RuleFeature.cpp:259: element->setNeedsStyleRecalc();
Ditto.

https://codereview.chromium.org/143873016/diff/600001/Source/core/css/RuleFea...
Source/core/css/RuleFeature.cpp:316: void
RuleFeatureSet::scheduleStyleInvalidationForOneClass(const AtomicString&
className, Element* element)
ForClass(), no need for the "OneClass". Multiple classes are SpaceSplitStrings.

https://codereview.chromium.org/143873016/diff/600001/Source/core/css/RuleFea...
Source/core/css/RuleFeature.cpp:324: RuleFeatureSet::InvalidationVec*
RuleFeatureSet::ensurePendingInvalidationVector(Element* element)
Returns a ref.

https://codereview.chromium.org/143873016/diff/600001/Source/core/css/RuleFea...
Source/core/css/RuleFeature.cpp:329: return addResult.iterator->value;
*addResult->value

https://codereview.chromium.org/143873016/diff/600001/Source/core/css/RuleFea...
Source/core/css/RuleFeature.cpp:360: someChildrenNeedStyleRecalc =
someChildrenNeedStyleRecalc || childRecalced;
Remove this boolean stuff.

https://codereview.chromium.org/143873016/diff/600001/Source/core/css/RuleFea...
Source/core/css/RuleFeature.cpp:370: if (InvalidationVec* invalidationVec =
m_pendingInvalidationMap.get(element)) {
You want two bits, you're doing tons of hash map lookups when they're not
needed.  One bit for self and one for child.

https://codereview.chromium.org/143873016/diff/600001/Source/core/css/RuleFea...
Source/core/css/RuleFeature.cpp:405: thisElementNeedsStyleRecalc =
thisElementNeedsStyleRecalc || someChildrenNeedStyleRecalc;
Instead of returning this bool, just walk the ancestors below.

https://codereview.chromium.org/143873016/diff/600001/Source/core/css/RuleFea...
Source/core/css/RuleFeature.cpp:409:
element->setNeedsStyleRecalc(LocalStyleChange);
This should just walk ancestors:

for (Element* ancestor = element; element; element =
element->parentOrShadowHostNode()) {
  if (!element->needsStyleRecalc())
     element->setNeedsStyleRecalc(LocalStyleChange);
}

https://codereview.chromium.org/143873016/diff/600001/Source/core/css/RuleFea...
File Source/core/css/RuleFeature.h (right):

https://codereview.chromium.org/143873016/diff/600001/Source/core/css/RuleFea...
Source/core/css/RuleFeature.h:86: void
scheduleStyleInvalidationForClassChange(const SpaceSplitString& changedClasses,
Element*);
Element&

https://codereview.chromium.org/143873016/diff/600001/Source/core/css/RuleFea...
Source/core/css/RuleFeature.h:87: void
scheduleStyleInvalidationForClassChange(const SpaceSplitString& oldClasses,
const SpaceSplitString& newClasses, Element*);
Element&

https://codereview.chromium.org/143873016/diff/600001/Source/core/css/RuleFea...
Source/core/css/RuleFeature.h:90: void computeStyleInvalidation(Document*);
Document&

https://codereview.chromium.org/143873016/diff/600001/Source/core/css/RuleFea...
Source/core/css/RuleFeature.h:106: typedef Vector<DescendantInvalidationSet*>
InvalidationVec;
Don't use abbreviations like Vec. I'd use InvalidationList.

https://codereview.chromium.org/143873016/diff/600001/Source/core/css/RuleFea...
Source/core/css/RuleFeature.h:107: typedef HashMap<Element*, InvalidationVec* >
PendingInvalidationMap;
No need for the space before >

https://codereview.chromium.org/143873016/diff/600001/Source/core/css/RuleFea...
Source/core/css/RuleFeature.h:127: bool
scheduleStyleInvalidationForClassChangeInternal(const SpaceSplitString&
oldClasses, const SpaceSplitString& newClasses, Element*);
Same, these should all take refs.

https://codereview.chromium.org/143873016/diff/600001/Source/core/dom/Documen...
File Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/143873016/diff/600001/Source/core/dom/Documen...
Source/core/dom/Document.cpp:1578: if (!styleResolver()) {
This isn't right, the resolver can get thrown away you'll miss doing
invalidations.

https://codereview.chromium.org/143873016/diff/600001/Source/core/dom/Documen...
Source/core/dom/Document.cpp:1583:
styleResolver()->ensureRuleFeatureSet().computeStyleInvalidation(this);
This can't work, the resolver can get thrown away. You either need to move the
logic to the StyleEngine or get rid of the throw away timer.

https://codereview.chromium.org/143873016/diff/600001/Source/core/dom/Documen...
Source/core/dom/Document.cpp:1692: updateNeedsStyleRecalcStateIfNeeded();
This should be the last of these three steps, the updateUseShadowTrees() step
can mutate the DOM.

[esprehn, 2014-01-28 00:11:59]

btw, it's nice if you wrap your description at 80 cols or so.

[chrishtr, 2014-01-28 01:11:54]

Addressed all except layout test.

https://codereview.chromium.org/143873016/diff/600001/Source/core/css/RuleFea...
File Source/core/css/RuleFeature.cpp (right):

https://codereview.chromium.org/143873016/diff/600001/Source/core/css/RuleFea...
Source/core/css/RuleFeature.cpp:251: &&
!RuntimeEnabledFeatures::targetedStyleRecalcEnabled())
The situation here is a little subtle. The code in
scheduleStyleInvalidationForClassChangeInternal is the same as what used to be
in checkSelectorForClassName in Element.cpp, except taht it has a
RuntimeEnabledFeatures::targetedStyleRecalcEnabled() check at each of the points
when the logic differs from with the flag off. Further, the return value of
scheduleStyleInvalidationForClassChangeInternal is ignored when the flag is on
but not when it is off, hence the code here.

On 2014/01/28 00:11:34, esprehn wrote:
> Your flag checks need to always be first, otherwise we run code even with the
> flag disabled. We usually keep it separate too.
> 
> if (!RuntimeEnabledFeatures::targetedStyleRecalcEnabled())
>   return;
> 
> if (scheduleStyleInvalidationForClassChangeInternal(changedClasses, element))
>   element->setNeedsStyleRecalc();

https://codereview.chromium.org/143873016/diff/600001/Source/core/css/RuleFea...
Source/core/css/RuleFeature.cpp:259: element->setNeedsStyleRecalc();
On 2014/01/28 00:11:34, esprehn wrote:
> Ditto.

Same.

https://codereview.chromium.org/143873016/diff/600001/Source/core/css/RuleFea...
Source/core/css/RuleFeature.cpp:316: void
RuleFeatureSet::scheduleStyleInvalidationForOneClass(const AtomicString&
className, Element* element)
On 2014/01/28 00:11:34, esprehn wrote:
> ForClass(), no need for the "OneClass". Multiple classes are
SpaceSplitStrings.

Made name more clear, it's really about adding a descendant invalidation set.

https://codereview.chromium.org/143873016/diff/600001/Source/core/css/RuleFea...
Source/core/css/RuleFeature.cpp:324: RuleFeatureSet::InvalidationVec*
RuleFeatureSet::ensurePendingInvalidationVector(Element* element)
On 2014/01/28 00:11:34, esprehn wrote:
> Returns a ref.

Done.

https://codereview.chromium.org/143873016/diff/600001/Source/core/css/RuleFea...
Source/core/css/RuleFeature.cpp:329: return addResult.iterator->value;
On 2014/01/28 00:11:34, esprehn wrote:
> *addResult->value

Done.

https://codereview.chromium.org/143873016/diff/600001/Source/core/css/RuleFea...
Source/core/css/RuleFeature.cpp:360: someChildrenNeedStyleRecalc =
someChildrenNeedStyleRecalc || childRecalced;
On 2014/01/28 00:11:34, esprehn wrote:
> Remove this boolean stuff.

See below.

https://codereview.chromium.org/143873016/diff/600001/Source/core/css/RuleFea...
Source/core/css/RuleFeature.cpp:370: if (InvalidationVec* invalidationVec =
m_pendingInvalidationMap.get(element)) {
On 2014/01/28 00:11:34, esprehn wrote:
> You want two bits, you're doing tons of hash map lookups when they're not
> needed.  One bit for self and one for child.

Done.

https://codereview.chromium.org/143873016/diff/600001/Source/core/css/RuleFea...
Source/core/css/RuleFeature.cpp:405: thisElementNeedsStyleRecalc =
thisElementNeedsStyleRecalc || someChildrenNeedStyleRecalc;
On 2014/01/28 00:11:34, esprehn wrote:
> Instead of returning this bool, just walk the ancestors below.

See below.

https://codereview.chromium.org/143873016/diff/600001/Source/core/css/RuleFea...
Source/core/css/RuleFeature.cpp:409:
element->setNeedsStyleRecalc(LocalStyleChange);
That'll do a bunch of of extra tree walking. Inefficient?

On 2014/01/28 00:11:34, esprehn wrote:
> This should just walk ancestors:
> 
> for (Element* ancestor = element; element; element =
> element->parentOrShadowHostNode()) {
>   if (!element->needsStyleRecalc())
>      element->setNeedsStyleRecalc(LocalStyleChange);
> }

https://codereview.chromium.org/143873016/diff/600001/Source/core/css/RuleFea...
File Source/core/css/RuleFeature.h (right):

https://codereview.chromium.org/143873016/diff/600001/Source/core/css/RuleFea...
Source/core/css/RuleFeature.h:86: void
scheduleStyleInvalidationForClassChange(const SpaceSplitString& changedClasses,
Element*);
On 2014/01/28 00:11:34, esprehn wrote:
> Element&

Done.

https://codereview.chromium.org/143873016/diff/600001/Source/core/css/RuleFea...
Source/core/css/RuleFeature.h:87: void
scheduleStyleInvalidationForClassChange(const SpaceSplitString& oldClasses,
const SpaceSplitString& newClasses, Element*);
On 2014/01/28 00:11:34, esprehn wrote:
> Element&

Done.

https://codereview.chromium.org/143873016/diff/600001/Source/core/css/RuleFea...
Source/core/css/RuleFeature.h:90: void computeStyleInvalidation(Document*);
On 2014/01/28 00:11:34, esprehn wrote:
> Document&

Done.

https://codereview.chromium.org/143873016/diff/600001/Source/core/css/RuleFea...
Source/core/css/RuleFeature.h:106: typedef Vector<DescendantInvalidationSet*>
InvalidationVec;
On 2014/01/28 00:11:34, esprehn wrote:
> Don't use abbreviations like Vec. I'd use InvalidationList.

Done.

https://codereview.chromium.org/143873016/diff/600001/Source/core/css/RuleFea...
Source/core/css/RuleFeature.h:107: typedef HashMap<Element*, InvalidationVec* >
PendingInvalidationMap;
On 2014/01/28 00:11:34, esprehn wrote:
> No need for the space before >

Done.

https://codereview.chromium.org/143873016/diff/600001/Source/core/css/RuleFea...
Source/core/css/RuleFeature.h:127: bool
scheduleStyleInvalidationForClassChangeInternal(const SpaceSplitString&
oldClasses, const SpaceSplitString& newClasses, Element*);
On 2014/01/28 00:11:34, esprehn wrote:
> Same, these should all take refs.

The Element pointer is the key to the invalidation set, so I need it here.

https://codereview.chromium.org/143873016/diff/600001/Source/core/dom/Documen...
File Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/143873016/diff/600001/Source/core/dom/Documen...
Source/core/dom/Document.cpp:1578: if (!styleResolver()) {
On 2014/01/28 00:11:34, esprehn wrote:
> This isn't right, the resolver can get thrown away you'll miss doing
> invalidations.

Added FIXME.

https://codereview.chromium.org/143873016/diff/600001/Source/core/dom/Documen...
Source/core/dom/Document.cpp:1583:
styleResolver()->ensureRuleFeatureSet().computeStyleInvalidation(this);
On 2014/01/28 00:11:34, esprehn wrote:
> This can't work, the resolver can get thrown away. You either need to move the
> logic to the StyleEngine or get rid of the throw away timer.

Added FIXME.

https://codereview.chromium.org/143873016/diff/600001/Source/core/dom/Documen...
Source/core/dom/Document.cpp:1692: updateNeedsStyleRecalcStateIfNeeded();
On 2014/01/28 00:11:34, esprehn wrote:
> This should be the last of these three steps, the updateUseShadowTrees() step
> can mutate the DOM.

Done.

[ojan, 2014-01-29 00:50:07]

https://codereview.chromium.org/143873016/diff/640001/Source/core/dom/Documen...
File Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/143873016/diff/640001/Source/core/dom/Documen...
Source/core/dom/Document.cpp:1579: clearChildNeedsStyleInvalidation();
I think we still need to do the style invalidation walk even if there's no
styleResolver, no?

https://codereview.chromium.org/143873016/diff/640001/Source/core/dom/Documen...
Source/core/dom/Document.cpp:1584: // crbug.com/335964 or move the invalidation
data elsewhere.
You can use use ensureStyleResolver(). I don't see any problem with recreateding
the styleResolver here if it doesn't exist. You're going to need it right after
this to do the rest of the styleRecalc anyways.

[chrishtr, 2014-01-29 01:56:41]

https://codereview.chromium.org/143873016/diff/640001/Source/core/dom/Documen...
File Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/143873016/diff/640001/Source/core/dom/Documen...
Source/core/dom/Document.cpp:1579: clearChildNeedsStyleInvalidation();
On 2014/01/29 00:50:07, ojan wrote:
> I think we still need to do the style invalidation walk even if there's no
> styleResolver, no?

The style invalidation data structure lives in the styleResolver() object right
now, so either that is incorrect (even with the style resolver reset timer
removed), or the code is ok. Is it ok so long as the style resolver is never
cleared?

https://codereview.chromium.org/143873016/diff/640001/Source/core/dom/Documen...
Source/core/dom/Document.cpp:1584: // crbug.com/335964 or move the invalidation
data elsewhere.
On 2014/01/29 00:50:07, ojan wrote:
> You can use use ensureStyleResolver(). I don't see any problem with
recreateding
> the styleResolver here if it doesn't exist. You're going to need it right
after
> this to do the rest of the styleRecalc anyways.

See above.

[ojan, 2014-01-29 02:00:24]

https://codereview.chromium.org/143873016/diff/640001/Source/core/dom/Documen...
File Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/143873016/diff/640001/Source/core/dom/Documen...
Source/core/dom/Document.cpp:1579: clearChildNeedsStyleInvalidation();
On 2014/01/29 01:56:42, Chris Harrelson wrote:
> On 2014/01/29 00:50:07, ojan wrote:
> > I think we still need to do the style invalidation walk even if there's no
> > styleResolver, no?
> 
> The style invalidation data structure lives in the styleResolver() object
right
> now, so either that is incorrect (even with the style resolver reset timer
> removed), or the code is ok. Is it ok so long as the style resolver is never
> cleared?

I see. Yeah, it seems like we have to block turning this flag on on getting rid
of the reset timer. Otherwise, you'll get in a situation where you don't do the
style invalidation in some cases and miss doing some recalcs you need to do,
right?

[chrishtr, 2014-01-29 02:02:31]

https://codereview.chromium.org/143873016/diff/640001/Source/core/dom/Documen...
File Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/143873016/diff/640001/Source/core/dom/Documen...
Source/core/dom/Document.cpp:1579: clearChildNeedsStyleInvalidation();
On 2014/01/29 02:00:25, ojan wrote:
> On 2014/01/29 01:56:42, Chris Harrelson wrote:
> > On 2014/01/29 00:50:07, ojan wrote:
> > > I think we still need to do the style invalidation walk even if there's no
> > > styleResolver, no?
> > 
> > The style invalidation data structure lives in the styleResolver() object
> right
> > now, so either that is incorrect (even with the style resolver reset timer
> > removed), or the code is ok. Is it ok so long as the style resolver is never
> > cleared?
> 
> I see. Yeah, it seems like we have to block turning this flag on on getting
rid
> of the reset timer. Otherwise, you'll get in a situation where you don't do
the
> style invalidation in some cases and miss doing some recalcs you need to do,
> right?

Yes. My plan is to try to land the change that removes the timer, but not block
this CL on that, since the code in this CL is behind a flag. If I can't get the
other change to land, I'll move the data for this CL elsewhere in a followup.

[ojan, 2014-01-29 02:19:27]

> Yes. My plan is to try to land the change that removes the timer, but not
block
> this CL on that, since the code in this CL is behind a flag. If I can't get
the
> other change to land, I'll move the data for this CL elsewhere in a followup.

sgtm

[chrishtr, 2014-01-29 23:14:11]

Added a layout test, and a FIXME to adjust the test to the runtime feature ASAP,
once I get it running in a virtual environment.

[esprehn, 2014-01-30 05:09:24]

Looking pretty good, get rid of the bools and this should be good to land.

https://codereview.chromium.org/143873016/diff/670001/Source/core/css/RuleFea...
File Source/core/css/RuleFeature.cpp (right):

https://codereview.chromium.org/143873016/diff/670001/Source/core/css/RuleFea...
Source/core/css/RuleFeature.cpp:100: m_targetedStyleRecalcEnabled =
RuntimeEnabledFeatures::targetedStyleRecalcEnabled();
This should be in the initializer list.

RuleFeatureSet::RuleFeatureSet()
  : m_targetedStyleRecalcEnabled(...)
{
}

https://codereview.chromium.org/143873016/diff/670001/Source/core/css/RuleFea...
Source/core/css/RuleFeature.cpp:255: && !m_targetedStyleRecalcEnabled)
I wouldn't wrap these.

https://codereview.chromium.org/143873016/diff/670001/Source/core/css/RuleFea...
Source/core/css/RuleFeature.cpp:278: bool
RuleFeatureSet::scheduleStyleInvalidationForClassChangeInternal(const
SpaceSplitString& oldClasses, const SpaceSplitString& newClasses, Element*
element)
Having two methods with the same name like this is pretty confusing. I might
suggest not naming them ...Internal() and using a more specific name. Then the
above functions would be.

bool shouldRecalcStyle = invalidateSubtreeForClassChange(...);
if (shouldRecalcStyle)
  element->setNeedsStyleRecalc();

https://codereview.chromium.org/143873016/diff/670001/Source/core/css/RuleFea...
Source/core/css/RuleFeature.cpp:339: for (Node* child = document.firstChild();
child; child = child->nextSibling()) {
A document can only have one element child, you don't need this loop.

if (Element* documentElement = document.documentElement()) {
  if (documentElement->needs...())
    compute...(documentElement);
}

see for example Document::recalcStyle.

https://codereview.chromium.org/143873016/diff/670001/Source/core/css/RuleFea...
Source/core/css/RuleFeature.cpp:342:
computeStyleInvalidationInternal(childElement, invalidationClasses, false);
I would name these methods invalidateStyleForClassChange() or
invalidateSubtree() or something like that, Internal() doesn't really make it
clear what's going on. It's good to avoid all the Internal stuff too, since you
call it in several places.

https://codereview.chromium.org/143873016/diff/670001/Source/core/css/RuleFea...
Source/core/css/RuleFeature.cpp:355: bool childRecalced =
computeStyleInvalidationInternal(childElement, invalidationClasses,
foundInvalidationSet);
invalidateSubtree(...)

https://codereview.chromium.org/143873016/diff/670001/Source/core/css/RuleFea...
Source/core/css/RuleFeature.cpp:359: for (ShadowRoot* root =
element->youngestShadowRoot(); root; root = root->olderShadowRoot()) {
Usually we do ShadowRoots first, I'd swap these two sections. I suppose it
doesn't matter though.

https://codereview.chromium.org/143873016/diff/670001/Source/core/css/RuleFea...
Source/core/css/RuleFeature.cpp:400: thisElementNeedsStyleRecalc = true;
element->setNeedsSylRecalc(LocalStyleChange);
and then mark all ancestors with LocalStyleChange too.

https://codereview.chromium.org/143873016/diff/670001/Source/core/css/RuleFea...
Source/core/css/RuleFeature.cpp:412: thisElementNeedsStyleRecalc =
thisElementNeedsStyleRecalc || someChildrenNeedStyleRecalc;
Get rid of the bools, just call setNeedsStyleRecalc(LocalStyleChange) up the
tree.

https://codereview.chromium.org/143873016/diff/670001/Source/core/css/RuleFea...
Source/core/css/RuleFeature.cpp:416:
element->setNeedsStyleRecalc(LocalStyleChange);
Remove this. If you just mark yourself above and then walk up the tree you get
the same behavior.

[chrishtr, 2014-01-30 17:28:14]

https://codereview.chromium.org/143873016/diff/670001/Source/core/css/RuleFea...
File Source/core/css/RuleFeature.cpp (right):

https://codereview.chromium.org/143873016/diff/670001/Source/core/css/RuleFea...
Source/core/css/RuleFeature.cpp:100: m_targetedStyleRecalcEnabled =
RuntimeEnabledFeatures::targetedStyleRecalcEnabled();
On 2014/01/30 05:09:24, esprehn wrote:
> This should be in the initializer list.
> 
> RuleFeatureSet::RuleFeatureSet()
>   : m_targetedStyleRecalcEnabled(...)
> {
> }

Done.

https://codereview.chromium.org/143873016/diff/670001/Source/core/css/RuleFea...
Source/core/css/RuleFeature.cpp:255: && !m_targetedStyleRecalcEnabled)
On 2014/01/30 05:09:24, esprehn wrote:
> I wouldn't wrap these.

Done.

https://codereview.chromium.org/143873016/diff/670001/Source/core/css/RuleFea...
Source/core/css/RuleFeature.cpp:278: bool
RuleFeatureSet::scheduleStyleInvalidationForClassChangeInternal(const
SpaceSplitString& oldClasses, const SpaceSplitString& newClasses, Element*
element)
On 2014/01/30 05:09:24, esprehn wrote:
> Having two methods with the same name like this is pretty confusing. I might
> suggest not naming them ...Internal() and using a more specific name. Then the
> above functions would be.
> 
> bool shouldRecalcStyle = invalidateSubtreeForClassChange(...);
> if (shouldRecalcStyle)
>   element->setNeedsStyleRecalc();

Done.

https://codereview.chromium.org/143873016/diff/670001/Source/core/css/RuleFea...
Source/core/css/RuleFeature.cpp:339: for (Node* child = document.firstChild();
child; child = child->nextSibling()) {
On 2014/01/30 05:09:24, esprehn wrote:
> A document can only have one element child, you don't need this loop.
> 
> if (Element* documentElement = document.documentElement()) {
>   if (documentElement->needs...())
>     compute...(documentElement);
> }
> 
> see for example Document::recalcStyle.

Done.

https://codereview.chromium.org/143873016/diff/670001/Source/core/css/RuleFea...
Source/core/css/RuleFeature.cpp:342:
computeStyleInvalidationInternal(childElement, invalidationClasses, false);
On 2014/01/30 05:09:24, esprehn wrote:
> I would name these methods invalidateStyleForClassChange() or
> invalidateSubtree() or something like that, Internal() doesn't really make it
> clear what's going on. It's good to avoid all the Internal stuff too, since
you
> call it in several places.

Done.

https://codereview.chromium.org/143873016/diff/670001/Source/core/css/RuleFea...
Source/core/css/RuleFeature.cpp:355: bool childRecalced =
computeStyleInvalidationInternal(childElement, invalidationClasses,
foundInvalidationSet);
On 2014/01/30 05:09:24, esprehn wrote:
> invalidateSubtree(...)

Done.

https://codereview.chromium.org/143873016/diff/670001/Source/core/css/RuleFea...
Source/core/css/RuleFeature.cpp:355: bool childRecalced =
computeStyleInvalidationInternal(childElement, invalidationClasses,
foundInvalidationSet);
On 2014/01/30 05:09:24, esprehn wrote:
> invalidateSubtree(...)

Done.

https://codereview.chromium.org/143873016/diff/670001/Source/core/css/RuleFea...
Source/core/css/RuleFeature.cpp:359: for (ShadowRoot* root =
element->youngestShadowRoot(); root; root = root->olderShadowRoot()) {
On 2014/01/30 05:09:24, esprehn wrote:
> Usually we do ShadowRoots first, I'd swap these two sections. I suppose it
> doesn't matter though.

Done.

https://codereview.chromium.org/143873016/diff/670001/Source/core/css/RuleFea...
Source/core/css/RuleFeature.cpp:400: thisElementNeedsStyleRecalc = true;
On 2014/01/30 05:09:24, esprehn wrote:
> element->setNeedsSylRecalc(LocalStyleChange);
> and then mark all ancestors with LocalStyleChange too.

I don't like that, as it will cause a bunch of redundant tree walking in
situations when multiple descendants of an element need local recalc. The logic
to add these bools is not very hard to understand.

https://codereview.chromium.org/143873016/diff/670001/Source/core/css/RuleFea...
Source/core/css/RuleFeature.cpp:412: thisElementNeedsStyleRecalc =
thisElementNeedsStyleRecalc || someChildrenNeedStyleRecalc;
On 2014/01/30 05:09:24, esprehn wrote:
> Get rid of the bools, just call setNeedsStyleRecalc(LocalStyleChange) up the
> tree.

See above.

https://codereview.chromium.org/143873016/diff/670001/Source/core/css/RuleFea...
Source/core/css/RuleFeature.cpp:416:
element->setNeedsStyleRecalc(LocalStyleChange);
On 2014/01/30 05:09:24, esprehn wrote:
> Remove this. If you just mark yourself above and then walk up the tree you get
> the same behavior.

See above.

[ojan, 2014-01-30 19:08:38]

A bunch more nits. :)

https://codereview.chromium.org/143873016/diff/670001/Source/core/css/RuleFea...
File Source/core/css/RuleFeature.cpp (right):

https://codereview.chromium.org/143873016/diff/670001/Source/core/css/RuleFea...
Source/core/css/RuleFeature.cpp:400: thisElementNeedsStyleRecalc = true;
On 2014/01/30 17:28:15, Chris Harrelson wrote:
> On 2014/01/30 05:09:24, esprehn wrote:
> > element->setNeedsSylRecalc(LocalStyleChange);
> > and then mark all ancestors with LocalStyleChange too.
> 
> I don't like that, as it will cause a bunch of redundant tree walking in
> situations when multiple descendants of an element need local recalc. The
logic
> to add these bools is not very hard to understand.

setNeedsStyleRecalc does the ancestor walk only once. If you call it on
something that already needs recalc, then it won't do the ancestor walk.

The extra bool here does help avoiding a bunch of extra branching in
setNeedsStyleRecalc itself, but I'm skeptical it matters.

Meh. I don't feel strongly either way.

https://codereview.chromium.org/143873016/diff/710001/Source/core/css/RuleFea...
File Source/core/css/RuleFeature.cpp (right):

https://codereview.chromium.org/143873016/diff/710001/Source/core/css/RuleFea...
Source/core/css/RuleFeature.cpp:336: if
(documentElement->childNeedsStyleInvalidation()) {
Nit: No curlies.

https://codereview.chromium.org/143873016/diff/710001/Source/core/css/RuleFea...
Source/core/css/RuleFeature.cpp:375: recalcWholeSubtree = true;
Can you just inline lines 381-384 here? Then you don't need the
recalcWholeSubtree bool at all.

https://codereview.chromium.org/143873016/diff/710001/Source/core/css/RuleFea...
File Source/core/css/RuleFeature.h (right):

https://codereview.chromium.org/143873016/diff/710001/Source/core/css/RuleFea...
Source/core/css/RuleFeature.h:85: // If a class changed on the element, this
method should be called in order to schedule style recalc.
We generally don't put "what" comments like this. It's clear from the method
name that this is what it does.

https://codereview.chromium.org/143873016/diff/710001/Source/core/css/RuleFea...
Source/core/css/RuleFeature.h:89: // Computes which elements in the document
need style recalculation.
Ditto. This comment doesn't really tell me anything. To understand what this
function does, I still need to look at the code.

https://codereview.chromium.org/143873016/diff/710001/Source/core/css/analyze...
File Source/core/css/analyzer/DescendantInvalidationSet.cpp (right):

https://codereview.chromium.org/143873016/diff/710001/Source/core/css/analyze...
Source/core/css/analyzer/DescendantInvalidationSet.cpp:81: for
(HashSet<AtomicString>::const_iterator it = m_classes.begin(); it !=
m_classes.end(); ++it) {
No curlies

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

https://codereview.chromium.org/143873016/diff/710001/Source/core/dom/Element...
Source/core/dom/Element.cpp:1051: bool testShouldInvalidateStyle =
inActiveDocument() && styleResolver && styleChangeType() < SubtreeStyleChange;
Hmmm...as an aside, I think this code might be wrong. If you change a classname
when there's no styleResolver, we won't mark the element as needing a recalc.
All the more reason to get rid of the throwaway timer.

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

https://codereview.chromium.org/143873016/diff/710001/Source/core/dom/Node.cp...
Source/core/dom/Node.cpp:646: 
extra line break

https://codereview.chromium.org/143873016/diff/710001/Source/core/dom/shadow/...
File Source/core/dom/shadow/SelectRuleFeatureSet.cpp (right):

https://codereview.chromium.org/143873016/diff/710001/Source/core/dom/shadow/...
Source/core/dom/shadow/SelectRuleFeatureSet.cpp:98: bool
SelectRuleFeatureSet::checkSelectorsForClassChange(const SpaceSplitString&
oldClasses, const SpaceSplitString& newClasses) const
It's a bummer there's so much copy-paste between this and
RuleFeatureSet::computeInvalidationSetsForClassChange. I don't have a good
suggestion for how to share the code though. :(

[chrishtr, 2014-01-30 19:25:47]

https://codereview.chromium.org/143873016/diff/670001/Source/core/css/RuleFea...
File Source/core/css/RuleFeature.cpp (right):

https://codereview.chromium.org/143873016/diff/670001/Source/core/css/RuleFea...
Source/core/css/RuleFeature.cpp:400: thisElementNeedsStyleRecalc = true;
On 2014/01/30 19:08:39, ojan wrote:
> On 2014/01/30 17:28:15, Chris Harrelson wrote:
> > On 2014/01/30 05:09:24, esprehn wrote:
> > > element->setNeedsSylRecalc(LocalStyleChange);
> > > and then mark all ancestors with LocalStyleChange too.
> > 
> > I don't like that, as it will cause a bunch of redundant tree walking in
> > situations when multiple descendants of an element need local recalc. The
> logic
> > to add these bools is not very hard to understand.
> 
> setNeedsStyleRecalc does the ancestor walk only once. If you call it on
> something that already needs recalc, then it won't do the ancestor walk.
> 
> The extra bool here does help avoiding a bunch of extra branching in
> setNeedsStyleRecalc itself, but I'm skeptical it matters.
> 
> Meh. I don't feel strongly either way.

It's not the ancestor child-needs-recalc bit setting that is the issue. It's
that we need to call setNeedsStyleRecalc(LocalStyleChange) on all ancestors of
this element, or else style sharing will break.
Elliott was suggesting an extra tree walk here. But that introduces redundant
tree walks in cases when many elements need descendant selector recalc.

https://codereview.chromium.org/143873016/diff/710001/Source/core/css/RuleFea...
File Source/core/css/RuleFeature.h (right):

https://codereview.chromium.org/143873016/diff/710001/Source/core/css/RuleFea...
Source/core/css/RuleFeature.h:85: // If a class changed on the element, this
method should be called in order to schedule style recalc.
On 2014/01/30 19:08:39, ojan wrote:
> We generally don't put "what" comments like this. It's clear from the method
> name that this is what it does.

Done.

https://codereview.chromium.org/143873016/diff/710001/Source/core/css/RuleFea...
Source/core/css/RuleFeature.h:89: // Computes which elements in the document
need style recalculation.
On 2014/01/30 19:08:39, ojan wrote:
> Ditto. This comment doesn't really tell me anything. To understand what this
> function does, I still need to look at the code.

Done.

https://codereview.chromium.org/143873016/diff/710001/Source/core/css/analyze...
File Source/core/css/analyzer/DescendantInvalidationSet.cpp (right):

https://codereview.chromium.org/143873016/diff/710001/Source/core/css/analyze...
Source/core/css/analyzer/DescendantInvalidationSet.cpp:81: for
(HashSet<AtomicString>::const_iterator it = m_classes.begin(); it !=
m_classes.end(); ++it) {
On 2014/01/30 19:08:39, ojan wrote:
> No curlies

Done.

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

https://codereview.chromium.org/143873016/diff/710001/Source/core/dom/Element...
Source/core/dom/Element.cpp:1051: bool testShouldInvalidateStyle =
inActiveDocument() && styleResolver && styleChangeType() < SubtreeStyleChange;
On 2014/01/30 19:08:39, ojan wrote:
> Hmmm...as an aside, I think this code might be wrong. If you change a
classname
> when there's no styleResolver, we won't mark the element as needing a recalc.
> All the more reason to get rid of the throwaway timer.

yikes.

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

https://codereview.chromium.org/143873016/diff/710001/Source/core/dom/Node.cp...
Source/core/dom/Node.cpp:646: 
On 2014/01/30 19:08:39, ojan wrote:
> extra line break

Done.

https://codereview.chromium.org/143873016/diff/710001/Source/core/dom/shadow/...
File Source/core/dom/shadow/SelectRuleFeatureSet.cpp (right):

https://codereview.chromium.org/143873016/diff/710001/Source/core/dom/shadow/...
Source/core/dom/shadow/SelectRuleFeatureSet.cpp:98: bool
SelectRuleFeatureSet::checkSelectorsForClassChange(const SpaceSplitString&
oldClasses, const SpaceSplitString& newClasses) const
On 2014/01/30 19:08:39, ojan wrote:
> It's a bummer there's so much copy-paste between this and
> RuleFeatureSet::computeInvalidationSetsForClassChange. I don't have a good
> suggestion for how to share the code though. :(

Agreed. I tried several approaches, but all turned out silly. I hope to be able
to refactor this class to call into a feature set instead, but for the moment it
was too hard.

[ojan, 2014-01-30 19:51:18]

https://codereview.chromium.org/143873016/diff/670001/Source/core/css/RuleFea...
File Source/core/css/RuleFeature.cpp (right):

https://codereview.chromium.org/143873016/diff/670001/Source/core/css/RuleFea...
Source/core/css/RuleFeature.cpp:400: thisElementNeedsStyleRecalc = true;
On 2014/01/30 19:25:48, Chris Harrelson wrote:
> On 2014/01/30 19:08:39, ojan wrote:
> > On 2014/01/30 17:28:15, Chris Harrelson wrote:
> > > On 2014/01/30 05:09:24, esprehn wrote:
> > > > element->setNeedsSylRecalc(LocalStyleChange);
> > > > and then mark all ancestors with LocalStyleChange too.
> > > 
> > > I don't like that, as it will cause a bunch of redundant tree walking in
> > > situations when multiple descendants of an element need local recalc. The
> > logic
> > > to add these bools is not very hard to understand.
> > 
> > setNeedsStyleRecalc does the ancestor walk only once. If you call it on
> > something that already needs recalc, then it won't do the ancestor walk.
> > 
> > The extra bool here does help avoiding a bunch of extra branching in
> > setNeedsStyleRecalc itself, but I'm skeptical it matters.
> > 
> > Meh. I don't feel strongly either way.
> 
> It's not the ancestor child-needs-recalc bit setting that is the issue. It's
> that we need to call setNeedsStyleRecalc(LocalStyleChange) on all ancestors of
> this element, or else style sharing will break.
> Elliott was suggesting an extra tree walk here. But that introduces redundant
> tree walks in cases when many elements need descendant selector recalc.

There's something I'm missing. If you just replace the two places where you set
thisElementNeedsStyleRecalc to true with
element->setNeedsStyleRecalc(LocalStyleChange), isn't the code equivalent?

https://codereview.chromium.org/143873016/diff/710001/Source/core/css/RuleFea...
File Source/core/css/RuleFeature.cpp (right):

https://codereview.chromium.org/143873016/diff/710001/Source/core/css/RuleFea...
Source/core/css/RuleFeature.cpp:375: recalcWholeSubtree = true;
On 2014/01/30 19:08:39, ojan wrote:
> Can you just inline lines 381-384 here? Then you don't need the
> recalcWholeSubtree bool at all.

Missed this comment?

[chrishtr, 2014-01-30 21:39:44]

https://codereview.chromium.org/143873016/diff/670001/Source/core/css/RuleFea...
File Source/core/css/RuleFeature.cpp (right):

https://codereview.chromium.org/143873016/diff/670001/Source/core/css/RuleFea...
Source/core/css/RuleFeature.cpp:400: thisElementNeedsStyleRecalc = true;
On 2014/01/30 19:51:18, ojan wrote:
> On 2014/01/30 19:25:48, Chris Harrelson wrote:
> > On 2014/01/30 19:08:39, ojan wrote:
> > > On 2014/01/30 17:28:15, Chris Harrelson wrote:
> > > > On 2014/01/30 05:09:24, esprehn wrote:
> > > > > element->setNeedsSylRecalc(LocalStyleChange);
> > > > > and then mark all ancestors with LocalStyleChange too.
> > > > 
> > > > I don't like that, as it will cause a bunch of redundant tree walking in
> > > > situations when multiple descendants of an element need local recalc.
The
> > > logic
> > > > to add these bools is not very hard to understand.
> > > 
> > > setNeedsStyleRecalc does the ancestor walk only once. If you call it on
> > > something that already needs recalc, then it won't do the ancestor walk.
> > > 
> > > The extra bool here does help avoiding a bunch of extra branching in
> > > setNeedsStyleRecalc itself, but I'm skeptical it matters.
> > > 
> > > Meh. I don't feel strongly either way.
> > 
> > It's not the ancestor child-needs-recalc bit setting that is the issue. It's
> > that we need to call setNeedsStyleRecalc(LocalStyleChange) on all ancestors
of
> > this element, or else style sharing will break.
> > Elliott was suggesting an extra tree walk here. But that introduces
redundant
> > tree walks in cases when many elements need descendant selector recalc.
> 
> There's something I'm missing. If you just replace the two places where you
set
> thisElementNeedsStyleRecalc to true with
> element->setNeedsStyleRecalc(LocalStyleChange), isn't the code equivalent?

Yes. I can make that change if you want. The return-value boolean and
accompanying logic I don't want to remove though.

https://codereview.chromium.org/143873016/diff/710001/Source/core/css/RuleFea...
File Source/core/css/RuleFeature.cpp (right):

https://codereview.chromium.org/143873016/diff/710001/Source/core/css/RuleFea...
Source/core/css/RuleFeature.cpp:375: recalcWholeSubtree = true;
On 2014/01/30 19:51:18, ojan wrote:
> On 2014/01/30 19:08:39, ojan wrote:
> > Can you just inline lines 381-384 here? Then you don't need the
> > recalcWholeSubtree bool at all.
> 
> Missed this comment?

Oops.

I can't inline line 382 in particular, so not sure what it would save. In fact I
would like to be able to share the code for lines 382-383 with 413-414, but felt
that a two-line helper function was maybe excessive.

[ojan, 2014-01-30 22:15:54]

https://codereview.chromium.org/143873016/diff/670001/Source/core/css/RuleFea...
File Source/core/css/RuleFeature.cpp (right):

https://codereview.chromium.org/143873016/diff/670001/Source/core/css/RuleFea...
Source/core/css/RuleFeature.cpp:400: thisElementNeedsStyleRecalc = true;
On 2014/01/30 21:39:44, Chris Harrelson wrote:
> On 2014/01/30 19:51:18, ojan wrote:
> > On 2014/01/30 19:25:48, Chris Harrelson wrote:
> > > On 2014/01/30 19:08:39, ojan wrote:
> > > > On 2014/01/30 17:28:15, Chris Harrelson wrote:
> > > > > On 2014/01/30 05:09:24, esprehn wrote:
> > > > > > element->setNeedsSylRecalc(LocalStyleChange);
> > > > > > and then mark all ancestors with LocalStyleChange too.
> > > > > 
> > > > > I don't like that, as it will cause a bunch of redundant tree walking
in
> > > > > situations when multiple descendants of an element need local recalc.
> The
> > > > logic
> > > > > to add these bools is not very hard to understand.
> > > > 
> > > > setNeedsStyleRecalc does the ancestor walk only once. If you call it on
> > > > something that already needs recalc, then it won't do the ancestor walk.
> > > > 
> > > > The extra bool here does help avoiding a bunch of extra branching in
> > > > setNeedsStyleRecalc itself, but I'm skeptical it matters.
> > > > 
> > > > Meh. I don't feel strongly either way.
> > > 
> > > It's not the ancestor child-needs-recalc bit setting that is the issue.
It's
> > > that we need to call setNeedsStyleRecalc(LocalStyleChange) on all
ancestors
> of
> > > this element, or else style sharing will break.
> > > Elliott was suggesting an extra tree walk here. But that introduces
> redundant
> > > tree walks in cases when many elements need descendant selector recalc.
> > 
> > There's something I'm missing. If you just replace the two places where you
> set
> > thisElementNeedsStyleRecalc to true with
> > element->setNeedsStyleRecalc(LocalStyleChange), isn't the code equivalent?
> 
> Yes. I can make that change if you want. The return-value boolean and
> accompanying logic I don't want to remove though.

Ah. Didn't notice that we're returning this value.

https://codereview.chromium.org/143873016/diff/710001/Source/core/css/RuleFea...
File Source/core/css/RuleFeature.cpp (right):

https://codereview.chromium.org/143873016/diff/710001/Source/core/css/RuleFea...
Source/core/css/RuleFeature.cpp:375: recalcWholeSubtree = true;
On 2014/01/30 21:39:45, Chris Harrelson wrote:
> On 2014/01/30 19:51:18, ojan wrote:
> > On 2014/01/30 19:08:39, ojan wrote:
> > > Can you just inline lines 381-384 here? Then you don't need the
> > > recalcWholeSubtree bool at all.
> > 
> > Missed this comment?
> 
> Oops.
> 
> I can't inline line 382 in particular, so not sure what it would save. In fact
I
> would like to be able to share the code for lines 382-383 with 413-414, but
felt
> that a two-line helper function was maybe excessive.

Agree that the two-line helper function is excessive. I don't understand the
rest of your comment though, isn't the following code exactly equivalent to what
you have here?

for (InvalidationList::const_iterator it = invalidationList->begin(); it !=
invalidationList->end(); ++it) {
    if ((*it)->wholeSubtreeInvalid()) {
        element->setNeedsStyleRecalc(SubtreeStyleChange);
        invalidationClasses.remove(oldSize, invalidationClasses.size() -
oldSize);
        element->clearChildNeedsStyleInvalidation();
        return true;
    }
    (*it)->getClasses(invalidationClasses);
}

[chrishtr, 2014-01-30 22:25:52]

https://codereview.chromium.org/143873016/diff/670001/Source/core/css/RuleFea...
File Source/core/css/RuleFeature.cpp (right):

https://codereview.chromium.org/143873016/diff/670001/Source/core/css/RuleFea...
Source/core/css/RuleFeature.cpp:400: thisElementNeedsStyleRecalc = true;
On 2014/01/30 22:15:55, ojan wrote:
> On 2014/01/30 21:39:44, Chris Harrelson wrote:
> > On 2014/01/30 19:51:18, ojan wrote:
> > > On 2014/01/30 19:25:48, Chris Harrelson wrote:
> > > > On 2014/01/30 19:08:39, ojan wrote:
> > > > > On 2014/01/30 17:28:15, Chris Harrelson wrote:
> > > > > > On 2014/01/30 05:09:24, esprehn wrote:
> > > > > > > element->setNeedsSylRecalc(LocalStyleChange);
> > > > > > > and then mark all ancestors with LocalStyleChange too.
> > > > > > 
> > > > > > I don't like that, as it will cause a bunch of redundant tree
walking
> in
> > > > > > situations when multiple descendants of an element need local
recalc.
> > The
> > > > > logic
> > > > > > to add these bools is not very hard to understand.
> > > > > 
> > > > > setNeedsStyleRecalc does the ancestor walk only once. If you call it
on
> > > > > something that already needs recalc, then it won't do the ancestor
walk.
> > > > > 
> > > > > The extra bool here does help avoiding a bunch of extra branching in
> > > > > setNeedsStyleRecalc itself, but I'm skeptical it matters.
> > > > > 
> > > > > Meh. I don't feel strongly either way.
> > > > 
> > > > It's not the ancestor child-needs-recalc bit setting that is the issue.
> It's
> > > > that we need to call setNeedsStyleRecalc(LocalStyleChange) on all
> ancestors
> > of
> > > > this element, or else style sharing will break.
> > > > Elliott was suggesting an extra tree walk here. But that introduces
> > redundant
> > > > tree walks in cases when many elements need descendant selector recalc.
> > > 
> > > There's something I'm missing. If you just replace the two places where
you
> > set
> > > thisElementNeedsStyleRecalc to true with
> > > element->setNeedsStyleRecalc(LocalStyleChange), isn't the code equivalent?
> > 
> > Yes. I can make that change if you want. The return-value boolean and
> > accompanying logic I don't want to remove though.
> 
> Ah. Didn't notice that we're returning this value.

Ack.

https://codereview.chromium.org/143873016/diff/710001/Source/core/css/RuleFea...
File Source/core/css/RuleFeature.cpp (right):

https://codereview.chromium.org/143873016/diff/710001/Source/core/css/RuleFea...
Source/core/css/RuleFeature.cpp:375: recalcWholeSubtree = true;
On 2014/01/30 22:15:55, ojan wrote:
> On 2014/01/30 21:39:45, Chris Harrelson wrote:
> > On 2014/01/30 19:51:18, ojan wrote:
> > > On 2014/01/30 19:08:39, ojan wrote:
> > > > Can you just inline lines 381-384 here? Then you don't need the
> > > > recalcWholeSubtree bool at all.
> > > 
> > > Missed this comment?
> > 
> > Oops.
> > 
> > I can't inline line 382 in particular, so not sure what it would save. In
fact
> I
> > would like to be able to share the code for lines 382-383 with 413-414, but
> felt
> > that a two-line helper function was maybe excessive.
> 
> Agree that the two-line helper function is excessive. I don't understand the
> rest of your comment though, isn't the following code exactly equivalent to
what
> you have here?
> 
> for (InvalidationList::const_iterator it = invalidationList->begin(); it !=
> invalidationList->end(); ++it) {
>     if ((*it)->wholeSubtreeInvalid()) {
>         element->setNeedsStyleRecalc(SubtreeStyleChange);
>         invalidationClasses.remove(oldSize, invalidationClasses.size() -
> oldSize);
>         element->clearChildNeedsStyleInvalidation();
>         return true;
>     }
>     (*it)->getClasses(invalidationClasses);
> }

Ah - yes it is. I forgot about the "return true" part, which short-circuits.

Done.

[ojan, 2014-01-30 22:29:52]

lgtm

[commit-bot: I haz the power, 2014-01-30 22:30:02]

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

[commit-bot: I haz the power, 2014-01-30 22:30:09]

Failed to apply patch for Source/core/dom/Node.h:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file Source/core/dom/Node.h
  Hunk #2 FAILED at 721.
  1 out of 2 hunks FAILED -- saving rejects to file Source/core/dom/Node.h.rej

Patch:       Source/core/dom/Node.h
Index: Source/core/dom/Node.h
diff --git a/Source/core/dom/Node.h b/Source/core/dom/Node.h
index
7f884a26e10150f1057b0f9f343a76af6b908c54..6bb7ae502f27b93712fe24ab0081d875a81f0c1e
100644
--- a/Source/core/dom/Node.h
+++ b/Source/core/dom/Node.h
@@ -371,6 +371,13 @@ public:
     void clearChildNeedsDistributionRecalc()  {
clearFlag(ChildNeedsDistributionRecalc); }
     void markAncestorsWithChildNeedsDistributionRecalc();
 
+    bool childNeedsStyleInvalidation() const { return
getFlag(ChildNeedsStyleInvalidation); }
+    void setChildNeedsStyleInvalidation()  {
setFlag(ChildNeedsStyleInvalidation); }
+    void clearChildNeedsStyleInvalidation()  {
clearFlag(ChildNeedsStyleInvalidation); }
+    void markAncestorsWithChildNeedsStyleInvalidation();
+    bool needsStyleInvalidation() { return getFlag(NeedsStyleInvalidation); }
+    void setNeedsStyleInvalidation();
+
     void recalcDistribution();
 
     bool shouldNotifyRendererWithIdenticalStyles() const { return
getFlag(NotifyRendererWithIdenticalStyles); }
@@ -714,10 +721,13 @@ private:
         CustomElement = 1 << 27,
         CustomElementUpgraded = 1 << 28,
 
+        ChildNeedsStyleInvalidation = 1 << 29,
+        NeedsStyleInvalidation = 1 << 30,
+
         DefaultNodeFlags = IsParsingChildrenFinishedFlag |
ChildNeedsStyleRecalcFlag | NeedsReattachStyleChange
     };
 
-    // 4 bits remaining.
+    // 1 bit remaining.
 
     bool getFlag(NodeFlags mask) const { return m_nodeFlags & mask; }
     void setFlag(bool f, NodeFlags mask) const { m_nodeFlags = (m_nodeFlags &
~mask) | (-(int32_t)f & mask); }

[commit-bot: I haz the power, 2014-01-30 22:30:09]

CQ bit was unchecked on CL. Ignoring.

[commit-bot: I haz the power, 2014-01-30 22:35:57]

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

[esprehn, 2014-01-30 23:14:45]

There's no extra tree walk, the ancestor chain walk should stop as soon as
it runs into an element with needsStyleRecalc.

All the return values and needless or-ing of them just makes this code more
confusing.

I'd really like if you'd fix it before landing. It just obfuscates
what this code is doing, and a future patch should just clone the styles on
ancestors and stop causing these recalcStyle anyway. It's easy to make such
a change with the ancestor walk, with your code it's not as obvious.

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[commit-bot: I haz the power, 2014-01-31 01:47:42]

Retried try job too often on win_blink_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_blink_...

[commit-bot: I haz the power, 2014-01-31 01:47:50]

CQ bit was unchecked on CL. Ignoring.

[commit-bot: I haz the power, 2014-01-31 01:48:10]

CQ bit was unchecked on CL. Ignoring.

[chrishtr, 2014-01-31 18:32:31]

Ah yes, good point, it can check if needsStyleRecalc is true. I didn't think of
that.

However, I whiteboarded this morning and think this will not actually be
correct. The reason is that some mutation outside of my code may have set
needsStyleRecalc on some of the nodes in the ancestor chain I need to edit. This
means that to be correct I will have to walk all the way up to the root of the
invalidation set subtree and recalc all elements that don't already have style
recalc. Which is more expensive, since it involves O(tree depth) operations
every time.

In fact, this problem occurs even if there are no mutations outside of my code,
since there may be two or more descendant invalidation sets to process in the
same tree walk. This means that I have to either traverse up the tree to the
root invalidation set node always.

The code could be changed to maintain a set of elements along the ancestor chain
or something, but that is more complicated and slower than returning a bool.

I'll discuss with Ojan and then commit if he agrees with this reasoning.

[chrishtr, 2014-01-31 19:01:44]

The CQ bit was checked by chrishtr@chromium.org

[commit-bot: I haz the power, 2014-01-31 19:02:04]

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

[chrishtr, 2014-01-31 19:03:12]

FYI I discussed with Ojan and he agrees with me that there is an issue with the
ancestor walk. Committing as-is.

[commit-bot: I haz the power, 2014-01-31 21:21:55]

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

[commit-bot: I haz the power, 2014-01-31 21:21:56]

Retried try job too often on win_blink_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_blink_...

[commit-bot: I haz the power, 2014-01-31 21:22:05]

CQ bit was unchecked on CL. Ignoring.

[commit-bot: I haz the power, 2014-01-31 21:22:05]

CQ bit was unchecked on CL. Ignoring.

[commit-bot: I haz the power, 2014-01-31 21:24:06]

CQ bit was unchecked on CL. Ignoring.

[chrishtr, 2014-01-31 21:25:21]

The CQ bit was checked by chrishtr@chromium.org

[commit-bot: I haz the power, 2014-01-31 21:25:47]

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

[commit-bot: I haz the power, 2014-01-31 22:48:26]

Change committed as 166247

[commit-bot: I haz the power, 2014-01-31 22:48:31]

CQ bit was unchecked on CL. Ignoring.

[commit-bot: I haz the power, 2014-01-31 22:48:32]

CQ bit was unchecked on CL. Ignoring.

[commit-bot: I haz the power, 2014-01-31 22:48:46]

CQ bit was unchecked on CL. Ignoring.