[Chromoting] Remove DataChannelManagerCallbacks constructor parameter of ClientSession

[Chromoting] Remove DataChannelManagerCallbacks constructor parameter of ClientSession

This change removes the constructor parameter of ClientSession, which is used to
initialize DataChannelManager. Instead, a public function
ClientSession::RegisterDataChannelManagerCallback() has been added to achieve
the goal.

The motion of this change is to avoid the base::Bind() to be kept in a secondary
place in ChromotingHost other than ClientSession. And I think it's also
reasonable to public the Register callback function from ClientSession. So we
do not need to write all registering logic in ClientSession.

BUG=650926
Review-Url: https://codereview.chromium.org/2967793002
Cr-Commit-Position: refs/heads/master@{#485482}
Committed: https://chromium.googlesource.com/chromium/src/+/b7da624213cfea4969a6b9385e9268e13d1d7843

[Hzj_jie, 2017-07-02 21:45:47]

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

[commit-bot: I haz the power, 2017-07-02 21:45:59]

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-07-02 21:58:28]

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

[commit-bot: I haz the power, 2017-07-02 21:58:30]

Dry run: Try jobs failed on following builders:
  win_clang on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)

[Hzj_jie, 2017-07-02 22:04:08]

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

[Hzj_jie, 2017-07-02 22:04:10]

Patchset #1 (id:1) has been deleted

[commit-bot: I haz the power, 2017-07-02 22:04:17]

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-07-02 22:22:38]

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

[commit-bot: I haz the power, 2017-07-02 22:22:39]

Dry run: This issue passed the CQ dry run.

[jarhar, 2017-07-05 17:51:42]

jarhar@google.com changed reviewers:
+ jarhar@google.com

[jarhar, 2017-07-05 17:51:42]

https://codereview.chromium.org/2967793002/diff/20001/remoting/host/chromotin...
File remoting/host/chromoting_host.cc (left):

https://codereview.chromium.org/2967793002/diff/20001/remoting/host/chromotin...
remoting/host/chromoting_host.cc:267:
std::vector<protocol::DataChannelManager::NameCallbackPair>()));
In order to register callbacks from this file, would you call
ClientSession::RegisterDataChannelManagerCallback() here before adding new
clients to clients_?

like this:
std::unique_ptr<ClientSession> new_client = base::MakeUnique(...);
new_client->RegisterDataChannelManagerCallback(...);
clients_.push_back(std::move(new_client));

https://codereview.chromium.org/2967793002/diff/20001/remoting/protocol/data_...
File remoting/protocol/data_channel_manager.cc (right):

https://codereview.chromium.org/2967793002/diff/20001/remoting/protocol/data_...
remoting/protocol/data_channel_manager.cc:23:
constructors_.push_back(std::make_pair(prefix, std::move(constructor)));
just curious, what effect does using std::move have on the callback? Why was it
not needed before?

[Hzj_jie, 2017-07-05 18:34:31]

https://codereview.chromium.org/2967793002/diff/20001/remoting/host/chromotin...
File remoting/host/chromoting_host.cc (left):

https://codereview.chromium.org/2967793002/diff/20001/remoting/host/chromotin...
remoting/host/chromoting_host.cc:267:
std::vector<protocol::DataChannelManager::NameCallbackPair>()));
On 2017/07/05 17:51:42, jarhar wrote:
> In order to register callbacks from this file, would you call
> ClientSession::RegisterDataChannelManagerCallback() here before adding new
> clients to clients_?
> 
> like this:
> std::unique_ptr<ClientSession> new_client = base::MakeUnique(...);
> new_client->RegisterDataChannelManagerCallback(...);
> clients_.push_back(std::move(new_client));

That's usually not necessary: Optional data channels and their callbacks won't
take effect before authentication.

https://codereview.chromium.org/2967793002/diff/20001/remoting/protocol/data_...
File remoting/protocol/data_channel_manager.cc (right):

https://codereview.chromium.org/2967793002/diff/20001/remoting/protocol/data_...
remoting/protocol/data_channel_manager.cc:23:
constructors_.push_back(std::make_pair(prefix, std::move(constructor)));
On 2017/07/05 17:51:42, jarhar wrote:
> just curious, what effect does using std::move have on the callback? Why was
it
> not needed before?

It does not impact the logic, but only the performance: Callback implements a
move constructor, so using it here can move its variables to avoid copying them
(bound objects).

So in general we should use std::move() to move Callback instances when passing
them into containers / functions. But we definitely missed most of them in the
entire Chromium.

I do not think in a short time, C++ will have an "automatic move" rule. So we
may need to take care of this issue for a long time.

[jarhar, 2017-07-06 19:58:32]

lgtm

Joe should look at this too, but it looks good to me

[Hzj_jie, 2017-07-06 20:51:44]

Yes, this is not an urgent change. I will wait for Joe's comments.

[Hzj_jie, 2017-07-10 21:56:03]

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

[commit-bot: I haz the power, 2017-07-10 21:56:16]

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-07-10 22:39:07]

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

