76861 – JSValue::toString() should return a JSString* instead of a UString

RESOLVED FIXED 76861

JSValue::toString() should return a JSString* instead of a UString

https://bugs.webkit.org/show_bug.cgi?id=76861

Summary JSValue::toString() should return a JSString* instead of a UString

Geoffrey Garen

Reported 2012-01-23 14:13:48 PST

JSValue::toString() should return a JSString* instead of a UString

Attachments
Patch (138.65 KB, patch) 2012-01-23 14:38 PST, Geoffrey Garen	no flags	Details Formatted Diff Diff
Patch (145.78 KB, patch) 2012-01-23 15:25 PST, Geoffrey Garen	no flags	Details Formatted Diff Diff
Patch (145.84 KB, patch) 2012-01-23 15:56 PST, Geoffrey Garen	no flags	Details Formatted Diff Diff
Patch (146.47 KB, patch) 2012-01-23 16:22 PST, Geoffrey Garen	no flags	Details Formatted Diff Diff
Patch (146.51 KB, patch) 2012-01-23 16:51 PST, Geoffrey Garen	no flags	Details Formatted Diff Diff
Patch (146.77 KB, patch) 2012-01-23 17:23 PST, Geoffrey Garen	no flags	Details Formatted Diff Diff
Patch (147.54 KB, patch) 2012-01-23 21:26 PST, Geoffrey Garen	no flags	Details Formatted Diff Diff
Patch (3.84 KB, patch) 2012-01-24 13:06 PST, Geoffrey Garen	barraclough: review+ webkit.review.bot: commit-queue-	Details Formatted Diff Diff
Show Obsolete (7) View All Add attachment proposed patch, testcase, etc.

Geoffrey Garen

Comment 1 2012-01-23 14:38:10 PST

Created attachment 123624 [details] Patch

Geoffrey Garen

Comment 2 2012-01-23 14:39:46 PST

