Bug 141923 - WTF::WeakPtr should have a "forget" method
Summary: WTF::WeakPtr should have a "forget" method
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Brent Fulgham
URL:
Keywords:
Depends on:
Blocks: 141931 141935
  Show dependency treegraph
 
Reported: 2015-02-23 13:53 PST by Brent Fulgham
Modified: 2015-02-23 16:57 PST (History)
5 users (show)

See Also:


Attachments
Patch (3.15 KB, patch)
2015-02-23 14:11 PST, Brent Fulgham
no flags Details | Formatted Diff | Diff
Patch v2 (Fix test build error) (3.15 KB, patch)
2015-02-23 14:30 PST, Brent Fulgham
mmaxfield: review+
Details | Formatted Diff | Diff
Patch (3.15 KB, patch)
2015-02-23 14:51 PST, Brent Fulgham
no flags Details | Formatted Diff | Diff
Patch v4 (Incorporate cdumez comments) (3.73 KB, patch)
2015-02-23 15:18 PST, Brent Fulgham
no flags Details | Formatted Diff | Diff
Patch v5 (More tests) (4.11 KB, patch)
2015-02-23 15:38 PST, Brent Fulgham
mmaxfield: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brent Fulgham 2015-02-23 13:53:44 PST
Sometimes we need to be able to clear a WeakPtr. Currently, you do so using the default constructor:

    m_aWeakPointerToSomething = WeakPtr<Something>();

It would be nicer to be able to write:

    m_aWeakPointerToSomething.forget();

With the expectation that the WeakPtr would be 'null' after calling 'forget'.

It should reduce the weak pointer reference count by one on the object being pointed to.
Comment 1 Brent Fulgham 2015-02-23 14:11:39 PST
Created attachment 247145 [details]
Patch
Comment 2 Brent Fulgham 2015-02-23 14:30:21 PST
Created attachment 247149 [details]
Patch v2 (Fix test build error)
Comment 3 Brent Fulgham 2015-02-23 14:51:55 PST
Created attachment 247150 [details]
Patch
Comment 4 Chris Dumez 2015-02-23 14:58:45 PST
Comment on attachment 247150 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=247150&action=review

> Source/WTF/wtf/WeakPtr.h:106
> +    void forget() { operator=(WeakPtr()); }

WeakPtrFactory seems to call this operation "revoke". Might be nice to be consistent.

Also, we could just do a "m_ref = WeakReference<T>::create(nullptr);" to avoid calling copyRef() unnecessarily.

> Tools/TestWebKitAPI/Tests/WTF/WeakPtr.cpp:127
> +    WeakPtr<int> weakPtr2 = factory->createWeakPtr();

We need to try WeakPtr copying / assignment as this is the reason why we need to create a new WeakReference when revoking (instead of simply clearing m_ref).
Comment 5 Chris Dumez 2015-02-23 15:00:27 PST
Comment on attachment 247150 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=247150&action=review

> Tools/TestWebKitAPI/Tests/WTF/WeakPtr.cpp:125
> +    WeakPtrFactory<int>* factory = new WeakPtrFactory<int>(&dummy);

nit: this could be stack-allocated. You could use a scope as you want to control life-time for your last checks.
Comment 6 Brent Fulgham 2015-02-23 15:18:56 PST
Created attachment 247152 [details]
Patch v4 (Incorporate cdumez comments)
Comment 7 Brent Fulgham 2015-02-23 15:21:13 PST
bfulgham-macpro:Tools bfulgham$ ../WebKitBuild/Debug/TestWebKitAPI --gtest_filter="WTF_Weak*"
**PASS** WTF_WeakPtr.Basic
**PASS** WTF_WeakPtr.Assignment
**PASS** WTF_WeakPtr.MultipleFactories
**PASS** WTF_WeakPtr.RevokeAll
**PASS** WTF_WeakPtr.NullFactory
**PASS** WTF_WeakPtr.Dereference
**PASS** WTF_WeakPtr.Forget
Comment 8 Chris Dumez 2015-02-23 15:24:54 PST
Comment on attachment 247152 [details]
Patch v4 (Incorporate cdumez comments)

View in context: https://bugs.webkit.org/attachment.cgi?id=247152&action=review

> Source/WTF/wtf/WeakPtr.h:106
> +    void forget() { m_ref = WeakReference<T>::create(nullptr); }

Any reason you kept using forget() instead of revoke() as in WeakPtrFactory?

> Tools/TestWebKitAPI/Tests/WTF/WeakPtr.cpp:125
> +    WeakPtrFactory<int>* factory = new WeakPtrFactory<int>(&dummy);

Still stack-allocated? Any reason you did not make this change?

