WIP: WebLayer::addAnimation should transfer the ownership of WebAnimation instance.

WIP: not for review.
This CL's goal is to clarify the direction of Issue 290217

WebLayer::addAnimation should transfer the ownership of WebAnimation instance.

Introduce WebPassOwnPtr to transfer OwnPtr or PassOwnPtr from Blink to Chromium.

BUG=290217

[jamesr, 2013-09-24 17:52:10]

I don't think you will be able to write a PassOwnPtr type that works for both
WTF::OwnPtr<> -> scoped_ptr<> and scoped_ptr<> -> WTF::OwnPtr<> types in the
version of C++ we use due to weaknesses in the type system.  A type that only
works one way isn't very compelling as a platform API feature since we will
still have to use raw pointers one direction and it'll be inconsistent.

Someday, when we're using C++11 or beyond, we can implement this as a proper
move-only type.  Before then I don't think this is very useful.

https://codereview.chromium.org/23455058/diff/1/public/platform/WebPassOwnPtr.h
File public/platform/WebPassOwnPtr.h (right):

https://codereview.chromium.org/23455058/diff/1/public/platform/WebPassOwnPtr...
public/platform/WebPassOwnPtr.h:57: ~WebPassOwnPtr() { delete m_ptr; }
you can't do this without ~T(), which will make it really hard to use this type
in practice.  WebPrivatePtr<> and WebPrivateOwnPtr<> get around this by
exporting a reset() function and requiring (with an ASSERTion) that this is
called before the destructor

[dshwang, 2013-09-24 18:23:22]

> I don't think you will be able to write a PassOwnPtr type that works for both
> WTF::OwnPtr<> -> scoped_ptr<> and scoped_ptr<> -> WTF::OwnPtr<> types in the
> version of C++ we use due to weaknesses in the type system.  A type that only
> works one way isn't very compelling as a platform API feature since we will
> still have to use raw pointers one direction and it'll be inconsistent.

Thank you for your concern. However, this class does not support scoped_ptr<>.
This class only supports WTF::OwnPtr<> -> WebPassOwnPtr<>.
If we want to convert it to scoped_ptr, we should do it manually. 
e.g.
WebPassOwnPtr<Foo> bar
scoped_ptr<Foo> foo(bar.leakPtr());

It means we don't need new c++11 feature.

https://codereview.chromium.org/23455058/diff/1/public/platform/WebPassOwnPtr.h
File public/platform/WebPassOwnPtr.h (right):

https://codereview.chromium.org/23455058/diff/1/public/platform/WebPassOwnPtr...
public/platform/WebPassOwnPtr.h:57: ~WebPassOwnPtr() { delete m_ptr; }
On 2013/09/24 17:52:10, jamesr wrote:
> you can't do this without ~T(), which will make it really hard to use this
type
> in practice.  WebPrivatePtr<> and WebPrivateOwnPtr<> get around this by
> exporting a reset() function and requiring (with an ASSERTion) that this is
> called before the destructor

Could you explain why it cannot work in detail or let me know relevant document
or link? This class works very well in my machine. It transfer ownership of
WebAnimation and delete WebAnimation correctly.

In addition, we can not use reset() mechanism because it is defined under
INSIDE_WEBKIT guard.
#if INSIDE_WEBKIT
    void reset(T* ptr)
    {
        delete m_ptr;
        m_ptr = ptr;
    }
#endif

[jamesr, 2013-09-24 18:40:50]

On 2013/09/24 18:23:22, dshwang wrote:
> > I don't think you will be able to write a PassOwnPtr type that works for
both
> > WTF::OwnPtr<> -> scoped_ptr<> and scoped_ptr<> -> WTF::OwnPtr<> types in the
> > version of C++ we use due to weaknesses in the type system.  A type that
only
> > works one way isn't very compelling as a platform API feature since we will
> > still have to use raw pointers one direction and it'll be inconsistent.
> 
> Thank you for your concern. However, this class does not support scoped_ptr<>.
> This class only supports WTF::OwnPtr<> -> WebPassOwnPtr<>.
> If we want to convert it to scoped_ptr, we should do it manually. 
> e.g.
> WebPassOwnPtr<Foo> bar
> scoped_ptr<Foo> foo(bar.leakPtr());

This not not very useful.

[jamesr, 2013-09-24 20:44:51]

The double negative was not intentional.  What I meant was that requiring the
caller to adopt ownership via a leakPtr() call is *not* useful since that means
the ownership transfer is outside the type system.  Adding a type is only useful
if the type system can help you.