Seems performance-neutral: Benchmark report for SunSpider, V8, and Kraken on garen (MacPro5,1). VMs tested: "BASELINE" at /Volumes/Big/ggaren/baseline-webkit/WebKitBuild/Release/jsc (r104476) "PATCH" at /Volumes/Big/ggaren/webkit/WebKitBuild/Release/jsc (r105540) Collected 24 samples per benchmark/VM, with 4 VM invocations per benchmark. Emitted a call to gc() between sample measurements. Used 1 benchmark iteration per VM invocation for warm-up. Used the jsc-specific preciseTime() function to get microsecond-level timing. Reporting benchmark execution times with 95% confidence intervals in milliseconds. BASELINE PATCH SunSpider: 3d-cube 6.6110+-0.0354 ? 6.6801+-0.0353 ? might be 1.0104x slower 3d-morph 10.3602+-0.0463 10.3560+-0.0460 3d-raytrace 8.7381+-0.0234 ! 8.8391+-0.0526 ! definitely 1.0116x slower access-binary-trees 1.8818+-0.0097 ? 1.8820+-0.0115 ? access-fannkuch 9.0918+-0.0154 ! 9.4960+-0.0081 ! definitely 1.0445x slower access-nbody 4.6114+-0.0194 4.5918+-0.0121 access-nsieve 4.0541+-0.0623 3.9881+-0.0236 might be 1.0165x faster bitops-3bit-bits-in-byte 1.4689+-0.0064 1.4661+-0.0016 bitops-bits-in-byte 6.0210+-0.0231 ? 6.0327+-0.0146 ? bitops-bitwise-and 3.8085+-0.0220 3.7922+-0.0066 bitops-nsieve-bits 6.5441+-0.0275 ? 6.5810+-0.0313 ? controlflow-recursive 2.7080+-0.0083 2.7039+-0.0075 crypto-aes 8.3035+-0.0603 8.2565+-0.0427 crypto-md5 2.9664+-0.0225 2.9416+-0.0217 crypto-sha1 2.6637+-0.0277 2.6326+-0.0139 might be 1.0118x faster date-format-tofte 11.8221+-0.0504 11.7626+-0.0614 date-format-xparb 11.0695+-0.1334 ! 11.4676+-0.0735 ! definitely 1.0360x slower math-cordic 8.3628+-0.0210 ? 8.4903+-0.1125 ? might be 1.0153x slower math-partial-sums 12.0228+-0.0349 12.0026+-0.0230 math-spectral-norm 3.0567+-0.0046 3.0542+-0.0032 regexp-dna 10.0438+-0.0567 10.0245+-0.0550 string-base64 5.0494+-0.0270 ! 5.1369+-0.0312 ! definitely 1.0173x slower string-fasta 8.3458+-0.0318 ^ 8.1463+-0.0230 ^ definitely 1.0245x faster string-tagcloud 14.6002+-0.0521 ^ 14.4003+-0.0540 ^ definitely 1.0139x faster string-unpack-code 23.3418+-0.0767 ? 23.4400+-0.0713 ? string-validate-input 6.7119+-0.0397 6.6901+-0.0234 <arithmetic> * 7.4715+-0.0170 ? 7.4944+-0.0155 ? might be 1.0031x slower <geometric> 6.1247+-0.0122 ? 6.1343+-0.0118 ? might be 1.0016x slower <harmonic> 4.8916+-0.0094 4.8883+-0.0098 might be 1.0007x faster BASELINE PATCH V8: crypto 89.5955+-0.1737 89.5781+-0.1318 deltablue 194.0910+-1.7153 192.3804+-1.3365 earley-boyer 108.7625+-1.5667 108.4443+-1.6310 raytrace 60.7802+-0.2804 60.5074+-0.2504 regexp 111.5169+-0.2266 ? 111.6835+-0.2607 ? richards 156.0449+-0.1767 ? 156.2165+-0.1462 ? splay 85.0744+-1.2860 83.9436+-0.2382 might be 1.0135x faster <arithmetic> 115.1236+-0.5042 114.6791+-0.3207 might be 1.0039x faster <geometric> * 107.8749+-0.4574 107.4618+-0.2753 might be 1.0038x faster <harmonic> 101.2839+-0.4243 100.8785+-0.2366 might be 1.0040x faster BASELINE PATCH Kraken: ai-astar 947.2241+-0.8833 946.5622+-0.8812 audio-beat-detection 222.4434+-0.4525 ? 222.5369+-0.4888 ? audio-dft 326.8621+-3.3635 324.7562+-0.3348 audio-fft 138.5094+-0.0789 138.5020+-0.1028 audio-oscillator 352.7950+-2.6635 350.4292+-1.9507 imaging-darkroom 338.9531+-4.8759 337.4513+-5.7336 imaging-desaturate 265.1648+-0.0966 ? 265.6183+-0.3809 ? imaging-gaussian-blur 604.0797+-3.2211 601.5251+-2.0723 json-parse-financial 78.4658+-0.1491 ^ 78.0021+-0.1352 ^ definitely 1.0059x faster json-stringify-tinderbox 93.6828+-0.3702 ^ 92.8008+-0.2356 ^ definitely 1.0095x faster stanford-crypto-aes 120.4857+-0.2230 ! 121.7085+-0.3545 ! definitely 1.0101x slower stanford-crypto-ccm 119.6886+-1.0952 118.8119+-0.8683 stanford-crypto-pbkdf2 228.5304+-0.8767 ^ 226.2212+-0.4595 ^ definitely 1.0102x faster stanford-crypto-sha256-iterative 103.8256+-0.0686 ! 104.7056+-0.2849 ! definitely 1.0085x slower <arithmetic> * 281.4793+-0.5644 280.6880+-0.4083 might be 1.0028x faster <geometric> 215.2715+-0.3574 214.7396+-0.2655 might be 1.0025x faster <harmonic> 172.5559+-0.2388 172.1969+-0.1845 might be 1.0021x faster BASELINE PATCH All benchmarks: <arithmetic> 105.1242+-0.2135 104.8349+-0.1529 might be 1.0028x faster <geometric> 27.1091+-0.0520 27.0970+-0.0454 might be 1.0004x faster <harmonic> 8.5994+-0.0164 8.5931+-0.0170 might be 1.0007x faster BASELINE PATCH Geomean of preferred means: <scaled-result> 60.9894+-0.1397 60.9169+-0.1087 might be 1.0012x faster

Gavin Barraclough

Comment 3 2012-01-23 14:48:44 PST

Comment on attachment 123624 [details] Patch ->value(exec)->value(exec)->value(exec)->value(exec)->value(exec)->value(exec)->value(exec)->value(exec)->value(exec)->value(exec)->value(exec)->value(exec)->value(exec)->value(exec)->value(exec)->value(exec)->value(exec)->value(exec)->value(exec)->value(exec)->value(exec)->value(exec)->value(exec).

Early Warning System Bot

Comment 4 2012-01-23 14:59:49 PST

Comment on attachment 123624 [details] Patch Attachment 123624 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/11117534

Geoffrey Garen

Comment 5 2012-01-23 15:25:18 PST

Created attachment 123634 [details] Patch

Geoffrey Garen

Comment 6 2012-01-23 15:56:04 PST

Created attachment 123644 [details] Patch

Geoffrey Garen

Comment 7 2012-01-23 16:22:40 PST

Created attachment 123649 [details] Patch

Geoffrey Garen

Comment 8 2012-01-23 16:51:46 PST

Created attachment 123662 [details] Patch

Geoffrey Garen

Comment 9 2012-01-23 17:23:51 PST

Created attachment 123671 [details] Patch

Gyuyoung Kim

Comment 10 2012-01-23 18:38:37 PST

Comment on attachment 123671 [details] Patch Attachment 123671 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/11117611

Geoffrey Garen

Comment 11 2012-01-23 21:26:29 PST

Created attachment 123695 [details] Patch

Geoffrey Garen

Comment 12 2012-01-23 23:34:20 PST

Committed r105698: <http://trac.webkit.org/changeset/105698>

Csaba Osztrogonác

Comment 13 2012-01-24 00:06:54 PST

