cc: Move TextureMailbox::ReleaseCallback to SingleReleaseCallback.

cc: Move TextureMailbox::ReleaseCallback to SingleReleaseCallback.

This moves the release callback out of the TextureMailbox so that we
can use it for other resource types than just texture mailboxes.

While doing this, we make a SingleReleaseCallback class that is
held in a scoped_ptr. This class DCHECKs that the callback is run
before it is destroyed, and ensures clear ownership semantics as
you must Pass() the callback around.

No change in behaviour, covered by existing tests.

R=piman
BUG=263069
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=223500

[danakj, 2013-09-12 00:43:18]

I'm thinking if it'd be nice to make a ScopedTextureMailbox that wraps a TM and
a callback, and automatically erases the TM when the callback is run. Which is
what TextureLayer and TextureLayerImpl are doing, for instance.

[piman, 2013-09-12 01:13:03]

https://codereview.chromium.org/23648014/diff/1/cc/layers/texture_layer.h
File cc/layers/texture_layer.h (right):

https://codereview.chromium.org/23648014/diff/1/cc/layers/texture_layer.h#new...
cc/layers/texture_layer.h:14: #include "cc/resources/scoped_release_callback.h"
File is missing from the CL :(

[danakj, 2013-09-12 01:20:15]

https://codereview.chromium.org/23648014/diff/1/cc/layers/texture_layer.h
File cc/layers/texture_layer.h (right):

https://codereview.chromium.org/23648014/diff/1/cc/layers/texture_layer.h#new...
cc/layers/texture_layer.h:14: #include "cc/resources/scoped_release_callback.h"
On 2013/09/12 01:13:03, piman wrote:
> File is missing from the CL :(

Ooops oops, fixed!

[piman, 2013-09-13 06:08:30]

LGTM+nits. +awong to take a look at the move emulation in ScopeReleaseCallback -
I think it's right, but he knows better.

https://codereview.chromium.org/23648014/diff/8001/cc/resources/scoped_releas...
File cc/resources/scoped_release_callback.cc (right):

https://codereview.chromium.org/23648014/diff/8001/cc/resources/scoped_releas...
cc/resources/scoped_release_callback.cc:18: :
callback_(base::ResetAndReturn(&other.object->callback_)) {}
nit: callback_(other.object->Leak())

https://codereview.chromium.org/23648014/diff/8001/cc/resources/scoped_releas...
cc/resources/scoped_release_callback.cc:32: rhs.callback_ = ReleaseCallback();
nit: replace those 2 lines by callback_ = rhs.Leak(); ?

https://codereview.chromium.org/23648014/diff/8001/cc/resources/scoped_releas...
File cc/resources/scoped_release_callback.h (right):

https://codereview.chromium.org/23648014/diff/8001/cc/resources/scoped_releas...
cc/resources/scoped_release_callback.h:36: bool IsEmpty() const { return
callback_.is_null(); }
nit: is_null() ? I'm not sure 'empty' qualifies a callback well.

[awong, 2013-09-13 07:07:19]

move semantics look correct (though I don't know how I feel about seeing more
and more move-only types in the code base). 

Had some unrelated questions though.

https://codereview.chromium.org/23648014/diff/8001/cc/resources/scoped_releas...
File cc/resources/scoped_release_callback.cc (right):

https://codereview.chromium.org/23648014/diff/8001/cc/resources/scoped_releas...
cc/resources/scoped_release_callback.cc:14:
ScopedReleaseCallback::ScopedReleaseCallback(const ReleaseCallback& callback)
Doesn't this already violate the rule that there can only be 1 owner of the
ReleaseCallback?

https://codereview.chromium.org/23648014/diff/8001/cc/resources/scoped_releas...
File cc/resources/scoped_release_callback.h (right):

https://codereview.chromium.org/23648014/diff/8001/cc/resources/scoped_releas...
cc/resources/scoped_release_callback.h:14: // A ReleaseCallback that can only
have one owner. It must have RunAndReset()
???  As is, can't people just copy ReleaseCallbacks everywhere? If this is such
a hard requirement, would it be sensible to make people return only
ScopedReleaseCallbacks and then somehow hide the ReleaseCallback type?

[danakj, 2013-09-13 17:32:59]

https://codereview.chromium.org/23648014/diff/8001/cc/resources/scoped_releas...
File cc/resources/scoped_release_callback.cc (right):

https://codereview.chromium.org/23648014/diff/8001/cc/resources/scoped_releas...
cc/resources/scoped_release_callback.cc:14:
ScopedReleaseCallback::ScopedReleaseCallback(const ReleaseCallback& callback)
On 2013/09/13 07:07:19, awong wrote:
> Doesn't this already violate the rule that there can only be 1 owner of the
> ReleaseCallback?

Well, the callback has to come from somewhere. I'd expect a cc/ embedder to call
ScopedReleaseCallback(base::Bind(foo...)). What would you expect?

Also this would allow a ScopedRC to be constructed out of a Leak()

https://codereview.chromium.org/23648014/diff/8001/cc/resources/scoped_releas...
File cc/resources/scoped_release_callback.h (right):

https://codereview.chromium.org/23648014/diff/8001/cc/resources/scoped_releas...
cc/resources/scoped_release_callback.h:14: // A ReleaseCallback that can only
have one owner. It must have RunAndReset()
On 2013/09/13 07:07:19, awong wrote:
> ???  As is, can't people just copy ReleaseCallbacks everywhere?

Yes, we have been careful to ensure they are always run once thru other
mechanisms (TextureLayer::MailboxHolder or via code review.

> If this is such
> a hard requirement, would it be sensible to make people return only
> ScopedReleaseCallbacks and then somehow hide the ReleaseCallback type?

Yes.. but I have two counter examples in the code currently.

1) VideoLayerImpl receives a ReleaseCallback that it runs once for each software
resource. We could just give a vector of Scoped callbacks though.. but that's
just the code as is:
https://code.google.com/p/chromium/codesearch#chromium/src/cc/layers/video_la...

More interesting is:

2) Move only types can't go in containers, so we Leak() the callback inside
ResourceProvider so it can live in the Resource hash map. This is a case where
we want to have a separate "ReleaseCallback" type still, at least for now:
https://codereview.chromium.org/23648014/diff/8001/cc/resources/resource_prov...

But, even without these, how would you envision creating a ScopedReleaseCallback
without first making a Callback and giving it to the constructor?

[awong, 2013-09-13 17:48:56]

On 2013/09/13 17:32:59, danakj wrote:
>
https://codereview.chromium.org/23648014/diff/8001/cc/resources/scoped_releas...
> File cc/resources/scoped_release_callback.cc (right):
> 
>
https://codereview.chromium.org/23648014/diff/8001/cc/resources/scoped_releas...
> cc/resources/scoped_release_callback.cc:14:
> ScopedReleaseCallback::ScopedReleaseCallback(const ReleaseCallback& callback)
> On 2013/09/13 07:07:19, awong wrote:
> > Doesn't this already violate the rule that there can only be 1 owner of the
> > ReleaseCallback?
> 
> Well, the callback has to come from somewhere. I'd expect a cc/ embedder to
call
> ScopedReleaseCallback(base::Bind(foo...)). What would you expect?

Yes...that makes sense. I think my mistake was I though ReleaseCallback was
always supposed to be "single-owner."  But even so, this class does nothing at
compile-time or run-time to verify single user on construction.

Is the goal to reduce the locations where you need to be careful about passing
ReleaseCallbacks to just the construction of the ScopedReleaseCallback?  If
that's the case, I'm going to play devils advocate and ask if this buys enough
type safety to be worth adding an API with a number of usage caveats.


> Also this would allow a ScopedRC to be constructed out of a Leak()
> 
>
https://codereview.chromium.org/23648014/diff/8001/cc/resources/scoped_releas...
> File cc/resources/scoped_release_callback.h (right):
> 
>
https://codereview.chromium.org/23648014/diff/8001/cc/resources/scoped_releas...
> cc/resources/scoped_release_callback.h:14: // A ReleaseCallback that can only
> have one owner. It must have RunAndReset()
> On 2013/09/13 07:07:19, awong wrote:
> > ???  As is, can't people just copy ReleaseCallbacks everywhere?
> 
> Yes, we have been careful to ensure they are always run once thru other
> mechanisms (TextureLayer::MailboxHolder or via code review.

Hmm...I think I'm misunderstanding the usage. Are we worried about ensuring only
a single-run or are we worried about multiple "owners" (aka references) making
it hard to know when the bound arguments are deleted?

If it's the former case, maybe it'd make sense to introduce something that you
use when calling Bind such that a second use of the callback crashes.

If it's the latter case...then we're fighting the callback type system and I
don't have any good ideas. :(

 
> > If this is such
> > a hard requirement, would it be sensible to make people return only
> > ScopedReleaseCallbacks and then somehow hide the ReleaseCallback type?
> 
> Yes.. but I have two counter examples in the code currently.
> 
> 1) VideoLayerImpl receives a ReleaseCallback that it runs once for each
software
> resource. We could just give a vector of Scoped callbacks though.. but that's
> just the code as is:
>
https://code.google.com/p/chromium/codesearch#chromium/src/cc/layers/video_la...
> 
> More interesting is:
> 
> 2) Move only types can't go in containers, so we Leak() the callback inside
> ResourceProvider so it can live in the Resource hash map. This is a case where
> we want to have a separate "ReleaseCallback" type still, at least for now:
>
https://codereview.chromium.org/23648014/diff/8001/cc/resources/resource_prov...
> 
> But, even without these, how would you envision creating a
ScopedReleaseCallback
> without first making a Callback and giving it to the constructor?

Hmmm...I think overall my concern is there's enough of gotchas in the usage that
it's unclear to me on first glance how to use the API safely.  At least, just
based on the the ScopedReleaseCallback header alone, I would not have known what
to check for in a code review (namedly that no one else is holding onto the
callback that I'm being constructed with).

Unforutnately, thus far I also don't have better ideas.

[danakj, 2013-09-13 18:50:51]

On Fri, Sep 13, 2013 at 1:48 PM, <ajwong@chromium.org> wrote:

> On 2013/09/13 17:32:59, danakj wrote:
>
> https://codereview.chromium.**org/23648014/diff/8001/cc/**
>
resources/scoped_release_**callback.cc<https://codereview.chromium.org/23648014/diff/8001/cc/resources/scoped_release_callback.cc>
>
>> File cc/resources/scoped_release_**callback.cc (right):
>>
>
>
> https://codereview.chromium.**org/23648014/diff/8001/cc/**
>
resources/scoped_release_**callback.cc#newcode14<https://codereview.chromium.org/23648014/diff/8001/cc/resources/scoped_release_callback.cc#newcode14>
>
>> cc/resources/scoped_release_**callback.cc:14:
>> ScopedReleaseCallback::**ScopedReleaseCallback(const ReleaseCallback&
>> callback)
>> On 2013/09/13 07:07:19, awong wrote:
>> > Doesn't this already violate the rule that there can only be 1 owner of
>> the
>> > ReleaseCallback?
>>
>
>  Well, the callback has to come from somewhere. I'd expect a cc/ embedder
>> to
>>
> call
>
>> ScopedReleaseCallback(base::**Bind(foo...)). What would you expect?
>>
>
> Yes...that makes sense. I think my mistake was I though ReleaseCallback was
> always supposed to be "single-owner."  But even so, this class does
> nothing at
> compile-time or run-time to verify single user on construction.
>
> Is the goal to reduce the locations where you need to be careful about
> passing
> ReleaseCallbacks to just the construction of the ScopedReleaseCallback?  If
> that's the case, I'm going to play devils advocate and ask if this buys
> enough
> type safety to be worth adding an API with a number of usage caveats.


The goal is to cause a runtime or compile error if either of these things
occurs:
1) The owner of the callback calls it more than one time.
(base::ResetAndReturn)
2) The owner of the callback does not call it at all (DCHECK)
3) There are multiple owners of the callback. (Move-only)


