Migrate Bind to BindOnce or BindRepeating in //base/message_loop

Migrate Bind to BindOnce or BindRepeating in //base/message_loop

This is manual migration from base::Callback to base::OnceCallback
around //base/message_loop, which is not covered by tool-based one.

Review-Url: https://codereview.chromium.org/2824533002
Cr-Commit-Position: refs/heads/master@{#465161}
Committed: https://chromium.googlesource.com/chromium/src/+/a8cc22075b8af00a5848dc252298148625fd31d5

[tzik, 2017-04-14 18:31:21]

The CQ bit was checked by tzik@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-14 18:31:38]

Dry run: 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, 2017-04-14 19:41:34]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-14 19:41:35]

Dry run: This issue passed the CQ dry run.

[tzik, 2017-04-17 08:10:53]

Description was changed from

==========
Migrate Bind to BindOnce or BindRepeating in //base/message_loop

BUG=
==========

to

==========
Migrate Bind to BindOnce or BindRepeating in //base/message_loop

This is manual migration from base::Callback to base::OnceCallback
around //base/message_loop, which is not covered by tool-based one.
==========

[tzik, 2017-04-17 08:10:53]

tzik@chromium.org changed reviewers:
+ dcheng@chromium.org

[tzik, 2017-04-17 08:11:03]

PTAL

[dcheng, 2017-04-17 17:45:07]

The CQ bit was checked by dcheng@chromium.org

[dcheng, 2017-04-17 17:45:08]

lgtm

[commit-bot: I haz the power, 2017-04-17 17:45:41]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-17 17:45:42]

This CL has an open dependency (Issue 2801593002 Patch 80001). Please resolve
the dependency and try again.
If you are sure that there is no real dependency, please use one of the options
listed in https://goo.gl/9Es4OR to land the CL.

[dcheng, 2017-04-17 17:59:30]

https://codereview.chromium.org/2824533002/diff/1/base/win/message_window.cc
File base/win/message_window.cc (right):

https://codereview.chromium.org/2824533002/diff/1/base/win/message_window.cc#...
base/win/message_window.cc:85: return DoCreate(std::move(message_callback),
NULL);
Actually, I do have one question here:

this avoids refcount churn, but I think it also makes these functions slightly
more expensive to call since the caller needs to emit instructions for invoking
the move ctor now, rather than simply passing a reference.

Do we have any measurements on how/if this will affect things?

(This question shouldn't block: we can resolve this independent of this CL, as I
think it's not too hard to fix these globally if we decide that we need to
mandate one style or another)

[tzik, 2017-04-18 05:20:29]

https://codereview.chromium.org/2824533002/diff/1/base/win/message_window.cc
File base/win/message_window.cc (right):

https://codereview.chromium.org/2824533002/diff/1/base/win/message_window.cc#...
base/win/message_window.cc:85: return DoCreate(std::move(message_callback),
NULL);
On 2017/04/17 17:59:30, dcheng (OOO through May 2) wrote:
> Actually, I do have one question here:
> 
> this avoids refcount churn, but I think it also makes these functions slightly
> more expensive to call since the caller needs to emit instructions for
invoking
> the move ctor now, rather than simply passing a reference.
> 
> Do we have any measurements on how/if this will affect things?
> 
> (This question shouldn't block: we can resolve this independent of this CL, as
I
> think it's not too hard to fix these globally if we decide that we need to
> mandate one style or another)

I believe the pass-by-value of Callback is lightweight enough comparing to the
pass-by-ref, and in this case, the generated code for each of them are likely
the same after the inlining and the compiler optimization.

On a simple case below on Linux release build, pass-by-value and pass-by-ref
generated the same binaries for |baz| and |qux|:
  void foo(const base::Closure& cb);
  void bar(base::Closure cb);

  void baz() { foo(base::Bind([]{})); }
  void qux() { bar(base::Bind([]{})); }

Stepping into it for more details with a pass-by-value function call:
  void foo(Closure);

  void bar() {
    Closure cb = base::Bind([]{});
    foo(std::move(cb));
  }

The caller of |foo| makes a temporary object, and pass it by ref.
  void bar() {
    Closure cb = base::Bind([]{});

    // The caller creates a temporary object, and pass it by ref.
    Closure temporary_object = std::move(cb);
    foo(&temporary_object);
  }

Then, everything is inlined as:
  void bar() {
    BindStateBase* cb_bind_state = base::Bind([]{}).bind_state_;

    // inlined from move-construction of Callback.
    BindStateBase* bind_state = cb_bind_state;
    cb_bind_state = nullptr;

    foo(&bind_state);

    // inlined from scoped_refptr dtor.
    if (bind_state)
      bind_state->Release();

    // inlined from scoped_refptr dtor.
    if (cb_bind_state)
      cb_bind_state->Release();
  }

Finally, compiler optimization should remove intermediate variables and
always-false condition:
  void bar() {
    BindStateBase* bind_state = base::Bind([]{});
    foo(&bind_state);
    if (bind_state)
      bind_state->Release();
  }
which is equivalent to pass-by-ref code.

[tzik, 2017-04-18 05:37:06]

The CQ bit was checked by tzik@chromium.org

[commit-bot: I haz the power, 2017-04-18 05:37:17]

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, 2017-04-18 07:01:18]

CQ is committing da patch.

Bot data: {"patchset_id": 1, "attempt_start_ts": 1492493826130400, "parent_rev":
"313a30da6762cab7c0d9cd958c6119fd3768afa1", "commit_rev":
"a8cc22075b8af00a5848dc252298148625fd31d5"}

[commit-bot: I haz the power, 2017-04-18 07:01:27]

Description was changed from

==========
Migrate Bind to BindOnce or BindRepeating in //base/message_loop

This is manual migration from base::Callback to base::OnceCallback
around //base/message_loop, which is not covered by tool-based one.
==========

to

==========
Migrate Bind to BindOnce or BindRepeating in //base/message_loop

This is manual migration from base::Callback to base::OnceCallback
around //base/message_loop, which is not covered by tool-based one.

Review-Url: https://codereview.chromium.org/2824533002
Cr-Commit-Position: refs/heads/master@{#465161}
Committed:
https://chromium.googlesource.com/chromium/src/+/a8cc22075b8af00a5848dc252298...
==========

[commit-bot: I haz the power, 2017-04-18 07:01:28]

Committed patchset #1 (id:1) as
https://chromium.googlesource.com/chromium/src/+/a8cc22075b8af00a5848dc252298...