[webkit-reviews] review denied: [Bug 64467] [V8] Avoid memory leaks with hidden references. : [Attachment 100689] Patch

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has denied Vitaly Repeshko
<vitalyr at chromium.org>'s request for review:
Bug 64467: [V8] Avoid memory leaks with hidden references.
https://bugs.webkit.org/show_bug.cgi?id=64467

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

------- Additional Comments from Adam Barth <abarth at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=100689&action=review


Looks reasonable, but I'd like to see one more iteration with Vector and the
namespace question questions addressed.

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:861
> -	       push(@implContentDecls, "	   
V8DOMWrapper::setHiddenReference(info.Holder(), wrapper);\n");
> +	       push(@implContentDecls, "	   
V8DOMWrapper::setNamedHiddenReference(info.Holder(), \"${attrName}\",
wrapper);\n");

Do we want to put attribute names into their own namespace?

> Source/WebCore/bindings/v8/V8DOMWrapper.cpp:193
> +static const char hiddenPrefix[] = "WebCore::";

Don't we have this constant already in V8HiddenPropertyNames somewhere?

> Source/WebCore/bindings/v8/V8DOMWrapper.cpp:199
> +    char* prefixedName = new char[prefixLength + nameLength + 1];

Can we use Vector<char> instead?  All this bare-handed lifting looks wrong. 
Vector<char> also can have an inline capacity, which means we can avoid the
malloc in the common case of a short name.

> Source/WebCore/bindings/v8/custom/V8WebGLRenderingContextCustom.cpp:164
> +    const char* referenceName;

I'd initialize this to zero.