>
>
>
>  Also this would allow a ScopedRC to be constructed out of a Leak()
>>
>
>
> https://codereview.chromium.**org/23648014/diff/8001/cc/**
>
resources/scoped_release_**callback.h<https://codereview.chromium.org/23648014/diff/8001/cc/resources/scoped_release_callback.h>
>
>> File cc/resources/scoped_release_**callback.h (right):
>>
>
>
> https://codereview.chromium.**org/23648014/diff/8001/cc/**
>
resources/scoped_release_**callback.h#newcode14<https://codereview.chromium.org/23648014/diff/8001/cc/resources/scoped_release_callback.h#newcode14>
>
>> cc/resources/scoped_release_**callback.h:14: // A ReleaseCallback that
>> can only
>> have one owner. It must have RunAndReset()
>> On 2013/09/13 07:07:19, awong wrote:
>> > ???  As is, can't people just copy ReleaseCallbacks everywhere?
>>
>
>  Yes, we have been careful to ensure they are always run once thru other
>> mechanisms (TextureLayer::MailboxHolder or via code review.
>>
>
> Hmm...I think I'm misunderstanding the usage. Are we worried about
> ensuring only
> a single-run or are we worried about multiple "owners" (aka references)
> making
> it hard to know when the bound arguments are deleted?
>

Every owner should call the callback before it goes away exactly once. And
there should be a single owner at any given time. Thus we ensure the
callback is always called a single time across its entire lifetime inside
of cc/

I'm not worried about the bound arguments here, the binding inside the
callback needs to worry about that if it cares (for instance, MailboxHolder
would deal with that, it produces one of these ScopedReleaseCallbacks)


>
> If it's the former case, maybe it'd make sense to introduce something that
> you
> use when calling Bind such that a second use of the callback crashes.
>
> If it's the latter case...then we're fighting the callback type system and
> I
> don't have any good ideas. :(
>
>
>
>  > If this is such
>> > a hard requirement, would it be sensible to make people return only
>> > ScopedReleaseCallbacks and then somehow hide the ReleaseCallback type?
>>
>
>  Yes.. but I have two counter examples in the code currently.
>>
>
>  1) VideoLayerImpl receives a ReleaseCallback that it runs once for each
>>
> software
>
>> resource. We could just give a vector of Scoped callbacks though.. but
>> that's
>> just the code as is:
>>
>
> https://code.google.com/p/**chromium/codesearch#chromium/**
> src/cc/layers/video_layer_**impl.cc&q=video_layer_im&sq=**
>
package:chromium&l=280<https://code.google.com/p/chromium/codesearch#chromium/src/cc/layers/video_layer_impl.cc&q=video_layer_im&sq=package:chromium&l=280>
>
>  More interesting is:
>>
>
>  2) Move only types can't go in containers, so we Leak() the callback
>> inside
>> ResourceProvider so it can live in the Resource hash map. This is a case
>> where
>> we want to have a separate "ReleaseCallback" type still, at least for now:
>>
>
> https://codereview.chromium.**org/23648014/diff/8001/cc/**
>
resources/resource_provider.cc<https://codereview.chromium.org/23648014/diff/8001/cc/resources/resource_provider.cc>
>
>  But, even without these, how would you envision creating a
>>
> ScopedReleaseCallback
>
>> without first making a Callback and giving it to the constructor?
>>
>
> Hmmm...I think overall my concern is there's enough of gotchas in the
> usage that
> it's unclear to me on first glance how to use the API safely.  At least,
> just
> based on the the ScopedReleaseCallback header alone, I would not have
> known what
> to check for in a code review (namedly that no one else is holding onto the
> callback that I'm being constructed with).
>

I will add a comment at the least.

Unforutnately, thus far I also don't have better ideas.
>
>
https://codereview.chromium.**org/23648014/<https://codereview.chromium.org/2...
>

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

[awong, 2013-09-13 19:08:29]

Brainstormy crazy idea.  Not sure it's a great idea so feel free to ignore:

// Wraps a callback with something that ensure the callback is executed
once and
// only once.  The caller is responsible for ensuring no one else has a
reference to
// the passed in callback that they can execute.
class SwayzeCallback {
 public:
  static scoped_ptr<SwayzeCallback> Create(const ReleaseCallback& baby) {
    return make_scoped_ptr(new SwayzeCallback(baby));
  }

  ~SwayzeCallback() {
     CHECK(!is_callback_in_the_corner_) << "Nobody puts (this)
ReleaseCallback in a corner!";
   }

   void Run(unsigned sync_point, bool is_lost) {
     CHECK(is_callback_in_the_corner_);
     is_callback_in_the_corner_ = false;
     callback_.Run(sync_point, is_lost);
   }

 private:
  SwayzeCallback() : is_callback_in_the_corner_(true) {}
  bool is_callback_in_the_corner_;
};

I'm trying to
  (1) avoid a new move-only type...this might not be a good goal
  (2) Focus on the "execute only once" property rather than the "single
owner" property since
       during construction you always have 2 owners even if one is just a
temporary.



On Fri, Sep 13, 2013 at 11:50 AM, Dana Jansens <danakj@chromium.org> wrote:

>
>
>
> On Fri, Sep 13, 2013 at 1:48 PM, <ajwong@chromium.org> wrote:
>
>> On 2013/09/13 17:32:59, danakj wrote:
>>
>> https://codereview.chromium.**org/23648014/diff/8001/cc/**
>>
resources/scoped_release_**callback.cc<https://codereview.chromium.org/23648014/diff/8001/cc/resources/scoped_release_callback.cc>
>>
>>> File cc/resources/scoped_release_**callback.cc (right):
>>>
>>
>>
>> https://codereview.chromium.**org/23648014/diff/8001/cc/**
>>
resources/scoped_release_**callback.cc#newcode14<https://codereview.chromium.org/23648014/diff/8001/cc/resources/scoped_release_callback.cc#newcode14>
>>
>>> cc/resources/scoped_release_**callback.cc:14:
>>> ScopedReleaseCallback::**ScopedReleaseCallback(const ReleaseCallback&
>>> callback)
>>> On 2013/09/13 07:07:19, awong wrote:
>>> > Doesn't this already violate the rule that there can only be 1 owner
>>> of the
>>> > ReleaseCallback?
>>>
>>
>>  Well, the callback has to come from somewhere. I'd expect a cc/ embedder
>>> to
>>>
>> call
>>
>>> ScopedReleaseCallback(base::**Bind(foo...)). What would you expect?
>>>
>>
>> Yes...that makes sense. I think my mistake was I though ReleaseCallback
>> was
>> always supposed to be "single-owner."  But even so, this class does
>> nothing at
>> compile-time or run-time to verify single user on construction.
>>
>> Is the goal to reduce the locations where you need to be careful about
>> passing
>> ReleaseCallbacks to just the construction of the ScopedReleaseCallback?
>>  If
>> that's the case, I'm going to play devils advocate and ask if this buys
>> enough
>> type safety to be worth adding an API with a number of usage caveats.
>
>
> The goal is to cause a runtime or compile error if either of these things
> occurs:
> 1) The owner of the callback calls it more than one time.
> (base::ResetAndReturn)
> 2) The owner of the callback does not call it at all (DCHECK)
> 3) There are multiple owners of the callback. (Move-only)
>
>
>>
>>
>>
>>  Also this would allow a ScopedRC to be constructed out of a Leak()
>>>
>>
>>
>> https://codereview.chromium.**org/23648014/diff/8001/cc/**
>>
resources/scoped_release_**callback.h<https://codereview.chromium.org/23648014/diff/8001/cc/resources/scoped_release_callback.h>
>>
>>> File cc/resources/scoped_release_**callback.h (right):
>>>
>>
>>
>> https://codereview.chromium.**org/23648014/diff/8001/cc/**
>>
resources/scoped_release_**callback.h#newcode14<https://codereview.chromium.org/23648014/diff/8001/cc/resources/scoped_release_callback.h#newcode14>
>>
>>> cc/resources/scoped_release_**callback.h:14: // A ReleaseCallback that
>>> can only
>>> have one owner. It must have RunAndReset()
>>> On 2013/09/13 07:07:19, awong wrote:
>>> > ???  As is, can't people just copy ReleaseCallbacks everywhere?
>>>
>>
>>  Yes, we have been careful to ensure they are always run once thru other
>>> mechanisms (TextureLayer::MailboxHolder or via code review.
>>>
>>
>> Hmm...I think I'm misunderstanding the usage. Are we worried about
>> ensuring only
>> a single-run or are we worried about multiple "owners" (aka references)
>> making
>> it hard to know when the bound arguments are deleted?
>>
>
> Every owner should call the callback before it goes away exactly once. And
> there should be a single owner at any given time. Thus we ensure the
> callback is always called a single time across its entire lifetime inside
> of cc/
>
> I'm not worried about the bound arguments here, the binding inside the
> callback needs to worry about that if it cares (for instance, MailboxHolder
> would deal with that, it produces one of these ScopedReleaseCallbacks)
>
>
>>
>> If it's the former case, maybe it'd make sense to introduce something
>> that you
>> use when calling Bind such that a second use of the callback crashes.
>>
>> If it's the latter case...then we're fighting the callback type system
>> and I
>> don't have any good ideas. :(
>>
>>
>>
>>  > If this is such
>>> > a hard requirement, would it be sensible to make people return only
>>> > ScopedReleaseCallbacks and then somehow hide the ReleaseCallback type?
>>>
>>
>>  Yes.. but I have two counter examples in the code currently.
>>>
>>
>>  1) VideoLayerImpl receives a ReleaseCallback that it runs once for each
>>>
>> software
>>
>>> resource. We could just give a vector of Scoped callbacks though.. but
>>> that's
>>> just the code as is:
>>>
>>
>> https://code.google.com/p/**chromium/codesearch#chromium/**
>> src/cc/layers/video_layer_**impl.cc&q=video_layer_im&sq=**
>>
package:chromium&l=280<https://code.google.com/p/chromium/codesearch#chromium/src/cc/layers/video_layer_impl.cc&q=video_layer_im&sq=package:chromium&l=280>
>>
>>  More interesting is:
>>>
>>
>>  2) Move only types can't go in containers, so we Leak() the callback
>>> inside
>>> ResourceProvider so it can live in the Resource hash map. This is a case
>>> where
>>> we want to have a separate "ReleaseCallback" type still, at least for
>>> now:
>>>
>>
>> https://codereview.chromium.**org/23648014/diff/8001/cc/**
>>
resources/resource_provider.cc<https://codereview.chromium.org/23648014/diff/8001/cc/resources/resource_provider.cc>
>>
>>  But, even without these, how would you envision creating a
>>>
>> ScopedReleaseCallback
>>
>>> without first making a Callback and giving it to the constructor?
>>>
>>
>> Hmmm...I think overall my concern is there's enough of gotchas in the
>> usage that
>> it's unclear to me on first glance how to use the API safely.  At least,
>> just
>> based on the the ScopedReleaseCallback header alone, I would not have
>> known what
>> to check for in a code review (namedly that no one else is holding onto
>> the
>> callback that I'm being constructed with).
>>
>
> I will add a comment at the least.
>
> Unforutnately, thus far I also don't have better ideas.
>>
>>
https://codereview.chromium.**org/23648014/<https://codereview.chromium.org/2...
>>
>
>

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

[danakj, 2013-09-13 19:17:15]

On Fri, Sep 13, 2013 at 3:08 PM, Albert J. Wong (王重傑)
<ajwong@chromium.org>wrote:

> Brainstormy crazy idea.  Not sure it's a great idea so feel free to ignore:
>
> // Wraps a callback with something that ensure the callback is executed
> once and
> // only once.  The caller is responsible for ensuring no one else has a
> reference to
> // the passed in callback that they can execute.
> class SwayzeCallback {
>  public:
>   static scoped_ptr<SwayzeCallback> Create(const ReleaseCallback& baby) {
>     return make_scoped_ptr(new SwayzeCallback(baby));
>   }
>
>   ~SwayzeCallback() {
>      CHECK(!is_callback_in_the_corner_) << "Nobody puts (this)
> ReleaseCallback in a corner!";
>    }
>
>    void Run(unsigned sync_point, bool is_lost) {
>      CHECK(is_callback_in_the_corner_);
>      is_callback_in_the_corner_ = false;
>      callback_.Run(sync_point, is_lost);
>    }
>
>  private:
>   SwayzeCallback() : is_callback_in_the_corner_(true) {}
>   bool is_callback_in_the_corner_;
> };
>
> I'm trying to
>   (1) avoid a new move-only type...this might not be a good goal
>

Maybe it is, I have no sense either way really.. do they give you a bad
feeling?


>   (2) Focus on the "execute only once" property rather than the "single
> owner" property since
>        during construction you always have 2 owners even if one is just a
> temporary.
>

You're absolutely right to point out the creation time weirdness. I would
like to also point out that this also has similar creation time weirdness
around the callback being run a single time. The callback may even already
have been run when it is given to this class. I think the
ownership/single-call problems have the same weakness at construction time,
so I'm not sure it's a strong argument to say that it's worth checking for
one of them but not the other?



>
>
>
> On Fri, Sep 13, 2013 at 11:50 AM, Dana Jansens <danakj@chromium.org>wrote:
>
>>
>>
>>
>> On Fri, Sep 13, 2013 at 1:48 PM, <ajwong@chromium.org> wrote:
>>
>>> On 2013/09/13 17:32:59, danakj wrote:
>>>
>>> https://codereview.chromium.**org/23648014/diff/8001/cc/**
>>>
resources/scoped_release_**callback.cc<https://codereview.chromium.org/23648014/diff/8001/cc/resources/scoped_release_callback.cc>
>>>
>>>> File cc/resources/scoped_release_**callback.cc (right):
>>>>
>>>
>>>
>>> https://codereview.chromium.**org/23648014/diff/8001/cc/**
>>>
resources/scoped_release_**callback.cc#newcode14<https://codereview.chromium.org/23648014/diff/8001/cc/resources/scoped_release_callback.cc#newcode14>
>>>
>>>> cc/resources/scoped_release_**callback.cc:14:
>>>> ScopedReleaseCallback::**ScopedReleaseCallback(const ReleaseCallback&
>>>> callback)
>>>> On 2013/09/13 07:07:19, awong wrote:
>>>> > Doesn't this already violate the rule that there can only be 1 owner
>>>> of the
>>>> > ReleaseCallback?
>>>>
>>>
>>>  Well, the callback has to come from somewhere. I'd expect a cc/
>>>> embedder to
>>>>
>>> call
>>>
>>>> ScopedReleaseCallback(base::**Bind(foo...)). What would you expect?
>>>>
>>>
>>> Yes...that makes sense. I think my mistake was I though ReleaseCallback
>>> was
>>> always supposed to be "single-owner."  But even so, this class does
>>> nothing at
>>> compile-time or run-time to verify single user on construction.
>>>
>>> Is the goal to reduce the locations where you need to be careful about
>>> passing
>>> ReleaseCallbacks to just the construction of the ScopedReleaseCallback?
>>>  If
>>> that's the case, I'm going to play devils advocate and ask if this buys
>>> enough
>>> type safety to be worth adding an API with a number of usage caveats.
>>
>>
>> The goal is to cause a runtime or compile error if either of these things
>> occurs:
>> 1) The owner of the callback calls it more than one time.
>> (base::ResetAndReturn)
>> 2) The owner of the callback does not call it at all (DCHECK)
>> 3) There are multiple owners of the callback. (Move-only)
>>
>>
>>>
>>>
>>>
>>>  Also this would allow a ScopedRC to be constructed out of a Leak()
>>>>
>>>
>>>
>>> https://codereview.chromium.**org/23648014/diff/8001/cc/**
>>>
resources/scoped_release_**callback.h<https://codereview.chromium.org/23648014/diff/8001/cc/resources/scoped_release_callback.h>
>>>
>>>> File cc/resources/scoped_release_**callback.h (right):
>>>>
>>>
>>>
>>> https://codereview.chromium.**org/23648014/diff/8001/cc/**
>>>
resources/scoped_release_**callback.h#newcode14<https://codereview.chromium.org/23648014/diff/8001/cc/resources/scoped_release_callback.h#newcode14>
>>>
>>>> cc/resources/scoped_release_**callback.h:14: // A ReleaseCallback that
>>>> can only
>>>> have one owner. It must have RunAndReset()
>>>> On 2013/09/13 07:07:19, awong wrote:
>>>> > ???  As is, can't people just copy ReleaseCallbacks everywhere?
>>>>
>>>
>>>  Yes, we have been careful to ensure they are always run once thru other
>>>> mechanisms (TextureLayer::MailboxHolder or via code review.
>>>>
>>>
>>> Hmm...I think I'm misunderstanding the usage. Are we worried about
>>> ensuring only
>>> a single-run or are we worried about multiple "owners" (aka references)
>>> making
>>> it hard to know when the bound arguments are deleted?
>>>
>>
>> Every owner should call the callback before it goes away exactly once.
>> And there should be a single owner at any given time. Thus we ensure the
>> callback is always called a single time across its entire lifetime inside
>> of cc/
>>
>> I'm not worried about the bound arguments here, the binding inside the
>> callback needs to worry about that if it cares (for instance, MailboxHolder
>> would deal with that, it produces one of these ScopedReleaseCallbacks)
>>
>>
>>>
>>> If it's the former case, maybe it'd make sense to introduce something
>>> that you
>>> use when calling Bind such that a second use of the callback crashes.
>>>
>>> If it's the latter case...then we're fighting the callback type system
>>> and I
>>> don't have any good ideas. :(
>>>
>>>
>>>
>>>  > If this is such
>>>> > a hard requirement, would it be sensible to make people return only
>>>> > ScopedReleaseCallbacks and then somehow hide the ReleaseCallback type?
>>>>
>>>
>>>  Yes.. but I have two counter examples in the code currently.
>>>>
>>>
>>>  1) VideoLayerImpl receives a ReleaseCallback that it runs once for each
>>>>
>>> software
>>>
>>>> resource. We could just give a vector of Scoped callbacks though.. but
>>>> that's
>>>> just the code as is:
>>>>
>>>
>>> https://code.google.com/p/**chromium/codesearch#chromium/**
>>> src/cc/layers/video_layer_**impl.cc&q=video_layer_im&sq=**
>>>
package:chromium&l=280<https://code.google.com/p/chromium/codesearch#chromium/src/cc/layers/video_layer_impl.cc&q=video_layer_im&sq=package:chromium&l=280>
>>>
>>>  More interesting is:
>>>>
>>>
>>>  2) Move only types can't go in containers, so we Leak() the callback
>>>> inside
>>>> ResourceProvider so it can live in the Resource hash map. This is a
>>>> case where
>>>> we want to have a separate "ReleaseCallback" type still, at least for
>>>> now:
>>>>
>>>
>>> https://codereview.chromium.**org/23648014/diff/8001/cc/**
>>>
resources/resource_provider.cc<https://codereview.chromium.org/23648014/diff/8001/cc/resources/resource_provider.cc>
>>>
>>>  But, even without these, how would you envision creating a
>>>>
>>> ScopedReleaseCallback
>>>
>>>> without first making a Callback and giving it to the constructor?
>>>>
>>>
>>> Hmmm...I think overall my concern is there's enough of gotchas in the
>>> usage that
>>> it's unclear to me on first glance how to use the API safely.  At least,
>>> just
>>> based on the the ScopedReleaseCallback header alone, I would not have
>>> known what
>>> to check for in a code review (namedly that no one else is holding onto
>>> the
>>> callback that I'm being constructed with).
>>>
>>
>> I will add a comment at the least.
>>
>> Unforutnately, thus far I also don't have better ideas.
>>>
>>>
https://codereview.chromium.**org/23648014/<https://codereview.chromium.org/2...
>>>
>>
>>
>

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

[awong, 2013-09-13 19:24:35]

On Fri, Sep 13, 2013 at 12:16 PM, Dana Jansens <danakj@chromium.org> wrote:

> On Fri, Sep 13, 2013 at 3:08 PM, Albert J. Wong (王重傑) <ajwong@chromium.org
> > wrote:
>
>> Brainstormy crazy idea.  Not sure it's a great idea so feel free to
>> ignore:
>>
>> // Wraps a callback with something that ensure the callback is executed
>> once and
>> // only once.  The caller is responsible for ensuring no one else has a
>> reference to
>> // the passed in callback that they can execute.
>> class SwayzeCallback {
>>  public:
>>   static scoped_ptr<SwayzeCallback> Create(const ReleaseCallback& baby) {
>>     return make_scoped_ptr(new SwayzeCallback(baby));
>>   }
>>
>>   ~SwayzeCallback() {
>>      CHECK(!is_callback_in_the_corner_) << "Nobody puts (this)
>> ReleaseCallback in a corner!";
>>    }
>>
>>    void Run(unsigned sync_point, bool is_lost) {
>>      CHECK(is_callback_in_the_corner_);
>>      is_callback_in_the_corner_ = false;
>>      callback_.Run(sync_point, is_lost);
>>    }
>>
>>  private:
>>   SwayzeCallback() : is_callback_in_the_corner_(true) {}
>>   bool is_callback_in_the_corner_;
>> };
>>
>> I'm trying to
>>   (1) avoid a new move-only type...this might not be a good goal
>>
>
>  Maybe it is, I have no sense either way really.. do they give you a bad
> feeling?
>

I might just be overly conservative about it. The construct is hard to
read, esp w/o real rvalue references. There are also some problems with
them. For instance, you can put them in containers (like you noticed) and
you won't be able to use your new type with Bind().  For this use case,
that seems like it's not a problem.


>
>
>>    (2) Focus on the "execute only once" property rather than the "single
>> owner" property since
>>        during construction you always have 2 owners even if one is just a
>> temporary.
>>
>
> You're absolutely right to point out the creation time weirdness. I would
> like to also point out that this also has similar creation time weirdness
> around the callback being run a single time. The callback may even already
> have been run when it is given to this class. I think the
> ownership/single-call problems have the same weakness at construction time,
> so I'm not sure it's a strong argument to say that it's worth checking for
> one of them but not the other?
>

There is a small difference in the contract.  For "execute only once" you
can clearly say executing through the handle affords you protection.  For
the "single owner", the contract seems violated immediately.

-Albert



>
>>
>>
>> On Fri, Sep 13, 2013 at 11:50 AM, Dana Jansens <danakj@chromium.org>wrote:
>>
>>>
>>>
>>>
>>> On Fri, Sep 13, 2013 at 1:48 PM, <ajwong@chromium.org> wrote:
>>>
>>>> On 2013/09/13 17:32:59, danakj wrote:
>>>>
>>>> https://codereview.chromium.**org/23648014/diff/8001/cc/**
>>>>
resources/scoped_release_**callback.cc<https://codereview.chromium.org/23648014/diff/8001/cc/resources/scoped_release_callback.cc>
>>>>
>>>>> File cc/resources/scoped_release_**callback.cc (right):
>>>>>
>>>>
>>>>
>>>> https://codereview.chromium.**org/23648014/diff/8001/cc/**
>>>>
resources/scoped_release_**callback.cc#newcode14<https://codereview.chromium.org/23648014/diff/8001/cc/resources/scoped_release_callback.cc#newcode14>
>>>>
>>>>> cc/resources/scoped_release_**callback.cc:14:
>>>>> ScopedReleaseCallback::**ScopedReleaseCallback(const ReleaseCallback&
>>>>> callback)
>>>>> On 2013/09/13 07:07:19, awong wrote:
>>>>> > Doesn't this already violate the rule that there can only be 1 owner
>>>>> of the
>>>>> > ReleaseCallback?
>>>>>
>>>>
>>>>  Well, the callback has to come from somewhere. I'd expect a cc/
>>>>> embedder to
>>>>>
>>>> call
>>>>
>>>>> ScopedReleaseCallback(base::**Bind(foo...)). What would you expect?
>>>>>
>>>>
>>>> Yes...that makes sense. I think my mistake was I though ReleaseCallback
>>>> was
>>>> always supposed to be "single-owner."  But even so, this class does
>>>> nothing at
>>>> compile-time or run-time to verify single user on construction.
>>>>
>>>> Is the goal to reduce the locations where you need to be careful about
>>>> passing
>>>> ReleaseCallbacks to just the construction of the ScopedReleaseCallback?
>>>>  If
>>>> that's the case, I'm going to play devils advocate and ask if this buys
>>>> enough
>>>> type safety to be worth adding an API with a number of usage caveats.
>>>
>>>
>>> The goal is to cause a runtime or compile error if either of these
>>> things occurs:
>>> 1) The owner of the callback calls it more than one time.
>>> (base::ResetAndReturn)
>>> 2) The owner of the callback does not call it at all (DCHECK)
>>> 3) There are multiple owners of the callback. (Move-only)
>>>
>>>
>>>>
>>>>
>>>>
>>>>  Also this would allow a ScopedRC to be constructed out of a Leak()
>>>>>
>>>>
>>>>
>>>> https://codereview.chromium.**org/23648014/diff/8001/cc/**
>>>>
resources/scoped_release_**callback.h<https://codereview.chromium.org/23648014/diff/8001/cc/resources/scoped_release_callback.h>
>>>>
>>>>> File cc/resources/scoped_release_**callback.h (right):
>>>>>
>>>>
>>>>
>>>> https://codereview.chromium.**org/23648014/diff/8001/cc/**
>>>>
resources/scoped_release_**callback.h#newcode14<https://codereview.chromium.org/23648014/diff/8001/cc/resources/scoped_release_callback.h#newcode14>
>>>>
>>>>> cc/resources/scoped_release_**callback.h:14: // A ReleaseCallback
>>>>> that can only
>>>>> have one owner. It must have RunAndReset()
>>>>> On 2013/09/13 07:07:19, awong wrote:
>>>>> > ???  As is, can't people just copy ReleaseCallbacks everywhere?
>>>>>
>>>>
>>>>  Yes, we have been careful to ensure they are always run once thru other
>>>>> mechanisms (TextureLayer::MailboxHolder or via code review.
>>>>>
>>>>
>>>> Hmm...I think I'm misunderstanding the usage. Are we worried about
>>>> ensuring only
>>>> a single-run or are we worried about multiple "owners" (aka references)
>>>> making
>>>> it hard to know when the bound arguments are deleted?
>>>>
>>>
>>> Every owner should call the callback before it goes away exactly once.
>>> And there should be a single owner at any given time. Thus we ensure the
>>> callback is always called a single time across its entire lifetime inside
>>> of cc/
>>>
>>> I'm not worried about the bound arguments here, the binding inside the
>>> callback needs to worry about that if it cares (for instance, MailboxHolder
>>> would deal with that, it produces one of these ScopedReleaseCallbacks)
>>>
>>>
>>>>
>>>> If it's the former case, maybe it'd make sense to introduce something
>>>> that you
>>>> use when calling Bind such that a second use of the callback crashes.
>>>>
>>>> If it's the latter case...then we're fighting the callback type system
>>>> and I
>>>> don't have any good ideas. :(
>>>>
>>>>
>>>>
>>>>  > If this is such
>>>>> > a hard requirement, would it be sensible to make people return only
>>>>> > ScopedReleaseCallbacks and then somehow hide the ReleaseCallback
>>>>> type?
>>>>>
>>>>
>>>>  Yes.. but I have two counter examples in the code currently.
>>>>>
>>>>
>>>>  1) VideoLayerImpl receives a ReleaseCallback that it runs once for each
>>>>>
>>>> software
>>>>
>>>>> resource. We could just give a vector of Scoped callbacks though.. but
>>>>> that's
>>>>> just the code as is:
>>>>>
>>>>
>>>> https://code.google.com/p/**chromium/codesearch#chromium/**
>>>> src/cc/layers/video_layer_**impl.cc&q=video_layer_im&sq=**
>>>>
package:chromium&l=280<https://code.google.com/p/chromium/codesearch#chromium/src/cc/layers/video_layer_impl.cc&q=video_layer_im&sq=package:chromium&l=280>
>>>>
>>>>  More interesting is:
>>>>>
>>>>
>>>>  2) Move only types can't go in containers, so we Leak() the callback
>>>>> inside
>>>>> ResourceProvider so it can live in the Resource hash map. This is a
>>>>> case where
>>>>> we want to have a separate "ReleaseCallback" type still, at least for
>>>>> now:
>>>>>
>>>>
>>>> https://codereview.chromium.**org/23648014/diff/8001/cc/**
>>>>
resources/resource_provider.cc<https://codereview.chromium.org/23648014/diff/8001/cc/resources/resource_provider.cc>
>>>>
>>>>  But, even without these, how would you envision creating a
>>>>>
>>>> ScopedReleaseCallback
>>>>
>>>>> without first making a Callback and giving it to the constructor?
>>>>>
>>>>
>>>> Hmmm...I think overall my concern is there's enough of gotchas in the
>>>> usage that
>>>> it's unclear to me on first glance how to use the API safely.  At
>>>> least, just
>>>> based on the the ScopedReleaseCallback header alone, I would not have
>>>> known what
>>>> to check for in a code review (namedly that no one else is holding onto
>>>> the
>>>> callback that I'm being constructed with).
>>>>
>>>
>>> I will add a comment at the least.
>>>
>>> Unforutnately, thus far I also don't have better ideas.
>>>>
>>>>
https://codereview.chromium.**org/23648014/<https://codereview.chromium.org/2...
>>>>
>>>
>>>
>>
>

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

[danakj, 2013-09-13 20:27:34]

On Fri, Sep 13, 2013 at 3:24 PM, Albert J. Wong (王重傑)
<ajwong@chromium.org>wrote:

> On Fri, Sep 13, 2013 at 12:16 PM, Dana Jansens <danakj@chromium.org>wrote:
>
>> On Fri, Sep 13, 2013 at 3:08 PM, Albert J. Wong (王重傑) <
>> ajwong@chromium.org> wrote:
>>
>>> Brainstormy crazy idea.  Not sure it's a great idea so feel free to
>>> ignore:
>>>
>>> // Wraps a callback with something that ensure the callback is executed
>>> once and
>>> // only once.  The caller is responsible for ensuring no one else has a
>>> reference to
>>> // the passed in callback that they can execute.
>>> class SwayzeCallback {
>>>  public:
>>>   static scoped_ptr<SwayzeCallback> Create(const ReleaseCallback& baby) {
>>>     return make_scoped_ptr(new SwayzeCallback(baby));
>>>   }
>>>
>>>   ~SwayzeCallback() {
>>>      CHECK(!is_callback_in_the_corner_) << "Nobody puts (this)
>>> ReleaseCallback in a corner!";
>>>    }
>>>
>>>    void Run(unsigned sync_point, bool is_lost) {
>>>      CHECK(is_callback_in_the_corner_);
>>>      is_callback_in_the_corner_ = false;
>>>      callback_.Run(sync_point, is_lost);
>>>    }
>>>
>>>  private:
>>>   SwayzeCallback() : is_callback_in_the_corner_(true) {}
>>>   bool is_callback_in_the_corner_;
>>> };
>>>
>>> I'm trying to
>>>   (1) avoid a new move-only type...this might not be a good goal
>>>
>>
>>  Maybe it is, I have no sense either way really.. do they give you a bad
>> feeling?
>>
>
> I might just be overly conservative about it. The construct is hard to
> read, esp w/o real rvalue references. There are also some problems with
> them. For instance, you can put them in containers (like you noticed) and
> you won't be able to use your new type with Bind().  For this use case,
> that seems like it's not a problem.
>
>
>>
>>
>>>    (2) Focus on the "execute only once" property rather than the "single
>>> owner" property since
>>>        during construction you always have 2 owners even if one is just
>>> a temporary.
>>>
>>
>> You're absolutely right to point out the creation time weirdness. I would
>> like to also point out that this also has similar creation time weirdness
>> around the callback being run a single time. The callback may even already
>> have been run when it is given to this class. I think the
>> ownership/single-call problems have the same weakness at construction time,
>> so I'm not sure it's a strong argument to say that it's worth checking for
>> one of them but not the other?
>>
>
> There is a small difference in the contract.  For "execute only once" you
> can clearly say executing through the handle affords you protection.  For
> the "single owner", the contract seems violated immediately.
>

Oh okay, thanks. I see what you mean. I guess the benefit here is we don't
have to leak and still get protection inside ResourceProvider. Maybe this
is a good idea :)


