Bug 23490 - Remove initialRefCount argument from RefCounted class
Summary: Remove initialRefCount argument from RefCounted class
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Template Framework (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac OS X 10.5
: P2 Normal
Assignee: David Kilzer (:ddkilzer)
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-01-22 18:34 PST by David Kilzer (:ddkilzer)
Modified: 2009-01-29 05:10 PST (History)
3 users (show)

See Also:


Attachments
Patch v1 (8.83 KB, patch)
2009-01-26 19:18 PST, David Kilzer (:ddkilzer)
darin: review-
Details | Formatted Diff | Diff
Patch v2 (requires more work) (5.25 KB, patch)
2009-01-27 13:34 PST, David Kilzer (:ddkilzer)
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description David Kilzer (:ddkilzer) 2009-01-22 18:34:22 PST
* SUMMARY
Now that nearly all classes(*) have switched to starting with a ref count of 1, the argument to the RefCounted class may be removed.

(*) There are two classes remaining that haven't been converted:

$ grep -r ': RefCounted<' WebCore | grep '(0)'
WebCore/page/chromium/AccessibilityObjectWrapper.h:            : RefCounted<AccessibilityObjectWrapper>(0), m_object(obj) { }

$ grep -r ': RefCounted<' WebKit | grep '(0)'
WebKit/wx/WebKitSupport/FrameLoaderClientWx.cpp:    : RefCounted<FrameLoaderClientWx>(0)
Comment 1 David Kilzer (:ddkilzer) 2009-01-26 19:18:09 PST
Created attachment 27063 [details]
Patch v1

Proposed fix.
Comment 2 Darin Adler 2009-01-26 19:37:28 PST
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.
Comment 3 Kevin Ollivier 2009-01-26 20:25:28 PST
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 ;-)
Comment 4 Darin Fisher (:fishd, Google) 2009-01-27 09:15:31 PST
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.
Comment 5 David Kilzer (:ddkilzer) 2009-01-27 13:34:25 PST
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 6 Darin Adler 2009-01-27 13:54:27 PST
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
Comment 7 David Kilzer (:ddkilzer) 2009-01-28 12:08:09 PST
$ 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
Comment 8 David Kilzer (:ddkilzer) 2009-01-28 12:09:08 PST
(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.
Comment 9 David Kilzer (:ddkilzer) 2009-01-28 12:27:08 PST
$ 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
Comment 10 David Kilzer (:ddkilzer) 2009-01-29 05:10:52 PST
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