[webkit-reviews] review granted: [Bug 204479] Fix broken String.prototype.replace() and replaceAll(). : [Attachment 384104] proposed patch.

[bugzilla-daemon at webkit.org]

Ross Kirsling <ross.kirsling at sony.com> has granted Mark Lam
<mark.lam at apple.com>'s request for review:
Bug 204479: Fix broken String.prototype.replace() and replaceAll().
https://bugs.webkit.org/show_bug.cgi?id=204479

Attachment 384104: proposed patch.

https://bugs.webkit.org/attachment.cgi?id=384104&action=review




--- Comment #3 from Ross Kirsling <ross.kirsling at sony.com> ---
Comment on attachment 384104
  --> https://bugs.webkit.org/attachment.cgi?id=384104
proposed patch.

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

It's a bit interesting that these tests fail silently in release, but I guess
that's the sort of issue it is.

> Source/JavaScriptCore/ChangeLog:10
> +	   String.prototype.replace() regressed due to r252683:
<https://trac.webkit.org/r252683>
> +	   for webkit.org/b/202471.  The patch failed to handle
InternalFunctions.

Ugh, it seems that I underestimated the reason for the two huge branches in
replaceUsingRegExpSearch. Sorry about that. :(

> Source/JavaScriptCore/ChangeLog:15
> +	   This patch also fixed a spec compliance bug for
String.prototype.replace() i.e.
> +	   the replaceValue needs to be evaluated before we check if there's a
match in the
> +	   source string.
> +	   Ref: 21.1.3.14-9 at
https://www.ecma-international.org/ecma-262/6.0/#sec-string.prototype.replace

Oh, good catch! I guess there haven't been any tests for this.

BTW, this is now 21.1.3.17-6 in the living spec:
https://tc39.es/ecma262/#sec-string.prototype.replace