Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1232033004:
Introduce WebPassOwnPtr. (Closed)
|
Created:
5 years, 5 months ago by yhirano Modified:
5 years, 5 months ago CC:
blink-reviews, dglazkov+blink Base URL:
svn://svn.chromium.org/blink/trunk Target Ref:
refs/heads/master Project:
blink Visibility:
Public. |
DescriptionIntroduce WePassOwnPtr.
As a preparation for WebCallbacks refactoring, this CL introduces
WebPassOwnPtr as a "public type" of PassOwnPtr.
BUG=493531
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=199024
Patch Set 1 #Patch Set 2 : #
Total comments: 6
Patch Set 3 : #
Total comments: 2
Patch Set 4 : #
Total comments: 4
Patch Set 5 : #Patch Set 6 : #Messages
Total messages: 19 (4 generated)
yhirano@chromium.org changed reviewers: + kinuko@chromium.org
I have a feeling that this one doesn't need to be paired with WebPrivateOwnPtr, and the name should be probably renamed to something that's not 'Private'? (As WebPrivate* types are intended to be used only for wrapping impl details) https://codereview.chromium.org/1232033004/diff/20001/public/platform/WebPriv... File public/platform/WebPrivateOwnPtr.h (right): https://codereview.chromium.org/1232033004/diff/20001/public/platform/WebPriv... public/platform/WebPrivateOwnPtr.h:45: WebPrivatePassOwnPtr(const WebPrivatePassOwnPtr<U>& o) Do we need this? https://codereview.chromium.org/1232033004/diff/20001/public/platform/WebPriv... public/platform/WebPrivateOwnPtr.h:53: T* get() const { return m_ptr; } Do we need this? I feel release() (in the other patch) and the assertion in the dtor are all we need in our use case? https://codereview.chromium.org/1232033004/diff/20001/public/platform/WebPriv... public/platform/WebPrivateOwnPtr.h:80: WebPrivateOwnPtr(const WebPrivatePassOwnPtr<U>& o) Do we actually use this?
> I have a feeling that this one doesn't need to be paired with WebPrivateOwnPtr, and the name should be probably renamed to something that's not 'Private'? (As WebPrivate* types are intended to be used only for wrapping impl details) Makes sense. I move renamed it to WebPassPtr and removed interaction with WebPrivateOwnPtr. PTAL. https://codereview.chromium.org/1232033004/diff/20001/public/platform/WebPriv... File public/platform/WebPrivateOwnPtr.h (right): https://codereview.chromium.org/1232033004/diff/20001/public/platform/WebPriv... public/platform/WebPrivateOwnPtr.h:45: WebPrivatePassOwnPtr(const WebPrivatePassOwnPtr<U>& o) On 2015/07/16 05:58:57, kinuko wrote: > Do we need this? Yes, if we pass a WebPrivatePassOwnPtr<Derived> to a function that takes a WebPrivatePassOwnPtr<Base>. I think it is likely to happen. https://codereview.chromium.org/1232033004/diff/20001/public/platform/WebPriv... public/platform/WebPrivateOwnPtr.h:53: T* get() const { return m_ptr; } On 2015/07/16 05:58:57, kinuko wrote: > Do we need this? I feel release() (in the other patch) and the assertion in the > dtor are all we need in our use case? Done. https://codereview.chromium.org/1232033004/diff/20001/public/platform/WebPriv... public/platform/WebPrivateOwnPtr.h:80: WebPrivateOwnPtr(const WebPrivatePassOwnPtr<U>& o) On 2015/07/16 05:58:57, kinuko wrote: > Do we actually use this? Deleted.
Thanks, it lgtm now, let's see what the owners say. Please update the issue subject and description too. Also it'd be probably useful (for reviewers) to link to the other CL where this one's used. https://codereview.chromium.org/1232033004/diff/40001/public/platform/WebPass... File public/platform/WebPassOwnPtr.h (right): https://codereview.chromium.org/1232033004/diff/40001/public/platform/WebPass... public/platform/WebPassOwnPtr.h:17: // side to blink side. Add a comment to note that the ownership must be taken by calling release() in the blink implementation before it gets destructed?
https://codereview.chromium.org/1232033004/diff/40001/public/platform/WebPass... File public/platform/WebPassOwnPtr.h (right): https://codereview.chromium.org/1232033004/diff/40001/public/platform/WebPass... public/platform/WebPassOwnPtr.h:17: // side to blink side. On 2015/07/16 06:49:35, kinuko wrote: > Add a comment to note that the ownership must be taken by calling release() in > the blink implementation before it gets destructed? I think implicit release is useful inside blink. What do you think?
On 2015/07/16 07:27:03, yhirano wrote: > https://codereview.chromium.org/1232033004/diff/40001/public/platform/WebPass... > File public/platform/WebPassOwnPtr.h (right): > > https://codereview.chromium.org/1232033004/diff/40001/public/platform/WebPass... > public/platform/WebPassOwnPtr.h:17: // side to blink side. > On 2015/07/16 06:49:35, kinuko wrote: > > Add a comment to note that the ownership must be taken by calling release() in > > the blink implementation before it gets destructed? > > I think implicit release is useful inside blink. What do you think? Makes sense to me.
yhirano@chromium.org changed reviewers: + tkent@chromium.org
tkent@, can you take a look at this change as an OWNER? The introduced class will be used in https://codereview.chromium.org/1234603003/, https://codereview.chromium.org/1235083006/ and https://codereview.chromium.org/1240763002/.
I'm negative to introduce new concept at this timing. After we allow base/ dependency in Blink, we can use scoped_ptr. After we allow C++ library feature, we can use std::unique_ptr. Can you wait until them?
On 2015/07/16 07:53:49, tkent wrote: > I'm negative to introduce new concept at this timing. > After we allow base/ dependency in Blink, we can use scoped_ptr. > After we allow C++ library feature, we can use std::unique_ptr. > Can you wait until them? It would be great if we could use them, and I will be happy to migrate from this class to the standard type. By the way, the CallbackPromiseAdapter / WebCallbacks refactoring would be much easier if the repository merge were done and I've waited for six months. Now I'm not sure if it would happen in the near future and I'm inclined to refactor before more users use them.
On 2015/07/16 08:04:28, yhirano wrote: > It would be great if we could use them, and I will be happy to migrate from this > class to the standard type. ok. Please add TODO(yhirano) to WebPassOwnPtr.h. Then I can accept this CL.
https://codereview.chromium.org/1232033004/diff/60001/public/platform/WebPass... File public/platform/WebPassOwnPtr.h (right): https://codereview.chromium.org/1232033004/diff/60001/public/platform/WebPass... public/platform/WebPassOwnPtr.h:8: #include "WebCommon.h" should be "public/platform/WebCommon.h" https://codereview.chromium.org/1232033004/diff/60001/public/platform/WebPass... public/platform/WebPassOwnPtr.h:55: mutable T* m_ptr; Please add a comment about the reason of mutable.
https://codereview.chromium.org/1232033004/diff/60001/public/platform/WebPass... File public/platform/WebPassOwnPtr.h (right): https://codereview.chromium.org/1232033004/diff/60001/public/platform/WebPass... public/platform/WebPassOwnPtr.h:8: #include "WebCommon.h" On 2015/07/16 08:17:50, tkent wrote: > should be "public/platform/WebCommon.h" Done. https://codereview.chromium.org/1232033004/diff/60001/public/platform/WebPass... public/platform/WebPassOwnPtr.h:55: mutable T* m_ptr; On 2015/07/16 08:17:50, tkent wrote: > Please add a comment about the reason of mutable. > Done.
lgtm
The CQ bit was checked by yhirano@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from kinuko@chromium.org Link to the patchset: https://codereview.chromium.org/1232033004/#ps100001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1232033004/100001
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://src.chromium.org/viewvc/blink?view=rev&revision=199024 |
