Bug 106860 - Split SelectorChecker's fast-checking logic into its own class.
Summary: Split SelectorChecker's fast-checking logic into its own class.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Dimitri Glazkov (Google)
URL:
Keywords:
Depends on:
Blocks: 89879
  Show dependency treegraph
 
Reported: 2013-01-14 20:00 PST by Dimitri Glazkov (Google)
Modified: 2013-02-21 21:17 PST (History)
12 users (show)

See Also:


Attachments
WIP (18.65 KB, patch)
2013-01-14 20:00 PST, Dimitri Glazkov (Google)
no flags Details | Formatted Diff | Diff
Things work. (49.00 KB, patch)
2013-01-17 20:55 PST, Dimitri Glazkov (Google)
no flags Details | Formatted Diff | Diff
Rebased, ready for EWS bubbles. (41.02 KB, patch)
2013-01-18 10:58 PST, Dimitri Glazkov (Google)
no flags Details | Formatted Diff | Diff
Awesome. (41.07 KB, patch)
2013-01-18 21:07 PST, Dimitri Glazkov (Google)
no flags Details | Formatted Diff | Diff
Patch (65.30 KB, patch)
2013-02-16 15:58 PST, Dimitri Glazkov (Google)
no flags Details | Formatted Diff | Diff
Patch for landing (34.92 KB, patch)
2013-02-21 20:55 PST, Dimitri Glazkov (Google)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dimitri Glazkov (Google) 2013-01-14 20:00:39 PST
Split SelectorChecker's fast-checking logic into its own class.
Comment 1 Dimitri Glazkov (Google) 2013-01-14 20:00:55 PST
Created attachment 182687 [details]
WIP
Comment 2 Dimitri Glazkov (Google) 2013-01-17 20:55:45 PST
Created attachment 183362 [details]
Things work.
Comment 3 Dimitri Glazkov (Google) 2013-01-18 10:58:19 PST
Created attachment 183507 [details]
Rebased, ready for EWS bubbles.
Comment 4 Dimitri Glazkov (Google) 2013-01-18 14:18:13 PST
Comment on attachment 183507 [details]
Rebased, ready for EWS bubbles.

Bubbles say "yay". Antti, Eric -- wdyt?
Comment 5 Build Bot 2013-01-18 15:00:20 PST
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
Comment 6 Dimitri Glazkov (Google) 2013-01-18 21:07:08 PST
Created attachment 183599 [details]
Awesome.
Comment 7 Antti Koivisto 2013-01-19 04:58:37 PST
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.
Comment 8 Darin Adler 2013-01-19 08:26:40 PST
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.
Comment 9 Dimitri Glazkov (Google) 2013-01-19 08:33:59 PST
(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
Comment 10 Dimitri Glazkov (Google) 2013-02-16 15:58:19 PST
Created attachment 188737 [details]
Patch
Comment 11 Eric Seidel (no email) 2013-02-20 22:53:46 PST
Are you just waiting for love here dglazkov?
Comment 12 Antti Koivisto 2013-02-21 03:52:18 PST
Comment on attachment 188737 [details]
Patch

r=me, looks good
Comment 13 Dimitri Glazkov (Google) 2013-02-21 20:55:13 PST
Created attachment 189671 [details]
Patch for landing
Comment 14 WebKit Review Bot 2013-02-21 21:17:20 PST
Comment on attachment 189671 [details]
Patch for landing

Clearing flags on attachment: 189671

Committed r143686: <http://trac.webkit.org/changeset/143686>
Comment 15 WebKit Review Bot 2013-02-21 21:17:25 PST
All reviewed patches have been landed.  Closing bug.