[Webkit-unassigned] [Bug 107207] [V8] Support selectively wrapping DOM accesses from certain V8 contexts.

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=107207





--- Comment #12 from Ulfar Erlingsson <ulfar.chromium at gmail.com>  2013-03-05 11:07:07 PST ---
(In reply to comment #11)
> (From update of attachment 191309 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=191309&action=review
> 
> Overall the approach looks good to me.

That's great to hear.

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

I'll run the performance tests and get those numbers today.  I don't expect there to be any measurable overhead, since there are only a few extra machine instructions plus one extra memory access per DOM wrapper invocation. 

Note: If the perf cost is as low as I expect, this mechanism can be used for various types of optional monitoring---such as gathering of trace data---without any cost being incurred by normal operation.

> > Source/WebCore/ChangeLog:7
> > +
>
> Please describe what your change is doing.

Will add.

> > Source/WebCore/ChangeLog:8
> > +        No new tests (OOPS!).
> 
> You need to update run-bindings-tests.
> 

Will do.  Sorry, I'm new to this.

> > 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.)

Instead of using V8 Persistent and weak callbacks, it would be cleaner to pass ownership of all the closure data instances to an array in V8PerIsolateData.  What do you think?

> > 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?
> 

Yes, I was also annoyed by this inconsistency, which came about because the V8 method callback data parameter can't be installed during the V8 Set Call (like with the SetAccessor API), but only afterwards with a SetCallHandler.  I'll try to clean up.

> > Source/WebCore/bindings/v8/WrapperTypeInfo.h:78
> > +        static WrapperTypeInfo* unwrapAddr(v8::Handle<v8::Value> typeInfoAddrWrapper)
> 
> Nit: "unwrapInfoAddr" would be a better name.

WIll do.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.