Bug 23490 – Remove initialRefCount argument from RefCounted class

• Component: Web Template Framework
• Status: RESOLVED FIXED

[David Kilzer (:ddkilzer)]

* 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)

[David Kilzer (:ddkilzer)]

Created attachment 27063 [details]
Patch v1

Proposed fix.

[Darin Adler]

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.

[Kevin Ollivier]

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 ;-)

[Darin Fisher (:fishd, Google)]

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.

[David Kilzer (:ddkilzer)]

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)

[Darin Adler]

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

[David Kilzer (:ddkilzer)]

$ 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

[David Kilzer (:ddkilzer)]

(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.

[David Kilzer (:ddkilzer)]

$ 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

[David Kilzer (:ddkilzer)]

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