[commit-bot: I haz the power, 2017-07-10 22:39:08]

Dry run: This issue passed the CQ dry run.

[Hzj_jie, 2017-07-10 22:48:04]

Description was changed from

==========
[Chromoting] Remove DataChannelManagerCallbacks constructor parameter of
ClientSession

This change removes the constructor parameter of ClientSession, which is used to
initialize DataChannelManager. Instead, a public function
ClientSession::RegisterDataChannelManagerCallback() has been added to achieve
the goal.

The motion of this change is to avoid the base::Bind() to be kept in a secondary
place in ChromotingHost other than ClientSession. And I think it's also
reasonable to public the Register callback function from ClientSession. So we
do not need to write all registering logic in ClientSession.

BUG=650926
==========

to

==========
[Chromoting] Remove DataChannelManagerCallbacks constructor parameter of
ClientSession

This change removes the constructor parameter of ClientSession, which is used to
initialize DataChannelManager. Instead, a public function
ClientSession::RegisterDataChannelManagerCallback() has been added to achieve
the goal.

The motion of this change is to avoid the base::Bind() to be kept in a secondary
place in ChromotingHost other than ClientSession. And I think it's also
reasonable to public the Register callback function from ClientSession. So we
do not need to write all registering logic in ClientSession.

BUG=650926
==========

[Hzj_jie, 2017-07-10 22:48:04]

zijiehe@chromium.org changed reviewers:
+ joedow@chromium.org

[joedow, 2017-07-10 23:36:45]

lgtm

https://codereview.chromium.org/2967793002/diff/40001/remoting/host/client_se...
File remoting/host/client_session.h (right):

https://codereview.chromium.org/2967793002/diff/40001/remoting/host/client_se...
remoting/host/client_session.h:147: void
RegisterDataChannelManagerCallbackForTesting(
nit: RegisterDataChannelManagerCallbackForTesting is a long name, shorten it to
something like RegisterCreateHandlerCallbackForTesting.

[Hzj_jie, 2017-07-11 00:30:44]

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

[commit-bot: I haz the power, 2017-07-11 00:31:03]

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-07-11 01:02:47]

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

[commit-bot: I haz the power, 2017-07-11 01:02:48]

Dry run: This issue passed the CQ dry run.

[Hzj_jie, 2017-07-11 01:11:15]

https://codereview.chromium.org/2967793002/diff/40001/remoting/host/client_se...
File remoting/host/client_session.h (right):

https://codereview.chromium.org/2967793002/diff/40001/remoting/host/client_se...
remoting/host/client_session.h:147: void
RegisterDataChannelManagerCallbackForTesting(
On 2017/07/10 23:36:45, joedow wrote:
> nit: RegisterDataChannelManagerCallbackForTesting is a long name, shorten it
to
> something like RegisterCreateHandlerCallbackForTesting.

Done.

[Hzj_jie, 2017-07-11 01:11:20]

The CQ bit was checked by zijiehe@chromium.org

[Hzj_jie, 2017-07-11 01:11:20]

The patchset sent to the CQ was uploaded after l-g-t-m from jarhar@google.com,
joedow@chromium.org
Link to the patchset: https://codereview.chromium.org/2967793002/#ps60001
(title: "Resolve review comments")

[commit-bot: I haz the power, 2017-07-11 01:11:41]

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-07-11 01:17:19]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1499735480501450,
"parent_rev": "d30c3532f0cead481b8ef4c7290e0eb15d8fb6d0", "commit_rev":
"a984943c0fa600613a9915771a4dd8ffc15106cc"}

[commit-bot: I haz the power, 2017-07-11 01:17:49]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1499735480501450,
"parent_rev": "38bc9db968e2187df4af71ab4052f6a909faff84", "commit_rev":
"b7da624213cfea4969a6b9385e9268e13d1d7843"}

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

Description was changed from

==========
[Chromoting] Remove DataChannelManagerCallbacks constructor parameter of
ClientSession

This change removes the constructor parameter of ClientSession, which is used to
initialize DataChannelManager. Instead, a public function
ClientSession::RegisterDataChannelManagerCallback() has been added to achieve
the goal.

The motion of this change is to avoid the base::Bind() to be kept in a secondary
place in ChromotingHost other than ClientSession. And I think it's also
reasonable to public the Register callback function from ClientSession. So we
do not need to write all registering logic in ClientSession.

BUG=650926
==========

to

==========
[Chromoting] Remove DataChannelManagerCallbacks constructor parameter of
ClientSession

This change removes the constructor parameter of ClientSession, which is used to
initialize DataChannelManager. Instead, a public function
ClientSession::RegisterDataChannelManagerCallback() has been added to achieve
the goal.

The motion of this change is to avoid the base::Bind() to be kept in a secondary
place in ChromotingHost other than ClientSession. And I think it's also
reasonable to public the Register callback function from ClientSession. So we
do not need to write all registering logic in ClientSession.

BUG=650926

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

[commit-bot: I haz the power, 2017-07-11 01:18:05]

Committed patchset #3 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/b7da624213cfea4969a6b9385e92...