Summary: | [V8] Bindings for node always check if they are a Document. | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Erik Arvidsson <arv> | ||||||
Component: | DOM | Assignee: | Erik Arvidsson <arv> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | abarth, haraken, japhet, webkit.review.bot | ||||||
Priority: | P2 | ||||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
Attachments: |
|
Description
Erik Arvidsson
2012-02-29 14:15:55 PST
We can also remove if (impl->document()) { for all nodes except DocumentType nodes. Created attachment 129727 [details]
Patch
Adam, Kentaro, Nate? Can anyone review this? 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 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. 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 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. (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. 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. 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). Created attachment 129930 [details]
Patch
Comment on attachment 129930 [details] Patch Clearing flags on attachment: 129930 Committed r109611: <http://trac.webkit.org/changeset/109611> All reviewed patches have been landed. Closing bug. |