> -Albert
>
>
>
>>
>>>
>>>
>>> On Fri, Sep 13, 2013 at 11:50 AM, Dana Jansens <danakj@chromium.org>wrote:
>>>
>>>>
>>>>
>>>>
>>>> On Fri, Sep 13, 2013 at 1:48 PM, <ajwong@chromium.org> wrote:
>>>>
>>>>> On 2013/09/13 17:32:59, danakj wrote:
>>>>>
>>>>> https://codereview.chromium.**org/23648014/diff/8001/cc/**
>>>>>
resources/scoped_release_**callback.cc<https://codereview.chromium.org/23648014/diff/8001/cc/resources/scoped_release_callback.cc>
>>>>>
>>>>>> File cc/resources/scoped_release_**callback.cc (right):
>>>>>>
>>>>>
>>>>>
>>>>> https://codereview.chromium.**org/23648014/diff/8001/cc/**
>>>>>
resources/scoped_release_**callback.cc#newcode14<https://codereview.chromium.org/23648014/diff/8001/cc/resources/scoped_release_callback.cc#newcode14>
>>>>>
>>>>>> cc/resources/scoped_release_**callback.cc:14:
>>>>>> ScopedReleaseCallback::**ScopedReleaseCallback(const
>>>>>> ReleaseCallback& callback)
>>>>>> On 2013/09/13 07:07:19, awong wrote:
>>>>>> > Doesn't this already violate the rule that there can only be 1
>>>>>> owner of the
>>>>>> > ReleaseCallback?
>>>>>>
>>>>>
>>>>>  Well, the callback has to come from somewhere. I'd expect a cc/
>>>>>> embedder to
>>>>>>
>>>>> call
>>>>>
>>>>>> ScopedReleaseCallback(base::**Bind(foo...)). What would you expect?
>>>>>>
>>>>>
>>>>> Yes...that makes sense. I think my mistake was I though
>>>>> ReleaseCallback was
>>>>> always supposed to be "single-owner."  But even so, this class does
>>>>> nothing at
>>>>> compile-time or run-time to verify single user on construction.
>>>>>
>>>>> Is the goal to reduce the locations where you need to be careful about
>>>>> passing
>>>>> ReleaseCallbacks to just the construction of the
>>>>> ScopedReleaseCallback?  If
>>>>> that's the case, I'm going to play devils advocate and ask if this
>>>>> buys enough
>>>>> type safety to be worth adding an API with a number of usage caveats.
>>>>
>>>>
>>>> The goal is to cause a runtime or compile error if either of these
>>>> things occurs:
>>>> 1) The owner of the callback calls it more than one time.
>>>> (base::ResetAndReturn)
>>>> 2) The owner of the callback does not call it at all (DCHECK)
>>>> 3) There are multiple owners of the callback. (Move-only)
>>>>
>>>>
>>>>>
>>>>>
>>>>>
>>>>>  Also this would allow a ScopedRC to be constructed out of a Leak()
>>>>>>
>>>>>
>>>>>
>>>>> https://codereview.chromium.**org/23648014/diff/8001/cc/**
>>>>>
resources/scoped_release_**callback.h<https://codereview.chromium.org/23648014/diff/8001/cc/resources/scoped_release_callback.h>
>>>>>
>>>>>> File cc/resources/scoped_release_**callback.h (right):
>>>>>>
>>>>>
>>>>>
>>>>> https://codereview.chromium.**org/23648014/diff/8001/cc/**
>>>>>
resources/scoped_release_**callback.h#newcode14<https://codereview.chromium.org/23648014/diff/8001/cc/resources/scoped_release_callback.h#newcode14>
>>>>>
>>>>>> cc/resources/scoped_release_**callback.h:14: // A ReleaseCallback
>>>>>> that can only
>>>>>> have one owner. It must have RunAndReset()
>>>>>> On 2013/09/13 07:07:19, awong wrote:
>>>>>> > ???  As is, can't people just copy ReleaseCallbacks everywhere?
>>>>>>
>>>>>
>>>>>  Yes, we have been careful to ensure they are always run once thru
>>>>>> other
>>>>>> mechanisms (TextureLayer::MailboxHolder or via code review.
>>>>>>
>>>>>
>>>>> Hmm...I think I'm misunderstanding the usage. Are we worried about
>>>>> ensuring only
>>>>> a single-run or are we worried about multiple "owners" (aka
>>>>> references) making
>>>>> it hard to know when the bound arguments are deleted?
>>>>>
>>>>
>>>> Every owner should call the callback before it goes away exactly once.
>>>> And there should be a single owner at any given time. Thus we ensure the
>>>> callback is always called a single time across its entire lifetime inside
>>>> of cc/
>>>>
>>>> I'm not worried about the bound arguments here, the binding inside the
>>>> callback needs to worry about that if it cares (for instance,
MailboxHolder
>>>> would deal with that, it produces one of these ScopedReleaseCallbacks)
>>>>
>>>>
>>>>>
>>>>> If it's the former case, maybe it'd make sense to introduce something
>>>>> that you
>>>>> use when calling Bind such that a second use of the callback crashes.
>>>>>
>>>>> If it's the latter case...then we're fighting the callback type system
>>>>> and I
>>>>> don't have any good ideas. :(
>>>>>
>>>>>
>>>>>
>>>>>  > If this is such
>>>>>> > a hard requirement, would it be sensible to make people return only
>>>>>> > ScopedReleaseCallbacks and then somehow hide the ReleaseCallback
>>>>>> type?
>>>>>>
>>>>>
>>>>>  Yes.. but I have two counter examples in the code currently.
>>>>>>
>>>>>
>>>>>  1) VideoLayerImpl receives a ReleaseCallback that it runs once for
>>>>>> each
>>>>>>
>>>>> software
>>>>>
>>>>>> resource. We could just give a vector of Scoped callbacks though..
>>>>>> but that's
>>>>>> just the code as is:
>>>>>>
>>>>>
>>>>> https://code.google.com/p/**chromium/codesearch#chromium/**
>>>>> src/cc/layers/video_layer_**impl.cc&q=video_layer_im&sq=**
>>>>>
package:chromium&l=280<https://code.google.com/p/chromium/codesearch#chromium/src/cc/layers/video_layer_impl.cc&q=video_layer_im&sq=package:chromium&l=280>
>>>>>
>>>>>  More interesting is:
>>>>>>
>>>>>
>>>>>  2) Move only types can't go in containers, so we Leak() the callback
>>>>>> inside
>>>>>> ResourceProvider so it can live in the Resource hash map. This is a
>>>>>> case where
>>>>>> we want to have a separate "ReleaseCallback" type still, at least for
>>>>>> now:
>>>>>>
>>>>>
>>>>> https://codereview.chromium.**org/23648014/diff/8001/cc/**
>>>>>
resources/resource_provider.cc<https://codereview.chromium.org/23648014/diff/8001/cc/resources/resource_provider.cc>
>>>>>
>>>>>  But, even without these, how would you envision creating a
>>>>>>
>>>>> ScopedReleaseCallback
>>>>>
>>>>>> without first making a Callback and giving it to the constructor?
>>>>>>
>>>>>
>>>>> Hmmm...I think overall my concern is there's enough of gotchas in the
>>>>> usage that
>>>>> it's unclear to me on first glance how to use the API safely.  At
>>>>> least, just
>>>>> based on the the ScopedReleaseCallback header alone, I would not have
>>>>> known what
>>>>> to check for in a code review (namedly that no one else is holding
>>>>> onto the
>>>>> callback that I'm being constructed with).
>>>>>
>>>>
>>>> I will add a comment at the least.
>>>>
>>>> Unforutnately, thus far I also don't have better ideas.
>>>>>
>>>>>
https://codereview.chromium.**org/23648014/<https://codereview.chromium.org/2...
>>>>>
>>>>
>>>>
>>>
>>
>

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

[danakj, 2013-09-13 20:29:15]

On Fri, Sep 13, 2013 at 4:27 PM, Dana Jansens <danakj@chromium.org> wrote:

> On Fri, Sep 13, 2013 at 3:24 PM, Albert J. Wong (王重傑) <ajwong@chromium.org
> > wrote:
>
>> On Fri, Sep 13, 2013 at 12:16 PM, Dana Jansens <danakj@chromium.org>wrote:
>>
>>> On Fri, Sep 13, 2013 at 3:08 PM, Albert J. Wong (王重傑) <
>>> ajwong@chromium.org> wrote:
>>>
>>>> Brainstormy crazy idea.  Not sure it's a great idea so feel free to
>>>> ignore:
>>>>
>>>> // Wraps a callback with something that ensure the callback is executed
>>>> once and
>>>> // only once.  The caller is responsible for ensuring no one else has a
>>>> reference to
>>>> // the passed in callback that they can execute.
>>>> class SwayzeCallback {
>>>>  public:
>>>>   static scoped_ptr<SwayzeCallback> Create(const ReleaseCallback& baby)
>>>> {
>>>>     return make_scoped_ptr(new SwayzeCallback(baby));
>>>>   }
>>>>
>>>>   ~SwayzeCallback() {
>>>>      CHECK(!is_callback_in_the_corner_) << "Nobody puts (this)
>>>> ReleaseCallback in a corner!";
>>>>    }
>>>>
>>>>    void Run(unsigned sync_point, bool is_lost) {
>>>>      CHECK(is_callback_in_the_corner_);
>>>>      is_callback_in_the_corner_ = false;
>>>>      callback_.Run(sync_point, is_lost);
>>>>    }
>>>>
>>>>  private:
>>>>   SwayzeCallback() : is_callback_in_the_corner_(true) {}
>>>>   bool is_callback_in_the_corner_;
>>>> };
>>>>
>>>> I'm trying to
>>>>   (1) avoid a new move-only type...this might not be a good goal
>>>>
>>>
>>>  Maybe it is, I have no sense either way really.. do they give you a bad
>>> feeling?
>>>
>>
>> I might just be overly conservative about it. The construct is hard to
>> read, esp w/o real rvalue references. There are also some problems with
>> them. For instance, you can put them in containers (like you noticed) and
>> you won't be able to use your new type with Bind().  For this use case,
>> that seems like it's not a problem.
>>
>>
>>>
>>>
>>>>    (2) Focus on the "execute only once" property rather than the
>>>> "single owner" property since
>>>>        during construction you always have 2 owners even if one is just
>>>> a temporary.
>>>>
>>>
>>> You're absolutely right to point out the creation time weirdness. I
>>> would like to also point out that this also has similar creation time
>>> weirdness around the callback being run a single time. The callback may
>>> even already have been run when it is given to this class. I think the
>>> ownership/single-call problems have the same weakness at construction time,
>>> so I'm not sure it's a strong argument to say that it's worth checking for
>>> one of them but not the other?
>>>
>>
>> There is a small difference in the contract.  For "execute only once" you
>> can clearly say executing through the handle affords you protection.  For
>> the "single owner", the contract seems violated immediately.
>>
>
> Oh okay, thanks. I see what you mean. I guess the benefit here is we don't
> have to leak and still get protection inside ResourceProvider. Maybe this
> is a good idea :)
>