Reopen, because it broke 2 tests at least on Qt: --- /ramdisk/qt-linux-64-release-quicktest/build/layout-test-results/fast/js/array-functions-non-arrays-expected.txt +++ /ramdisk/qt-linux-64-release-quicktest/build/layout-test-results/fast/js/array-functions-non-arrays-actual.txt @@ -57,9 +57,9 @@ PASS Array.prototype.slice.call(new TwoItemConstructor, 0, 1) is ['b'] PASS properties(Array.prototype.sort.call({})) is '' PASS properties(Array.prototype.sort.call(['b', 'a'])) is '0:a, 1:b, length:2(DontDelete, DontEnum)' -PASS properties(Array.prototype.sort.call({ length:2, 0:'b', 1:'a' })) is '0:a, 1:b, length:2' +FAIL properties(Array.prototype.sort.call({ length:2, 0:'b', 1:'a' })) should be 0:a, 1:b, length:2. Was 0:b, 1:a, length:2. PASS properties(Array.prototype.sort.call(new OneItemConstructor)) is '0:a(FromPrototype), length:1(FromPrototype)' -PASS properties(Array.prototype.sort.call(new TwoItemConstructor)) is '0:a, 1:b, length:2(FromPrototype)' +FAIL properties(Array.prototype.sort.call(new TwoItemConstructor)) should be 0:a, 1:b, length:2(FromPrototype). Was 0:b(FromPrototype), 1:a(FromPrototype), length:2(FromPrototype). PASS Array.prototype.splice.call({}, 0, 1) is [] PASS Array.prototype.splice.call(['b', 'a'], 0, 1) is ['b'] PASS Array.prototype.splice.call({ length:2, 0:'b', 1:'a' }, 0, 1) is ['b'] --- /ramdisk/qt-linux-64-release-quicktest/build/layout-test-results/sputnik/Conformance/15_Native_Objects/15.4_Array/15.4.4/15.4.4.11_Array_prototype_sort/S15.4.4.11_A3_T1-expected.txt +++ /ramdisk/qt-linux-64-release-quicktest/build/layout-test-results/sputnik/Conformance/15_Native_Objects/15.4_Array/15.4.4/15.4.4.11_Array_prototype_sort/S15.4.4.11_A3_T1-actual.txt @@ -1,6 +1,6 @@ S15.4.4.11_A3_T1 -PASS +FAIL SputnikError: #1: Check ToString operator TEST COMPLETE

Csaba Osztrogonác

Comment 14 2012-01-24 00:31:56 PST

These tests fail on SL too. And this patch broke GTK build too: ../../Source/WebCore/bindings/js/JSPeerConnectionCustom.cpp: In static member function ‘static void* WebCore::JSPeerConnectionConstructor::constructJSPeerConnection(JSC::ExecState*)’: ../../Source/WebCore/bindings/js/JSPeerConnectionCustom.cpp:51: error: invalid initialization of reference of type ‘const JSC::UString&’ from expression of type ‘JSC::JSString*’ ../../Source/WebCore/bindings/js/JSDOMBinding.h:322: error: in passing argument 1 of ‘WTF::String WebCore::ustringToString(const JSC::UString&)’ And an interpreter buildfix landed in http://trac.webkit.org/changeset/105702

Ilya Tikhonovsky

Comment 15 2012-01-24 02:50:42 PST

build fixes for Qt and Gtk: http://trac.webkit.org/changeset/105713 http://trac.webkit.org/changeset/105703 http://trac.webkit.org/changeset/105702 please check that changes are fine.

Csaba Osztrogonác

Comment 16 2012-01-24 02:53:18 PST

I skipped these tests on Qt: http://trac.webkit.org/changeset/105715 Please unskip them with the proper fix.

Geoffrey Garen

Comment 17 2012-01-24 11:01:34 PST

> build fixes for Qt and Gtk: These are fine. Thanks!

Geoffrey Garen

Comment 18 2012-01-24 13:06:09 PST

Created attachment 123789 [details] Patch

WebKit Review Bot

Comment 19 2012-01-24 13:43:01 PST

Comment on attachment 123789 [details] Patch Attachment 123789 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11343056 New failing tests: media/audio-garbage-collect.html

Geoffrey Garen

Comment 20 2012-01-24 14:31:47 PST

Committed r105811: <http://trac.webkit.org/changeset/105811>

Geoffrey Garen

Comment 21 2012-01-24 14:37:34 PST

Bindings test expectations fixed: Committed r105813: <http://trac.webkit.org/changeset/105813>

Csaba Osztrogonác

Comment 22 2012-01-25 00:05:26 PST

I unskipped the now passing tests - http://trac.webkit.org/changeset/105853

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

Status RESOLVED

Resolution FIXED

Priority P2

Severity Normal

Classification Unclassified

Version 528+ (Nightly build)

Hardware Unspecified

OS Unspecified

Product WebKit

Component New Bugs

Assignee

Geoffrey Garen

Reported

2012-01-23 14:13 PST

Modified

2012-01-25 00:05 PST History

CC List

8 users Show

URL

Keywords

Depends on

Blocks