WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
140878
Fix CSS Selector's tag name matching when mixing HTML and XML
https://bugs.webkit.org/show_bug.cgi?id=140878
Summary
Fix CSS Selector's tag name matching when mixing HTML and XML
Benjamin Poulain
Reported
2015-01-25 22:34:48 PST
Fix CSS Selector's tag name matching when mixing HTML and XML
Attachments
Patch
(338.09 KB, patch)
2015-01-25 23:01 PST
,
Benjamin Poulain
darin
: review+
Details
Formatted Diff
Diff
View All
Add attachment
proposed patch, testcase, etc.
Benjamin Poulain
Comment 1
2015-01-25 23:01:06 PST
Created
attachment 245329
[details]
Patch
Darin Adler
Comment 2
2015-01-25 23:37:14 PST
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
Comment 3
2015-01-26 12:40:21 PST
Committed
r179132
: <
http://trac.webkit.org/changeset/179132
>
Benjamin Poulain
Comment 4
2015-01-27 01:37:49 PST
***
Bug 83438
has been marked as a duplicate of this bug. ***
Benjamin Poulain
Comment 5
2015-01-27 01:38:34 PST
***
Bug 79444
has been marked as a duplicate of this bug. ***
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug