Summary: | V8 binding optimizations (redundant refs, unlikely errors) | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Jens Alfke <jens> | ||||||
Component: | WebCore Misc. | Assignee: | Jens Alfke <jens> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Enhancement | CC: | commit-queue | ||||||
Priority: | P3 | ||||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | All | ||||||||
OS: | All | ||||||||
Attachments: |
|
Description
Jens Alfke
2009-10-20 13:48:41 PDT
Created attachment 41523 [details]
patch 1
Comment on attachment 41523 [details] patch 1 I don't know that I'll be able to do the final review of this, but I can give some quick feedback on a part of this. > Index: WebCore/ChangeLog > + (WebCore::V8DOMWrapper::convertToV8Object): Added overload that takes a Ref<> Answering "Why?" is much better than answering "what?" (Answering "What was done?" is pretty useless because it can be determined by looking at the code.) Functions shouldn't take a RefPtr. They should take either a PassRefPtr<T> or a T* (http://webkit.org/coding/RefPtr.html). In general RefPtrs shouldn't be used as parameters, but this is a special case to handle generated code like the below (from V8Document.cpp): RefPtr<NodeIterator> result = imp->createNodeIterator(root, whatToShow, filter.get(), expandEntityReferences, ec); ... return V8DOMWrapper::convertToV8Object(V8ClassIndex::NODEITERATOR, result); The normal convention would be to add ".release()" after the result parameter in the second line, but the first line may or may not declare 'result' as a RefPtr and the second line doesn't know which it is. That said, I think I can add a bit of code to the binding generator to get that .release() call in there... Created attachment 41605 [details]
patch 2
This updated patch avoids creating a new method that takes a Ref<>. As a bonus, it now changes only one source file.
Comment on attachment 41605 [details]
patch 2
r=me.
Comment on attachment 41605 [details] patch 2 Clearing flags on attachment: 41605 Committed r49996: <http://trac.webkit.org/changeset/49996> All reviewed patches have been landed. Closing bug. |