Refactor SignalStrategy so that it can be reused for multiple connections.

Refactor SignalStrategy so that it can be reused for multiple connections.

Also updated SessionManager implementation so that session managers can 
be created before signaling is connected.

BUG=107276
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=116192

[Wez, 2011-12-21 23:35:29]

http://codereview.chromium.org/9005034/diff/3001/remoting/host/chromoting_hos...
File remoting/host/chromoting_host.cc (right):

http://codereview.chromium.org/9005034/diff/3001/remoting/host/chromoting_hos...
remoting/host/chromoting_host.cc:107: this, allow_nat_traversal_);
nit: We should replace Init() with an AcceptConnections() method on
SessionManager.

http://codereview.chromium.org/9005034/diff/3001/remoting/host/chromoting_host.h
File remoting/host/chromoting_host.h (left):

http://codereview.chromium.org/9005034/diff/3001/remoting/host/chromoting_hos...
remoting/host/chromoting_host.h:104: virtual void OnJidChange(const std::string&
full_jid) OVERRIDE;
We can get rid of this because we'll see DISCONNECT->CONNECTED transition on
changes?

http://codereview.chromium.org/9005034/diff/3001/remoting/jingle_glue/javascr...
File remoting/jingle_glue/javascript_signal_strategy.cc (right):

http://codereview.chromium.org/9005034/diff/3001/remoting/jingle_glue/javascr...
remoting/jingle_glue/javascript_signal_strategy.cc:50: return CONNECTED;
Shouldn't this be based upon the state of |xmpp_proxy_|?

http://codereview.chromium.org/9005034/diff/3001/remoting/jingle_glue/signal_...
File remoting/jingle_glue/signal_strategy.h (right):

http://codereview.chromium.org/9005034/diff/3001/remoting/jingle_glue/signal_...
remoting/jingle_glue/signal_strategy.h:29: CLOSED,
CONNECTED -> DISCONNECTED, or OPEN -> CLOSED

http://codereview.chromium.org/9005034/diff/3001/remoting/jingle_glue/signal_...
remoting/jingle_glue/signal_strategy.h:37: virtual void
OnSignalStrategyStateChange(State state) = 0;
Is it safe to tear-down the SignallingStrategy from this callback?

http://codereview.chromium.org/9005034/diff/3001/remoting/jingle_glue/signal_...
remoting/jingle_glue/signal_strategy.h:37: virtual void
OnSignalStrategyStateChange(State state) = 0;
Consider giving this a default empty implementation, since a number of callers
don't need to hook it.

http://codereview.chromium.org/9005034/diff/3001/remoting/jingle_glue/signal_...
remoting/jingle_glue/signal_strategy.h:40: virtual bool
OnSignalStrategyIncomingStanza(
Same here?

http://codereview.chromium.org/9005034/diff/3001/remoting/jingle_glue/signal_...
remoting/jingle_glue/signal_strategy.h:48: // disconnects it and opens a new
connection. Connection is finished
Will the implicit disconnect trigger a CLOSED notification?

http://codereview.chromium.org/9005034/diff/3001/remoting/jingle_glue/signal_...
remoting/jingle_glue/signal_strategy.h:53: virtual void Disconnect() = 0;
Will the Listeners see CLOSED notifications?

http://codereview.chromium.org/9005034/diff/3001/remoting/jingle_glue/signal_...
remoting/jingle_glue/signal_strategy.h:59: virtual std::string GetLocalJid()
const = 0;
nit: GetLocalEndpointId?  This interface is notionally not specific to the Talk
transport.

http://codereview.chromium.org/9005034/diff/3001/remoting/jingle_glue/signal_...
remoting/jingle_glue/signal_strategy.h:62: // messages. Doesn't take ownership
of the |listener|.
Do Listeners need removing before teardown?

http://codereview.chromium.org/9005034/diff/3001/remoting/protocol/pepper_ses...
File remoting/protocol/pepper_session_manager.cc (right):

http://codereview.chromium.org/9005034/diff/3001/remoting/protocol/pepper_ses...
remoting/protocol/pepper_session_manager.cc:64: ready_ = true;
Could you base "ready" on whether we already had stuff in |transport_config_|,
and thereby remove the need for the |ready_| flag?

http://codereview.chromium.org/9005034/diff/3001/remoting/protocol/session_ma...
File remoting/protocol/session_manager.h (right):

http://codereview.chromium.org/9005034/diff/3001/remoting/protocol/session_ma...
remoting/protocol/session_manager.h:92: // Called when the session manager is
ready. May be called from
nit: ready for what? Perhaps just merge this sentence with the one about
outgoing connections? Can we also accept incoming connections after this
notification?

[Sergey Ulanov, 2011-12-22 21:45:10]

http://codereview.chromium.org/9005034/diff/3001/remoting/host/chromoting_hos...
File remoting/host/chromoting_host.cc (right):

http://codereview.chromium.org/9005034/diff/3001/remoting/host/chromoting_hos...
remoting/host/chromoting_host.cc:107: this, allow_nat_traversal_);
On 2011/12/21 23:35:30, Wez wrote:
> nit: We should replace Init() with an AcceptConnections() method on
> SessionManager.
Init() must be called before Connect() even if we don't need to accept incoming
connections, so I think Init() is the right name for this method.

http://codereview.chromium.org/9005034/diff/3001/remoting/host/chromoting_host.h
File remoting/host/chromoting_host.h (left):

