Bug 171737

Summary:

Move trivial String prototype functions to JS builtins

Product:

WebKit

Reporter:

Oliver Hunt <oliver>

Component:

New Bugs

Assignee:

Oliver Hunt <oliver>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

buildbot, commit-queue, ddkilzer, fpizlo, ggaren, joepeck, keith_miller, mark.lam, mmaxfield, msaboff, oliver, rniwa, saam, ysuzuki

Priority:

Keywords:

InRadar

Version:

WebKit Nightly Build

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

171786

Bug Blocks:

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	none
Patch	none
Archive of layout-test-results from ews101 for mac-elcapitan	none
Archive of layout-test-results from ews106 for mac-elcapitan-wk2	none
Archive of layout-test-results from ews124 for ios-simulator-wk2	none
Archive of layout-test-results from ews116 for mac-elcapitan	none
Patch	none
Archive of layout-test-results from ews101 for mac-elcapitan	none
Archive of layout-test-results from ews105 for mac-elcapitan-wk2	none
Patch	none

Oliver Hunt

Reported 2017-05-05 10:48:09 PDT

Move trivial String prototype functions to JS builtins

Attachments
Patch (16.79 KB, patch) 2017-05-05 10:51 PDT, Oliver Hunt	no flags	Details Formatted Diff Diff
Patch (16.85 KB, patch) 2017-05-05 11:07 PDT, Oliver Hunt	no flags	Details Formatted Diff Diff
Patch (17.65 KB, patch) 2017-05-05 12:04 PDT, Oliver Hunt	no flags	Details Formatted Diff Diff
Patch (17.64 KB, patch) 2017-05-05 13:01 PDT, Oliver Hunt	no flags	Details Formatted Diff Diff
Archive of layout-test-results from ews101 for mac-elcapitan (975.98 KB, application/zip) 2017-05-05 14:08 PDT, Build Bot	no flags	Details
Archive of layout-test-results from ews106 for mac-elcapitan-wk2 (914.47 KB, application/zip) 2017-05-05 14:13 PDT, Build Bot	no flags	Details
Archive of layout-test-results from ews124 for ios-simulator-wk2 (20.27 MB, application/zip) 2017-05-05 14:19 PDT, Build Bot	no flags	Details
Archive of layout-test-results from ews116 for mac-elcapitan (1.75 MB, application/zip) 2017-05-05 14:25 PDT, Build Bot	no flags	Details
Patch (21.43 KB, patch) 2017-05-05 14:30 PDT, Oliver Hunt	no flags	Details Formatted Diff Diff
Archive of layout-test-results from ews101 for mac-elcapitan (1.17 MB, application/zip) 2017-05-05 15:12 PDT, Build Bot	no flags	Details
Archive of layout-test-results from ews105 for mac-elcapitan-wk2 (1.30 MB, application/zip) 2017-05-05 15:17 PDT, Build Bot	no flags	Details
Patch (26.71 KB, patch) 2017-05-05 15:27 PDT, Oliver Hunt	no flags	Details Formatted Diff Diff
Show Obsolete (5) View All Add attachment proposed patch, testcase, etc.

Oliver Hunt

Comment 1 2017-05-05 10:51:25 PDT

Created attachment 309182 [details] Patch

Oliver Hunt

Comment 2 2017-05-05 10:52:46 PDT

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

Oliver Hunt

Comment 3 2017-05-05 10:54:50 PDT

<rdar://problem/32005106>

Geoffrey Garen

Comment 4 2017-05-05 10:55:39 PDT

Comment on attachment 309182 [details] Patch r=me Shiny

Yusuke Suzuki

Comment 5 2017-05-05 11:00:00 PDT

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

Comment 6 2017-05-05 11:07:11 PDT

Created attachment 309187 [details] Patch

Oliver Hunt

Comment 7 2017-05-05 11:11:06 PDT

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

Comment 8 2017-05-05 11:11:35 PDT

(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 9 2017-05-05 11:12:06 PDT

Comment on attachment 309187 [details] Patch Yusuke's spotted a horrifying issue (String.replace can be dropped)

Oliver Hunt

Comment 10 2017-05-05 12:04:26 PDT

Created attachment 309195 [details] Patch

Oliver Hunt

Comment 11 2017-05-05 12:07:04 PDT

Comment on attachment 309195 [details] Patch Correct the .replace issue yusuke saw tests passed locally

Saam Barati

Comment 12 2017-05-05 12:37:06 PDT

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 13 2017-05-05 12:37:50 PDT

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 14 2017-05-05 12:38:41 PDT

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 15 2017-05-05 12:46:34 PDT

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

Comment 16 2017-05-05 13:01:28 PDT

Created attachment 309200 [details] Patch

Build Bot

Comment 17 2017-05-05 14:08:05 PDT

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

Comment 18 2017-05-05 14:08:07 PDT

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 19 2017-05-05 14:13:05 PDT

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

Comment 20 2017-05-05 14:13:07 PDT

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 21 2017-05-05 14:19:05 PDT

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

Comment 22 2017-05-05 14:19:07 PDT

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 23 2017-05-05 14:25:09 PDT

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

Comment 24 2017-05-05 14:25:11 PDT

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

Comment 25 2017-05-05 14:30:27 PDT

Created attachment 309212 [details] Patch

Oliver Hunt

Comment 26 2017-05-05 14:31:09 PDT

Getting build bots to give me expected outputs.

Build Bot

Comment 27 2017-05-05 15:12:33 PDT

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

Comment 28 2017-05-05 15:12:35 PDT

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 29 2017-05-05 15:17:04 PDT

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

Comment 30 2017-05-05 15:17:06 PDT

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

Comment 31 2017-05-05 15:27:20 PDT

Created attachment 309223 [details] Patch

Oliver Hunt

Comment 32 2017-05-05 15:27:53 PDT

Comment on attachment 309223 [details] Patch Finally should pass everything

Saam Barati

Comment 33 2017-05-05 16:46:50 PDT

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 34 2017-05-05 16:49:13 PDT

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 35 2017-05-05 17:33:23 PDT

Comment on attachment 309223 [details] Patch Clearing flags on attachment: 309223 Committed r216301: <http://trac.webkit.org/changeset/216301>

WebKit Commit Bot

Comment 36 2017-05-05 17:33:25 PDT

All reviewed patches have been landed. Closing bug.

Myles C. Maxfield

Comment 37 2017-05-05 18:16:16 PDT

Reopening to attach new patch.

Joseph Pecoraro

Comment 38 2017-05-05 18:52:49 PDT

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

Comment 39 2017-05-05 21:14:50 PDT

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

David Kilzer (:ddkilzer)

Comment 40 2017-05-07 14:46:27 PDT

(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

Comment 41 2017-05-08 09:21:21 PDT

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

Note You need to log in before you can comment on or make changes to this bug.