Bug 77525

Summary: SelectorChecker::checkSelector: move parameters into a struct
Product: WebKit Reporter: Roland Steiner <rolandsteiner>
Component: CSSAssignee: Roland Steiner <rolandsteiner>
Status: RESOLVED FIXED    
Severity: Normal CC: dglazkov, koivisto, macpherson, menard, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 73192, 77527, 77528    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
patch for EWS
none
patch for EWS none

Description Roland Steiner 2012-02-01 01:32:07 PST
The signature of SelectorChecker::checkSelector is getting bloaty - move the parameters into a struct
Comment 1 Roland Steiner 2012-02-03 01:04:46 PST
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.
Comment 2 Roland Steiner 2012-02-03 01:06:49 PST
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.
Comment 3 Antti Koivisto 2012-02-03 01:32:03 PST
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.
Comment 4 Antti Koivisto 2012-02-03 01:34:33 PST
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.
Comment 5 Roland Steiner 2012-02-06 01:31:11 PST
Created attachment 125595 [details]
Patch

New patch, using const references as discussed. Also added a constructor to SelectorCheckingCOntext.
Comment 6 WebKit Review Bot 2012-02-06 02:44:09 PST
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
Comment 7 Antti Koivisto 2012-02-07 21:45:31 PST
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.
Comment 8 Roland Steiner 2012-02-07 22:52:49 PST
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.
Comment 9 Roland Steiner 2012-02-08 18:15:50 PST
Created attachment 126213 [details]
Patch

Patch, small readability update. Also poke EWS bots.
Comment 10 Antti Koivisto 2012-02-08 21:00:27 PST
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.
Comment 11 Roland Steiner 2012-02-08 22:00:42 PST
Created attachment 126239 [details]
patch for EWS

testing patch for EWS, incl. the last changes discussed on IRC.
Comment 12 WebKit Review Bot 2012-02-08 23:14:41 PST
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
Comment 13 Roland Steiner 2012-02-09 02:21:58 PST
Reverted r107197 for reason:

broke

Committed r107205: <http://trac.webkit.org/changeset/107205>
Comment 14 Roland Steiner 2012-02-09 03:08:15 PST
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.
Comment 15 Roland Steiner 2012-02-09 04:11:01 PST
Committed in r107212 with a bungled commit message (fixed in r107214). 'Dem bots and scripts really hated me today... :P