[dshwang, 2013-09-24 21:14:37]

On 2013/09/24 20:44:51, jamesr wrote:
> The double negative was not intentional.  What I meant was that requiring the
> caller to adopt ownership via a leakPtr() call is *not* useful since that
means
> the ownership transfer is outside the type system.  Adding a type is only
useful
> if the type system can help you.

I understand what you really want.

But I think this class still has its own value.
The goal of WebPassOwnPtr is to replace comments.

In Blink side, WebPassOwnPtr helps platform API look like transferring ownership
clearly. In addition, we can pass OwnPtr or PassOwnPtr to chromium inside type
system.
e.g. in GraphicsLayer::addAnimation
OwnPtr<WebAnimation> animation;
platformLayer()->addAnimation(animation.release());

In Chromium side, WebPassOwnPtr helps developers understand that the platform
API transfers ownership, without comment.
bool WebLayerImpl::addAnimation(WebPassOwnPtr<WebKit::WebAnimation> animation) {
  return layer_->AddAnimation(
      static_cast<WebAnimationImpl*>(animation)->PassAnimation());
}

I think it is not bad to maintain this class until we use c++11 features.

Could you reply, lastly? If you disagree, I'll handle it in the way that you
mentioned in crrev.com/23834010/#msg10.

[dshwang, 2013-09-24 21:14:38]

On 2013/09/24 20:44:51, jamesr wrote:
> The double negative was not intentional.  What I meant was that requiring the
> caller to adopt ownership via a leakPtr() call is *not* useful since that
means
> the ownership transfer is outside the type system.  Adding a type is only
useful
> if the type system can help you.

I understand what you really want.

But I think this class still has its own value.
The goal of WebPassOwnPtr is to replace comments.

In Blink side, WebPassOwnPtr helps platform API look like transferring ownership
clearly. In addition, we can pass OwnPtr or PassOwnPtr to chromium inside type
system.
e.g. in GraphicsLayer::addAnimation
OwnPtr<WebAnimation> animation;
platformLayer()->addAnimation(animation.release());

In Chromium side, WebPassOwnPtr helps developers understand that the platform
API transfers ownership, without comment.
bool WebLayerImpl::addAnimation(WebPassOwnPtr<WebKit::WebAnimation> animation) {
  return layer_->AddAnimation(
      static_cast<WebAnimationImpl*>(animation)->PassAnimation());
}

I think it is not bad to maintain this class until we use c++11 features.

Could you reply, lastly? If you disagree, I'll handle it in the way that you
mentioned in crrev.com/23834010/#msg10.

[dshwang, 2013-09-25 07:14:48]

On 2013/09/24 21:14:38, dshwang wrote:
> I understand what you really want.
> 
> But I think this class still has its own value.
> The goal of WebPassOwnPtr is to replace comments.
> 
> In Blink side, WebPassOwnPtr helps platform API look like transferring
ownership
> clearly. In addition, we can pass OwnPtr or PassOwnPtr to chromium inside type
> system.
> e.g. in GraphicsLayer::addAnimation
> OwnPtr<WebAnimation> animation;
> platformLayer()->addAnimation(animation.release());
> 
> In Chromium side, WebPassOwnPtr helps developers understand that the platform
> API transfers ownership, without comment.
> bool WebLayerImpl::addAnimation(WebPassOwnPtr<WebKit::WebAnimation> animation)
{
>   return layer_->AddAnimation(
>       static_cast<WebAnimationImpl*>(animation)->PassAnimation());
> }
> 
> I think it is not bad to maintain this class until we use c++11 features.
> 
> Could you reply, lastly? If you disagree, I'll handle it in the way that you
> mentioned in crrev.com/23834010/#msg10.

@jamesr, How do you think?

[jamesr, 2013-09-25 22:10:00]

I don't think an incomplete smart pointer type like this one is compelling
enough to make people learn.  If it actually could enforce memory safety I'd be
for it, but since it actually can't and requires developers to use manual memory
transfers via leakPtr() you should go another way.

[trchen, 2013-09-25 22:22:09]

On 2013/09/25 22:10:00, jamesr wrote:
> I don't think an incomplete smart pointer type like this one is compelling
> enough to make people learn.  If it actually could enforce memory safety I'd
be
> for it, but since it actually can't and requires developers to use manual
memory
> transfers via leakPtr() you should go another way.

