Bug 77525 – SelectorChecker::checkSelector: move parameters into a struct

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Roland Steiner]

The signature of SelectorChecker::checkSelector is getting bloaty - move the parameters into a struct

[Roland Steiner]

Created attachment 125283 [details]
Patch

Patch comments: Passing the struct SelectorCheckingContext by value in checkSelector: this corresponds to the previous parameter-passing and the internals are also modified rather heavily. In comparison, I pass them by const reference to checkOneSelector, because only :not and :-webkit-any need to modify anything.

[Roland Steiner]

Addendum: I'm also using references in checkOneSelector to avoid replacing 'e' and 'sel' with the much longer 'context.m_element' and 'context.m_selector' everywhere. Not sure if that's groovy.

[Antti Koivisto]

Comment on attachment 125283 [details]
Patch

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

> Source/WebCore/css/CSSStyleSelector.cpp:2057
> +    SelectorChecker::SelectorCheckingContext context = {
> +        ruleData.selector(), // m_selector
> +        m_element, // m_element
> +        false, // m_isSubSelector
> +        SelectorChecker::VisitedMatchEnabled, // m_visitedMatchType
> +        style(), // m_elementStyle
> +        m_parentNode ? m_parentNode->renderStyle() : 0 // m_elementParentStyle
> +    };

You should initialize using dot notation so yo don't need these comments
context.foo = bar;

SelectorCheckingContext should have constructor that initializes the mandatory stuff (selector and element at least) and sets the rest to defaults.

Drop the m_ prefixes from the field names (those are not used with public structs).

> Source/WebCore/css/SelectorChecker.cpp:454
> +SelectorChecker::SelectorMatch SelectorChecker::checkSelector(SelectorCheckingContext context, PseudoId& dynamicPseudo) const

This should use const SelectorCheckingContext&. To modify the context you should make a copy explicitly.

[Antti Koivisto]

Comment on attachment 125283 [details]
Patch

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

> Source/WebCore/css/SelectorChecker.cpp:1004
> +                SelectorChecker::SelectorCheckingContext subContext = {

SelectorChecker:: scope is implied, you don't need to use it explicitly.

[Roland Steiner]

Created attachment 125595 [details]
Patch

New patch, using const references as discussed. Also added a constructor to SelectorCheckingCOntext.

[WebKit Review Bot]

Comment on attachment 125595 [details]
Patch

Attachment 125595 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11430332

New failing tests:
scrollbars/disabled-scrollbar.html
scrollbars/scrollbar-orientation.html
scrollbars/scrollbar-buttons.html
scrollbars/overflow-scrollbar-combinations.html
scrollbars/listbox-scrollbar-combinations.html

[Antti Koivisto]

Comment on attachment 125595 [details]
Patch

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

looks pretty ok now

> Source/WebCore/css/SelectorChecker.cpp:507
> +        historyContext.element = context.element->previousElementSibling();
> +        if (!historyContext.element)
> +            return SelectorFailsAllSiblings;
> +        historyContext.setCompoundSelector();

This still somehow seems bit messy.

> Source/WebCore/css/SelectorChecker.h:56
> +        SelectorCheckingContext(CSSSelector* selector, Element* element, bool isSubSelector, VisitedMatchType visitedMatchType, RenderStyle* elementStyle, RenderStyle* elementParentStyle)

You are never setting isSubSelector to any other value than false so the parameter can be removed.

[Roland Steiner]

Created attachment 126011 [details]
Patch

New patch, removed isSubSelector parameter to constructor, removed setCompoundSelector & replaced with sub-selector and compound selector 'copy' constructors. Note: the compound selector constructor ignores the old context for now, however, the signature will come in handy when being extended for <style scoped>. Also updated variable names 'e'->'element' and 'sel'->'selector' in checkOneSelector() as requested. The only real changes in this function concern pseudoAny and pseudoNot.

[Roland Steiner]

Created attachment 126213 [details]
Patch

Patch, small readability update. Also poke EWS bots.

[Antti Koivisto]

Comment on attachment 126213 [details]
Patch

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

r=me, a few comments to consider

> Source/WebCore/css/SelectorChecker.cpp:697
> +    Element* const & element = context.element;
> +    CSSSelector* const & selector = context.selector;

I think our coding style would have const& here (without space). Not entirely sure...

> Source/WebCore/css/SelectorChecker.h:77
> +
> +        inline SelectorCheckingContext createSubContext(CSSSelector* subSelector) const
> +        {
> +            return SelectorCheckingContext(subSelector, element, visitedMatchType, elementStyle, elementParentStyle, true);
> +        }
> +        inline SelectorCheckingContext createSubContext(CSSSelector* subSelector, VisitedMatchType newMatchType) const
> +        {
> +            return SelectorCheckingContext(subSelector, element, newMatchType, elementStyle, elementParentStyle, true);
> +        }
> +        inline SelectorCheckingContext createCompoundContext(CSSSelector* compoundSelector, Element* newElement, VisitedMatchType newMatchType) const
> +        {
> +            return SelectorCheckingContext(compoundSelector, newElement, newMatchType);
> +        }

inline keywords here do nothing and should be removed.

I feel that while these functions make the code more compact they also make it somewhat more magical. A better pattern would be to just make a copy and explicitly set the fields that you are altering.

[Roland Steiner]

Created attachment 126239 [details]
patch for EWS

testing patch for EWS, incl. the last changes discussed on IRC.

[WebKit Review Bot]

Comment on attachment 126239 [details]
patch for EWS

Attachment 126239 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11449792

New failing tests:
platform/chromium/compositing/layout-width-change.html
scrollbars/scrollbar-orientation.html
scrollbars/disabled-scrollbar.html
scrollbars/scrollbar-buttons.html
scrollbars/listbox-scrollbar-combinations.html
scrollbars/overflow-scrollbar-combinations.html

[Roland Steiner]

Reverted r107197 for reason:

broke

Committed r107205: <http://trac.webkit.org/changeset/107205>

[Roland Steiner]

Created attachment 126268 [details]
patch for EWS

Argh, must have pressed Undo once too many - that's what I get for hurrying... :P One mor round of EWS to make sure I got it right this time.

[Roland Steiner]

Committed in r107212 with a bungled commit message (fixed in r107214). 'Dem bots and scripts really hated me today... :P