Add Reset to ScopedClosureRunner.

Move ScopedClosureRunner to callback_helpers, add Reset.

BUG=none
TEST=none
TBR=ben@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=220473

[Avi (use Gerrit), 2013-08-29 18:46:02]

ajwong: review
caitkp: so you don't have to do that horrible scoped_ptr<ScopedClosureRunner>
bit in your sample code

[awong, 2013-08-29 18:57:12]

https://codereview.chromium.org/23514018/diff/1/base/bind_helpers.cc
File base/bind_helpers.cc (right):

https://codereview.chromium.org/23514018/diff/1/base/bind_helpers.cc#newcode24
base/bind_helpers.cc:24: if (!closure_.is_null())
Maybe follow the Release() pattern of caching the closure_ before returning?

As is, I worry that Run() could delete this ScopedClosure.

One could even do 

Closure old_closure = Release();
closure_ = closure;
if (!old_closure.is_null())
  old_closure.run();

[Avi (use Gerrit), 2013-08-29 19:01:30]

OK, then.

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

On 2013/08/29 19:01:30, Avi wrote:
> OK, then.

LGTM

Also occurred to me that this is in the wrong file...it should be in
callback_helpers.h since it has nothing to do with Bind(). :-/

[Avi (use Gerrit), 2013-08-29 19:41:14]

Let's do it then.

[Cait (Slow), 2013-08-29 19:41:20]

Thanks -- this definitely makes things less clunky :) 

I think we also need to expose a default ctor for ScopedClosureRunner, though,
since we can only set it after the callback has been registered, but we want it
to be scoped to the lifetime of the client:

class Foo {
 public:
  Foo() { bar.Reset(base::Bind(&Foo::Do, base::Unretained(this)));}

  ~Foo() { /* closure runs when bar gets destroyed */ }

  void Do() { ... }

 private:
  ScopedClosureRunner bar;
};

(unless I'm missing something?)

The other option is to hold a ptr to the ScopedClosureRunner, but then you have
to manually delete it in the dtor...


On 2013/08/29 18:46:02, Avi wrote:
> ajwong: review
> caitkp: so you don't have to do that horrible scoped_ptr<ScopedClosureRunner>
> bit in your sample code

[Avi (use Gerrit), 2013-08-29 19:47:11]

Cait, you're right. Have a default constructor!

[Avi (use Gerrit), 2013-08-29 19:55:13]

BTW, one more Reset, so that if someone needs to deregister before the end of
the scope they can do a null reset.

[awong, 2013-08-29 21:00:39]

LGTM w/nit

https://codereview.chromium.org/23514018/diff/26001/base/callback_helpers_uni...
File base/callback_helpers_unittest.cc (right):

https://codereview.chromium.org/23514018/diff/26001/base/callback_helpers_uni...
base/callback_helpers_unittest.cc:51: int run_count = 0;
Call this run_count_3?

[commit-bot: I haz the power, 2013-08-29 21:09:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/avi@chromium.org/23514018/32001

[commit-bot: I haz the power, 2013-08-29 21:09:46]

Failed to request the patch to try. Please note that binary files are still
unsupported at the moment, this is being worked on.

Thanks for your patience.

HTTP Error 404: Not Found

[commit-bot: I haz the power, 2013-08-29 21:11:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/avi@chromium.org/23514018/35001

[commit-bot: I haz the power, 2013-08-29 21:46:31]

Sorry for I got bad news for ya.
Compile failed with a clobber build on android_clang_dbg.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_cl...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-08-29 22:12:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/avi@chromium.org/23514018/2001

[commit-bot: I haz the power, 2013-08-29 22:53:38]

Sorry for I got bad news for ya.
Compile failed with a clobber build on android_clang_dbg.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_cl...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-08-29 22:54:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/avi@chromium.org/23514018/63001

[commit-bot: I haz the power, 2013-08-30 02:04:06]

Change committed as 220473