Bug 56894 - move :not over to using selectorList instead of simpleSelector
Summary: move :not over to using selectorList instead of simpleSelector
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: Ojan Vafai
URL:
Keywords:
Depends on: 56998
Blocks:
  Show dependency treegraph
 
Reported: 2011-03-22 20:40 PDT by Ojan Vafai
Modified: 2011-03-24 00:20 PDT (History)
5 users (show)

See Also:


Attachments
Patch (11.35 KB, patch)
2011-03-22 21:16 PDT, Ojan Vafai
koivisto: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ojan Vafai 2011-03-22 20:40:43 PDT
move :not over to using selectorList instead of simpleSelector
Comment 1 Ojan Vafai 2011-03-22 21:16:46 PDT
Created attachment 86567 [details]
Patch
Comment 2 Darin Adler 2011-03-23 00:23:11 PDT
Comment on attachment 86567 [details]
Patch

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

> Source/WebCore/css/CSSGrammar.y:1199
> +            // Use the selector vector instead of just storing the single selector to avoid
> +            // bloating memory in CSSSelector::RareData.

I am not sure I understand the meaning of this comment. There is no way to just store a single selector any more after this patch, so the comment makes sense only if you know the history of the code. Is that right?
Comment 3 Ojan Vafai 2011-03-23 00:28:57 PDT
(In reply to comment #2)
> (From update of attachment 86567 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=86567&action=review
> 
> > Source/WebCore/css/CSSGrammar.y:1199
> > +            // Use the selector vector instead of just storing the single selector to avoid
> > +            // bloating memory in CSSSelector::RareData.
> 
> I am not sure I understand the meaning of this comment. There is no way to just store a single selector any more after this patch, so the comment makes sense only if you know the history of the code. Is that right?

Right. It's a poorly phrased comment. I'll just delete it.
Comment 4 Antti Koivisto 2011-03-23 02:02:36 PDT
Comment on attachment 86567 [details]
Patch

+                // FIXME: should we check if the subSelectors are sibling selectors?

Doesn't the current grammar assume that the subselectors are simple so there is no need? It would be good to explain that in the comment.
Comment 5 Antti Koivisto 2011-03-23 02:03:09 PDT
...and perhaps assert() for that too.
Comment 6 Ojan Vafai 2011-03-23 19:28:27 PDT
(In reply to comment #4)
> (From update of attachment 86567 [details])
> +                // FIXME: should we check if the subSelectors are sibling selectors?
> 
> Doesn't the current grammar assume that the subselectors are simple so there is no need? It would be good to explain that in the comment.

(In reply to comment #5)
> ...and perhaps assert() for that too.

Duh. Good point. I'm trying to convince the CSS WG to change that, but in the meantime ASSERT added.
Comment 7 Ojan Vafai 2011-03-23 19:34:31 PDT
Committed r81845: <http://trac.webkit.org/changeset/81845>
Comment 8 WebKit Review Bot 2011-03-24 00:11:35 PDT
http://trac.webkit.org/changeset/81845 might have broken Leopard Intel Debug (Tests) and Windows XP Debug (Tests)
The following tests are not passing:
fast/selectors/077.html
fast/selectors/077b.html
fast/selectors/078b.html
inspector/audits/audits-panel-functional.html
inspector/console/command-line-api-inspect.html
inspector/console/command-line-api.html
inspector/console/console-api-on-call-frame.html
inspector/console/console-assert.html
inspector/cookie-parser.html
inspector/cookie-resource-match.html
inspector/cpu-profiler-profiling.html
inspector/evaluate-in-page.html
inspector/inspected-objects-not-overriden.html
inspector/report-API-errors.html
inspector/report-protocol-errors.html
inspector/storage-panel-dom-storage.html
inspector/syntax-highlight-css.html
inspector/syntax-highlight-html.html
inspector/syntax-highlight-javascript.html
inspector/utilities.html