Bug 105660 - Split fast-rejection filter logic off SelectorChecker.
Summary: Split fast-rejection filter logic off SelectorChecker.
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: 2012-12-21 14:46 PST by Dimitri Glazkov (Google)
Modified: 2012-12-23 22:23 PST (History)
14 users (show)

See Also:


Attachments
Patch (21.76 KB, patch)
2012-12-21 14:50 PST, Dimitri Glazkov (Google)
no flags Details | Formatted Diff | Diff
Now with actual files. (32.49 KB, patch)
2012-12-21 15:55 PST, Dimitri Glazkov (Google)
no flags Details | Formatted Diff | Diff
Patch for landing (32.29 KB, patch)
2012-12-23 19:59 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) 2012-12-21 14:46:29 PST
Split fast-rejection filter logic off SelectorChecker.
Comment 1 Dimitri Glazkov (Google) 2012-12-21 14:50:00 PST
Created attachment 180561 [details]
Patch
Comment 2 Build Bot 2012-12-21 14:56:14 PST
Comment on attachment 180561 [details]
Patch

Attachment 180561 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/15460466
Comment 3 EFL EWS Bot 2012-12-21 14:59:33 PST
Comment on attachment 180561 [details]
Patch

Attachment 180561 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/15449585
Comment 4 Early Warning System Bot 2012-12-21 15:03:33 PST
Comment on attachment 180561 [details]
Patch

Attachment 180561 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/15456567
Comment 5 kov's GTK+ EWS bot 2012-12-21 15:05:43 PST
Comment on attachment 180561 [details]
Patch

Attachment 180561 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/15445627
Comment 6 Early Warning System Bot 2012-12-21 15:06:00 PST
Comment on attachment 180561 [details]
Patch

Attachment 180561 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/15451569
Comment 7 Eric Seidel (no email) 2012-12-21 15:09:48 PST
Comment on attachment 180561 [details]
Patch

Failed to add the files. :)
Comment 8 Dimitri Glazkov (Google) 2012-12-21 15:55:54 PST
Created attachment 180567 [details]
Now with actual files.
Comment 9 Eric Seidel (no email) 2012-12-21 15:59:38 PST
Comment on attachment 180567 [details]
Now with actual files.

Its a bit odd that the stack is in teh filter object, but seems OK.  Definitely better that the current location.
Comment 10 Antti Koivisto 2012-12-21 16:27:47 PST
Comment on attachment 180567 [details]
Now with actual files.

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

Looks like a good refactoring.

> Source/WebCore/ChangeLog:7
> +        The awesome Bloom filter and parent stack logic don't need to be in SelectorChecker. They nicely abstract out
> +        into their own pretty thing, named thereby SelectorFilter.

It is not exactly "abstracting out". It thankfully doesn't get any more abstract.

> Source/WebCore/css/StyleResolver.h:496
>      SelectorChecker m_checker;
> +    SelectorFilter m_filter;

m_filter does seems like overly generic name. So does m_checker, obviously, but there is no need to propagate bad naming.
Comment 11 Antti Koivisto 2012-12-21 16:29:01 PST
m_selectorFilter and m_selectorChecker would be way more informative.
Comment 12 Dimitri Glazkov (Google) 2012-12-23 19:58:21 PST
(In reply to comment #10)
> (From update of attachment 180567 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=180567&action=review
> 
> Looks like a good refactoring.
> 
> > Source/WebCore/ChangeLog:7
> > +        The awesome Bloom filter and parent stack logic don't need to be in SelectorChecker. They nicely abstract out
> > +        into their own pretty thing, named thereby SelectorFilter.
> 
> It is not exactly "abstracting out". It thankfully doesn't get any more abstract.

Changed to "factor out".

> 
> > Source/WebCore/css/StyleResolver.h:496
> >      SelectorChecker m_checker;
> > +    SelectorFilter m_filter;
> 
> m_filter does seems like overly generic name. So does m_checker, obviously, but there is no need to propagate bad naming.

Renamed to m_selectorFilter, will change m_checker in a separate patch.
Comment 13 Dimitri Glazkov (Google) 2012-12-23 19:59:50 PST
Created attachment 180636 [details]
Patch for landing
Comment 14 WebKit Review Bot 2012-12-23 20:39:06 PST
Comment on attachment 180636 [details]
Patch for landing

Rejecting attachment 180636 [details] from commit-queue.

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', u'--status-host=queues.webkit.org', ..." exit_code: 2 cwd: /mnt/git/webkit-commit-queue

Last 500 characters of output:
Kit/chromium/third_party/yasm/source/patched-yasm --revision 167605 --non-interactive --force --accept theirs-conflict --ignore-externals' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'
55>At revision 167605.

________ running '/usr/bin/python tools/clang/scripts/update.py --mac-only' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'

________ running '/usr/bin/python gyp_webkit' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'
Updating webkit projects from gyp files...

Full output: http://queues.webkit.org/results/15495309
Comment 15 Eric Seidel (no email) 2012-12-23 20:50:01 PST
I think I've made the error reporting less-awesome.

But this was the error:
Merge conflict during commit: Conflict at '/trunk/Source/WebCore/ChangeLog' at /usr/lib/git-core/git-svn line 570
Comment 16 WebKit Review Bot 2012-12-23 21:06:23 PST
Comment on attachment 180636 [details]
Patch for landing

Clearing flags on attachment: 180636

Committed r138432: <http://trac.webkit.org/changeset/138432>
Comment 17 WebKit Review Bot 2012-12-23 21:06:30 PST
All reviewed patches have been landed.  Closing bug.
Comment 18 Eric Seidel (no email) 2012-12-23 22:23:46 PST
I've filed bug 105705 about the less-than-stellar reporting from the EWS.