Bug 56894 – move :not over to using selectorList instead of simpleSelector

• Component: New Bugs
• Status: RESOLVED FIXED

[Ojan Vafai]

move :not over to using selectorList instead of simpleSelector

[Ojan Vafai]

Created attachment 86567 [details]
Patch

[Darin Adler]

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?

[Ojan Vafai]

(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.

[Antti Koivisto]

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.

[Antti Koivisto]

...and perhaps assert() for that too.

[Ojan Vafai]

(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.

[Ojan Vafai]

Committed r81845: <http://trac.webkit.org/changeset/81845>

[WebKit Review Bot]

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