Bug 56894

Summary:

move :not over to using selectorList instead of simpleSelector

Product:

WebKit

Reporter:

Ojan Vafai <ojan>

Component:

New Bugs

Assignee:

Ojan Vafai <ojan>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

abarth, darin, eric, koivisto, webkit.review.bot

Priority:

Version:

528+ (Nightly build)

Hardware:

Other

OS:

OS X 10.5

Bug Depends on:

56998

Bug Blocks:

Attachments:

Description	Flags
Patch	koivisto: review+

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