CSS Selector an-plus-b serialization is incorrect

[Keith Miller]

CSS Selector an-plus-b serialization is incorrect

[Keith Miller]

Created attachment 410063 [details]
Patch

[Sam Weinig]

Comment on attachment 410063 [details]
Patch

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

> Source/WebCore/css/parser/CSSSelectorParser.cpp:763
> +    if (!ab.first)
> +        return commonVM().numericStrings.add(ab.second);

Why is the JS VM involved here? That seems almost certainly wrong.

[Keith Miller]

Created attachment 410064 [details]
Patch

[Keith Miller]

Comment on attachment 410063 [details]
Patch

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

>> Source/WebCore/css/parser/CSSSelectorParser.cpp:763
>> +        return commonVM().numericStrings.add(ab.second);
> 
> Why is the JS VM involved here? That seems almost certainly wrong.

Would recommend reading the ChangeLog :P

[Keith Miller]

Created attachment 410077 [details]
Patch

[Keith Miller]

Created attachment 410134 [details]
Patch

[Sam Weinig]

(In reply to Keith Miller from comment #4)
> Comment on attachment 410063 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=410063&action=review
> 
> >> Source/WebCore/css/parser/CSSSelectorParser.cpp:763
> >> +        return commonVM().numericStrings.add(ab.second);
> > 
> > Why is the JS VM involved here? That seems almost certainly wrong.
> 
> Would recommend reading the ChangeLog :P

This doesn't seem like a good idea to me. In general, I don't think utilizing internal data structures of JavaScriptCore outside of the bindings (and the purpose driven JITs) is a good idea in WebCore code. If this optimization is really needed, we should probably find a way to make it work without involving JSC.

[Keith Miller]

Comment on attachment 410063 [details]
Patch

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

>>>> Source/WebCore/css/parser/CSSSelectorParser.cpp:763
>>>> +        return commonVM().numericStrings.add(ab.second);
>>> 
>>> Why is the JS VM involved here? That seems almost certainly wrong.
>> 
>> Would recommend reading the ChangeLog :P
> 
> This doesn't seem like a good idea to me. In general, I don't think utilizing internal data structures of JavaScriptCore outside of the bindings (and the purpose driven JITs) is a good idea in WebCore code. If this optimization is really needed, we should probably find a way to make it work without involving JSC.

I don't know if I would call this an internal data structure of JSC anymore than the AtomicString table is an internal JSC data structure. That said, I did it mostly because I figured it would be simple enough, so I'm happy to remove it.

[Keith Miller]

Created attachment 410158 [details]
Patch

[Keith Miller]

Created attachment 410159 [details]
Patch

[EWS]

Committed r267812: <https://trac.webkit.org/changeset/267812>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 410159 [details].

[Radar WebKit Bug Importer]

<rdar://problem/69815393>