Hm, actually the scoped_ptr kinda defeats collections still, /o\ We'd need
to copy it out of the scoped_ptr which is maybe more awkward.


>
>
>> -Albert
>>
>>
>>
>>>
>>>>
>>>>
>>>> On Fri, Sep 13, 2013 at 11:50 AM, Dana Jansens <danakj@chromium.org>wrote:
>>>>
>>>>>
>>>>>
>>>>>
>>>>> On Fri, Sep 13, 2013 at 1:48 PM, <ajwong@chromium.org> wrote:
>>>>>
>>>>>> On 2013/09/13 17:32:59, danakj wrote:
>>>>>>
>>>>>> https://codereview.chromium.**org/23648014/diff/8001/cc/**
>>>>>>
resources/scoped_release_**callback.cc<https://codereview.chromium.org/23648014/diff/8001/cc/resources/scoped_release_callback.cc>
>>>>>>
>>>>>>> File cc/resources/scoped_release_**callback.cc (right):
>>>>>>>
>>>>>>
>>>>>>
>>>>>> https://codereview.chromium.**org/23648014/diff/8001/cc/**
>>>>>>
resources/scoped_release_**callback.cc#newcode14<https://codereview.chromium.org/23648014/diff/8001/cc/resources/scoped_release_callback.cc#newcode14>
>>>>>>
>>>>>>> cc/resources/scoped_release_**callback.cc:14:
>>>>>>> ScopedReleaseCallback::**ScopedReleaseCallback(const
>>>>>>> ReleaseCallback& callback)
>>>>>>> On 2013/09/13 07:07:19, awong wrote:
>>>>>>> > Doesn't this already violate the rule that there can only be 1
>>>>>>> owner of the
>>>>>>> > ReleaseCallback?
>>>>>>>
>>>>>>
>>>>>>  Well, the callback has to come from somewhere. I'd expect a cc/
>>>>>>> embedder to
>>>>>>>
>>>>>> call
>>>>>>
>>>>>>> ScopedReleaseCallback(base::**Bind(foo...)). What would you expect?
>>>>>>>
>>>>>>
>>>>>> Yes...that makes sense. I think my mistake was I though
>>>>>> ReleaseCallback was
>>>>>> always supposed to be "single-owner."  But even so, this class does
>>>>>> nothing at
>>>>>> compile-time or run-time to verify single user on construction.
>>>>>>
>>>>>> Is the goal to reduce the locations where you need to be careful
>>>>>> about passing
>>>>>> ReleaseCallbacks to just the construction of the
>>>>>> ScopedReleaseCallback?  If
>>>>>> that's the case, I'm going to play devils advocate and ask if this
>>>>>> buys enough
>>>>>> type safety to be worth adding an API with a number of usage caveats.
>>>>>
>>>>>
>>>>> The goal is to cause a runtime or compile error if either of these
>>>>> things occurs:
>>>>> 1) The owner of the callback calls it more than one time.
>>>>> (base::ResetAndReturn)
>>>>> 2) The owner of the callback does not call it at all (DCHECK)
>>>>> 3) There are multiple owners of the callback. (Move-only)
>>>>>
>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>  Also this would allow a ScopedRC to be constructed out of a Leak()
>>>>>>>
>>>>>>
>>>>>>
>>>>>> https://codereview.chromium.**org/23648014/diff/8001/cc/**
>>>>>>
resources/scoped_release_**callback.h<https://codereview.chromium.org/23648014/diff/8001/cc/resources/scoped_release_callback.h>
>>>>>>
>>>>>>> File cc/resources/scoped_release_**callback.h (right):
>>>>>>>
>>>>>>
>>>>>>
>>>>>> https://codereview.chromium.**org/23648014/diff/8001/cc/**
>>>>>>
resources/scoped_release_**callback.h#newcode14<https://codereview.chromium.org/23648014/diff/8001/cc/resources/scoped_release_callback.h#newcode14>
>>>>>>
>>>>>>> cc/resources/scoped_release_**callback.h:14: // A ReleaseCallback
>>>>>>> that can only
>>>>>>> have one owner. It must have RunAndReset()
>>>>>>> On 2013/09/13 07:07:19, awong wrote:
>>>>>>> > ???  As is, can't people just copy ReleaseCallbacks everywhere?
>>>>>>>
>>>>>>
>>>>>>  Yes, we have been careful to ensure they are always run once thru
>>>>>>> other
>>>>>>> mechanisms (TextureLayer::MailboxHolder or via code review.
>>>>>>>
>>>>>>
>>>>>> Hmm...I think I'm misunderstanding the usage. Are we worried about
>>>>>> ensuring only
>>>>>> a single-run or are we worried about multiple "owners" (aka
>>>>>> references) making
>>>>>> it hard to know when the bound arguments are deleted?
>>>>>>
>>>>>
>>>>> Every owner should call the callback before it goes away exactly once.
>>>>> And there should be a single owner at any given time. Thus we ensure the
>>>>> callback is always called a single time across its entire lifetime inside
>>>>> of cc/
>>>>>
>>>>> I'm not worried about the bound arguments here, the binding inside the
>>>>> callback needs to worry about that if it cares (for instance,
MailboxHolder
>>>>> would deal with that, it produces one of these ScopedReleaseCallbacks)
>>>>>
>>>>>
>>>>>>
>>>>>> If it's the former case, maybe it'd make sense to introduce something
>>>>>> that you
>>>>>> use when calling Bind such that a second use of the callback crashes.
>>>>>>
>>>>>> If it's the latter case...then we're fighting the callback type
>>>>>> system and I
>>>>>> don't have any good ideas. :(
>>>>>>
>>>>>>
>>>>>>
>>>>>>  > If this is such
>>>>>>> > a hard requirement, would it be sensible to make people return only
>>>>>>> > ScopedReleaseCallbacks and then somehow hide the ReleaseCallback
>>>>>>> type?
>>>>>>>
>>>>>>
>>>>>>  Yes.. but I have two counter examples in the code currently.
>>>>>>>
>>>>>>
>>>>>>  1) VideoLayerImpl receives a ReleaseCallback that it runs once for
>>>>>>> each
>>>>>>>
>>>>>> software
>>>>>>
>>>>>>> resource. We could just give a vector of Scoped callbacks though..
>>>>>>> but that's
>>>>>>> just the code as is:
>>>>>>>
>>>>>>
>>>>>> https://code.google.com/p/**chromium/codesearch#chromium/**
>>>>>> src/cc/layers/video_layer_**impl.cc&q=video_layer_im&sq=**
>>>>>>
package:chromium&l=280<https://code.google.com/p/chromium/codesearch#chromium/src/cc/layers/video_layer_impl.cc&q=video_layer_im&sq=package:chromium&l=280>
>>>>>>
>>>>>>  More interesting is:
>>>>>>>
>>>>>>
>>>>>>  2) Move only types can't go in containers, so we Leak() the callback
>>>>>>> inside
>>>>>>> ResourceProvider so it can live in the Resource hash map. This is a
>>>>>>> case where
>>>>>>> we want to have a separate "ReleaseCallback" type still, at least
>>>>>>> for now:
>>>>>>>
>>>>>>
>>>>>> https://codereview.chromium.**org/23648014/diff/8001/cc/**
>>>>>>
resources/resource_provider.cc<https://codereview.chromium.org/23648014/diff/8001/cc/resources/resource_provider.cc>
>>>>>>
>>>>>>  But, even without these, how would you envision creating a
>>>>>>>
>>>>>> ScopedReleaseCallback
>>>>>>
>>>>>>> without first making a Callback and giving it to the constructor?
>>>>>>>
>>>>>>
>>>>>> Hmmm...I think overall my concern is there's enough of gotchas in the
>>>>>> usage that
>>>>>> it's unclear to me on first glance how to use the API safely.  At
>>>>>> least, just
>>>>>> based on the the ScopedReleaseCallback header alone, I would not have
>>>>>> known what
>>>>>> to check for in a code review (namedly that no one else is holding
>>>>>> onto the
>>>>>> callback that I'm being constructed with).
>>>>>>
>>>>>
>>>>> I will add a comment at the least.
>>>>>
>>>>> Unforutnately, thus far I also don't have better ideas.
>>>>>>
>>>>>>
https://codereview.chromium.**org/23648014/<https://codereview.chromium.org/2...
>>>>>>
>>>>>
>>>>>
>>>>
>>>
>>
>

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

[awong, 2013-09-13 20:33:26]

On Fri, Sep 13, 2013 at 1:28 PM, Dana Jansens <danakj@chromium.org> wrote:

> On Fri, Sep 13, 2013 at 4:27 PM, Dana Jansens <danakj@chromium.org> wrote:
>
>> On Fri, Sep 13, 2013 at 3:24 PM, Albert J. Wong (王重傑) <
>> ajwong@chromium.org> wrote:
>>
>>> On Fri, Sep 13, 2013 at 12:16 PM, Dana Jansens <danakj@chromium.org>wrote:
>>>
>>>> On Fri, Sep 13, 2013 at 3:08 PM, Albert J. Wong (王重傑) <
>>>> ajwong@chromium.org> wrote:
>>>>
>>>>> Brainstormy crazy idea.  Not sure it's a great idea so feel free to
>>>>> ignore:
>>>>>
>>>>> // Wraps a callback with something that ensure the callback is
>>>>> executed once and
>>>>> // only once.  The caller is responsible for ensuring no one else has
>>>>> a reference to
>>>>> // the passed in callback that they can execute.
>>>>> class SwayzeCallback {
>>>>>  public:
>>>>>   static scoped_ptr<SwayzeCallback> Create(const ReleaseCallback&
>>>>> baby) {
>>>>>     return make_scoped_ptr(new SwayzeCallback(baby));
>>>>>   }
>>>>>
>>>>>   ~SwayzeCallback() {
>>>>>      CHECK(!is_callback_in_the_corner_) << "Nobody puts (this)
>>>>> ReleaseCallback in a corner!";
>>>>>    }
>>>>>
>>>>>    void Run(unsigned sync_point, bool is_lost) {
>>>>>      CHECK(is_callback_in_the_corner_);
>>>>>      is_callback_in_the_corner_ = false;
>>>>>      callback_.Run(sync_point, is_lost);
>>>>>    }
>>>>>
>>>>>  private:
>>>>>   SwayzeCallback() : is_callback_in_the_corner_(true) {}
>>>>>   bool is_callback_in_the_corner_;
>>>>> };
>>>>>
>>>>> I'm trying to
>>>>>   (1) avoid a new move-only type...this might not be a good goal
>>>>>
>>>>
>>>>  Maybe it is, I have no sense either way really.. do they give you a
>>>> bad feeling?
>>>>
>>>
>>> I might just be overly conservative about it. The construct is hard to
>>> read, esp w/o real rvalue references. There are also some problems with
>>> them. For instance, you can put them in containers (like you noticed) and
>>> you won't be able to use your new type with Bind().  For this use case,
>>> that seems like it's not a problem.
>>>
>>>
>>>>
>>>>
>>>>>    (2) Focus on the "execute only once" property rather than the
>>>>> "single owner" property since
>>>>>        during construction you always have 2 owners even if one is
>>>>> just a temporary.
>>>>>
>>>>
>>>> You're absolutely right to point out the creation time weirdness. I
>>>> would like to also point out that this also has similar creation time
>>>> weirdness around the callback being run a single time. The callback may
>>>> even already have been run when it is given to this class. I think the
>>>> ownership/single-call problems have the same weakness at construction
time,
>>>> so I'm not sure it's a strong argument to say that it's worth checking for
>>>> one of them but not the other?
>>>>
>>>
>>> There is a small difference in the contract.  For "execute only once"
>>> you can clearly say executing through the handle affords you protection.
>>>  For the "single owner", the contract seems violated immediately.
>>>
>>
>> Oh okay, thanks. I see what you mean. I guess the benefit here is we
>> don't have to leak and still get protection inside ResourceProvider. Maybe
>> this is a good idea :)
>>
>
> Hm, actually the scoped_ptr kinda defeats collections still, /o\ We'd need
> to copy it out of the scoped_ptr which is maybe more awkward.
>


 static ReleaseCallback Create(const ReleaseCallback& baby) {
    return Bind(&SwayzeCallback::Run, Owned(new SwayzeCallback(baby)));
  }

