Move trivial String prototype functions to JS builtins

[Oliver Hunt]

Move trivial String prototype functions to JS builtins

[Oliver Hunt]

Created attachment 309182 [details]
Patch

[Oliver Hunt]

Still waiting on my build completing, but obviously it will work right?

[Oliver Hunt]

<rdar://problem/32005106>

[Geoffrey Garen]

Comment on attachment 309182 [details]
Patch

r=me

Shiny

[Yusuke Suzuki]

Comment on attachment 309182 [details]
Patch

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

a bit comments.

> Source/JavaScriptCore/builtins/StringPrototype.js:318
> +        let escapedV = V.replace('"', '"');

String.prototype.replace can be altered by user code. I suggest exposing @replace in StringPrototype and using it.

> Source/JavaScriptCore/builtins/StringPrototype.js:330
> +    return createHTML("String.prototype.link", this, "a", "name", url)

Need to use `@createHTML`.

> Source/JavaScriptCore/runtime/StringPrototype.cpp:90
>  EncodedJSValue JSC_HOST_CALL stringProtoFuncAnchor(ExecState*);

We can drop other function declarations.

[Oliver Hunt]

Created attachment 309187 [details]
Patch

[Oliver Hunt]

Just pushing some fixes because apparently blindly assuming my code would be correct before waiting for the compile to finish wasn't correct :D

Who would have thunk it?

No meaningful changes (forgot @, and accidentally dropped blink() :D )

[Oliver Hunt]

(In reply to Yusuke Suzuki from comment #5)
> Comment on attachment 309182 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=309182&action=review
> 
> a bit comments.
> 
> > Source/JavaScriptCore/builtins/StringPrototype.js:318
> > +        let escapedV = V.replace('"', '"');
> 
Zomg, i'm an idiot, will definitely do that :D

Such unsafe code :D

[Oliver Hunt]

Comment on attachment 309187 [details]
Patch

Yusuke's spotted a horrifying issue (String.replace can be dropped)

[Oliver Hunt]

Created attachment 309195 [details]
Patch

[Oliver Hunt]

Comment on attachment 309195 [details]
Patch

Correct the .replace issue yusuke saw tests passed locally

[Saam Barati]

Comment on attachment 309195 [details]
Patch

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

> Source/JavaScriptCore/builtins/StringPrototype.js:324
> +    return p4;

Maybe use a template literal for these last 3 concatenations?

[Saam Barati]

Comment on attachment 309195 [details]
Patch

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

> Source/JavaScriptCore/builtins/StringPrototype.js:319
> +        p1 = p1 + " " + @toString(attribute) + '="' + escapedV + '"'

Maybe same here?

[Oliver Hunt]

Comment on attachment 309195 [details]
Patch

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

>> Source/JavaScriptCore/builtins/StringPrototype.js:324
>> +    return p4;
> 
> Maybe use a template literal for these last 3 concatenations?

I pondered doing that (I initially had template literals) but i decided it would be better to just match the spec text.

[Build Bot]

Comment on attachment 309195 [details]
Patch

Attachment 309195 [details] did not pass jsc-ews (mac):
Output: http://webkit-queues.webkit.org/results/3680420

New failing tests:
ChakraCore.yaml/ChakraCore/test/Strings/StringTagFunctions.js.default
ChakraCore.yaml/ChakraCore/test/Strings/HTMLHelpers.js.default

[Oliver Hunt]

Created attachment 309200 [details]
Patch

[Build Bot]

Comment on attachment 309200 [details]
Patch

Attachment 309200 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/3680795

New failing tests:
js/dom/string-fontcolor.html
js/dom/string-anchor.html
js/dom/string-fontsize.html
js/dom/string-link.html

[Build Bot]

Created attachment 309203 [details]
Archive of layout-test-results from ews101 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 309200 [details]
Patch

Attachment 309200 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/3680801

New failing tests:
js/dom/string-fontcolor.html
js/dom/string-anchor.html
js/dom/string-fontsize.html
js/dom/string-link.html

[Build Bot]

Created attachment 309205 [details]
Archive of layout-test-results from ews106 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 309200 [details]
Patch

Attachment 309200 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/3680753

New failing tests:
js/dom/string-fontcolor.html
js/dom/string-anchor.html
js/dom/string-fontsize.html
js/dom/string-link.html

[Build Bot]

Created attachment 309208 [details]
Archive of layout-test-results from ews124 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews124  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 309200 [details]
Patch

Attachment 309200 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/3680812

New failing tests:
js/dom/string-fontcolor.html
js/dom/string-anchor.html
js/dom/string-fontsize.html
js/dom/string-link.html

[Build Bot]

Created attachment 309211 [details]
Archive of layout-test-results from ews116 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Oliver Hunt]

