Bug 140878 – Fix CSS Selector's tag name matching when mixing HTML and XML

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Benjamin Poulain]

Fix CSS Selector's tag name matching when mixing HTML and XML

[Benjamin Poulain]

Created attachment 245329 [details]
Patch

[Darin Adler]

Comment on attachment 245329 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=245329&action=review

> Source/WebCore/css/CSSSelector.h:330
>          unsigned m_hasRareData           : 1;
> +        unsigned m_hasNameWithCase       : 1;

It’s a little inconsistent how the data is stored in a single data union, but the flags for what’s in that union are fully independent.

> Source/WebCore/css/CSSSelector.h:380
> +            NameWithCase *m_nameWithCase;

Wrong position for the "*" here, it should be by the type.

> Source/WebCore/css/RuleSet.h:223
> +    if (isHTMLName)
> +        return m_tagLowercaseLocalNameRules.get(key);
> +    return m_tagLocalNameRules.get(key);

Given these are both the same type it might be better to share the same call to get and just chose the map with the boolean. Will the compiler’s common subexpression code notice these two giant inlined functions are identical?

> Source/WebCore/css/SelectorChecker.cpp:547
>  }
>  
> +
> +static inline bool tagMatches(const Element& element, const CSSSelector& simpleSelector)

Extra blank line here.

> Source/WebCore/css/SelectorFilter.cpp:129
> +            (*hash++) = tagLowercaseLocalName.impl()->existingHash() * TagNameSalt;

The parentheses around (*hash++) on this line and others make this code look strange to me. Could we omit those?

> Source/WebCore/css/SelectorFilter.cpp:132
>          break;
> +        }
>      default:

This is not where we normally put the braces for switch statement cases. You should look at other code or the style guide for guidance.

> Source/WebCore/cssjit/SelectorCompiler.cpp:188
> +    const CSSSelector* tagNameSelector;

I suggest putting the default { nullptr } here instead of in the constructor. Would work well for other data members too.

> Source/WebCore/cssjit/SelectorCompiler.cpp:231
> +    const CSSSelector* tagNameSelector;

I suggest putting the default { nullptr } here instead of in the constructor.

> Source/WebCore/cssjit/SelectorCompiler.cpp:232
>      bool inverted;

And here too, and then get rid of the constructor.

> Source/WebCore/cssjit/SelectorCompiler.cpp:1374
> +    if (!lhs || !rhs)
>          return TagNameEquality::MaybeEqual;

This behavior of null is strange.

[Benjamin Poulain]

Committed r179132: <http://trac.webkit.org/changeset/179132>

[Benjamin Poulain]

*** Bug 83438 has been marked as a duplicate of this bug. ***

[Benjamin Poulain]

*** Bug 79444 has been marked as a duplicate of this bug. ***