[webkit-reviews] review granted: [Bug 202471] Implement String.prototype.replaceAll : [Attachment 383841] Patch

[bugzilla-daemon at webkit.org]

Yusuke Suzuki <ysuzuki at apple.com> has granted Ross Kirsling
<ross.kirsling at sony.com>'s request for review:
Bug 202471: Implement String.prototype.replaceAll
https://bugs.webkit.org/show_bug.cgi?id=202471

Attachment 383841: Patch

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




--- Comment #16 from Yusuke Suzuki <ysuzuki at apple.com> ---
Comment on attachment 383841
  --> https://bugs.webkit.org/attachment.cgi?id=383841
Patch

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

r=me with nits.

> Source/JavaScriptCore/runtime/StringPrototype.cpp:803
> +	   if (callType != CallType::None) {

`if (cachedCall)` would be nice since we are using cachedCall in this brace.
And this is the same to `callType != CallType::None`.

> Source/JavaScriptCore/runtime/StringPrototype.cpp:823
> +	   if (callType != CallType::None) {

`if (cachedCall)` would be clean.

> Source/JavaScriptCore/runtime/StringPrototype.cpp:842
> +    RELEASE_AND_RETURN(scope, jsSpliceSubstringsWithSeparators(globalObject,
jsString, string, sourceRanges.data(), sourceRanges.size(),
replacements.data(), replacements.size()));

Previously, we are calling `jsString()` with three substrings. But now,
jsSpliceSubstringsWithSeparators is allocating a new String buffer for this
case (`!isGlobal`) case.
Is it efficient in terms of memory? Should we do this in
jsSpliceSubstringsWithSeparators?