Bug 23490 - Remove initialRefCount argument from RefCounted class
: Remove initialRefCount argument from RefCounted class
Status: RESOLVED FIXED
: WebKit
Web Template Framework
: 528+ (Nightly build)
: Macintosh Mac OS X 10.5
: P2 Normal
Assigned To:
:
:
:
:
  Show dependency treegraph
 
Reported: 2009-01-22 18:34 PST by
Modified: 2009-01-29 05:10 PST (History)


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


Note

You need to log in before you can comment on or make changes to this bug.


Description From 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 From 2009-01-26 19:18:09 PST -------
Created an attachment (id=27063) [details]
Patch v1

Proposed fix.
------- Comment #2 From 2009-01-26 19:37:28 PST -------
(From update of attachment 27063 [details])
> +        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 From 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 From 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 From 2009-01-27 13:34:25 PST -------
Created an attachment (id=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 From 2009-01-27 13:54:27 PST -------
(From update of attachment 27084 [details])
> -    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 From 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 From 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 From 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 From 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