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

[bugzilla-daemon at webkit.org]

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

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

------- Additional Comments from Kentaro Hara <haraken at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=191309&action=review


Overall the approach looks good to me.

I'm interested in performance impacts when you enable the wrapping? Did you
observe performance impacts on Dromaeo/dom-*.html or Bindings/* ?

> Source/WebCore/ChangeLog:7
> +

Please describe what your change is doing.

> Source/WebCore/ChangeLog:8
> +	   No new tests (OOPS!).

You need to update run-bindings-tests.

> Source/WebCore/bindings/v8/V8DOMCoverWrapping.cpp:104
> +    v8::Persistent<v8::Value> dataHandle =
v8::Persistent<v8::Value>::New(isolate, v8::External::New(instanceData));
> +    dataHandle.MakeWeak(isolate, instanceData, weakReferenceCallback);

Can't you use ScopedPersistent to manage the lifetime? (although the lifetime
will be almost the same as the lifetime of WebKit.)

> Source/WebCore/bindings/v8/V8DOMCoverWrapping.cpp:107
> +    // Override the method callback, redirecting it to the coverwrapping
dispatcher.
> +    callbackTemplate->SetCallHandler(dispatchCoverWrappedMethod,
dataHandle);

It's a bit inconsistent that you are setting callbacks in
V8DOMCoverWrapping.cpp whereas you are setting getters/setters in
V8DOMConfigurations.cpp. Could you make it more consistent?

> Source/WebCore/bindings/v8/WrapperTypeInfo.h:78
> +	   static WrapperTypeInfo* unwrapAddr(v8::Handle<v8::Value>
typeInfoAddrWrapper)

Nit: "unwrapInfoAddr" would be a better name.