Do we absolutely need to use leakPtr?
What is the drawback to provide an explicit(or implicit!) converter to
scoped_ptr?

[jamesr, 2013-09-25 22:27:00]

On 2013/09/25 22:22:09, trchen wrote:
> On 2013/09/25 22:10:00, jamesr wrote:
> > I don't think an incomplete smart pointer type like this one is compelling
> > enough to make people learn.  If it actually could enforce memory safety I'd
> be
> > for it, but since it actually can't and requires developers to use manual
> memory
> > transfers via leakPtr() you should go another way.
> 
> Do we absolutely need to use leakPtr?
> What is the drawback to provide an explicit(or implicit!) converter to
> scoped_ptr?

Can't implement that in C++98 due to restrictions on how move emulation works.

[trchen, 2013-09-25 22:49:18]

On 2013/09/25 22:27:00, jamesr wrote:
> On 2013/09/25 22:22:09, trchen wrote:
> > On 2013/09/25 22:10:00, jamesr wrote:
> > > I don't think an incomplete smart pointer type like this one is compelling
> > > enough to make people learn.  If it actually could enforce memory safety
I'd
> > be
> > > for it, but since it actually can't and requires developers to use manual
> > memory
> > > transfers via leakPtr() you should go another way.
> > 
> > Do we absolutely need to use leakPtr?
> > What is the drawback to provide an explicit(or implicit!) converter to
> > scoped_ptr?
> 
> Can't implement that in C++98 due to restrictions on how move emulation works.

I don't quite get this. What is not allowing us to do this?

template <typename T>
scoped_ptr<T> ToScoped(WebPassOwnPtr<T> &ptr) { return
scoped_ptr<T>(ptr.leakPtr()); }

WebPassOwnPtr<Foo> bar
scoped_ptr<Foo> foo(ToScoped(bar));

IMO it is much better to maintain RAII across blink boundary, even if that means
we can't enforce move-only pointers. I always have a cold sweat when I see
ownership passed around with raw pointers. Another use case is seen in
ScrollingCoordinator.cpp:209

    OwnPtr<WebScrollbarLayer> scrollbarLayer =
adoptPtr(WebKit::Platform::current()->compositorSupport()->createScrollbarLayer(new
WebKit::WebScrollbarImpl(scrollbar), painter, geometry.leakPtr()));

And look at our default implementation:
virtual WebScrollbarLayer* createScrollbarLayer(WebScrollbar*,
WebScrollbarThemePainter, WebScrollbarThemeGeometry*) { return 0; }

