[Webkit-unassigned] [Bug 68633] Optimize matching of common pseudo classes

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=68633





--- Comment #4 from Darin Adler <darin at apple.com>  2011-09-22 12:33:32 PST ---
(From update of attachment 108379)
View in context: https://bugs.webkit.org/attachment.cgi?id=108379&action=review

The optimization here looks great. r=me as is (but I see Hyatt beat me to it). I have comments, but they are all optional ideas for possible improvement.

> Source/WebCore/ChangeLog:14
> +        This patch adds a new list to RuleSet for common pseude class rules. Selectors with the rightmost component of 

Typo: pseude

> Source/WebCore/css/CSSStyleSelector.cpp:237
>      const Vector<RuleData>* getIDRules(AtomicStringImpl* key) const { return m_idRules.get(key); }
>      const Vector<RuleData>* getClassRules(AtomicStringImpl* key) const { return m_classRules.get(key); }
>      const Vector<RuleData>* getTagRules(AtomicStringImpl* key) const { return m_tagRules.get(key); }
> -    const Vector<RuleData>* getPseudoRules(AtomicStringImpl* key) const { return m_pseudoRules.get(key); }
> +    const Vector<RuleData>* getShadowPseudoRules(AtomicStringImpl* key) const { return m_shadowPseudoRules.get(key); }
> +    const Vector<RuleData>* getCommonPseudoClassRules() const { return &m_commonPseudoClassRules; }
>      const Vector<RuleData>* getUniversalRules() const { return &m_universalRules; }
>      const Vector<RuleData>* getPageRules() const { return &m_pageRules; }

I’m getting increasingly annoyed that all of these functions use “get” in a way that does not match usual WebKit naming style.

> Source/WebCore/css/CSSStyleSelector.cpp:529
> +namespace {
> +inline bool fastReject(SelectorChecker& checker, bool canUseFastRejectSelector, const Element*, const RuleData& ruleData)
> +{ 
> +    if (!canUseFastRejectSelector)
> +        return false;
> +    return checker.fastRejectSelector<RuleData::maximumIdentifierCount>(ruleData.descendantSelectorIdentifierHashes());
> +}
> +inline bool pseudoClassReject(SelectorChecker& checker, bool canUseFastRejectSelector, const Element* element, const RuleData& ruleData)
> +{ 
> +    if (!checker.commonPseudoClassSelectorMatches(element, ruleData.selector()))
> +        return true;
> +    return fastReject(checker, canUseFastRejectSelector, element, ruleData);
> +}
> +}

While we can use an anonymous namespace to get internal linkage, normally we just use “static” to do that instead. Or maybe that doesn’t work because of the use of these functions as template arguments?

> Source/WebCore/css/CSSStyleSelector.cpp:556
> +        const Vector<RuleData>* commonPseudoClassRules = rules->getCommonPseudoClassRules();
> +        matchRulesForList<pseudoClassReject>(commonPseudoClassRules, firstRuleIndex, lastRuleIndex, includeEmptyRules);

Not sure the local variable makes this easier to read.

> Source/WebCore/css/SelectorChecker.cpp:378
> +    bool first = true;

I would call this onFirstSelector or something like that. The name first doesn’t make it clear it’s a boolean.

> Source/WebCore/css/SelectorChecker.cpp:386
>      for (; selector; selector = selector->tagHistory()) {
>          if (selector->relation() != CSSSelector::Descendant && selector->relation() != CSSSelector::Child && selector->relation() != CSSSelector::SubSelector)
>              return false;
> +        if (first) {
> +            first = false;
> +            if (isCommonPseudoClassSelector(selector))
> +                continue;
> +        }

Should we unroll the loop a little just to make the loop structure less confusing? It would make it possible to get rid of the boolean. I think that if the relation check and match checks were named inline helper functions it would be easy to unroll a bit and keep the code looking great.

> Source/WebCore/css/SelectorChecker.cpp:1308
> +bool SelectorChecker::commonPseudoClassSelectorMatches(const Element* element, const CSSSelector* selector) const

Is there code we could make this share with? It seems unpleasant to have those forcePseudoState calls in multiple places.

Is there a good reason the Element* is const? I think using const* for elements usually just gets in the way and makes for wordier code.

> Source/WebCore/css/SelectorChecker.cpp:1315
> +        return element->isLink() && (m_isMatchingVisitedPseudoClass || InspectorInstrumentation::forcePseudoState(const_cast<Element*>(element), CSSSelector::PseudoVisited));

Wow, that forcePseudoState thing is strange!

> Source/WebCore/css/SelectorChecker.cpp:1319
> +        return (element->focused() && element->document()->frame() && element->document()->frame()->selection()->isFocusedAndActive()) 

Seems like this expression should be in a helper function taking an element pointer, isFocusedInActiveFrame or something like that.

> Source/WebCore/css/SelectorChecker.cpp:1320
> +        || InspectorInstrumentation::forcePseudoState(const_cast<Element*>(element), CSSSelector::PseudoFocus);

This should be indented one more tab stop.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.