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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Fri Feb 3 01:32:03 PST 2012


Antti Koivisto <koivisto at iki.fi> has denied 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 125283: Patch
https://bugs.webkit.org/attachment.cgi?id=125283&action=review

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


More information about the webkit-reviews mailing list