Bug 3294 - String.prototype.replace() fails with function as second param
Summary: String.prototype.replace() fails with function as second param
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 412
Hardware: Mac OS X 10.4
: P2 Normal
Assignee: Darin Adler
Depends on:
Reported: 2005-06-07 05:45 PDT by Gavin Kistner
Modified: 2006-01-04 05:50 PST (History)
1 user (show)

See Also:

5 replace() tests, including one for this bug. (4.27 KB, text/html)
2005-06-07 05:46 PDT, Gavin Kistner
no flags Details
Handle the second argument being a function (3.83 KB, patch)
2005-06-20 02:27 PDT, Anders Carlsson
darin: review-
Details | Formatted Diff | Diff
Address comments (3.76 KB, patch)
2005-06-20 10:09 PDT, Anders Carlsson
no flags Details | Formatted Diff | Diff
Remove the space after substr (3.75 KB, patch)
2005-06-20 10:14 PDT, Anders Carlsson
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Gavin Kistner 2005-06-07 05:45:17 PDT
ECMAScript-262 rev3 section (page 102) allows the second parameter to the replace() method 
of String objects to be a function. However, JavaScriptCore does not support this feature (at least in the 
build of Safari 2.0.412), instead calling .toString() on the second parameter supplied to the replace() 
method, and using that as the return string.

The attached file "ReplaceTest.html" (also viewable at http://phrogz.net/JS/replaceTest.html) performs five 
tests of the replace method.

The severity is normal because a workaround exists - similar functionality can be achieved (more 
painfully) by using a regexp with the 'g' flag and repeatedly calling .exec, working through the original 
string and building the output.
Comment 1 Gavin Kistner 2005-06-07 05:46:18 PDT
Created attachment 2127 [details]
5 replace() tests, including one for this bug.
Comment 2 Gavin Kistner 2005-06-07 05:48:23 PDT
(This bug was copied from Apple's Bug Reporter #3749350)
Comment 3 Anders Carlsson 2005-06-20 02:27:33 PDT
Created attachment 2489 [details]
Handle the second argument being a function

Here's a patch that makes the test cases work for me.
Comment 4 Darin Adler 2005-06-20 08:49:01 PDT
Comment on attachment 2489 [details]
Handle the second argument being a function

I think the args for the replacement function should be declared inside each if
statement. There's no reason to keep the args list around between invocations.

And if you name it args, then the code is going to be a bit smaller too.

Does this do the right thing when the replacement function raises a JavaScript

Extra space after substr before "(".

Minor issues; this looks pretty good.
Comment 5 Anders Carlsson 2005-06-20 10:09:29 PDT
Created attachment 2504 [details]
Address comments

Here's a new patch that addresses the issues.

Regarding exceptions, the following code has been tested in kjs and found to be

	function test(s) {
		throw ReferenceError;
	try {
		result = testStr.replace("hello", test);
	} catch (e) {

and gives the result
--> (Internal Function)

If this isn't the correct behavior, then I suspect the same error applies for
compareWithCompareFunctionForQSort in array_object.cpp
Comment 6 Anders Carlsson 2005-06-20 10:14:21 PDT
Created attachment 2505 [details]
Remove the space after substr

Sorry, forgot the space after substr
Comment 7 Darin Adler 2005-06-20 11:20:09 PDT
Comment on attachment 2505 [details]
Remove the space after substr

r=me, given that we can land the attached test as a layout test.
Comment 8 Joost de Valk (AlthA) 2005-07-13 23:05:08 PDT
Fixed in 10.4.2
Comment 9 John Sullivan 2005-07-18 08:54:01 PDT
This appears fixed in tip of tree CVS, but does not appear fixed in 10.4.2 (contradicting the comment from 
Joost de Valk).
Comment 10 Darin Adler 2005-10-09 16:05:17 PDT
In Radar as <rdar://problem/4290603> String.replace() does not support a function as a parameter