http://codereview.chromium.org/9005034/diff/3001/remoting/host/chromoting_hos...
remoting/host/chromoting_host.h:104: virtual void OnJidChange(const std::string&
full_jid) OVERRIDE;
On 2011/12/21 23:35:30, Wez wrote:
> We can get rid of this because we'll see DISCONNECT->CONNECTED transition on
> changes?
Because I added GetLocalJid() method.

http://codereview.chromium.org/9005034/diff/3001/remoting/jingle_glue/javascr...
File remoting/jingle_glue/javascript_signal_strategy.cc (right):

http://codereview.chromium.org/9005034/diff/3001/remoting/jingle_glue/javascr...
remoting/jingle_glue/javascript_signal_strategy.cc:50: return CONNECTED;
On 2011/12/21 23:35:30, Wez wrote:
> Shouldn't this be based upon the state of |xmpp_proxy_|?

Ideally yes, but XmppProxy doesn't provide any information about connections
status, nor there is Javascript interface for it. It's not a problem because
this code is used only on the client side. I've added TODO to fix it.

http://codereview.chromium.org/9005034/diff/3001/remoting/jingle_glue/signal_...
File remoting/jingle_glue/signal_strategy.h (right):

http://codereview.chromium.org/9005034/diff/3001/remoting/jingle_glue/signal_...
remoting/jingle_glue/signal_strategy.h:29: CLOSED,
On 2011/12/21 23:35:30, Wez wrote:
> CONNECTED -> DISCONNECTED, or OPEN -> CLOSED

Done.

http://codereview.chromium.org/9005034/diff/3001/remoting/jingle_glue/signal_...
remoting/jingle_glue/signal_strategy.h:37: virtual void
OnSignalStrategyStateChange(State state) = 0;
On 2011/12/21 23:35:30, Wez wrote:
> Consider giving this a default empty implementation, since a number of callers
> don't need to hook it.

Done.

http://codereview.chromium.org/9005034/diff/3001/remoting/jingle_glue/signal_...
remoting/jingle_glue/signal_strategy.h:37: virtual void
OnSignalStrategyStateChange(State state) = 0;
On 2011/12/21 23:35:30, Wez wrote:
> Is it safe to tear-down the SignallingStrategy from this callback?

Done.

http://codereview.chromium.org/9005034/diff/3001/remoting/jingle_glue/signal_...
remoting/jingle_glue/signal_strategy.h:40: virtual bool
OnSignalStrategyIncomingStanza(
On 2011/12/21 23:35:30, Wez wrote:
> Same here?

Done.

http://codereview.chromium.org/9005034/diff/3001/remoting/jingle_glue/signal_...
remoting/jingle_glue/signal_strategy.h:48: // disconnects it and opens a new
connection. Connection is finished
On 2011/12/21 23:35:30, Wez wrote:
> Will the implicit disconnect trigger a CLOSED notification?

Done.

http://codereview.chromium.org/9005034/diff/3001/remoting/jingle_glue/signal_...
remoting/jingle_glue/signal_strategy.h:53: virtual void Disconnect() = 0;
On 2011/12/21 23:35:30, Wez wrote:
> Will the Listeners see CLOSED notifications?

Done.

http://codereview.chromium.org/9005034/diff/3001/remoting/jingle_glue/signal_...
remoting/jingle_glue/signal_strategy.h:59: virtual std::string GetLocalJid()
const = 0;
On 2011/12/21 23:35:30, Wez wrote:
> nit: GetLocalEndpointId?  This interface is notionally not specific to the
Talk
> transport.

This interface is specific to XMPP signalling messages (though it may be
implemented with different types of XMPP transport), and XMPP uses JIDs to
identify endpoints.

http://codereview.chromium.org/9005034/diff/3001/remoting/jingle_glue/signal_...
remoting/jingle_glue/signal_strategy.h:62: // messages. Doesn't take ownership
of the |listener|.
On 2011/12/21 23:35:30, Wez wrote:
> Do Listeners need removing before teardown?

Done.

http://codereview.chromium.org/9005034/diff/3001/remoting/protocol/pepper_ses...
File remoting/protocol/pepper_session_manager.cc (right):

http://codereview.chromium.org/9005034/diff/3001/remoting/protocol/pepper_ses...
remoting/protocol/pepper_session_manager.cc:64: ready_ = true;
On 2011/12/21 23:35:30, Wez wrote:
> Could you base "ready" on whether we already had stuff in |transport_config_|,
> and thereby remove the need for the |ready_| flag?

I don't think it is possible. transport_config_ content may be empty even after
we've received jingle-info message.

http://codereview.chromium.org/9005034/diff/3001/remoting/protocol/session_ma...
File remoting/protocol/session_manager.h (right):

http://codereview.chromium.org/9005034/diff/3001/remoting/protocol/session_ma...
remoting/protocol/session_manager.h:92: // Called when the session manager is
ready. May be called from
On 2011/12/21 23:35:30, Wez wrote:
> nit: ready for what? Perhaps just merge this sentence with the one about
> outgoing connections? 
Done

> Can we also accept incoming connections after this notification?
OnIncomingSession() may be called even before this method is called.

[Wez, 2012-01-03 15:07:05]

lgtm

[commit-bot: I haz the power, 2012-01-03 15:07:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sergeyu@chromium.org/9005034/14001

[commit-bot: I haz the power, 2012-01-03 15:07:26]

Can't process patch for file remoting/host/log_to_server.cc.
File's status is None, patchset upload is incomplete.