Bug 141923 – WTF::WeakPtr should have a "forget" method

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Brent Fulgham]

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.

[Brent Fulgham]

Created attachment 247145 [details]
Patch

[Brent Fulgham]

Created attachment 247149 [details]
Patch v2 (Fix test build error)

[Brent Fulgham]

Created attachment 247150 [details]
Patch

[Chris Dumez]

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

[Chris Dumez]

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.

[Brent Fulgham]

Created attachment 247152 [details]
Patch v4 (Incorporate cdumez comments)

[Brent Fulgham]

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

[Chris Dumez]

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?

[Myles C. Maxfield]

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)

[Chris Dumez]

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.

[Brent Fulgham]

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

[Chris Dumez]

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.

[Brent Fulgham]

Created attachment 247153 [details]
Patch v5 (More tests)

[Brent Fulgham]

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

[Brent Fulgham]

Committed r180528: <http://trac.webkit.org/changeset/180528>