Bug 27372 – [v8] switch to faster methods to obtain and set implementation

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[anton muhin]

Recently new methods to access internal fields were introduced which should be faster.  Switch to them plus refactor a bit the code to pass v8::Handle<v8::Object> (instead of <v8::Value>) into the methods fetching implementation from the wrapper.

Please, note, this change should not be landed before http://codereview.chromium.org/159003 is submitted.

[anton muhin]

Created attachment 32942 [details]
Initial version

[Mads Ager]

This looks fine to me.

I have landed the change in chromium that this change depends on, so it is safe to land this change once it gets the r+.

[Adam Barth]

Comment on attachment 32942 [details]
Initial version

Thanks for the patch.  This looks great.  One question, how do we know this is safe:

+        node = V8DOMWrapper::convertDOMWrapperToNode<Node>(v8::Handle<v8::Object>::Cast(args[0]));

or

+        contextNode = V8DOMWrapper::convertDOMWrapperToNode<Node>(v8::Handle<v8::Object>::Cast(args[1]));

Why can't the page pass a value as the argument?

[anton muhin]

(In reply to comment #2)
> This looks fine to me.
> 
> I have landed the change in chromium that this change depends on, so it is safe
> to land this change once it gets the r+.

Thanks a lot, Mads.

[anton muhin]

(In reply to comment #3)
> (From update of attachment 32942 [details])
> Thanks for the patch.  This looks great.  One question, how do we know this is
> safe:
> 
> +        node =
> V8DOMWrapper::convertDOMWrapperToNode<Node>(v8::Handle<v8::Object>::Cast(args[0]));
> 
> or
> 
> +        contextNode =
> V8DOMWrapper::convertDOMWrapperToNode<Node>(v8::Handle<v8::Object>::Cast(args[1]));
> 
> Why can't the page pass a value as the argument?

Adam, could you double check, but we did this cast unconditionally in DOMWrapper::convertToNativeObject anyway.  I just moved cast into callers, not callee not to penalize callers which already have an object

[Adam Barth]

Comment on attachment 32942 [details]
Initial version

Awesome.  Thanks for explain this patch to me.  Yes, I see now that the callers all have branches guarding the caste (and that we've retained the assert that's keeping us honest).

[Adam Barth]

Will land.

[Adam Barth]

Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	WebCore/ChangeLog
	M	WebCore/bindings/scripts/CodeGeneratorV8.pm
	M	WebCore/bindings/v8/V8DOMWrapper.cpp
	M	WebCore/bindings/v8/V8DOMWrapper.h
	M	WebCore/bindings/v8/custom/V8CanvasRenderingContext2DCustom.cpp
	M	WebCore/bindings/v8/custom/V8ClipboardCustom.cpp
	M	WebCore/bindings/v8/custom/V8DocumentCustom.cpp
	M	WebCore/bindings/v8/custom/V8ElementCustom.cpp
	M	WebCore/bindings/v8/custom/V8HTMLCanvasElementCustom.cpp
	M	WebCore/bindings/v8/custom/V8HTMLOptionsCollectionCustom.cpp
	M	WebCore/bindings/v8/custom/V8HTMLSelectElementCustom.cpp
	M	WebCore/bindings/v8/custom/V8InspectorControllerCustom.cpp
	M	WebCore/bindings/v8/custom/V8NodeCustom.cpp
	M	WebCore/bindings/v8/custom/V8XSLTProcessorCustom.cpp
Committed r46145
	M	WebCore/ChangeLog
	M	WebCore/bindings/scripts/CodeGeneratorV8.pm
	M	WebCore/bindings/v8/V8DOMWrapper.cpp
	M	WebCore/bindings/v8/V8DOMWrapper.h
	M	WebCore/bindings/v8/custom/V8HTMLCanvasElementCustom.cpp
	M	WebCore/bindings/v8/custom/V8DocumentCustom.cpp
	M	WebCore/bindings/v8/custom/V8NodeCustom.cpp
	M	WebCore/bindings/v8/custom/V8CanvasRenderingContext2DCustom.cpp
	M	WebCore/bindings/v8/custom/V8InspectorControllerCustom.cpp
	M	WebCore/bindings/v8/custom/V8ElementCustom.cpp
	M	WebCore/bindings/v8/custom/V8HTMLOptionsCollectionCustom.cpp
	M	WebCore/bindings/v8/custom/V8ClipboardCustom.cpp
	M	WebCore/bindings/v8/custom/V8HTMLSelectElementCustom.cpp
	M	WebCore/bindings/v8/custom/V8XSLTProcessorCustom.cpp
r46145 = b8d7bc3aef4a0028123fa2cd9ca298b39e32e9dd (trunk)
No changes between current HEAD and refs/remotes/trunk
Resetting to the latest refs/remotes/trunk
http://trac.webkit.org/changeset/46145