[webkit-reviews] review denied: [Bug 84781] objectProtoFuncToString creates new string every invocation : [Attachment 138653] Patch

[bugzilla-daemon at webkit.org]

Geoffrey Garen <ggaren at apple.com> has denied Michael Saboff
<msaboff at apple.com>'s request for review:
Bug 84781: objectProtoFuncToString creates new string every invocation
https://bugs.webkit.org/show_bug.cgi?id=84781

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

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


Patch looks good, but some minor changes needed here before committing.

> Source/JavaScriptCore/runtime/ObjectPrototype.cpp:259
> +    if (!thisObject->structure()->hasObjectToStringValue()) {

Our typical style here is just to call objectToStringValue():
RefPtr<> toStringValue = thisObject->structure()->objectToStringValue();
if (!toStringValue) {
    ...
    toStringValue = ...
}
...

> Source/JavaScriptCore/runtime/ObjectPrototype.cpp:264
> +	   thisObject->structure()->setObjectToStringValue(exec->globalData(),
thisObject, jsNontrivialString(exec, result));

The appropriate pattern is to make the local a RefPtr, to avoid accidentally
dereferencing NULL, and then use .release() when passing it to
setObjectToStringValue here.

> Source/JavaScriptCore/runtime/Structure.cpp:797
> +#if 1

Please remove the #if.