?


>
>>
>>
>>>  -Albert
>>>
>>>
>>>
>>>>
>>>>>
>>>>>
>>>>> On Fri, Sep 13, 2013 at 11:50 AM, Dana Jansens
<danakj@chromium.org>wrote:
>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>> On Fri, Sep 13, 2013 at 1:48 PM, <ajwong@chromium.org> wrote:
>>>>>>
>>>>>>> On 2013/09/13 17:32:59, danakj wrote:
>>>>>>>
>>>>>>> https://codereview.chromium.**org/23648014/diff/8001/cc/**
>>>>>>>
resources/scoped_release_**callback.cc<https://codereview.chromium.org/23648014/diff/8001/cc/resources/scoped_release_callback.cc>
>>>>>>>
>>>>>>>> File cc/resources/scoped_release_**callback.cc (right):
>>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> https://codereview.chromium.**org/23648014/diff/8001/cc/**
>>>>>>>
resources/scoped_release_**callback.cc#newcode14<https://codereview.chromium.org/23648014/diff/8001/cc/resources/scoped_release_callback.cc#newcode14>
>>>>>>>
>>>>>>>> cc/resources/scoped_release_**callback.cc:14:
>>>>>>>> ScopedReleaseCallback::**ScopedReleaseCallback(const
>>>>>>>> ReleaseCallback& callback)
>>>>>>>> On 2013/09/13 07:07:19, awong wrote:
>>>>>>>> > Doesn't this already violate the rule that there can only be 1
>>>>>>>> owner of the
>>>>>>>> > ReleaseCallback?
>>>>>>>>
>>>>>>>
>>>>>>>  Well, the callback has to come from somewhere. I'd expect a cc/
>>>>>>>> embedder to
>>>>>>>>
>>>>>>> call
>>>>>>>
>>>>>>>> ScopedReleaseCallback(base::**Bind(foo...)). What would you expect?
>>>>>>>>
>>>>>>>
>>>>>>> Yes...that makes sense. I think my mistake was I though
>>>>>>> ReleaseCallback was
>>>>>>> always supposed to be "single-owner."  But even so, this class does
>>>>>>> nothing at
>>>>>>> compile-time or run-time to verify single user on construction.
>>>>>>>
>>>>>>> Is the goal to reduce the locations where you need to be careful
>>>>>>> about passing
>>>>>>> ReleaseCallbacks to just the construction of the
>>>>>>> ScopedReleaseCallback?  If
>>>>>>> that's the case, I'm going to play devils advocate and ask if this
>>>>>>> buys enough
>>>>>>> type safety to be worth adding an API with a number of usage caveats.
>>>>>>
>>>>>>
>>>>>> The goal is to cause a runtime or compile error if either of these
>>>>>> things occurs:
>>>>>> 1) The owner of the callback calls it more than one time.
>>>>>> (base::ResetAndReturn)
>>>>>> 2) The owner of the callback does not call it at all (DCHECK)
>>>>>> 3) There are multiple owners of the callback. (Move-only)
>>>>>>
>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>  Also this would allow a ScopedRC to be constructed out of a Leak()
>>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> https://codereview.chromium.**org/23648014/diff/8001/cc/**
>>>>>>>
resources/scoped_release_**callback.h<https://codereview.chromium.org/23648014/diff/8001/cc/resources/scoped_release_callback.h>
>>>>>>>
>>>>>>>> File cc/resources/scoped_release_**callback.h (right):
>>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> https://codereview.chromium.**org/23648014/diff/8001/cc/**
>>>>>>>
resources/scoped_release_**callback.h#newcode14<https://codereview.chromium.org/23648014/diff/8001/cc/resources/scoped_release_callback.h#newcode14>
>>>>>>>
>>>>>>>> cc/resources/scoped_release_**callback.h:14: // A ReleaseCallback
>>>>>>>> that can only
>>>>>>>> have one owner. It must have RunAndReset()
>>>>>>>> On 2013/09/13 07:07:19, awong wrote:
>>>>>>>> > ???  As is, can't people just copy ReleaseCallbacks everywhere?
>>>>>>>>
>>>>>>>
>>>>>>>  Yes, we have been careful to ensure they are always run once thru
>>>>>>>> other
>>>>>>>> mechanisms (TextureLayer::MailboxHolder or via code review.
>>>>>>>>
>>>>>>>
>>>>>>> Hmm...I think I'm misunderstanding the usage. Are we worried about
>>>>>>> ensuring only
>>>>>>> a single-run or are we worried about multiple "owners" (aka
>>>>>>> references) making
>>>>>>> it hard to know when the bound arguments are deleted?
>>>>>>>
>>>>>>
>>>>>> Every owner should call the callback before it goes away exactly
>>>>>> once. And there should be a single owner at any given time. Thus we
ensure
>>>>>> the callback is always called a single time across its entire lifetime
>>>>>> inside of cc/
>>>>>>
>>>>>> I'm not worried about the bound arguments here, the binding inside
>>>>>> the callback needs to worry about that if it cares (for instance,
>>>>>> MailboxHolder would deal with that, it produces one of these
>>>>>> ScopedReleaseCallbacks)
>>>>>>
>>>>>>
>>>>>>>
>>>>>>> If it's the former case, maybe it'd make sense to introduce
>>>>>>> something that you
>>>>>>> use when calling Bind such that a second use of the callback crashes.
>>>>>>>
>>>>>>> If it's the latter case...then we're fighting the callback type
>>>>>>> system and I
>>>>>>> don't have any good ideas. :(
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>  > If this is such
>>>>>>>> > a hard requirement, would it be sensible to make people return
>>>>>>>> only
>>>>>>>> > ScopedReleaseCallbacks and then somehow hide the ReleaseCallback
>>>>>>>> type?
>>>>>>>>
>>>>>>>
>>>>>>>  Yes.. but I have two counter examples in the code currently.
>>>>>>>>
>>>>>>>
>>>>>>>  1) VideoLayerImpl receives a ReleaseCallback that it runs once for
>>>>>>>> each
>>>>>>>>
>>>>>>> software
>>>>>>>
>>>>>>>> resource. We could just give a vector of Scoped callbacks though..
>>>>>>>> but that's
>>>>>>>> just the code as is:
>>>>>>>>
>>>>>>>
>>>>>>> https://code.google.com/p/**chromium/codesearch#chromium/**
>>>>>>> src/cc/layers/video_layer_**impl.cc&q=video_layer_im&sq=**
>>>>>>>
package:chromium&l=280<https://code.google.com/p/chromium/codesearch#chromium/src/cc/layers/video_layer_impl.cc&q=video_layer_im&sq=package:chromium&l=280>
>>>>>>>
>>>>>>>  More interesting is:
>>>>>>>>
>>>>>>>
>>>>>>>  2) Move only types can't go in containers, so we Leak() the
>>>>>>>> callback inside
>>>>>>>> ResourceProvider so it can live in the Resource hash map. This is a
>>>>>>>> case where
>>>>>>>> we want to have a separate "ReleaseCallback" type still, at least
>>>>>>>> for now:
>>>>>>>>
>>>>>>>
>>>>>>> https://codereview.chromium.**org/23648014/diff/8001/cc/**
>>>>>>>
resources/resource_provider.cc<https://codereview.chromium.org/23648014/diff/8001/cc/resources/resource_provider.cc>
>>>>>>>
>>>>>>>  But, even without these, how would you envision creating a
>>>>>>>>
>>>>>>> ScopedReleaseCallback
>>>>>>>
>>>>>>>> without first making a Callback and giving it to the constructor?
>>>>>>>>
>>>>>>>
>>>>>>> Hmmm...I think overall my concern is there's enough of gotchas in
>>>>>>> the usage that
>>>>>>> it's unclear to me on first glance how to use the API safely.  At
>>>>>>> least, just
>>>>>>> based on the the ScopedReleaseCallback header alone, I would not
>>>>>>> have known what
>>>>>>> to check for in a code review (namedly that no one else is holding
>>>>>>> onto the
>>>>>>> callback that I'm being constructed with).
>>>>>>>
>>>>>>
>>>>>> I will add a comment at the least.
>>>>>>
>>>>>> Unforutnately, thus far I also don't have better ideas.
>>>>>>>
>>>>>>>
https://codereview.chromium.**org/23648014/<https://codereview.chromium.org/2...
>>>>>>>
>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>

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

[danakj, 2013-09-13 20:38:26]

On Fri, Sep 13, 2013 at 4:33 PM, Albert J. Wong (王重傑)
<ajwong@chromium.org>wrote:

>
>
>
> On Fri, Sep 13, 2013 at 1:28 PM, Dana Jansens <danakj@chromium.org> wrote:
>
>> On Fri, Sep 13, 2013 at 4:27 PM, Dana Jansens <danakj@chromium.org>wrote:
>>
>>> On Fri, Sep 13, 2013 at 3:24 PM, Albert J. Wong (王重傑) <
>>> ajwong@chromium.org> wrote:
>>>
>>>> On Fri, Sep 13, 2013 at 12:16 PM, Dana Jansens <danakj@chromium.org>wrote:
>>>>
>>>>> On Fri, Sep 13, 2013 at 3:08 PM, Albert J. Wong (王重傑) <
>>>>> ajwong@chromium.org> wrote:
>>>>>
>>>>>> Brainstormy crazy idea.  Not sure it's a great idea so feel free to
>>>>>> ignore:
>>>>>>
>>>>>> // Wraps a callback with something that ensure the callback is
>>>>>> executed once and
>>>>>> // only once.  The caller is responsible for ensuring no one else has
>>>>>> a reference to
>>>>>> // the passed in callback that they can execute.
>>>>>> class SwayzeCallback {
>>>>>>  public:
>>>>>>   static scoped_ptr<SwayzeCallback> Create(const ReleaseCallback&
>>>>>> baby) {
>>>>>>     return make_scoped_ptr(new SwayzeCallback(baby));
>>>>>>   }
>>>>>>
>>>>>>   ~SwayzeCallback() {
>>>>>>      CHECK(!is_callback_in_the_corner_) << "Nobody puts (this)
>>>>>> ReleaseCallback in a corner!";
>>>>>>    }
>>>>>>
>>>>>>    void Run(unsigned sync_point, bool is_lost) {
>>>>>>      CHECK(is_callback_in_the_corner_);
>>>>>>      is_callback_in_the_corner_ = false;
>>>>>>      callback_.Run(sync_point, is_lost);
>>>>>>    }
>>>>>>
>>>>>>  private:
>>>>>>   SwayzeCallback() : is_callback_in_the_corner_(true) {}
>>>>>>   bool is_callback_in_the_corner_;
>>>>>> };
>>>>>>
>>>>>> I'm trying to
>>>>>>   (1) avoid a new move-only type...this might not be a good goal
>>>>>>
>>>>>
>>>>>  Maybe it is, I have no sense either way really.. do they give you a
>>>>> bad feeling?
>>>>>
>>>>
>>>> I might just be overly conservative about it. The construct is hard to
>>>> read, esp w/o real rvalue references. There are also some problems with
>>>> them. For instance, you can put them in containers (like you noticed) and
>>>> you won't be able to use your new type with Bind().  For this use case,
>>>> that seems like it's not a problem.
>>>>
>>>>
>>>>>
>>>>>
>>>>>>    (2) Focus on the "execute only once" property rather than the
>>>>>> "single owner" property since
>>>>>>        during construction you always have 2 owners even if one is
>>>>>> just a temporary.
>>>>>>
>>>>>
>>>>> You're absolutely right to point out the creation time weirdness. I
>>>>> would like to also point out that this also has similar creation time
>>>>> weirdness around the callback being run a single time. The callback may
>>>>> even already have been run when it is given to this class. I think the
>>>>> ownership/single-call problems have the same weakness at construction
time,
>>>>> so I'm not sure it's a strong argument to say that it's worth checking
for
>>>>> one of them but not the other?
>>>>>
>>>>
>>>> There is a small difference in the contract.  For "execute only once"
>>>> you can clearly say executing through the handle affords you protection.
>>>>  For the "single owner", the contract seems violated immediately.
>>>>
>>>
>>> Oh okay, thanks. I see what you mean. I guess the benefit here is we
>>> don't have to leak and still get protection inside ResourceProvider. Maybe
>>> this is a good idea :)
>>>
>>
>> Hm, actually the scoped_ptr kinda defeats collections still, /o\ We'd
>> need to copy it out of the scoped_ptr which is maybe more awkward.
>>
>
>
>  static ReleaseCallback Create(const ReleaseCallback& baby) {
>     return Bind(&SwayzeCallback::Run, Owned(new SwayzeCallback(baby)));
>   }
>
> ?
>

