Bug 108595

Created attachment 185974 [details] Patch

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?

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.

Created attachment 186988 [details] Patch

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.

Comment on attachment 186988 [details] Patch I am big fan of this patch. Antti, what do you think?

Comment on attachment 186988 [details] Patch LGTM.

I would like to land this patch if no objection.

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

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.

Comment on attachment 186988 [details] Patch Clearing flags on attachment: 186988 Committed r142591: <http://trac.webkit.org/changeset/142591>

All reviewed patches have been landed. Closing bug.

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.