Bug 106860 – Split SelectorChecker's fast-checking logic into its own class.

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Dimitri Glazkov (Google)]

Split SelectorChecker's fast-checking logic into its own class.

[Dimitri Glazkov (Google)]

Created attachment 182687 [details]
WIP

[Dimitri Glazkov (Google)]

Created attachment 183362 [details]
Things work.

[Dimitri Glazkov (Google)]

Created attachment 183507 [details]
Rebased, ready for EWS bubbles.

[Dimitri Glazkov (Google)]

Comment on attachment 183507 [details]
Rebased, ready for EWS bubbles.

Bubbles say "yay". Antti, Eric -- wdyt?

[Build Bot]

Comment on attachment 183507 [details]
Rebased, ready for EWS bubbles.

Attachment 183507 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/15942697

[Dimitri Glazkov (Google)]

Created attachment 183599 [details]
Awesome.

[Antti Koivisto]

Comment on attachment 183599 [details]
Awesome.

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

Looks pretty good. Please hold this until kling gets his complicated CSSSelector changes in though.

> Source/WebCore/css/FastPathSelectorMatcher.h:39
> +    FastPathSelectorMatcher(CSSSelector*, Element*);

I think the naming should be consistent with SelectorChecker. I'd also prefer prefix style naming for subsystems. This one could be SelectorCheckerFastPath. What do you think?

> Source/WebCore/css/FastPathSelectorMatcher.h:45
> +    static bool canUseFastPath(const CSSSelector*);

FastPath is redundant.

[Darin Adler]

Comment on attachment 183599 [details]
Awesome.

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

>> Source/WebCore/css/FastPathSelectorMatcher.h:39
>> +    FastPathSelectorMatcher(CSSSelector*, Element*);
> 
> I think the naming should be consistent with SelectorChecker. I'd also prefer prefix style naming for subsystems. This one could be SelectorCheckerFastPath. What do you think?

For what it’s worth, I agree with both of Antti’s suggestions here and I think SelectorCheckerFastPath is a good name.

[Dimitri Glazkov (Google)]

(In reply to comment #8)
> (From update of attachment 183599 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=183599&action=review
> 
> >> Source/WebCore/css/FastPathSelectorMatcher.h:39
> >> +    FastPathSelectorMatcher(CSSSelector*, Element*);
> > 
> > I think the naming should be consistent with SelectorChecker. I'd also prefer prefix style naming for subsystems. This one could be SelectorCheckerFastPath. What do you think?
> 
> For what it’s worth, I agree with both of Antti’s suggestions here and I think SelectorCheckerFastPath is a good name.

SelectorCheckerFastPath it is! Man, I totally wanted to punk Kling and land this before him :P

[Dimitri Glazkov (Google)]

Created attachment 188737 [details]
Patch

[Eric Seidel (no email)]

Are you just waiting for love here dglazkov?

[Antti Koivisto]

Comment on attachment 188737 [details]
Patch

r=me, looks good

[Dimitri Glazkov (Google)]

Created attachment 189671 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 189671 [details]
Patch for landing

Clearing flags on attachment: 189671

Committed r143686: <http://trac.webkit.org/changeset/143686>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.