Bug 79947 - [V8] Bindings for node always check if they are a Document.
Summary: [V8] Bindings for node always check if they are a Document.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Erik Arvidsson
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-02-29 14:15 PST by Erik Arvidsson
Modified: 2012-03-02 13:45 PST (History)
4 users (show)

See Also:


Attachments
Patch (2.73 KB, patch)
2012-03-01 11:47 PST, Erik Arvidsson
no flags Details | Formatted Diff | Diff
Patch (11.00 KB, patch)
2012-03-02 11:35 PST, Erik Arvidsson
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Erik Arvidsson 2012-02-29 14:15:55 PST
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.
Comment 1 Adam Barth 2012-02-29 14:53:59 PST
We can also remove 

if (impl->document()) {

for all nodes except DocumentType nodes.
Comment 2 Erik Arvidsson 2012-03-01 11:47:21 PST
Created attachment 129727 [details]
Patch
Comment 3 Erik Arvidsson 2012-03-02 09:03:17 PST
Adam, Kentaro, Nate? Can anyone review this?
Comment 4 Adam Barth 2012-03-02 09:15:48 PST
Comment on attachment 129727 [details]
Patch

Can you run run-bindings-tests --reset-results so we can see the change in the generated code?
Comment 5 Adam Barth 2012-03-02 09:19:41 PST
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.
Comment 6 Adam Barth 2012-03-02 09:20:25 PST
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.
Comment 7 Erik Arvidsson 2012-03-02 10:15:49 PST
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.
Comment 8 Erik Arvidsson 2012-03-02 10:17:19 PST
(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.
Comment 9 Adam Barth 2012-03-02 10:37:45 PST
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.
Comment 10 Erik Arvidsson 2012-03-02 10:55:07 PST
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).
Comment 11 Erik Arvidsson 2012-03-02 11:35:50 PST
Created attachment 129930 [details]
Patch
Comment 12 WebKit Review Bot 2012-03-02 13:45:27 PST
Comment on attachment 129930 [details]
Patch

Clearing flags on attachment: 129930

Committed r109611: <http://trac.webkit.org/changeset/109611>
Comment 13 WebKit Review Bot 2012-03-02 13:45:34 PST
All reviewed patches have been landed.  Closing bug.