| Summary: | Fix CSS Selector's tag name matching when mixing HTML and XML | ||||||
|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Benjamin Poulain <benjamin> | ||||
| Component: | New Bugs | Assignee: | Benjamin Poulain <benjamin> | ||||
| Status: | RESOLVED FIXED | ||||||
| Severity: | Normal | CC: | bzbarsky, rwlbuis | ||||
| Priority: | P2 | ||||||
| Version: | 528+ (Nightly build) | ||||||
| Hardware: | Unspecified | ||||||
| OS: | Unspecified | ||||||
| Attachments: |
|
||||||
|
Description
Benjamin Poulain
2015-01-25 22:34:48 PST
Created attachment 245329 [details]
Patch
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. Committed r179132: <http://trac.webkit.org/changeset/179132> *** Bug 83438 has been marked as a duplicate of this bug. *** |