Bug 108595 – [Refactoring] Make m_selectorChecker in StyleResolver an on-stack object.

• Component: CSS
• Status: RESOLVED FIXED

[Takashi Sakamoto]

StyleResolver uses m_selectorChecker.mode() as a part of StyleResolver's state while resolving styles.
It would be better to move the state from SelectorChecker to StyleResolver.

[Takashi Sakamoto]

Created attachment 185974 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 185974 [details]
Patch

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

This seems reasonable.  I'm too tired to do a real review, but I can look again tomorrow if antti doesn't beat me to it. :)

> Source/WebCore/css/SelectorChecker.cpp:72
> +inline bool commonPseudoClassSelectorMatches(const Element* element, const CSSSelector* selector, SelectorChecker::VisitedMatchType visitedMatchType)

I'm confused why these are moving to inlines?  Also, do you need to mark these static to avoid the compiler creating exportable symbols for them?

[Dimitri Glazkov (Google)]

Comment on attachment 185974 [details]
Patch

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

This is glorious. Antti, you like?

>> Source/WebCore/css/SelectorChecker.cpp:72
>> +inline bool commonPseudoClassSelectorMatches(const Element* element, const CSSSelector* selector, SelectorChecker::VisitedMatchType visitedMatchType)
> 
> I'm confused why these are moving to inlines?  Also, do you need to mark these static to avoid the compiler creating exportable symbols for them?

There's some unnecessary moves here in this patch, and the diff is all getting confused.

> Source/WebCore/css/SelectorChecker.cpp:-1060
> -    return element->document()->frame() && element->document()->frame()->selection()->isFocusedAndActive();

This should really be on Element or Document.

> Source/WebCore/css/StyleResolver.cpp:1364
> +    if (document()->inQuirksMode())

I am curious why we put the flags into m_selectorChecker originally? Is this like a performance optimization? Doesn't seem like it.

[Takashi Sakamoto]

Created attachment 186988 [details]
Patch

[Takashi Sakamoto]

Comment on attachment 185974 [details]
Patch

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

Thank you for reviewing.

>>> Source/WebCore/css/SelectorChecker.cpp:72
>>> +inline bool commonPseudoClassSelectorMatches(const Element* element, const CSSSelector* selector, SelectorChecker::VisitedMatchType visitedMatchType)
>> 
>> I'm confused why these are moving to inlines?  Also, do you need to mark these static to avoid the compiler creating exportable symbols for them?
> 
> There's some unnecessary moves here in this patch, and the diff is all getting confused.

Yeah. The diff looks very dirty. I reverted "inline" changes and made some methods "static", instead.

>> Source/WebCore/css/SelectorChecker.cpp:-1060
>> -    return element->document()->frame() && element->document()->frame()->selection()->isFocusedAndActive();
> 
> This should really be on Element or Document.

I see. I can do this in this patch? Or is it better to do in another patch?

>> Source/WebCore/css/StyleResolver.cpp:1364
>> +    if (document()->inQuirksMode())
> 
> I am curious why we put the flags into m_selectorChecker originally? Is this like a performance optimization? Doesn't seem like it.

I heard that inQuirksMode was very slow method. So it was probably bad to use document()->inQuirksMode(). However I'm not sure whether this is true or not.

[Dimitri Glazkov (Google)]

Comment on attachment 186988 [details]
Patch

I am big fan of this patch. Antti, what do you think?

[Eric Seidel (no email)]

Comment on attachment 186988 [details]
Patch

LGTM.

[Takashi Sakamoto]

I would like to land this patch if no objection.

[Takashi Sakamoto]

(In reply to comment #8)
> I would like to land this patch if no objection.

in a few days.

[Eric Seidel (no email)]

Comment on attachment 186988 [details]
Patch

It's been 4 days (yes 2 of them were a weekend).  I think you're good to go.  We can always roll it out, or amend if someone feels strongly.

[WebKit Review Bot]

Comment on attachment 186988 [details]
Patch

Clearing flags on attachment: 186988

Committed r142591: <http://trac.webkit.org/changeset/142591>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Antti Koivisto]

Comment on attachment 186988 [details]
Patch

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

Yeah, looked fine.

> Source/WebCore/css/StyleResolver.cpp:2134
> +    SelectorChecker selectorChecker(document());
> +    selectorChecker.setMode(state.mode);

Mode could be a constructor parameter and setMode() removed.