Bug 130553 – Change CodeGeneratorJS.pm special cases for "DOMWindow" to be general purpose

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Michael Saboff]

There are many places in CodeGeneratorJS.pm where there is if ($interfaceName eq "DOMWindow") that will emit code to return the JSDOMWindowShell* associated with the JSDOMWindow.  This should be changed to be more general purpose so it can be used by JSWorkerGlobalScope based objects that have a similar need.

[Michael Saboff]

Created attachment 227374 [details]
Patch

[Mark Lam]

Comment on attachment 227374 [details]
Patch

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

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:2379
>                  push(@implContent, "    if (UNLIKELY(!castedThis)) {\n");
>                  push(@implContent, "        if (JSDOMWindowShell* shell = jsDynamicCast<JSDOMWindowShell*>(JSValue::decode(thisValue)))\n");
>                  push(@implContent, "            castedThis = shell->window();\n");
>                  push(@implContent, "    }\n");

Is this section still needed?  In the other 3 sites where you did this change, you removed this null check and the computing of the result from shell->window().

[Michael Saboff]

(In reply to comment #2)
> (From update of attachment 227374 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=227374&action=review
> 
> > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:2379
> >                  push(@implContent, "    if (UNLIKELY(!castedThis)) {\n");
> >                  push(@implContent, "        if (JSDOMWindowShell* shell = jsDynamicCast<JSDOMWindowShell*>(JSValue::decode(thisValue)))\n");
> >                  push(@implContent, "            castedThis = shell->window();\n");
> >                  push(@implContent, "    }\n");
> 
> Is this section still needed?  In the other 3 sites where you did this change, you removed this null check and the computing of the result from shell->window().

You are right, this section is redundant with what's in toJSDOMWimdow().  I'll remove it.

[Geoffrey Garen]

Comment on attachment 227374 [details]
Patch

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

r=me

> Source/WebCore/bindings/scripts/IDLAttributes.txt:46
> +CustomProxyToJSObject

Seems like we should just call this "CustomToJSObject". Even though only proxies happen to use this feature, the feature doesn't need to be tightly coupled to proxies.

[Michael Saboff]

(In reply to comment #4)
> (From update of attachment 227374 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=227374&action=review
> 
> r=me
> 
> > Source/WebCore/bindings/scripts/IDLAttributes.txt:46
> > +CustomProxyToJSObject
> 
> Seems like we should just call this "CustomToJSObject". Even though only proxies happen to use this feature, the feature doesn't need to be tightly coupled to proxies.

CustomToJSObject is already taken and used in a wider variety of places.  Also there are two instances where the custom to*() returns a RefPtr.  Therefore I chose a different name.  I'm open to suggestions.

[Michael Saboff]

(In reply to comment #5)
> (In reply to comment #4)
> > (From update of attachment 227374 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=227374&action=review
> > 
> > r=me
> > 
> > > Source/WebCore/bindings/scripts/IDLAttributes.txt:46
> > > +CustomProxyToJSObject
> > 
> > Seems like we should just call this "CustomToJSObject". Even though only proxies happen to use this feature, the feature doesn't need to be tightly coupled to proxies.
> 
> CustomToJSObject is already taken and used in a wider variety of places.  Also there are two instances where the custom to*() returns a RefPtr.  Therefore I chose a different name.  I'm open to suggestions.

I looked into this again.  CustomToJSObject is used for a custom "toJS()".  It is JSCustomToNativeObject that is used for a custom "to<className>()", which I also tried.

[Michael Saboff]

Committed r166070: <http://trac.webkit.org/changeset/166070>

[Michael Saboff]

<rdar://problem/15940627>

[Michael Saboff]

Reopened as changed was rolled out in <http://trac.webkit.org/changeset/166249> due to page load time performance regression.

[Michael Saboff]

Relanded in change set r166040 - <http://trac.webkit.org/changeset/166404>