[webkit-reviews] review denied: [Bug 131704] Simple ES6 feature:String prototype additions : [Attachment 236762] Patch

[bugzilla-daemon at webkit.org]

Geoffrey Garen <ggaren at apple.com> has denied Diego Pino <dpino at igalia.com>'s
request for review:
Bug 131704: Simple ES6 feature:String prototype additions
https://bugs.webkit.org/show_bug.cgi?id=131704

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

------- Additional Comments from Geoffrey Garen <ggaren at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=236762&action=review


Looks pretty good, but I think I see some bugs.

> Source/JavaScriptCore/runtime/StringPrototype.cpp:1561
> +    if (a0.isObject() && a0.inherits(RegExpObject::info()))

jsDynamicCast<RegExpObject> is the best way to do this.

> Source/JavaScriptCore/runtime/StringPrototype.cpp:1564
> +    String str = exec->thisValue().toString(exec)->value(exec);

You need to do some kind of type check on exec->thisValue(), since it can be
anything, including an environment record (JSActivation), which you should not
use directly. Perhaps the specification calls for something like
"checkObjectCoercible"?

You should add a test for the case where this function is called with an
environment record as 'this'. You can do so by doing something like
"(function() { var f = String.prototype.startsWith; (function() {
startsWith("a"); })(); })()".

> Source/JavaScriptCore/runtime/StringPrototype.cpp:1582
> +	   if (str.characterAt(i) != searchStr.characterAt(j))

You should use bracket access instead, as the WTFString.h header indicates:

    // Workaround for a compiler bug. Use operator[] instead.
    UChar characterAt(unsigned index) const

> Source/JavaScriptCore/runtime/StringPrototype.cpp:1588
> +EncodedJSValue JSC_HOST_CALL stringProtoFuncEndsWith(ExecState* exec)

Same comments as above.

> Source/JavaScriptCore/runtime/StringPrototype.cpp:1623
> +EncodedJSValue JSC_HOST_CALL stringProtoFuncContains(ExecState* exec)

Ditto.