Summary: | Change CodeGeneratorJS.pm special cases for "DOMWindow" to be general purpose | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Michael Saboff <msaboff> | ||||
Component: | JavaScriptCore | Assignee: | Michael Saboff <msaboff> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | CC: | cdumez, cgarcia, commit-queue, esprehn+autocc, kondapallykalyan | ||||
Priority: | P2 | Keywords: | InRadar | ||||
Version: | 528+ (Nightly build) | ||||||
Hardware: | All | ||||||
OS: | All | ||||||
Bug Depends on: | |||||||
Bug Blocks: | 130554 | ||||||
Attachments: |
|
Description
Michael Saboff
2014-03-20 18:40:22 PDT
Created attachment 227374 [details]
Patch
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(). (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. 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. (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. (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. Committed r166070: <http://trac.webkit.org/changeset/166070> Reopened as changed was rolled out in <http://trac.webkit.org/changeset/166249> due to page load time performance regression. Relanded in change set r166040 - <http://trac.webkit.org/changeset/166404> |