Ohhh, one must go deeper. I see! Yes of course. :)

If we're okay with throwing single-ownership out with the bath water, this
would be reasonable I think.



>
>>
>>>
>>>
>>>>  -Albert
>>>>
>>>>
>>>>
>>>>>
>>>>>>
>>>>>>
>>>>>> On Fri, Sep 13, 2013 at 11:50 AM, Dana Jansens
<danakj@chromium.org>wrote:
>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> On Fri, Sep 13, 2013 at 1:48 PM, <ajwong@chromium.org> wrote:
>>>>>>>
>>>>>>>> On 2013/09/13 17:32:59, danakj wrote:
>>>>>>>>
>>>>>>>> https://codereview.chromium.**org/23648014/diff/8001/cc/**
>>>>>>>>
resources/scoped_release_**callback.cc<https://codereview.chromium.org/23648014/diff/8001/cc/resources/scoped_release_callback.cc>
>>>>>>>>
>>>>>>>>> File cc/resources/scoped_release_**callback.cc (right):
>>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> https://codereview.chromium.**org/23648014/diff/8001/cc/**
>>>>>>>>
resources/scoped_release_**callback.cc#newcode14<https://codereview.chromium.org/23648014/diff/8001/cc/resources/scoped_release_callback.cc#newcode14>
>>>>>>>>
>>>>>>>>> cc/resources/scoped_release_**callback.cc:14:
>>>>>>>>> ScopedReleaseCallback::**ScopedReleaseCallback(const
>>>>>>>>> ReleaseCallback& callback)
>>>>>>>>> On 2013/09/13 07:07:19, awong wrote:
>>>>>>>>> > Doesn't this already violate the rule that there can only be 1
>>>>>>>>> owner of the
>>>>>>>>> > ReleaseCallback?
>>>>>>>>>
>>>>>>>>
>>>>>>>>  Well, the callback has to come from somewhere. I'd expect a cc/
>>>>>>>>> embedder to
>>>>>>>>>
>>>>>>>> call
>>>>>>>>
>>>>>>>>> ScopedReleaseCallback(base::**Bind(foo...)). What would you
>>>>>>>>> expect?
>>>>>>>>>
>>>>>>>>
>>>>>>>> Yes...that makes sense. I think my mistake was I though
>>>>>>>> ReleaseCallback was
>>>>>>>> always supposed to be "single-owner."  But even so, this class does
>>>>>>>> nothing at
>>>>>>>> compile-time or run-time to verify single user on construction.
>>>>>>>>
>>>>>>>> Is the goal to reduce the locations where you need to be careful
>>>>>>>> about passing
>>>>>>>> ReleaseCallbacks to just the construction of the
>>>>>>>> ScopedReleaseCallback?  If
>>>>>>>> that's the case, I'm going to play devils advocate and ask if this
>>>>>>>> buys enough
>>>>>>>> type safety to be worth adding an API with a number of usage
>>>>>>>> caveats.
>>>>>>>
>>>>>>>
>>>>>>> The goal is to cause a runtime or compile error if either of these
>>>>>>> things occurs:
>>>>>>> 1) The owner of the callback calls it more than one time.
>>>>>>> (base::ResetAndReturn)
>>>>>>> 2) The owner of the callback does not call it at all (DCHECK)
>>>>>>> 3) There are multiple owners of the callback. (Move-only)
>>>>>>>
>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>  Also this would allow a ScopedRC to be constructed out of a Leak()
>>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> https://codereview.chromium.**org/23648014/diff/8001/cc/**
>>>>>>>>
resources/scoped_release_**callback.h<https://codereview.chromium.org/23648014/diff/8001/cc/resources/scoped_release_callback.h>
>>>>>>>>
>>>>>>>>> File cc/resources/scoped_release_**callback.h (right):
>>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> https://codereview.chromium.**org/23648014/diff/8001/cc/**
>>>>>>>>
resources/scoped_release_**callback.h#newcode14<https://codereview.chromium.org/23648014/diff/8001/cc/resources/scoped_release_callback.h#newcode14>
>>>>>>>>
>>>>>>>>> cc/resources/scoped_release_**callback.h:14: // A ReleaseCallback
>>>>>>>>> that can only
>>>>>>>>> have one owner. It must have RunAndReset()
>>>>>>>>> On 2013/09/13 07:07:19, awong wrote:
>>>>>>>>> > ???  As is, can't people just copy ReleaseCallbacks everywhere?
>>>>>>>>>
>>>>>>>>
>>>>>>>>  Yes, we have been careful to ensure they are always run once thru
>>>>>>>>> other
>>>>>>>>> mechanisms (TextureLayer::MailboxHolder or via code review.
>>>>>>>>>
>>>>>>>>
>>>>>>>> Hmm...I think I'm misunderstanding the usage. Are we worried about
>>>>>>>> ensuring only
>>>>>>>> a single-run or are we worried about multiple "owners" (aka
>>>>>>>> references) making
>>>>>>>> it hard to know when the bound arguments are deleted?
>>>>>>>>
>>>>>>>
>>>>>>> Every owner should call the callback before it goes away exactly
>>>>>>> once. And there should be a single owner at any given time. Thus we
ensure
>>>>>>> the callback is always called a single time across its entire lifetime
>>>>>>> inside of cc/
>>>>>>>
>>>>>>> I'm not worried about the bound arguments here, the binding inside
>>>>>>> the callback needs to worry about that if it cares (for instance,
>>>>>>> MailboxHolder would deal with that, it produces one of these
>>>>>>> ScopedReleaseCallbacks)
>>>>>>>
>>>>>>>
>>>>>>>>
>>>>>>>> If it's the former case, maybe it'd make sense to introduce
>>>>>>>> something that you
>>>>>>>> use when calling Bind such that a second use of the callback
>>>>>>>> crashes.
>>>>>>>>
>>>>>>>> If it's the latter case...then we're fighting the callback type
>>>>>>>> system and I
>>>>>>>> don't have any good ideas. :(
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>  > If this is such
>>>>>>>>> > a hard requirement, would it be sensible to make people return
>>>>>>>>> only
>>>>>>>>> > ScopedReleaseCallbacks and then somehow hide the ReleaseCallback
>>>>>>>>> type?
>>>>>>>>>
>>>>>>>>
>>>>>>>>  Yes.. but I have two counter examples in the code currently.
>>>>>>>>>
>>>>>>>>
>>>>>>>>  1) VideoLayerImpl receives a ReleaseCallback that it runs once for
>>>>>>>>> each
>>>>>>>>>
>>>>>>>> software
>>>>>>>>
>>>>>>>>> resource. We could just give a vector of Scoped callbacks though..
>>>>>>>>> but that's
>>>>>>>>> just the code as is:
>>>>>>>>>
>>>>>>>>
>>>>>>>> https://code.google.com/p/**chromium/codesearch#chromium/**
>>>>>>>> src/cc/layers/video_layer_**impl.cc&q=video_layer_im&sq=**
>>>>>>>>
package:chromium&l=280<https://code.google.com/p/chromium/codesearch#chromium/src/cc/layers/video_layer_impl.cc&q=video_layer_im&sq=package:chromium&l=280>
>>>>>>>>
>>>>>>>>  More interesting is:
>>>>>>>>>
>>>>>>>>
>>>>>>>>  2) Move only types can't go in containers, so we Leak() the
>>>>>>>>> callback inside
>>>>>>>>> ResourceProvider so it can live in the Resource hash map. This is
>>>>>>>>> a case where
>>>>>>>>> we want to have a separate "ReleaseCallback" type still, at least
>>>>>>>>> for now:
>>>>>>>>>
>>>>>>>>
>>>>>>>> https://codereview.chromium.**org/23648014/diff/8001/cc/**
>>>>>>>>
resources/resource_provider.cc<https://codereview.chromium.org/23648014/diff/8001/cc/resources/resource_provider.cc>
>>>>>>>>
>>>>>>>>  But, even without these, how would you envision creating a
>>>>>>>>>
>>>>>>>> ScopedReleaseCallback
>>>>>>>>
>>>>>>>>> without first making a Callback and giving it to the constructor?
>>>>>>>>>
>>>>>>>>
>>>>>>>> Hmmm...I think overall my concern is there's enough of gotchas in
>>>>>>>> the usage that
>>>>>>>> it's unclear to me on first glance how to use the API safely.  At
>>>>>>>> least, just
>>>>>>>> based on the the ScopedReleaseCallback header alone, I would not
>>>>>>>> have known what
>>>>>>>> to check for in a code review (namedly that no one else is holding
>>>>>>>> onto the
>>>>>>>> callback that I'm being constructed with).
>>>>>>>>
>>>>>>>
>>>>>>> I will add a comment at the least.
>>>>>>>
>>>>>>> Unforutnately, thus far I also don't have better ideas.
>>>>>>>>
>>>>>>>>
https://codereview.chromium.**org/23648014/<https://codereview.chromium.org/2...
>>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>

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

[danakj, 2013-09-14 00:14:03]

PTAL. Here's how it looks with scoped_ptr<ScopedReleaseCallback>.

Also, i fixed all the code outside cc/

[danakj, 2013-09-14 01:30:07]

rebased

[piman, 2013-09-14 01:51:33]

lgtm

https://codereview.chromium.org/23648014/diff/28001/content/renderer/pepper/p...
File content/renderer/pepper/pepper_graphics_2d_host.cc (right):

https://codereview.chromium.org/23648014/diff/28001/content/renderer/pepper/p...
content/renderer/pepper/pepper_graphics_2d_host.cc:579: *mailbox =
cc::TextureMailbox(mem, pixel_image_size);
nit: you can now replace mem by memory.get(), saving one line and an ugly
abbreviated name.

https://codereview.chromium.org/23648014/diff/30001/cc/layers/texture_layer_u...
File cc/layers/texture_layer_unittest.cc (right):

https://codereview.chromium.org/23648014/diff/30001/cc/layers/texture_layer_u...
cc/layers/texture_layer_unittest.cc:1536: cc::TextureMailbox* mailbox, bool
use_shared_memory) OVERRIDE {
post-rebase, you need to fix this one, and the other one below...

[danakj, 2013-09-16 16:42:20]

https://codereview.chromium.org/23648014/diff/28001/content/renderer/pepper/p...
File content/renderer/pepper/pepper_graphics_2d_host.cc (right):

https://codereview.chromium.org/23648014/diff/28001/content/renderer/pepper/p...
content/renderer/pepper/pepper_graphics_2d_host.cc:579: *mailbox =
cc::TextureMailbox(mem, pixel_image_size);
On 2013/09/14 01:51:33, piman wrote:
> nit: you can now replace mem by memory.get(), saving one line and an ugly
> abbreviated name.

Done.

https://codereview.chromium.org/23648014/diff/30001/cc/layers/texture_layer_u...
File cc/layers/texture_layer_unittest.cc (right):

https://codereview.chromium.org/23648014/diff/30001/cc/layers/texture_layer_u...
cc/layers/texture_layer_unittest.cc:1536: cc::TextureMailbox* mailbox, bool
use_shared_memory) OVERRIDE {
On 2013/09/14 01:51:33, piman wrote:
> post-rebase, you need to fix this one, and the other one below...

Done.

[commit-bot: I haz the power, 2013-09-16 16:42:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/danakj@chromium.org/23648014/7001

[danakj, 2013-09-16 16:43:01]

+enne/jamesr for webkit/

[enne (OOO), 2013-09-16 21:01:01]

webkit/ lgtm rubberstamp

[commit-bot: I haz the power, 2013-09-16 21:03:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/danakj@chromium.org/23648014/75001

[commit-bot: I haz the power, 2013-09-16 21:22:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/danakj@chromium.org/23648014/85001

[commit-bot: I haz the power, 2013-09-17 01:24:18]

Change committed as 223500