[webkit-reviews] review denied: [Bug 101257] [WTF] Add replace() method to WTFString that takes replacement string as literal : [Attachment 172427] Patch

[bugzilla-daemon at webkit.org]

Benjamin Poulain <benjamin at webkit.org> has denied Christophe Dumez
<christophe.dumez at intel.com>'s request for review:
Bug 101257: [WTF] Add replace() method to WTFString that takes replacement
string as literal
https://bugs.webkit.org/show_bug.cgi?id=101257

Attachment 172427: Patch
https://bugs.webkit.org/attachment.cgi?id=172427&action=review

------- Additional Comments from Benjamin Poulain <benjamin at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=172427&action=review


r- for the early return on (!repStrLength). Totally my fault, sorry about that.


Otherwise I think the patch is a great addition. A quick grep tells me it could
already be used in a few places in WebKit.
Feel free to change that in the same patch.

> Source/WTF/wtf/text/StringImpl.cpp:1473
> +    if (!repStrLength)
> +	   return this;

Thinking about this a bit more, I was wrong before.

If repStrLength is 0, the function should just remove any occurrence of pattern
in the source.
Now what I do not understand is why the function returns early if StringImpl is
null in the other implementation (although that is unrelated to your patch).

I'd say this is a call for some tests. Please add those cases in the API tests.


> Source/WTF/wtf/text/StringImpl.h:714
> +    WTF_EXPORT_STRING_API PassRefPtr<StringImpl> replace(UChar, const
UChar*, unsigned replacementLength);

I am not sure we need this, especially if it is exported.

Do you know of cases where it would be useful to pass a UChar*?