We leaked two objects already if the embedder doesn't provide a reasonable
implementation. :(

[dshwang, 2013-09-26 14:36:54]

On 2013/09/25 22:49:18, trchen wrote:
> On 2013/09/25 22:27:00, jamesr wrote:
> > On 2013/09/25 22:22:09, trchen wrote:
> > > On 2013/09/25 22:10:00, jamesr wrote:
> > > > I don't think an incomplete smart pointer type like this one is
compelling
> > > > enough to make people learn.  If it actually could enforce memory safety
> I'd
> > > be
> > > > for it, but since it actually can't and requires developers to use
manual
> > > memory
> > > > transfers via leakPtr() you should go another way.
> > > 
> > > Do we absolutely need to use leakPtr?
> > > What is the drawback to provide an explicit(or implicit!) converter to
> > > scoped_ptr?
> > 
> > Can't implement that in C++98 due to restrictions on how move emulation
works.
> 
> I don't quite get this. What is not allowing us to do this?
> 
> template <typename T>
> scoped_ptr<T> ToScoped(WebPassOwnPtr<T> &ptr) { return
> scoped_ptr<T>(ptr.leakPtr()); }
> 
> WebPassOwnPtr<Foo> bar
> scoped_ptr<Foo> foo(ToScoped(bar));
> 
> IMO it is much better to maintain RAII across blink boundary, even if that
means
> we can't enforce move-only pointers. I always have a cold sweat when I see
> ownership passed around with raw pointers. Another use case is seen in
> ScrollingCoordinator.cpp:209
> 
>     OwnPtr<WebScrollbarLayer> scrollbarLayer =
>
adoptPtr(WebKit::Platform::current()->compositorSupport()->createScrollbarLayer(new
> WebKit::WebScrollbarImpl(scrollbar), painter, geometry.leakPtr()));
> 
> And look at our default implementation:
> virtual WebScrollbarLayer* createScrollbarLayer(WebScrollbar*,
> WebScrollbarThemePainter, WebScrollbarThemeGeometry*) { return 0; }
> 
> We leaked two objects already if the embedder doesn't provide a reasonable
> implementation. :(

Thank you for your opinion.
I agree with you that it is better to maintain RAII across blink boundary.

It looks viable to me : scoped_ptr<T> operator in WebPassOwnPtr
for example, in WebPassOwnPtr.h
#if !defined(INSIDE_WEBKIT)
   scoped_ptr<T> operator()  { return scoped_ptr<T>(leakPtr()); }
#endif

It seems to be possible that type system can convert OwnPtr -> WebPassOwnPtr ->
scoped_ptr
although reverse converting is not supported: scoped_ptr -> OwnPtr.
I think we only need OwnPtr -> scoped_ptr. it is enough.

After I check if it works well, I'll report here. I'm OOO today.

[dshwang, 2013-09-27 09:35:17]

I understand that it is impossible that c++ type system converts
WebPassOwnPtr<T> -> scoped_ptr<T>.

However scoped_ptr<T> WebPassOwnPtr::toScopedPtr() looks good to me, as @trchen
mentioned,
For example, in chromium
bool WebLayerImpl::addAnimation(WebKit::WebPassOwnPtr<WebKit::WebAnimation>
pop_animation) {
  scoped_ptr<WebKit::WebAnimation> animation(pop_animation.toScopedPtr());
  return layer_->AddAnimation(
      static_cast<WebAnimationImpl*>(animation.get())->PassAnimation());
}

WebPassOwnPtr can clarify ownership transfer and help avoid to not free raw
pointer, although WebPassOwnPtr is not complete smart pointer in that it can not
convert WebPassOwnPtr<T> -> scoped_ptr<T> inside type system.

@jamesr could you reply your opinion again?

[trchen, 2013-09-28 01:45:25]

I have a more fundamental question. What is the historical reason to prohibit
the use of std pointers in WebKit? I understand that we still want the nice
move-sematics of PassOwnPtr and PassRefPtr internally, until we're ready for
C++11. But is there a reason why we shouldn't use std pointers in public API?

[jamesr1, 2013-09-28 04:57:14]

Which std pointers?
On Sep 27, 2013 6:45 PM, <trchen@chromium.org> wrote:

> I have a more fundamental question. What is the historical reason to
> prohibit
> the use of std pointers in WebKit? I understand that we still want the nice
> move-sematics of PassOwnPtr and PassRefPtr internally, until we're ready
> for
> C++11. But is there a reason why we shouldn't use std pointers in public
> API?
>
>
https://codereview.chromium.**org/23455058/<https://codereview.chromium.org/2...
>

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[dshwang, 2013-09-30 11:32:29]

On 2013/09/28 04:57:14, jamesr1 wrote:
> Which std pointers?
> On Sep 27, 2013 6:45 PM, <mailto:trchen@chromium.org> wrote:
> 
> > I have a more fundamental question. What is the historical reason to
> > prohibit
> > the use of std pointers in WebKit? I understand that we still want the nice
> > move-sematics of PassOwnPtr and PassRefPtr internally, until we're ready
> > for
> > C++11. But is there a reason why we shouldn't use std pointers in public
> > API?

I guess @trchen mentioned std::shared_ptr and std::unique_ptr.
It's good point. I'm also curious why we use RefPtr, OwnPtr and scoped_ptr
instead of std smart pointer.

[dshwang, 2013-10-07 11:56:42]

Hi,

We have three options. If we decide which is the best, I can resolve this issue.

1. Use raw pointer.
pros: simple
cons: easy to make a mistake to not free pointer

2. Use WebPassOwnPtr
pros: hard to make a mistake to not free pointer
cons: introduce not-complete smart pointer

3. Use std::unique_ptr
pros: hard to make a mistake to not free pointer
cons: chromium code style prevents from using unique_ptr to transfer a
ownership.

@jamesr @trchen how about your opinion?
personally, I prefer #2.

[abarth-chromium, 2013-10-07 19:12:48]

On 2013/10/07 11:56:42, dshwang wrote:
> 3. Use std::unique_ptr
> pros: hard to make a mistake to not free pointer
> cons: chromium code style prevents from using unique_ptr to transfer a
> ownership.

My understanding is that we cannot use std::unique_ptr because we do not yet
require all clients to use C++11.

[jamesr, 2013-10-07 19:37:40]

1. We can't do 3 until all of our toolchains support C++11 and 2 had too
many confusing holes.

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[trchen, 2013-10-07 21:12:32]

On 2013/10/07 19:37:40, jamesr wrote:
> 1. We can't do 3 until all of our toolchains support C++11 and 2 had too
> many confusing holes.

I still feel it is the best practice to maintain RAII. 1 is not a sound option.

However I think the issue with 2 and 3 is the DLL hell problem when passing
objects across library boundary. For chromium it is okay since we always ship
blink as an internal dependency (i.e. they are always compiled by the same
compiler and against same version of libraries), but it would be a trouble if
people want to ship blink as a system library.

Gruesome details: On Windows, different libraries can be linked against
different version of C runtime (MSVCRxx.DLL), and they can have different heap
structure. If you free a memory block using a different C runtime than where the
memory is malloc'd, boom the program crashes. Same principle applies to C++
objects. You need to delete and new object within the same link unit.

I would say my previous comment was wrong. It is not a good idea to allow
transferring ownership from WebOwnPtr to a scoped_ptr. However I still think it
is good to have smart pointers, with the premise that we explicitly instantiate
the required templates in Blink.

[dshwang, 2013-10-08 12:35:32]

On 2013/10/07 19:37:40, jamesr wrote:
> 1. We can't do 3 until all of our toolchains support C++11 and 2 had too
> many confusing holes.

Could you re-consider Patch Set 3?
There are some improvements.

USAGE
in Blink,
OwnPtr<WebAnimation> ani;
layer->addAnimation(ani.release());

in Chromium
WebPassOwnPtr<WebAnimation> pop_ani;
scoped_ptr<WebAnimation> ani(pop_ani.leakPtr());
ani->foo();

PROS.
1. it is impossible to leak memory.
If developer forgets freeing a pointer, ~WebPassOwnPtr frees the pointer.

2. the semantics of leakPtr() is familar with us.
WebPassOwnPtr::leakPtr() is the same to scoped_ptr::leakPtr(). It is hard to
forget freeing the pointer after extracting raw point using
WebPassOwnPtr::leakPtr().

3. The API looks like hard to make a mistake.
WebPassOwnPtr has only one public method: WebPassOwnPtr::leakPtr(). There are
not any getters in WebPassOwnPtr such as operator->(), operator*().
If developer want to use the pointer, developer must extract raw pointer using
WebPassOwnPtr::leakPtr().
Creating scoped_ptr using WebPassOwnPtr::leakPtr() will be our coding style.

imo, only Cons. is that we must use WebPassOwnPtr::leakPtr() to create
scoped_ptr.
However, it is self-explainable (transfer a ownership) and prevents from leaking
memory.

Can I listen to your opinion again?


On 2013/10/07 21:12:32, trchen wrote:
> I would say my previous comment was wrong. It is not a good idea to allow
> transferring ownership from WebOwnPtr to a scoped_ptr. However I still think
it
> is good to have smart pointers, with the premise that we explicitly
instantiate
> the required templates in Blink.

Thank you for your explanation. It's good to know.
imo WebPassOwnPtr in Patch Set 3 does not have any problem in that sense.

[jamesr, 2013-10-08 20:52:22]

There are still several basic issues with this:

1.) It's not symmetric, it only works for Blink->Chromium transfers.  The
asymmetry is going to confuse developers.
2.) It requires calling leakPtr() in the common operation.3
3.) It requires knowing about PassXXX semantics on the chromium side. 
Currently, PassXXXPtr semantics are purely internal to Blink and chromium uses
move emulation on scoped_ptr<>.  The chromium-side code using this interface
will look foreign and buggy to chromium developers, especially since it requires
calling leakPtr() which looks like it should leak.

It's really tempting to try to use the type system to handle memory but in cases
like this the type system we have in C++98 is simply not powerful enough to
express what we want.  Having a partial solution like this is worse than having
nothing at all since the abstraction leaks out through all the holes in the type
system which means developers have to fully understand the implementation in
order to use this thus defeating the point of having an abstraction.

This discussion is really going nowhere.  When we have C++11 or better we can do
better, until then we're stuck with raw pointers + comments.

[dshwang, 2013-10-09 10:03:42]

On 2013/10/08 20:52:22, jamesr wrote:
> This discussion is really going nowhere.  When we have C++11 or better we can
do
> better, until then we're stuck with raw pointers + comments.

Ok, I'll use raw pointers + comments. Thank you for your explanation.