Created attachment 309212 [details]
Patch

[Oliver Hunt]

Getting build bots to give me expected outputs.

[Build Bot]

Comment on attachment 309212 [details]
Patch

Attachment 309212 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/3681157

New failing tests:
js/dom/string-fontcolor.html
js/dom/string-anchor.html
js/dom/string-fontsize.html
js/dom/string-link.html

[Build Bot]

Created attachment 309218 [details]
Archive of layout-test-results from ews101 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 309212 [details]
Patch

Attachment 309212 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/3681163

New failing tests:
js/dom/string-fontcolor.html
js/dom/string-anchor.html
js/dom/string-fontsize.html
js/dom/string-link.html

[Build Bot]

Created attachment 309219 [details]
Archive of layout-test-results from ews105 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Oliver Hunt]

Created attachment 309223 [details]
Patch

[Oliver Hunt]

Comment on attachment 309223 [details]
Patch

Finally should pass everything

[Saam Barati]

Comment on attachment 309223 [details]
Patch

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

> Source/JavaScriptCore/builtins/StringPrototype.js:312
> +    if (string == null)

Do we care about document.all here counting as undefined?

[Oliver Hunt]

Comment on attachment 309223 [details]
Patch

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

>> Source/JavaScriptCore/builtins/StringPrototype.js:312
>> +    if (string == null)
> 
> Do we care about document.all here counting as undefined?

I believe not -- this matches the impl of all the other string functions

[WebKit Commit Bot]

Comment on attachment 309223 [details]
Patch

Clearing flags on attachment: 309223

Committed r216301: <http://trac.webkit.org/changeset/216301>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Myles C. Maxfield]

Reopening to attach new patch.

[Joseph Pecoraro]

Comment on attachment 309223 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:11
> +        Core implementation is basically a 1-for-1 match to the spec.

Indeed it does! Would be nice to link to it:
https://tc39.github.io/ecma262/#sec-createhtml

> Source/JavaScriptCore/builtins/StringPrototype.js:319
> +        p1 = p1 + " " + @toString(attribute) + '="' + escapedV + '"'

Nit: Semicolon

> Source/JavaScriptCore/builtins/StringPrototype.js:321
> +    let p2 = p1 + ">"

Nit: Semicolon

> Source/JavaScriptCore/builtins/StringPrototype.js:330
> +    return @createHTML("String.prototype.link", this, "a", "name", url)

Nit: Semicolon

[Oliver Hunt]

(In reply to Myles C. Maxfield from comment #37)
> Reopening to attach new patch.

Wait did this get rolled out?

[David Kilzer (:ddkilzer)]

(In reply to Oliver Hunt from comment #39)
> (In reply to Myles C. Maxfield from comment #37)
> > Reopening to attach new patch.
> 
> Wait did this get rolled out?

No, it didn't get rolled out.  Myles re-opened the wrong bug.

Moving back to RESOLVED/FIXED.

[Mark Lam]

FYI, this patch caused a regression: https://bugs.webkit.org/show_bug.cgi?id=171786