> Tools/TestWebKitAPI/Tests/WTF/WeakPtr.cpp:142
> +    WeakPtr<int> weakPtr4 = weakPtr2;

The more interesting case is actually copying a WeakPtr that has been revoked / forgotten. Is this covered as well?
Comment 9 Myles C. Maxfield 2015-02-23 15:27:45 PST
Comment on attachment 247152 [details]
Patch v4 (Incorporate cdumez comments)

View in context: https://bugs.webkit.org/attachment.cgi?id=247152&action=review

>> Source/WTF/wtf/WeakPtr.h:106
>> +    void forget() { m_ref = WeakReference<T>::create(nullptr); }
> 
> Any reason you kept using forget() instead of revoke() as in WeakPtrFactory?

I think forget() is better, as revoke()'s semantics are different (revoking all vs forgetting this specific one)
Comment 10 Chris Dumez 2015-02-23 15:30:14 PST
Comment on attachment 247152 [details]
Patch v4 (Incorporate cdumez comments)

View in context: https://bugs.webkit.org/attachment.cgi?id=247152&action=review

>>> Source/WTF/wtf/WeakPtr.h:106
>>> +    void forget() { m_ref = WeakReference<T>::create(nullptr); }
>> 
>> Any reason you kept using forget() instead of revoke() as in WeakPtrFactory?
> 
> I think forget() is better, as revoke()'s semantics are different (revoking all vs forgetting this specific one)

Well, WeakPtr is actually using the name "revokeAll()". So using revoke() here does not seem confusing to me.
Comment 11 Brent Fulgham 2015-02-23 15:31:36 PST
(In reply to comment #8)
> Comment on attachment 247152 [details]
> Patch v4 (Incorporate cdumez comments)
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=247152&action=review
> 
> > Source/WTF/wtf/WeakPtr.h:106
> > +    void forget() { m_ref = WeakReference<T>::create(nullptr); }
> 
> Any reason you kept using forget() instead of revoke() as in WeakPtrFactory?

Yes. The term 'forget' seems better for this operation, which should be limited to a single WeakPtr. I think 'revokeAll' might not be a great name either, but at least the scary-sounding name is appropriate for an operation that will cause all WeakPtrs to this object to become unusable.

> > Tools/TestWebKitAPI/Tests/WTF/WeakPtr.cpp:125
> > +    WeakPtrFactory<int>* factory = new WeakPtrFactory<int>(&dummy);
> 
> Still stack-allocated? Any reason you did not make this change?

I'll change it.
 
> > Tools/TestWebKitAPI/Tests/WTF/WeakPtr.cpp:142
> > +    WeakPtr<int> weakPtr4 = weakPtr2;
> 
> The more interesting case is actually copying a WeakPtr that has been
> revoked / forgotten. Is this covered as well?

No; I'll add that.
Comment 12 Chris Dumez 2015-02-23 15:36:42 PST
Comment on attachment 247152 [details]
Patch v4 (Incorporate cdumez comments)

View in context: https://bugs.webkit.org/attachment.cgi?id=247152&action=review

BTW, where are you supposed to use this? This doesn't seem like a common operation in the current code base.

>>>>> Source/WTF/wtf/WeakPtr.h:106
>>>>> +    void forget() { m_ref = WeakReference<T>::create(nullptr); }
>>>> 
>>>> Any reason you kept using forget() instead of revoke() as in WeakPtrFactory?
>>> 
>>> I think forget() is better, as revoke()'s semantics are different (revoking all vs forgetting this specific one)
>> 
>> Well, WeakPtr is actually using the name "revokeAll()". So using revoke() here does not seem confusing to me.
> 
> Yes. The term 'forget' seems better for this operation, which should be limited to a single WeakPtr. I think 'revokeAll' might not be a great name either, but at least the scary-sounding name is appropriate for an operation that will cause all WeakPtrs to this object to become unusable.

Ok, not a big deal for me. I just thought I would mention that WeakPtrFactory was using a different name for a similar operation.
Comment 13 Brent Fulgham 2015-02-23 15:38:29 PST
Created attachment 247153 [details]
Patch v5 (More tests)
Comment 14 Brent Fulgham 2015-02-23 15:42:37 PST
(In reply to comment #12)
> BTW, where are you supposed to use this? This doesn't seem like a common
> operation in the current code base.

It's a piece of a new patch that I hope to get in later today! :-)

> Ok, not a big deal for me. I just thought I would mention that
> WeakPtrFactory was using a different name for a similar operation.

Ok. We can always change it if we decide that the naming is bad.
Comment 15 Brent Fulgham 2015-02-23 15:44:16 PST
Committed r180528: <http://trac.webkit.org/changeset/180528>