Add move constructor in ScopedClosureRunner.

Add move constructor in ScopedClosureRunner.

With this change ScopedClosureRunner will be movable. This will
allow to store instances of that class in STL containers, e.g. to store
list of callbacks to be called when certain event happens.

Committed: https://crrev.com/e4be1917688638c0ce8b2e3ddbe70089bbee9fb7
Cr-Commit-Position: refs/heads/master@{#402031}

[Sergey Ulanov, 2016-06-23 19:15:57]

Patchset #1 (id:1) has been deleted

[Sergey Ulanov, 2016-06-23 19:16:41]

sergeyu@chromium.org changed reviewers:
+ danakj@chromium.org

[Sergey Ulanov, 2016-06-23 19:17:13]

Description was changed from

==========
Add move constructor in ScopedClosureRunner.

With this change ScopedClosureRunner will become movable. This will
allow to store instances of that class in STL containers, e.g. to store
list of callbacks to be called when certain event happens.
==========

to

==========
Add move constructor in ScopedClosureRunner.

With this change ScopedClosureRunner will be movable. This will
allow to store instances of that class in STL containers, e.g. to store
list of callbacks to be called when certain event happens.
==========

[danakj, 2016-06-23 20:19:28]

unit test?

https://codereview.chromium.org/2091503004/diff/20001/base/callback_helpers.h
File base/callback_helpers.h (right):

https://codereview.chromium.org/2091503004/diff/20001/base/callback_helpers.h...
base/callback_helpers.h:31: // that theClosure is executed no matter how the
current scope exits.
the Closure

https://codereview.chromium.org/2091503004/diff/20001/base/callback_helpers.h...
base/callback_helpers.h:35: ScopedClosureRunner(ScopedClosureRunner&& other);
Any reason you're not defining operator= along with this as the style guide
asks?

Can you order this below the "normal" constructors, and separate it with
whitespace?

[Sergey Ulanov, 2016-06-23 23:30:32]

added unittest now

https://codereview.chromium.org/2091503004/diff/20001/base/callback_helpers.h
File base/callback_helpers.h (right):

https://codereview.chromium.org/2091503004/diff/20001/base/callback_helpers.h...
base/callback_helpers.h:31: // that theClosure is executed no matter how the
current scope exits.
On 2016/06/23 20:19:28, danakj wrote:
> the Closure

Done.

https://codereview.chromium.org/2091503004/diff/20001/base/callback_helpers.h...
base/callback_helpers.h:35: ScopedClosureRunner(ScopedClosureRunner&& other);
On 2016/06/23 20:19:28, danakj wrote:
> Any reason you're not defining operator= along with this as the style guide
> asks?

One small concern I have with operator= is that when assigning a=b the closure
in |a| has to be called. That behavior may be confusing and unexpected.  Should
we require that |a| is empty in that case, e.g. with a DCHECK? WDYT?

> Can you order this below the "normal" constructors, and separate it with
> whitespace?

Done.

[danakj, 2016-06-24 20:06:30]

LGTM

https://codereview.chromium.org/2091503004/diff/20001/base/callback_helpers.h
File base/callback_helpers.h (right):

https://codereview.chromium.org/2091503004/diff/20001/base/callback_helpers.h...
base/callback_helpers.h:35: ScopedClosureRunner(ScopedClosureRunner&& other);
On 2016/06/23 23:30:32, Sergey Ulanov wrote:
> On 2016/06/23 20:19:28, danakj wrote:
> > Any reason you're not defining operator= along with this as the style guide
> > asks?
> 
> One small concern I have with operator= is that when assigning a=b the closure
> in |a| has to be called. That behavior may be confusing and unexpected. 
Should
> we require that |a| is empty in that case, e.g. with a DCHECK? WDYT?

I think it makes sense that it does what reset does. Can you add a comment on
the operator= and on Reset() in the header saying that it runs the currently
owned closure?

[Sergey Ulanov, 2016-06-24 23:44:01]

https://codereview.chromium.org/2091503004/diff/20001/base/callback_helpers.h
File base/callback_helpers.h (right):

https://codereview.chromium.org/2091503004/diff/20001/base/callback_helpers.h...
base/callback_helpers.h:35: ScopedClosureRunner(ScopedClosureRunner&& other);
On 2016/06/24 20:06:30, danakj wrote:
> On 2016/06/23 23:30:32, Sergey Ulanov wrote:
> > On 2016/06/23 20:19:28, danakj wrote:
> > > Any reason you're not defining operator= along with this as the style
guide
> > > asks?
> > 
> > One small concern I have with operator= is that when assigning a=b the
closure
> > in |a| has to be called. That behavior may be confusing and unexpected. 
> Should
> > we require that |a| is empty in that case, e.g. with a DCHECK? WDYT?
> 
> I think it makes sense that it does what reset does. Can you add a comment on
> the operator= and on Reset() in the header saying that it runs the currently
> owned closure?

Done.

BTW, As far as I can tell there is only one place where Reset() is called and it
looks to be used used incorrectly there:
https://codesearch.chromium.org/chromium/src/content/browser/compositor/surfa...

[Sergey Ulanov, 2016-06-24 23:44:18]

The CQ bit was checked by sergeyu@chromium.org

[Sergey Ulanov, 2016-06-24 23:44:18]

The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org
Link to the patchset: https://codereview.chromium.org/2091503004/#ps60001
(title: "comment")

[commit-bot: I haz the power, 2016-06-24 23:44:51]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-06-25 00:51:21]

Description was changed from

==========
Add move constructor in ScopedClosureRunner.

With this change ScopedClosureRunner will be movable. This will
allow to store instances of that class in STL containers, e.g. to store
list of callbacks to be called when certain event happens.
==========

to

==========
Add move constructor in ScopedClosureRunner.

With this change ScopedClosureRunner will be movable. This will
allow to store instances of that class in STL containers, e.g. to store
list of callbacks to be called when certain event happens.
==========

[commit-bot: I haz the power, 2016-06-25 00:51:21]

Committed patchset #3 (id:60001)

[commit-bot: I haz the power, 2016-06-25 00:53:41]

Description was changed from

==========
Add move constructor in ScopedClosureRunner.

With this change ScopedClosureRunner will be movable. This will
allow to store instances of that class in STL containers, e.g. to store
list of callbacks to be called when certain event happens.
==========

to

==========
Add move constructor in ScopedClosureRunner.

With this change ScopedClosureRunner will be movable. This will
allow to store instances of that class in STL containers, e.g. to store
list of callbacks to be called when certain event happens.

Committed: https://crrev.com/e4be1917688638c0ce8b2e3ddbe70089bbee9fb7
Cr-Commit-Position: refs/heads/master@{#402031}
==========

[commit-bot: I haz the power, 2016-06-25 00:53:42]

Patchset 3 (id:??) landed as
https://crrev.com/e4be1917688638c0ce8b2e3ddbe70089bbee9fb7
Cr-Commit-Position: refs/heads/master@{#402031}