Add a first pass of a class descendant invalidator, and a containing RuleSetAnalyzer

Add a first pass of a class descendant invalidator, and a containing RuleSetAnalyzer

This allows us to (for some rule sets) only recalculate the elements
necessary when changing the class on an element, rather than then
entire subtree.

It's the first in a sequence of CLs to continue refactoring logic into
RuleSetAnalyzer, and expand the scope of targeted invalidation.

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

[dglazkov, 2014-01-08 22:32:44]

holy moley!

[chrishtr, 2014-01-08 22:34:54]

FYI I chatted w/Elliot and will take over this patch. I had written a
similar but less aggressive one..


On Wed, Jan 8, 2014 at 2:32 PM, <dglazkov@chromium.org> wrote:

> holy moley!
>
> https://codereview.chromium.org/129633003/
>

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

[chrishtr, 2014-01-14 21:02:03]

Ready for review. Passes all layout tests.

I'm running some performance tests now.

[esprehn, 2014-01-14 21:32:56]

My goal was to combine the invalidation into a single tree walk so you didn't
need to walk it so many times, right now if you add N class names to a div we'll
walk the subtree N times, and if you add classes to a div and then to it's
ancestor we'll do it again as well.

The design of this was such that you'd mark elements as needing invalidation in
the checkClassName*() functions:

setNeedsClassInvalidation();

and setChildNeedsClassInvalidation() on all your ancestors.

Then at the start of Document::recalcStyle we'd walk the document following the
bits down the tree. Whenever you hit an element where needsClassInvalidation()
is true you add it's invalidation sets to the list of sets to check and continue
walking recursively.

ex.

