Summary: | Remove initialRefCount argument from RefCounted class | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | David Kilzer (:ddkilzer) <ddkilzer> | ||||||
Component: | Web Template Framework | Assignee: | David Kilzer (:ddkilzer) <ddkilzer> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | darin, fishd, kevino | ||||||
Priority: | P2 | ||||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | Mac | ||||||||
OS: | OS X 10.5 | ||||||||
Attachments: |
|
Description
David Kilzer (:ddkilzer)
2009-01-22 18:34:22 PST
Created attachment 27063 [details]
Patch v1
Proposed fix.
Comment on attachment 27063 [details] Patch v1 > + static PassRefPtr<AccessibilityObjectWrapper> create(PassRefPtr<AccessibilityObject> obj) > + { > + return adoptRef(new AccessibilityObjectWrapper(obj)); > + } > + static PassRefPtr<AccessibilityObjectWrapper> create() > + { > + return adoptRef(new AccessibilityObjectWrapper()); > + } This is an abstract class which can't be instantiated, because of the pure virtual detach function, so these create functions won't do anyone any good. Presumably the code that calls new and needs to do adoptRef is somewhere else in the Chromium project, not checked into the WebKit source tree. So you can't make this change without creating a Chrome-specific storage leak unless the Chrome team checks in some more of their code! > + AccessibilityObjectWrapper(PassRefPtr<AccessibilityObject> obj) : m_object(obj) { } How about "object" instead of "obj"? > - AccessibilityObject* m_object; > + RefPtr<AccessibilityObject> m_object; Are you sure it's right to change this to a RefPtr? This doesn't seem related to your RefCounted initial reference count fix. Maybe the Chrome folks have a reason to not use a RefPtr here? > + return adoptRef(new FrameLoaderClientWx()); No need for the parentheses here. Why not take the initial reference count argument out of RefCountedBase too, while you're at it? The only client for that is in ByteArray.h and it passes a 1 too. review- for now, but this looks mostly fine. Actually, it doesn't look like the other ports use RefCounted with FrameLoaderClient, and I think this is mostly just a relic of how things were done some time ago, so I wonder if it isn't good to just remove it and just use a straight pointer like the rest of the ports? (I had a patch for that in the works, but you beat me to it ;-) Yeah, we haven't begun upstreaming chromium's WebKit layer. We are still a ways from being able to separate it from our tree. I think we used an initial ref count of 0 to be compatible with some existing code that hadn't yet been translated over to the new hotness. Please don't worry about breaking our port in cases like this where you don't have access to all of the related code. Created attachment 27084 [details] Patch v2 (requires more work) Changes since Patch v1: - Removed changes to AccessibilityObject stuff. Chromium can't be fixed until their full tree lands. (Comment #2 and Comment #4) I'll land the mac header guard fix separately. - Also removed the argument to RefCountedBase and updated ByteArray. (Comment #2) - Made FrameLoaderClientWx not RefCounted. (Comment #3) Comment on attachment 27084 [details] Patch v2 (requires more work) > - RefCounted(int initialRefCount = 1) > - : RefCountedBase(initialRefCount) > + RefCounted() > { > } This entire constructor definition could be removed! We don't need it at all any more. r=me $ git svn dcommit Committing to http://svn.webkit.org/repository/webkit/trunk ... M JavaScriptCore/ChangeLog M JavaScriptCore/wtf/ByteArray.h M JavaScriptCore/wtf/RefCounted.h M WebKit/wx/ChangeLog M WebKit/wx/WebKitSupport/FrameLoaderClientWx.cpp M WebKit/wx/WebKitSupport/FrameLoaderClientWx.h Committed r40319 http://trac.webkit.org/changeset/40319 (In reply to comment #6) > (From update of attachment 27084 [details] [review]) > > - RefCounted(int initialRefCount = 1) > > - : RefCountedBase(initialRefCount) > > + RefCounted() > > { > > } > > This entire constructor definition could be removed! We don't need it at all > any more. This change was made for the final patch. $ git svn dcommit Committing to http://svn.webkit.org/repository/webkit/trunk ... M JavaScriptCore/ChangeLog M WebKit/wx/ChangeLog Committed r40320 Fixed ChangeLog dates. :( http://trac.webkit.org/changeset/40320 Follow-up build fix for Wx: $ git svn dcommit Committing to http://svn.webkit.org/repository/webkit/trunk ... M WebKit/wx/ChangeLog M WebKit/wx/WebKitSupport/FrameLoaderClientWx.cpp M WebKit/wx/WebKitSupport/FrameLoaderClientWx.h Committed r40361 http://trac.webkit.org/changeset/40361 |