[webkit-reviews] review granted: [Bug 77525] SelectorChecker::checkSelector: move parameters into a struct : [Attachment 126213] Patch

[bugzilla-daemon at webkit.org]

Antti Koivisto <koivisto at iki.fi> has granted Roland Steiner
<rolandsteiner at chromium.org>'s request for review:
Bug 77525: SelectorChecker::checkSelector: move parameters into a struct
https://bugs.webkit.org/show_bug.cgi?id=77525

Attachment 126213: Patch
https://bugs.webkit.org/attachment.cgi?id=126213&action=review

------- Additional Comments from Antti Koivisto <koivisto at iki.fi>
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.