Bug 79947 – [V8] Bindings for node always check if they are a Document.

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Erik Arvidsson]

All the bindings for interfaces that extend Node have the following:

v8::Handle<v8::Object> V8HTMLAnchorElement::wrapSlow(HTMLAnchorElement* impl)
{
    v8::Handle<v8::Object> wrapper;
    V8Proxy* proxy = 0;
    if (impl->document()) {
        proxy = V8Proxy::retrieve(impl->document()->frame());
        if (proxy && static_cast<Node*>(impl->document()) == static_cast<Node*>(impl)) {
            if (proxy->windowShell()->context().IsEmpty() && proxy->windowShell()->initContextIfNeeded()) {
                // initContextIfNeeded may have created a wrapper for the object, retry from the start.
                return V8HTMLAnchorElement::wrap(impl);
            }
        }
    }
    ...

The important line here is

        if (proxy && static_cast<Node*>(impl->document()) == static_cast<Node*>(impl)) {

which seems to be only true if impl is a document.

I believe we can remove this check in the code generator.

[Adam Barth]

We can also remove 

if (impl->document()) {

for all nodes except DocumentType nodes.

[Erik Arvidsson]

Created attachment 129727 [details]
Patch

[Erik Arvidsson]

Adam, Kentaro, Nate? Can anyone review this?

[Adam Barth]

Comment on attachment 129727 [details]
Patch

Can you run run-bindings-tests --reset-results so we can see the change in the generated code?

[Adam Barth]

Comment on attachment 129727 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=129727&action=review

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:3038
> +    if (IsNodeSubType($dataNode) && $interfaceName ne "DocumentType") {
> +        $proxyInit = "V8Proxy::retrieve(impl->document()->frame())";
> +    } else {
> +        $proxyInit = "0";

Why is $proxyInit always zero when $interfaceName ne "DocumentType" ?  I would have expected the DocumentType case to need a null-check on impl->document().  Presumably when impl->document() is non-null, the "DocumentType" case should work the same as the other nodes.

[Adam Barth]

Do you have any stats on how much performance we gain with this change?  If so, you might want to put them in the ChangeLog.

[Erik Arvidsson]

Comment on attachment 129727 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=129727&action=review

>> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:3038
>> +        $proxyInit = "0";
> 
> Why is $proxyInit always zero when $interfaceName ne "DocumentType" ?  I would have expected the DocumentType case to need a null-check on impl->document().  Presumably when impl->document() is non-null, the "DocumentType" case should work the same as the other nodes.

I misunderstood your earlier comment. I thought you said that the document() of a DocumentType i always null. However it might be null so a check is needed for that. I will update this.

[Erik Arvidsson]

(In reply to comment #6)
> Do you have any stats on how much performance we gain with this change?  If so, you might want to put them in the ChangeLog.

I haven't. The perf tests takes too long to run. I was just going to monitor the perf bots instead.

[Adam Barth]

Oh, sorry.  The DocumentType node can have a null document (but also can have a non-null document).  Other types of nodes always have non-null documents.

[Erik Arvidsson]

Sorry about forgetting about run-binding-tests. However, none of the test bindings are Nodes so this will not be visible in there (The added UNLIKELY will show up though).

[Erik Arvidsson]

Created attachment 129930 [details]
Patch

[WebKit Review Bot]

Comment on attachment 129930 [details]
Patch

Clearing flags on attachment: 129930

Committed r109611: <http://trac.webkit.org/changeset/109611>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.