Element::invalidateClasses(Vector<InvalidationSet> invalidationSets) {
  size_t oldSize = invalidationSets.size();
  if (needsClassInvalidation()) {
    for (AtomicString className : changedClasses) {
      invalidationSets.add(resolver.invalidationSetForClassName(className);
    }
  }
  for (InvalidationSet set : invalidationSets) {
   if (set.invalidatesElement(this))
     setNeedsStyleChange(LocalStyleChange);
  }
  for (Node* child = firstChild(); child; child->nextSibling()) {
     if (child->isElement())
       toElement(child)->invalidateClasses(invalidationSets);
  }
  for (ShadowRoot* root = youngestShadowRoot(); ...) {
    ...
  }
  // Pop all the sets we added at this level when coming out of recursion.
  invalidationSets.remove(oldSize, invalidationSets.size());
}

That might be too hard or too big for this first patch, we might want to land
this feature behind a flag and iterate.

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

https://codereview.chromium.org/129633003/diff/490001/Source/core/css/RuleFea...
Source/core/css/RuleFeature.cpp:72: if (other.getRuleSetAnalyzer())
if (RuleSetAnalyzer* analyzer = ...)
  analyzer->...();

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

https://codereview.chromium.org/129633003/diff/490001/Source/core/css/RuleFea...
Source/core/css/RuleFeature.h:84: const RuleSetAnalyzer* getRuleSetAnalyzer()
const;
ruleSetAnalyzer(), no get prefix.

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

https://codereview.chromium.org/129633003/diff/490001/Source/core/css/analyze...
Source/core/css/analyzer/DescendantInvalidationSet.cpp:56: addTagName(*it);
If allDescendantsMightBeInvalid is true we should probably just abort all the
add calls and early return.

m_allDescendantsMightBeInvalid = m_allDescendantsMightBeInvalid ||
other.m_allDescendantsMightBeInvalid;
if (m...)
  return;

at the top of this function.

https://codereview.chromium.org/129633003/diff/490001/Source/core/css/analyze...
Source/core/css/analyzer/DescendantInvalidationSet.cpp:62: if
(allDescendantsMightBeInvalid) {
Braces are not needed.

https://codereview.chromium.org/129633003/diff/490001/Source/core/css/analyze...
Source/core/css/analyzer/DescendantInvalidationSet.cpp:75: if
(element->classNames().contains(*it)) {
Braces are not needed and this should be a return otherwise you look at all the
classes even once one of them matches,

if (element->classNames().contains(*it)) 
  return true;

https://codereview.chromium.org/129633003/diff/490001/Source/core/css/analyze...
Source/core/css/analyzer/DescendantInvalidationSet.cpp:82:
shouldRecalcThisElement = true;
This function needs to be split and have an early return, not set some variable
like this.

if (m_tagNames.contains(element->tagQName().localName()))
  return true;

Right now you do lots of needless hash lookups even though you already know
something matches.

https://codereview.chromium.org/129633003/diff/490001/Source/core/css/analyze...
Source/core/css/analyzer/DescendantInvalidationSet.cpp:87: for (Node* child =
element->firstChild(); child; child = child->nextSibling()) {
This doesn't handle ShadowRoots, but I think we always skip rules that could
apply to ShadowRoots (ex. host or ^^ or ^) so they should never end up down this
path.

It might be worth adding a comment that system doesn't handle tree boundary
crossing rules on purpose.

https://codereview.chromium.org/129633003/diff/490001/Source/core/css/analyze...
Source/core/css/analyzer/DescendantInvalidationSet.cpp:93: }
This function needs to be split, you want

bool invalidatesElement(Element&) that just has lots of returns in it, and
another one that walks the tree and calls it.

https://codereview.chromium.org/129633003/diff/490001/Source/core/css/analyze...
Source/core/css/analyzer/DescendantInvalidationSet.cpp:99:
element->document().ensureStyleResolver().addToStyleSharingList(*element);
Remove this, you shouldn't be messing with the style sharing list from inside
here. There's no reason for that (and it does nothing right now since you're not
inside recalcStyle when this code runs).

https://codereview.chromium.org/129633003/diff/490001/Source/core/css/analyze...
File Source/core/css/analyzer/DescendantInvalidationSet.h (right):

https://codereview.chromium.org/129633003/diff/490001/Source/core/css/analyze...
Source/core/css/analyzer/DescendantInvalidationSet.h:44: // Tracks data to
determine which elements of a DOM subtree ened to have style
s/ened/need/

https://codereview.chromium.org/129633003/diff/490001/Source/core/css/analyze...
Source/core/css/analyzer/DescendantInvalidationSet.h:70: bool
allDescendantsMightBeInvalid;
m_ missing

https://codereview.chromium.org/129633003/diff/490001/Source/core/css/analyze...
Source/core/css/analyzer/DescendantInvalidationSet.h:74: HashSet<AtomicString>
m_tagNames;
HashSets are pretty big in terms of memory usage, that's why I had used OwnPtr
originally. We can optimize that later though.

https://codereview.chromium.org/129633003/diff/490001/Source/core/css/analyze...
File Source/core/css/analyzer/RuleSetAnalyzer.cpp (right):

https://codereview.chromium.org/129633003/diff/490001/Source/core/css/analyze...
Source/core/css/analyzer/RuleSetAnalyzer.cpp:90: void ExtractClassIdOrTag(const
CSSSelector& selector, HashSet<AtomicString>* classes, AtomicString* id,
AtomicString* tagName)
lower case function names

https://codereview.chromium.org/129633003/diff/490001/Source/core/css/analyze...
Source/core/css/analyzer/RuleSetAnalyzer.cpp:90: void ExtractClassIdOrTag(const
CSSSelector& selector, HashSet<AtomicString>* classes, AtomicString* id,
AtomicString* tagName)
pass references for the classes, id and tagNames

https://codereview.chromium.org/129633003/diff/490001/Source/core/css/analyze...
Source/core/css/analyzer/RuleSetAnalyzer.cpp:104: }
remove braces

https://codereview.chromium.org/129633003/diff/490001/Source/core/css/analyze...
Source/core/css/analyzer/RuleSetAnalyzer.cpp:183: DescendantInvalidationSet*
RuleSetAnalyzer::getClassInvalidationSet(const AtomicString& className) const
::invalidationSetForClass(...), get*() prefixes methods return things through
out params like: void getFoo(Vector*), regular getters or find methods should
not start with get.

https://codereview.chromium.org/129633003/diff/490001/Source/core/css/analyze...
Source/core/css/analyzer/RuleSetAnalyzer.cpp:191: if (descendantInvalidationSet)
{
We usually combine the get and the if statement:

if (DescendantInvalidationSet* descendantInvalidationSet =
classInvalidationSets.get(className)) {
 descendantInvalidationSet->invalidateElementSubtree(element);
 return true;
}
return false;

https://codereview.chromium.org/129633003/diff/490001/Source/core/css/analyze...
File Source/core/css/analyzer/RuleSetAnalyzer.h (right):

https://codereview.chromium.org/129633003/diff/490001/Source/core/css/analyze...
Source/core/css/analyzer/RuleSetAnalyzer.h:53: RuleSetAnalyzer() { }
Put this in the cpp file, doesn't need to be inline.

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

https://codereview.chromium.org/129633003/diff/490001/Source/core/dom/Element...
Source/core/dom/Element.cpp:1054:
ruleSetAnalyzer->invalidateElementSubtreeForClassChange(changedClasses[i],
element);
I had envisioned doing this in a single pass, not walking the subtree
repeatedly. This is doing to doing awful things to Modernizer apps:

ex.

body.className = "foo bar baz biz bam" will walk the entire document 5 times.

We can land this as-is, but we should do it behind a flag then and iterate on
combining the passes.

https://codereview.chromium.org/129633003/diff/490001/Source/core/dom/Element...
Source/core/dom/Element.cpp:1054:
ruleSetAnalyzer->invalidateElementSubtreeForClassChange(changedClasses[i],
element);
You ignore the return value of invalidateElementSubtreeForClassChange which is
not right, you want to combine the getRuleSetAnalyzer() and the
invalidateElementSubtreeForClassChange steps into a method on checker.

if (checker.invalidateForClass(changedClasses[i], element))
  continue;
if (checker.hasSelectorForClass(changedClasses[i]))
  return true;

https://codereview.chromium.org/129633003/diff/490001/Source/core/dom/Element...
Source/core/dom/Element.cpp:1086: if (ruleSetAnalyzer)
Combing if and get, if (const RuleSet.... = checker.rulesetAnalyzer(...)).

invalidateElementSubtreeForClassChange also returns a bool, but you ignore it
here which isn't right.

https://codereview.chromium.org/129633003/diff/490001/Source/core/dom/Element...
Source/core/dom/Element.cpp:1159: if (checkSelectorForClassChange(oldClasses,
featureSet, this))
These should be methods now, don't pass |this|, just turn
checkSelectorForClassChange into a method.

https://codereview.chromium.org/129633003/diff/490001/Source/core/dom/shadow/...
File Source/core/dom/shadow/SelectRuleFeatureSet.h (right):

https://codereview.chromium.org/129633003/diff/490001/Source/core/dom/shadow/...
Source/core/dom/shadow/SelectRuleFeatureSet.h:70: inline const RuleSetAnalyzer*
SelectRuleFeatureSet::getRuleSetAnalyzer() const
Poor shadow DOM, add a FIXME to make ShadowDOM smarter someday.

[esprehn, 2014-01-14 21:34:58]

On 2014/01/14 21:32:56, esprehn wrote:
> My goal was to combine the invalidation into a single tree walk so you didn't
> need to walk it so many times, right now if you add N class names to a div
we'll
> walk the subtree N times, and if you add classes to a div and then to it's
> ancestor we'll do it again as well.
> 
> The design of this was such that you'd mark elements as needing invalidation
in
> the checkClassName*() functions:
> 
> setNeedsClassInvalidation();
> 
> and setChildNeedsClassInvalidation() on all your ancestors.
> 
> Then at the start of Document::recalcStyle we'd walk the document following
the
> bits down the tree. Whenever you hit an element where needsClassInvalidation()
> is true you add it's invalidation sets to the list of sets to check and
continue
> walking recursively.
> 
> ex.
> 
> Element::invalidateClasses(Vector<InvalidationSet> invalidationSets) {
>   size_t oldSize = invalidationSets.size();
>   if (needsClassInvalidation()) {
>     for (AtomicString className : changedClasses) {
>       invalidationSets.add(resolver.invalidationSetForClassName(className);
>     }
>   }
>   for (InvalidationSet set : invalidationSets) {
>    if (set.invalidatesElement(this))
>      setNeedsStyleChange(LocalStyleChange);
>   }
>   for (Node* child = firstChild(); child; child->nextSibling()) {
>      if (child->isElement())
>    

Sorry that should be:

if (child->isElementNode() && child->childNeedsClassInvalidation())

look at Node::recalcDistribution for the pattern of how this works:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

[chrishtr, 2014-01-14 23:33:00]

Removed the invalidation part for now, as that has to be rewritten to be more
efficient.

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

https://codereview.chromium.org/129633003/diff/490001/Source/core/css/RuleFea...
Source/core/css/RuleFeature.cpp:72: if (other.getRuleSetAnalyzer())
On 2014/01/14 21:32:56, esprehn wrote:
> if (RuleSetAnalyzer* analyzer = ...)
>   analyzer->...();

Done.

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

https://codereview.chromium.org/129633003/diff/490001/Source/core/css/analyze...
Source/core/css/analyzer/DescendantInvalidationSet.cpp:56: addTagName(*it);
On 2014/01/14 21:32:56, esprehn wrote:
> If allDescendantsMightBeInvalid is true we should probably just abort all the
> add calls and early return.
> 
> m_allDescendantsMightBeInvalid = m_allDescendantsMightBeInvalid ||
> other.m_allDescendantsMightBeInvalid;
> if (m...)
>   return;
> 
> at the top of this function.

Done.

https://codereview.chromium.org/129633003/diff/490001/Source/core/css/analyze...
Source/core/css/analyzer/DescendantInvalidationSet.cpp:62: if
(allDescendantsMightBeInvalid) {
On 2014/01/14 21:32:56, esprehn wrote:
> Braces are not needed.

Done.

https://codereview.chromium.org/129633003/diff/490001/Source/core/css/analyze...
Source/core/css/analyzer/DescendantInvalidationSet.cpp:75: if
(element->classNames().contains(*it)) {
On 2014/01/14 21:32:56, esprehn wrote:
> Braces are not needed and this should be a return otherwise you look at all
the
> classes even once one of them matches,
> 
> if (element->classNames().contains(*it)) 
>   return true;

Done.

https://codereview.chromium.org/129633003/diff/490001/Source/core/css/analyze...
Source/core/css/analyzer/DescendantInvalidationSet.cpp:82:
shouldRecalcThisElement = true;
On 2014/01/14 21:32:56, esprehn wrote:
> This function needs to be split and have an early return, not set some
variable
> like this.
> 
> if (m_tagNames.contains(element->tagQName().localName()))
>   return true;
> 
> Right now you do lots of needless hash lookups even though you already know
> something matches.

Done.

https://codereview.chromium.org/129633003/diff/490001/Source/core/css/analyze...
Source/core/css/analyzer/DescendantInvalidationSet.cpp:87: for (Node* child =
element->firstChild(); child; child = child->nextSibling()) {
On 2014/01/14 21:32:56, esprehn wrote:
> This doesn't handle ShadowRoots, but I think we always skip rules that could
> apply to ShadowRoots (ex. host or ^^ or ^) so they should never end up down
this
> path.
> 
> It might be worth adding a comment that system doesn't handle tree boundary
> crossing rules on purpose.

Done.

https://codereview.chromium.org/129633003/diff/490001/Source/core/css/analyze...
Source/core/css/analyzer/DescendantInvalidationSet.cpp:93: }
On 2014/01/14 21:32:56, esprehn wrote:
> This function needs to be split, you want
> 
> bool invalidatesElement(Element&) that just has lots of returns in it, and
> another one that walks the tree and calls it.

Done.

https://codereview.chromium.org/129633003/diff/490001/Source/core/css/analyze...
Source/core/css/analyzer/DescendantInvalidationSet.cpp:99:
element->document().ensureStyleResolver().addToStyleSharingList(*element);
On 2014/01/14 21:32:56, esprehn wrote:
> Remove this, you shouldn't be messing with the style sharing list from inside
> here. There's no reason for that (and it does nothing right now since you're
not
> inside recalcStyle when this code runs).

Done.

https://codereview.chromium.org/129633003/diff/490001/Source/core/css/analyze...
File Source/core/css/analyzer/DescendantInvalidationSet.h (right):

https://codereview.chromium.org/129633003/diff/490001/Source/core/css/analyze...
Source/core/css/analyzer/DescendantInvalidationSet.h:44: // Tracks data to
determine which elements of a DOM subtree ened to have style
On 2014/01/14 21:32:56, esprehn wrote:
> s/ened/need/

Done.

https://codereview.chromium.org/129633003/diff/490001/Source/core/css/analyze...
Source/core/css/analyzer/DescendantInvalidationSet.h:74: HashSet<AtomicString>
m_tagNames;
On 2014/01/14 21:32:56, esprehn wrote:
> HashSets are pretty big in terms of memory usage, that's why I had used OwnPtr
> originally. We can optimize that later though.

Done.

https://codereview.chromium.org/129633003/diff/490001/Source/core/css/analyze...
File Source/core/css/analyzer/RuleSetAnalyzer.cpp (right):

https://codereview.chromium.org/129633003/diff/490001/Source/core/css/analyze...
Source/core/css/analyzer/RuleSetAnalyzer.cpp:90: void ExtractClassIdOrTag(const
CSSSelector& selector, HashSet<AtomicString>* classes, AtomicString* id,
AtomicString* tagName)
On 2014/01/14 21:32:56, esprehn wrote:
> pass references for the classes, id and tagNames

Done.

https://codereview.chromium.org/129633003/diff/490001/Source/core/css/analyze...
Source/core/css/analyzer/RuleSetAnalyzer.cpp:104: }
On 2014/01/14 21:32:56, esprehn wrote:
> remove braces

Done.

https://codereview.chromium.org/129633003/diff/490001/Source/core/css/analyze...
Source/core/css/analyzer/RuleSetAnalyzer.cpp:183: DescendantInvalidationSet*
RuleSetAnalyzer::getClassInvalidationSet(const AtomicString& className) const
On 2014/01/14 21:32:56, esprehn wrote:
> ::invalidationSetForClass(...), get*() prefixes methods return things through
> out params like: void getFoo(Vector*), regular getters or find methods should
> not start with get.

Done.

https://codereview.chromium.org/129633003/diff/490001/Source/core/css/analyze...
Source/core/css/analyzer/RuleSetAnalyzer.cpp:191: if (descendantInvalidationSet)
{
On 2014/01/14 21:32:56, esprehn wrote:
> We usually combine the get and the if statement:
> 
> if (DescendantInvalidationSet* descendantInvalidationSet =
> classInvalidationSets.get(className)) {
>  descendantInvalidationSet->invalidateElementSubtree(element);
>  return true;
> }
> return false;

Done.

https://codereview.chromium.org/129633003/diff/490001/Source/core/css/analyze...
File Source/core/css/analyzer/RuleSetAnalyzer.h (right):

https://codereview.chromium.org/129633003/diff/490001/Source/core/css/analyze...
Source/core/css/analyzer/RuleSetAnalyzer.h:53: RuleSetAnalyzer() { }
On 2014/01/14 21:32:56, esprehn wrote:
> Put this in the cpp file, doesn't need to be inline.

Done.

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

https://codereview.chromium.org/129633003/diff/490001/Source/core/dom/Element...
Source/core/dom/Element.cpp:1054:
ruleSetAnalyzer->invalidateElementSubtreeForClassChange(changedClasses[i],
element);
On 2014/01/14 21:32:56, esprehn wrote:
> I had envisioned doing this in a single pass, not walking the subtree
> repeatedly. This is doing to doing awful things to Modernizer apps:
> 
> ex.
> 
> body.className = "foo bar baz biz bam" will walk the entire document 5 times.
> 
> We can land this as-is, but we should do it behind a flag then and iterate on
> combining the passes.

Done.

https://codereview.chromium.org/129633003/diff/490001/Source/core/dom/Element...
Source/core/dom/Element.cpp:1054:
ruleSetAnalyzer->invalidateElementSubtreeForClassChange(changedClasses[i],
element);
On 2014/01/14 21:32:56, esprehn wrote:
> You ignore the return value of invalidateElementSubtreeForClassChange which is
> not right, you want to combine the getRuleSetAnalyzer() and the
> invalidateElementSubtreeForClassChange steps into a method on checker.
> 
> if (checker.invalidateForClass(changedClasses[i], element))
>   continue;
> if (checker.hasSelectorForClass(changedClasses[i]))
>   return true;

Done.

https://codereview.chromium.org/129633003/diff/490001/Source/core/dom/Element...
Source/core/dom/Element.cpp:1086: if (ruleSetAnalyzer)
On 2014/01/14 21:32:56, esprehn wrote:
> Combing if and get, if (const RuleSet.... = checker.rulesetAnalyzer(...)).
> 
> invalidateElementSubtreeForClassChange also returns a bool, but you ignore it
> here which isn't right.

Done.

https://codereview.chromium.org/129633003/diff/490001/Source/core/dom/Element...
Source/core/dom/Element.cpp:1159: if (checkSelectorForClassChange(oldClasses,
featureSet, this))
On 2014/01/14 21:32:56, esprehn wrote:
> These should be methods now, don't pass |this|, just turn
> checkSelectorForClassChange into a method.

Done.

https://codereview.chromium.org/129633003/diff/490001/Source/core/dom/shadow/...
File Source/core/dom/shadow/SelectRuleFeatureSet.h (right):

https://codereview.chromium.org/129633003/diff/490001/Source/core/dom/shadow/...
Source/core/dom/shadow/SelectRuleFeatureSet.h:70: inline const RuleSetAnalyzer*
SelectRuleFeatureSet::getRuleSetAnalyzer() const
On 2014/01/14 21:32:56, esprehn wrote:
> Poor shadow DOM, add a FIXME to make ShadowDOM smarter someday.

Done.

[esprehn, 2014-01-15 04:27:32]

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

https://codereview.chromium.org/129633003/diff/610001/Source/core/css/RuleFea...
Source/core/css/RuleFeature.h:84: const RuleSetAnalyzer* getRuleSetAnalyzer()
const;
Still named wrong.

https://codereview.chromium.org/129633003/diff/610001/Source/core/css/RuleFea...
Source/core/css/RuleFeature.h:94: RefPtr<RuleSetAnalyzer> ruleSetAnalyzer;
Missing m_ prefix.

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

https://codereview.chromium.org/129633003/diff/610001/Source/core/dom/Element...
Source/core/dom/Element.cpp:1048: bool checkSelectorForClassChange(const
SpaceSplitString& changedClasses, const Checker& checker)
It doesn't seem like you need to touch this file at all now, can you just revert
it?

https://codereview.chromium.org/129633003/diff/610001/Source/core/dom/shadow/...
File Source/core/dom/shadow/SelectRuleFeatureSet.h (right):

https://codereview.chromium.org/129633003/diff/610001/Source/core/dom/shadow/...
Source/core/dom/shadow/SelectRuleFeatureSet.h:51: const RuleSetAnalyzer*
getRuleSetAnalyzer() const;
wrong name.

[chrishtr, 2014-01-15 18:28:21]

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

https://codereview.chromium.org/129633003/diff/610001/Source/core/css/RuleFea...
Source/core/css/RuleFeature.h:94: RefPtr<RuleSetAnalyzer> ruleSetAnalyzer;
On 2014/01/15 04:27:32, esprehn wrote:
> Missing m_ prefix.

Done.

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

https://codereview.chromium.org/129633003/diff/610001/Source/core/dom/Element...
Source/core/dom/Element.cpp:1048: bool checkSelectorForClassChange(const
SpaceSplitString& changedClasses, const Checker& checker)
On 2014/01/15 04:27:32, esprehn wrote:
> It doesn't seem like you need to touch this file at all now, can you just
revert
> it?

Done.

[chrishtr, 2014-01-15 18:29:16]

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

https://codereview.chromium.org/129633003/diff/610001/Source/core/css/RuleFea...
Source/core/css/RuleFeature.h:84: const RuleSetAnalyzer* getRuleSetAnalyzer()
const;
On 2014/01/15 04:27:32, esprehn wrote:
> Still named wrong.

Done.

https://codereview.chromium.org/129633003/diff/610001/Source/core/dom/shadow/...
File Source/core/dom/shadow/SelectRuleFeatureSet.h (right):

https://codereview.chromium.org/129633003/diff/610001/Source/core/dom/shadow/...
Source/core/dom/shadow/SelectRuleFeatureSet.h:51: const RuleSetAnalyzer*
getRuleSetAnalyzer() const;
On 2014/01/15 04:27:32, esprehn wrote:
> wrong name.

Done.

[chrishtr, 2014-01-16 20:07:52]

Does the patch look ok now? I'm already working on a second patch to implement
the invalidation in a tree walk.

[esprehn, 2014-01-16 20:11:47]

lgtm, fix nits before landing.

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

https://codereview.chromium.org/129633003/diff/900001/Source/core/css/RuleFea...
Source/core/css/RuleFeature.cpp:110: return m_ruleSetAnalyzer.get();
return *m_ruleSetAnalyzer;

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

https://codereview.chromium.org/129633003/diff/900001/Source/core/css/RuleFea...
Source/core/css/RuleFeature.h:85: RuleSetAnalyzer* ensureRuleSetAnalyzer();
This should return a reference

https://codereview.chromium.org/129633003/diff/900001/Source/core/css/analyze...
File Source/core/css/analyzer/DescendantInvalidationSet.h (right):

https://codereview.chromium.org/129633003/diff/900001/Source/core/css/analyze...
Source/core/css/analyzer/DescendantInvalidationSet.h:46: class
DescendantInvalidationSet : public RefCounted<DescendantInvalidationSet> {
FINAL

https://codereview.chromium.org/129633003/diff/900001/Source/core/css/analyze...
File Source/core/css/analyzer/RuleSetAnalyzer.cpp (right):

https://codereview.chromium.org/129633003/diff/900001/Source/core/css/analyze...
Source/core/css/analyzer/RuleSetAnalyzer.cpp:178: DescendantInvalidationSet*
RuleSetAnalyzer::ensureClassInvalidationSet(const AtomicString& className)
Return a reference.

[chrishtr, 2014-01-16 20:46:21]

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

https://codereview.chromium.org/129633003/diff/900001/Source/core/css/RuleFea...
Source/core/css/RuleFeature.cpp:110: return m_ruleSetAnalyzer.get();
On 2014/01/16 20:11:48, esprehn wrote:
> return *m_ruleSetAnalyzer;

Done.

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

https://codereview.chromium.org/129633003/diff/900001/Source/core/css/RuleFea...
Source/core/css/RuleFeature.h:85: RuleSetAnalyzer* ensureRuleSetAnalyzer();
On 2014/01/16 20:11:48, esprehn wrote:
> This should return a reference

Done.

https://codereview.chromium.org/129633003/diff/900001/Source/core/css/analyze...
File Source/core/css/analyzer/DescendantInvalidationSet.h (right):

https://codereview.chromium.org/129633003/diff/900001/Source/core/css/analyze...
Source/core/css/analyzer/DescendantInvalidationSet.h:46: class
DescendantInvalidationSet : public RefCounted<DescendantInvalidationSet> {
On 2014/01/16 20:11:48, esprehn wrote:
> FINAL

Done.

https://codereview.chromium.org/129633003/diff/900001/Source/core/css/analyze...
File Source/core/css/analyzer/RuleSetAnalyzer.cpp (right):

https://codereview.chromium.org/129633003/diff/900001/Source/core/css/analyze...
Source/core/css/analyzer/RuleSetAnalyzer.cpp:178: DescendantInvalidationSet*
RuleSetAnalyzer::ensureClassInvalidationSet(const AtomicString& className)
On 2014/01/16 20:11:48, esprehn wrote:
> Return a reference.

Done.

[commit-bot: I haz the power, 2014-01-16 20:46:36]

No LGTM from a valid reviewer yet. Only full committers are accepted.
Even if an LGTM may have been provided, it was from a non-committer or
a lowly provisional committer, _not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[esprehn, 2014-01-16 20:53:58]

lgtm

[dglazkov, 2014-01-16 20:56:16]

lGtM

[commit-bot: I haz the power, 2014-01-16 21:00:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/esprehn@chromium.org/129633003/1120001

[commit-bot: I haz the power, 2014-01-16 22:59:41]

Change committed as 165256