Enables delegating signal strategy for It2Me Host.

Enables delegating signal strategy for It2Me Host.

Motivation:
Allow the It2me host to delegates signaling messages sending/receiving
via native messaging.

Summary of changes:
- Add a new flag useSignalingProxy on the connect message for
  com.google.chrome.remote_assistance.
- Move delegating_signal_strategy from the plugin directory to the
  signaling directory so that it can be shared by the client and the
  host
- Change the threading behavior of delegating_signal_strategy so that
  the delegate and the listener can be run on two different threads.

Committed: https://crrev.com/4e572c184b4eb0a662c0a188acfa11f40173dff1
Cr-Commit-Position: refs/heads/master@{#423692}

[kelvinp, 2016-10-04 21:57:40]

Patchset #1 (id:1) has been deleted

[kelvinp, 2016-10-04 21:58:20]

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

[kelvinp, 2016-10-04 21:58:21]

PTAL

[Sergey Ulanov, 2016-10-05 21:49:08]

Looks mostly good, but there are couple of threading issues, which should be
easy to fix.

https://codereview.chromium.org/2384063008/diff/20001/remoting/client/plugin/...
File remoting/client/plugin/chromoting_instance.cc (right):

https://codereview.chromium.org/2384063008/diff/20001/remoting/client/plugin/...
remoting/client/plugin/chromoting_instance.cc:676: plugin_task_runner_,
Please run 'git cl format'.
These 3 parameters can fit on one line.

https://codereview.chromium.org/2384063008/diff/20001/remoting/host/it2me/it2...
File remoting/host/it2me/it2me_native_messaging_host.cc (right):

https://codereview.chromium.org/2384063008/diff/20001/remoting/host/it2me/it2...
remoting/host/it2me/it2me_native_messaging_host.cc:89:
delegating_signal_strategy_(nullptr),
please put this initializer in the class declaration in the header.

https://codereview.chromium.org/2384063008/diff/20001/remoting/host/it2me/it2...
remoting/host/it2me/it2me_native_messaging_host.cc:234:
XmppSignalStrategy::XmppServerConfig xmpp_config = xmpp_server_config_;
This code uses XmppSignalStrategy::XmppServerConfig even when it doesn't use
XmppSignalStrategy. I think all code that initializes XmppServerConfig can be
moved below where XmppSignalStrategy is created and then xmpp_server_config_ can
be removed.

https://codereview.chromium.org/2384063008/diff/20001/remoting/host/it2me/it2...
remoting/host/it2me/it2me_native_messaging_host.cc:240: bool use_signaling_proxy
= false;
nit: add empty line here

https://codereview.chromium.org/2384063008/diff/20001/remoting/host/it2me/it2...
remoting/host/it2me/it2me_native_messaging_host.cc:288:
signal_strategy.reset(new XmppSignalStrategy(
nit: add empty line here

https://codereview.chromium.org/2384063008/diff/20001/remoting/host/it2me/it2...
remoting/host/it2me/it2me_native_messaging_host.cc:294: if
(!message->GetString("hostJid", &host_jid)) {
maybe call this localJid and local_jid?

https://codereview.chromium.org/2384063008/diff/20001/remoting/host/it2me/it2...
remoting/host/it2me/it2me_native_messaging_host.cc:301: task_runner(),
'git cl format' please

https://codereview.chromium.org/2384063008/diff/20001/remoting/host/it2me/it2...
File remoting/host/it2me/it2me_native_messaging_host.h (right):

https://codereview.chromium.org/2384063008/diff/20001/remoting/host/it2me/it2...
remoting/host/it2me/it2me_native_messaging_host.h:97: DelegatingSignalStrategy*
delegating_signal_strategy_;
 = nullptr;

https://codereview.chromium.org/2384063008/diff/20001/remoting/signaling/dele...
File remoting/signaling/delegating_signal_strategy.cc (right):

https://codereview.chromium.org/2384063008/diff/20001/remoting/signaling/dele...
remoting/signaling/delegating_signal_strategy.cc:33: base::Unretained(this),
message));
base::Unretained() is not safe here.
Use a weak ptr?

https://codereview.chromium.org/2384063008/diff/20001/remoting/signaling/dele...
remoting/signaling/delegating_signal_strategy.cc:52:
CHECK(client_task_runner_->BelongsToCurrentThread());
Here and below, please use DCHECK instead of CHECK for thread checks

https://codereview.chromium.org/2384063008/diff/20001/remoting/signaling/dele...
File remoting/signaling/delegating_signal_strategy.h (right):

https://codereview.chromium.org/2384063008/diff/20001/remoting/signaling/dele...
remoting/signaling/delegating_signal_strategy.h:23: // 2. |OnIncomingMessage|
and  |send_iq_callback| will always be called in
s/|OnIncomingMessage|/OnIncomingMessage()/

https://codereview.chromium.org/2384063008/diff/20001/remoting/signaling/dele...
remoting/signaling/delegating_signal_strategy.h:23: // 2. |OnIncomingMessage|
and  |send_iq_callback| will always be called in
Also please note that |send_iq_callback| may be called after this class is
destroyed.

https://codereview.chromium.org/2384063008/diff/20001/remoting/signaling/dele...
remoting/signaling/delegating_signal_strategy.h:34:
scoped_refptr<base::SingleThreadTaskRunner> delegate_task_runner,
This class is always created on delegate_task_runner. Remove this parameter and
use base::ThreadTaskRunnerHandle::Get() to initialize delegate_task_runner_.

https://codereview.chromium.org/2384063008/diff/20001/remoting/signaling/xmpp...
File remoting/signaling/xmpp_signal_strategy.cc (right):

https://codereview.chromium.org/2384063008/diff/20001/remoting/signaling/xmpp...
remoting/signaling/xmpp_signal_strategy.cc:167:
thread_checker_.DetachFromThread();
I don't think it's safe to detach from the thread here. Particularly
keep_alive_timer_ is initialized above and it gets attached to the current
thread, so SendKeepAlive() is called on the wrong thread. I would expect
DCHECK() in SendKeepAlive() fail as it would be called on the wrong thread.
Should be easy to fix by moving timer initialization to Connect().

https://codereview.chromium.org/2384063008/diff/20001/remoting/signaling/xmpp...
remoting/signaling/xmpp_signal_strategy.cc:167:
thread_checker_.DetachFromThread();
Please add a comment xmpp_signal_strategy.h to make it clear that the class can
be created on a thread different from the one on which it's used.

[kelvinp, 2016-10-06 00:43:06]

Thank you Sergey.  PTAL

https://codereview.chromium.org/2384063008/diff/20001/remoting/client/plugin/...
File remoting/client/plugin/chromoting_instance.cc (right):

https://codereview.chromium.org/2384063008/diff/20001/remoting/client/plugin/...
remoting/client/plugin/chromoting_instance.cc:676: plugin_task_runner_,
On 2016/10/05 21:49:06, Sergey Ulanov wrote:
> Please run 'git cl format'.
> These 3 parameters can fit on one line.

Done.

https://codereview.chromium.org/2384063008/diff/20001/remoting/host/it2me/it2...
File remoting/host/it2me/it2me_native_messaging_host.cc (right):

https://codereview.chromium.org/2384063008/diff/20001/remoting/host/it2me/it2...
remoting/host/it2me/it2me_native_messaging_host.cc:89:
delegating_signal_strategy_(nullptr),
On 2016/10/05 21:49:06, Sergey Ulanov wrote:
> please put this initializer in the class declaration in the header.

Done.

https://codereview.chromium.org/2384063008/diff/20001/remoting/host/it2me/it2...
remoting/host/it2me/it2me_native_messaging_host.cc:234:
XmppSignalStrategy::XmppServerConfig xmpp_config = xmpp_server_config_;
On 2016/10/05 21:49:06, Sergey Ulanov wrote:
> This code uses XmppSignalStrategy::XmppServerConfig even when it doesn't use
> XmppSignalStrategy. I think all code that initializes XmppServerConfig can be
> moved below where XmppSignalStrategy is created and then xmpp_server_config_
can
> be removed.

Done.

https://codereview.chromium.org/2384063008/diff/20001/remoting/host/it2me/it2...
remoting/host/it2me/it2me_native_messaging_host.cc:240: bool use_signaling_proxy
= false;
On 2016/10/05 21:49:06, Sergey Ulanov wrote:
> nit: add empty line here

Done.

https://codereview.chromium.org/2384063008/diff/20001/remoting/host/it2me/it2...
remoting/host/it2me/it2me_native_messaging_host.cc:288:
signal_strategy.reset(new XmppSignalStrategy(
On 2016/10/05 21:49:06, Sergey Ulanov wrote:
> nit: add empty line here

Done.

https://codereview.chromium.org/2384063008/diff/20001/remoting/host/it2me/it2...
remoting/host/it2me/it2me_native_messaging_host.cc:294: if
(!message->GetString("hostJid", &host_jid)) {
On 2016/10/05 21:49:06, Sergey Ulanov wrote:
> maybe call this localJid and local_jid?

Done.

https://codereview.chromium.org/2384063008/diff/20001/remoting/host/it2me/it2...
remoting/host/it2me/it2me_native_messaging_host.cc:301: task_runner(),
On 2016/10/05 21:49:06, Sergey Ulanov wrote:
> 'git cl format' please

Done.

https://codereview.chromium.org/2384063008/diff/20001/remoting/host/it2me/it2...
File remoting/host/it2me/it2me_native_messaging_host.h (right):

https://codereview.chromium.org/2384063008/diff/20001/remoting/host/it2me/it2...
remoting/host/it2me/it2me_native_messaging_host.h:97: DelegatingSignalStrategy*
delegating_signal_strategy_;
On 2016/10/05 21:49:07, Sergey Ulanov wrote:
>  = nullptr;

Done.

https://codereview.chromium.org/2384063008/diff/20001/remoting/signaling/dele...
File remoting/signaling/delegating_signal_strategy.cc (right):

https://codereview.chromium.org/2384063008/diff/20001/remoting/signaling/dele...
remoting/signaling/delegating_signal_strategy.cc:33: base::Unretained(this),
message));
On 2016/10/05 21:49:07, Sergey Ulanov wrote:
> base::Unretained() is not safe here.
> Use a weak ptr?

Done.

https://codereview.chromium.org/2384063008/diff/20001/remoting/signaling/dele...
remoting/signaling/delegating_signal_strategy.cc:52:
CHECK(client_task_runner_->BelongsToCurrentThread());
On 2016/10/05 21:49:07, Sergey Ulanov wrote:
> Here and below, please use DCHECK instead of CHECK for thread checks

Done.

https://codereview.chromium.org/2384063008/diff/20001/remoting/signaling/dele...
File remoting/signaling/delegating_signal_strategy.h (right):

https://codereview.chromium.org/2384063008/diff/20001/remoting/signaling/dele...
remoting/signaling/delegating_signal_strategy.h:23: // 2. |OnIncomingMessage|
and  |send_iq_callback| will always be called in
On 2016/10/05 21:49:07, Sergey Ulanov wrote:
> s/|OnIncomingMessage|/OnIncomingMessage()/

Done.

https://codereview.chromium.org/2384063008/diff/20001/remoting/signaling/dele...
remoting/signaling/delegating_signal_strategy.h:23: // 2. |OnIncomingMessage|
and  |send_iq_callback| will always be called in
On 2016/10/05 21:49:07, Sergey Ulanov wrote:
> Also please note that |send_iq_callback| may be called after this class is
> destroyed.

Done.

https://codereview.chromium.org/2384063008/diff/20001/remoting/signaling/dele...
remoting/signaling/delegating_signal_strategy.h:34:
scoped_refptr<base::SingleThreadTaskRunner> delegate_task_runner,
On 2016/10/05 21:49:07, Sergey Ulanov wrote:
> This class is always created on delegate_task_runner. Remove this parameter
and
> use base::ThreadTaskRunnerHandle::Get() to initialize delegate_task_runner_.

Done.

https://codereview.chromium.org/2384063008/diff/20001/remoting/signaling/xmpp...
File remoting/signaling/xmpp_signal_strategy.cc (right):

https://codereview.chromium.org/2384063008/diff/20001/remoting/signaling/xmpp...
remoting/signaling/xmpp_signal_strategy.cc:167:
thread_checker_.DetachFromThread();
On 2016/10/05 21:49:07, Sergey Ulanov wrote:
> I don't think it's safe to detach from the thread here. Particularly
> keep_alive_timer_ is initialized above and it gets attached to the current
> thread, so SendKeepAlive() is called on the wrong thread. I would expect
> DCHECK() in SendKeepAlive() fail as it would be called on the wrong thread.
> Should be easy to fix by moving timer initialization to Connect().

Done.

https://codereview.chromium.org/2384063008/diff/20001/remoting/signaling/xmpp...
remoting/signaling/xmpp_signal_strategy.cc:167:
thread_checker_.DetachFromThread();
On 2016/10/05 21:49:07, Sergey Ulanov wrote:
> Please add a comment xmpp_signal_strategy.h to make it clear that the class
can
> be created on a thread different from the one on which it's used.

Done.

[Sergey Ulanov, 2016-10-06 18:28:57]

lgtm

https://codereview.chromium.org/2384063008/diff/80001/remoting/signaling/dele...
File remoting/signaling/delegating_signal_strategy.h (right):

https://codereview.chromium.org/2384063008/diff/80001/remoting/signaling/dele...
remoting/signaling/delegating_signal_strategy.h:24: // 2. OnIncomingMessage()
and |send_iq_callback| will always be called on the
Suggest separating this statement into two. |send_iq_callback| is called by this
class, while  OnIncomingMessage() will be called by the client. I.e. add a new
item between (2) and (3): OnIncomingMessage() must be called on the same same
thread on which this object is created.

https://codereview.chromium.org/2384063008/diff/80001/remoting/signaling/dele...
remoting/signaling/delegating_signal_strategy.h:27: // 3. The caller should
invoke all methods on the |client_task_runner|.
Maybe rephrase this as "SignalStrategy interface must be used on
|client_task_runner|."

https://codereview.chromium.org/2384063008/diff/80001/remoting/signaling/xmpp...
File remoting/signaling/xmpp_signal_strategy.cc (right):

https://codereview.chromium.org/2384063008/diff/80001/remoting/signaling/xmpp...
remoting/signaling/xmpp_signal_strategy.cc:158: keep_alive_timer_(true /*
retain_user_task */, true /* is_repeating */) {
Change keep_alive_timer_ to  base::RepeatingTimer, then it won't need to be
initialized explicitly here.

[kelvinp, 2016-10-06 19:21:57]

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

[commit-bot: I haz the power, 2016-10-06 19:22:32]

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

[kelvinp, 2016-10-06 20:30:28]

FYI

https://codereview.chromium.org/2384063008/diff/80001/remoting/signaling/dele...
File remoting/signaling/delegating_signal_strategy.h (right):

https://codereview.chromium.org/2384063008/diff/80001/remoting/signaling/dele...
remoting/signaling/delegating_signal_strategy.h:24: // 2. OnIncomingMessage()
and |send_iq_callback| will always be called on the
On 2016/10/06 18:28:57, Sergey Ulanov wrote:
> Suggest separating this statement into two. |send_iq_callback| is called by
this
> class, while  OnIncomingMessage() will be called by the client. I.e. add a new
> item between (2) and (3): OnIncomingMessage() must be called on the same same
> thread on which this object is created.

Done.

https://codereview.chromium.org/2384063008/diff/80001/remoting/signaling/dele...
remoting/signaling/delegating_signal_strategy.h:27: // 3. The caller should
invoke all methods on the |client_task_runner|.
On 2016/10/06 18:28:57, Sergey Ulanov wrote:
> Maybe rephrase this as "SignalStrategy interface must be used on
> |client_task_runner|."

Done.

https://codereview.chromium.org/2384063008/diff/80001/remoting/signaling/xmpp...
File remoting/signaling/xmpp_signal_strategy.cc (right):

https://codereview.chromium.org/2384063008/diff/80001/remoting/signaling/xmpp...
remoting/signaling/xmpp_signal_strategy.cc:158: keep_alive_timer_(true /*
retain_user_task */, true /* is_repeating */) {
On 2016/10/06 18:28:57, Sergey Ulanov wrote:
> Change keep_alive_timer_ to  base::RepeatingTimer, then it won't need to be
> initialized explicitly here.

Done.

[kelvinp, 2016-10-06 20:32:07]

The CQ bit was checked by kelvinp@chromium.org

[kelvinp, 2016-10-06 20:32:07]

The patchset sent to the CQ was uploaded after l-g-t-m from sergeyu@chromium.org
Link to the patchset: https://codereview.chromium.org/2384063008/#ps140001
(title: "Fix typos")

[commit-bot: I haz the power, 2016-10-06 20:33:13]

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-10-06 21:37:28]

Committed patchset #7 (id:140001)

[commit-bot: I haz the power, 2016-10-06 21:40:21]

Description was changed from

==========
Enables delegating signal strategy for It2Me Host.

Motivation:
Allow the It2me host to delegates signaling messages sending/receiving
via native messaging.

Summary of changes:
- Add a new flag useSignalingProxy on the connect message for
  com.google.chrome.remote_assistance.
- Move delegating_signal_strategy from the plugin directory to the
  signaling directory so that it can be shared by the client and the
  host
- Change the threading behavior of delegating_signal_strategy so that
  the delegate and the listener can be run on two different threads.
==========

to

==========
Enables delegating signal strategy for It2Me Host.

Motivation:
Allow the It2me host to delegates signaling messages sending/receiving
via native messaging.

Summary of changes:
- Add a new flag useSignalingProxy on the connect message for
  com.google.chrome.remote_assistance.
- Move delegating_signal_strategy from the plugin directory to the
  signaling directory so that it can be shared by the client and the
  host
- Change the threading behavior of delegating_signal_strategy so that
  the delegate and the listener can be run on two different threads.

Committed: https://crrev.com/4e572c184b4eb0a662c0a188acfa11f40173dff1
Cr-Commit-Position: refs/heads/master@{#423692}
==========

[commit-bot: I haz the power, 2016-10-06 21:40:22]

Patchset 7 (id:??) landed as
https://crrev.com/4e572c184b4eb0a662c0a188acfa11f40173dff1
Cr-Commit-Position: refs/heads/master@{#423692}