Aside from the ref-counter munging, it still seems like there's value in using PassRefPtr as a signifier that a given API takes ownership of a pointer.
So I'm not certain (in the case of HTMLNameCollection) that it'd be a better solution to change it not to take an ordinary pointer - yes, it would eliminate some ref churn, but at the cost of losing that connotation. A better solution (since the constructor is private) would be to make the constructor explicitly take a CollectionCache* parameter, thereby allowing us to directly pass the document straight through to the base class. -atw On Thu, Oct 29, 2009 at 3:31 PM, Maciej Stachowiak <[email protected]> wrote: > > On Oct 29, 2009, at 12:56 PM, Darin Adler wrote: > > On Oct 27, 2009, at 10:55 AM, Jens Alfke wrote: >> >> Looking at how refcounting is implemented in WebCore, I was surprised to >>> find that there are a lot of functions/methods that take PassRefPtr<>s as >>> parameters instead of regular pointers to those objects. I can't see any >>> benefit to this, and it adds the overhead of a ref() and deref() at every >>> call-site. >>> >> >> Have you read the RefPtr document? It is at < >> http://webkit.org/coding/RefPtr.html>. >> >> Only functions that take ownership of the passed-in objects should take >> PassRefPtr. This makes it so we can optimally handle the case where that >> object was just created and lets us hand off the reference instead of having >> to do a round of reference count thrash. If the function is going to be >> storing the value in a RefPtr, then the PassRefPtr does not introduce any >> reference count thrash even if the passed in object was not just created. >> >> It’s wrong for the HTMLNameCollection constructor to take a PassRefPtr for >> its document argument because it doesn’t take ownership of the document. So >> that one is definitely a mistake. >> > > However, its base class HTMLCollection can take ownership. The > HTMLNameCollection constructor misses out on the optimization though: > > HTMLNameCollection::HTMLNameCollection(PassRefPtr<Document> document, > CollectionType type, const String& name) > : HTMLCollection(document.get(), type, > document->nameCollectionInfo(type, name)) > , m_name(name) > { > } > > Note that it passes document.get(), but also uses document after that so it > couldn't just pass document. Thus, it probably shouldn't take a PassRefPtr > (unless there is a clever way to fix it that I'm not seeing). > > - Maciej > > > > >> It is true that PassRefPtr arguments can be tricky as you mentioned and >> the RefPtr document also mentions. >> >> -- Darin >> >> _______________________________________________ >> webkit-dev mailing list >> [email protected] >> http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev >> > > _______________________________________________ > webkit-dev mailing list > [email protected] > http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev >
_______________________________________________ webkit-dev mailing list [email protected] http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
