[webkit-reviews] review denied: [Bug 107207] Support selectively wrapping DOM accesses from certain V8 contexts. : [Attachment 183488] Patch

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has denied Ulfar Erlingsson
<ulfar.chromium at gmail.com>'s request for review:
Bug 107207: Support selectively wrapping DOM accesses from certain V8 contexts.
https://bugs.webkit.org/show_bug.cgi?id=107207

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

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


> Source/WebCore/bindings/v8/V8DOMConfiguration.cpp:99
> +	   v8::Handle<v8::FunctionTemplate> callbackTemplate =
v8::FunctionTemplate::New(callback.callback, v8Undefined(), signature);

This looks like a memory leak.	Where is the matching call to Dispose?

> Source/WebCore/bindings/v8/V8DOMCoverWrapping.cpp:71
> +    v8::HandleScope scope;

It's very unlikely that you need a HandelScope here given that you're taking a
v8::Local as a parameter.

> Source/WebCore/bindings/v8/V8DOMCoverWrapping.cpp:82
> +    delete instanceData->m_wrapper;
> +    delete instanceData;

We try to avoid calling "delete" manually.  Instead, we use OwnPtr and related
classes to do this work automatically.

> Source/WebCore/bindings/v8/V8DOMCoverWrapping.cpp:125
> +    v8::Persistent<v8::Value> dataHandle =
v8::Persistent<v8::Value>::New(v8::External::New(instanceData));

Looks like another memory leak.

> Source/WebCore/bindings/v8/V8DOMCoverWrappingPolicy.cpp:103
> +	   logEntryFormatStr->append(className);

Please use StringBuilder rather than calling "append" on a String.

> Source/WebCore/bindings/v8/V8DOMCoverWrappingPolicy.cpp:112
> +// TODO: Figure out a way to make these policies be robust wrt the IDL
files, etc.

TODO -> FIXME