| Summary: | WTF::WeakPtr should have a "forget" method | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Brent Fulgham <bfulgham> | ||||||||||||
| Component: | WebCore Misc. | Assignee: | Brent Fulgham <bfulgham> | ||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||
| Severity: | Normal | CC: | benjamin, bfulgham, cdumez, cmarcelo, commit-queue | ||||||||||||
| Priority: | P2 | ||||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||
| Hardware: | All | ||||||||||||||
| OS: | All | ||||||||||||||
| Bug Depends on: | |||||||||||||||
| Bug Blocks: | 141931, 141935 | ||||||||||||||
| Attachments: |
|
||||||||||||||
Created attachment 247145 [details]
Patch
Created attachment 247149 [details]
Patch v2 (Fix test build error)
Created attachment 247150 [details]
Patch
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 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. Created attachment 247152 [details]
Patch v4 (Incorporate cdumez comments)
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 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 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 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. (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 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. Created attachment 247153 [details]
Patch v5 (More tests)
(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. Committed r180528: <http://trac.webkit.org/changeset/180528> |
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.