Bug 3294

Summary: String.prototype.replace() fails with function as second param
Product: WebKit Reporter: Gavin Kistner <gavin@refinery.com>
Component: JavaScriptCoreAssignee: Darin Adler <darin@apple.com>
Status: VERIFIED FIXED    
Severity: Normal CC: jhurshman@gmail.com
Priority: P2    
Version: 412   
Hardware: Macintosh   
OS: Mac OS X 10.4   
Attachments:
Description Flags
5 replace() tests, including one for this bug.
none
Handle the second argument being a function
darin: review-
Address comments
none
Remove the space after substr darin: review+

Description From 2005-06-07 05:45:17 PST
ECMAScript-262 rev3 section 15.5.4.11 (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 From 2005-06-07 05:46:18 PST -------
Created an attachment (id=2127) [details]
5 replace() tests, including one for this bug.
------- Comment #2 From 2005-06-07 05:48:23 PST -------
(This bug was copied from Apple's Bug Reporter #3749350)
------- Comment #3 From 2005-06-20 02:27:33 PST -------
Created an attachment (id=2489) [details]
Handle the second argument being a function

Here's a patch that makes the test cases work for me.
------- Comment #4 From 2005-06-20 08:49:01 PST -------
(From update of attachment 2489 [details])
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
exception?

Extra space after substr before "(".

Minor issues; this looks pretty good.
------- Comment #5 From 2005-06-20 10:09:29 PST -------
Created an attachment (id=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
working:

	function test(s) {
		throw ReferenceError;
	}
	
	try {
		testStr="hello";
		result = testStr.replace("hello", test);
	} catch (e) {
		debug(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 From 2005-06-20 10:14:21 PST -------
Created an attachment (id=2505) [details]
Remove the space after substr

Sorry, forgot the space after substr
------- Comment #7 From 2005-06-20 11:20:09 PST -------
(From update of attachment 2505 [details])
r=me, given that we can land the attached test as a layout test.
------- Comment #8 From 2005-07-13 23:05:08 PST -------
Fixed in 10.4.2
------- Comment #9 From 2005-07-18 08:54:01 PST -------
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 From 2005-10-09 16:05:17 PST -------
In Radar as <rdar://problem/4290603> String.replace() does not support a function as a parameter 
(3294).