RESOLVED FIXED56894
move :not over to using selectorList instead of simpleSelector 
https://bugs.webkit.org/show_bug.cgi?id=56894
Summary move :not over to using selectorList instead of simpleSelector
Ojan Vafai
Reported 2011-03-22 20:40:43 PDT
move :not over to using selectorList instead of simpleSelector
Attachments
Patch (11.35 KB, patch)
2011-03-22 21:16 PDT, Ojan Vafai 		koivisto: review+
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
Ojan Vafai
Comment 1 2011-03-22 21:16:46 PDT
Created attachment 86567 [details] Patch
Darin Adler
Comment 2 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?
Ojan Vafai
Comment 3 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.
Antti Koivisto
Comment 4 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.
Antti Koivisto
Comment 5 2011-03-23 02:03:09 PDT
...and perhaps assert() for that too.
Ojan Vafai
Comment 6 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.
Ojan Vafai
Comment 7 2011-03-23 19:34:31 PDT
Committed r81845: <http://trac.webkit.org/changeset/81845>
WebKit Review Bot
Comment 8 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
Note You need to log in before you can comment on or make changes to this bug.