Minor cleanups in JingleSession and JingleSessionManager.

Minor cleanups in JingleSession and JingleSessionManager.

BUG=None
TEST=None

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

[Wez, 2011-07-06 21:24:25]

http://codereview.chromium.org/7312013/diff/2001/remoting/protocol/connection...
File remoting/protocol/connection_to_client.cc (right):

http://codereview.chromium.org/7312013/diff/2001/remoting/protocol/connection...
remoting/protocol/connection_to_client.cc:61: // If there is a channel then
close it and release the reference.
nit: Update this comment, e.g. "... release it, causing it to close"?

http://codereview.chromium.org/7312013/diff/2001/remoting/protocol/jingle_ses...
File remoting/protocol/jingle_session.cc (right):

http://codereview.chromium.org/7312013/diff/2001/remoting/protocol/jingle_ses...
remoting/protocol/jingle_session.cc:162: Close();
Is this safe?  Isn't it possible for |cricket_session_| to call back in to this
JingleSession after Terminate() has completed?

http://codereview.chromium.org/7312013/diff/2001/remoting/protocol/jingle_ses...
File remoting/protocol/jingle_session_manager.cc (right):

http://codereview.chromium.org/7312013/diff/2001/remoting/protocol/jingle_ses...
remoting/protocol/jingle_session_manager.cc:31: return
JingleSessionManager::Create();
Eeep, recursion!

http://codereview.chromium.org/7312013/diff/2001/remoting/protocol/jingle_ses...
remoting/protocol/jingle_session_manager.cc:40: port_allocator_session_factory);
Eeep, recursion!

http://codereview.chromium.org/7312013/diff/2001/remoting/protocol/jingle_ses...
remoting/protocol/jingle_session_manager.cc:59: Close();
Once we have removed ourself from |cricket_session_manager_|, there is no
possibility that it call us back?

http://codereview.chromium.org/7312013/diff/2001/remoting/protocol/jingle_ses...
File remoting/protocol/jingle_session_manager.h (right):

http://codereview.chromium.org/7312013/diff/2001/remoting/protocol/jingle_ses...
remoting/protocol/jingle_session_manager.h:43: static JingleSessionManager*
Create(
nit: It's not clear from the name what kinds of network manager, etc, Connect()
causes to be used; give it a more descriptive name?

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

http://codereview.chromium.org/7312013/diff/2001/remoting/protocol/session_ma...
remoting/protocol/session_manager.h:5: // The purprose of SessionManager is to
facilitate creation of chromotocol
nit: Typo "purprose" -> "purpose".

http://codereview.chromium.org/7312013/diff/2001/remoting/protocol/session_ma...
remoting/protocol/session_manager.h:22: // SessionManager owns all sessions it
creates. The server must not be
nit: "sessions" -> "Sessions".
nit: "server" -> "manager".

http://codereview.chromium.org/7312013/diff/2001/remoting/protocol/session_ma...
remoting/protocol/session_manager.h:24: // shutting down object must be
destroyed in the following order:
nit: Isn't this generally true, not just when shutting down?
nit: Typo "object" -> "objects".
nit: I'd suggest re-expressing this in terms of object lifetimes; the
SessionManager must out-live all Session instances, and in turn the JingleClient
must out-live the SessionManager.

http://codereview.chromium.org/7312013/diff/2001/remoting/protocol/session_ma...
remoting/protocol/session_manager.h:91: // take ownership of the session if it
ACCEPTs connection.
nit: "connection" -> "it" (the callback is ACCEPTing the session).

http://codereview.chromium.org/7312013/diff/2001/remoting/protocol/session_ma...
remoting/protocol/session_manager.h:100: // for both private key and
certificate.
nit: "Doesn't accept..." -> "Caller retains ownership of ..."

It feels like it would be cleaner to split details specific to server-side
SessionManagers to a derived interface, or at least a different API.  So you'd
have:

  void Initialize(local_jid, signal_strategy);

  // Server-side:
  void Listen(callback, private_key, certificate);

  // Client-side:
  void Connect(jid, key, token, config, callback);

[Sergey Ulanov, 2011-07-06 23:15:01]

http://codereview.chromium.org/7312013/diff/2001/remoting/protocol/connection...
File remoting/protocol/connection_to_client.cc (right):

http://codereview.chromium.org/7312013/diff/2001/remoting/protocol/connection...
remoting/protocol/connection_to_client.cc:61: // If there is a channel then
close it and release the reference.
On 2011/07/06 21:24:25, Wez wrote:
> nit: Update this comment, e.g. "... release it, causing it to close"?

Done.

http://codereview.chromium.org/7312013/diff/2001/remoting/protocol/jingle_ses...
File remoting/protocol/jingle_session.cc (right):

http://codereview.chromium.org/7312013/diff/2001/remoting/protocol/jingle_ses...
remoting/protocol/jingle_session.cc:162: Close();
On 2011/07/06 21:24:25, Wez wrote:
> Is this safe?  Isn't it possible for |cricket_session_| to call back in to
this
> JingleSession after Terminate() has completed?
It may. Added line that resets |state_change_callback_| so that we don't call it
while being destructed.

http://codereview.chromium.org/7312013/diff/2001/remoting/protocol/jingle_ses...
File remoting/protocol/jingle_session_manager.cc (right):

http://codereview.chromium.org/7312013/diff/2001/remoting/protocol/jingle_ses...
remoting/protocol/jingle_session_manager.cc:31: return
JingleSessionManager::Create();
On 2011/07/06 21:24:25, Wez wrote:
> Eeep, recursion!
Ouch.

http://codereview.chromium.org/7312013/diff/2001/remoting/protocol/jingle_ses...
remoting/protocol/jingle_session_manager.cc:40: port_allocator_session_factory);
On 2011/07/06 21:24:25, Wez wrote:
> Eeep, recursion!

Done.

http://codereview.chromium.org/7312013/diff/2001/remoting/protocol/jingle_ses...
remoting/protocol/jingle_session_manager.cc:59: Close();
On 2011/07/06 21:24:25, Wez wrote:
> Once we have removed ourself from |cricket_session_manager_|, there is no
> possibility that it call us back?
No, it should not call us after that. We also destroy cricket_session_manager_
in Close().

http://codereview.chromium.org/7312013/diff/2001/remoting/protocol/jingle_ses...
File remoting/protocol/jingle_session_manager.h (right):

http://codereview.chromium.org/7312013/diff/2001/remoting/protocol/jingle_ses...
remoting/protocol/jingle_session_manager.h:43: static JingleSessionManager*
Create(
On 2011/07/06 21:24:25, Wez wrote:
> nit: It's not clear from the name what kinds of network manager, etc,
Connect()
> causes to be used; give it a more descriptive name?

Done.

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

http://codereview.chromium.org/7312013/diff/2001/remoting/protocol/session_ma...
remoting/protocol/session_manager.h:5: // The purprose of SessionManager is to
facilitate creation of chromotocol
On 2011/07/06 21:24:25, Wez wrote:
> nit: Typo "purprose" -> "purpose".

Done.

http://codereview.chromium.org/7312013/diff/2001/remoting/protocol/session_ma...
remoting/protocol/session_manager.h:22: // SessionManager owns all sessions it
creates. The server must not be
On 2011/07/06 21:24:25, Wez wrote:
> nit: "sessions" -> "Sessions".
> nit: "server" -> "manager".

Done.

http://codereview.chromium.org/7312013/diff/2001/remoting/protocol/session_ma...
remoting/protocol/session_manager.h:24: // shutting down object must be
destroyed in the following order:
On 2011/07/06 21:24:25, Wez wrote:
> nit: Isn't this generally true, not just when shutting down?
> nit: Typo "object" -> "objects".
> nit: I'd suggest re-expressing this in terms of object lifetimes; the
> SessionManager must out-live all Session instances, and in turn the
JingleClient
> must out-live the SessionManager.

Done.

http://codereview.chromium.org/7312013/diff/2001/remoting/protocol/session_ma...
remoting/protocol/session_manager.h:91: // take ownership of the session if it
ACCEPTs connection.
On 2011/07/06 21:24:25, Wez wrote:
> nit: "connection" -> "it" (the callback is ACCEPTing the session).

Done.

http://codereview.chromium.org/7312013/diff/2001/remoting/protocol/session_ma...
remoting/protocol/session_manager.h:100: // for both private key and
certificate.
On 2011/07/06 21:24:25, Wez wrote:
> nit: "Doesn't accept..." -> "Caller retains ownership of ..."
Done
> 
> It feels like it would be cleaner to split details specific to server-side
> SessionManagers to a derived interface, or at least a different API.  So you'd
> have:
> 
>   void Initialize(local_jid, signal_strategy);
> 
>   // Server-side:
>   void Listen(callback, private_key, certificate);
> 
>   // Client-side:
>   void Connect(jid, key, token, config, callback);

I actually think we should have two separate interfaces - we never need to
accept and create connections from the same host. Added TODO for this.

[Wez, 2011-07-06 23:26:55]

LGTM modulo some nits, and a bunch of changes from another CL having crept in.

http://codereview.chromium.org/7312013/diff/3011/remoting/jingle_glue/jingle_...
File remoting/jingle_glue/jingle_thread.cc (right):

http://codereview.chromium.org/7312013/diff/3011/remoting/jingle_glue/jingle_...
remoting/jingle_glue/jingle_thread.cc:45: }
Aren't these changes from CL 7227017?

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

http://codereview.chromium.org/7312013/diff/3011/remoting/protocol/session_ma...
remoting/protocol/session_manager.h:12: // returned sessionion is initially in
INITIALIZING state. Later state is
nit: Old typo: "sessionion"

http://codereview.chromium.org/7312013/diff/3011/remoting/protocol/session_ma...
remoting/protocol/session_manager.h:24: // use. Caller owns Sessions created by
a SessionManager (except
The comment about closing makes the presence of a Close() call that "closes all
current sessions" confusing; are you trying to make the distinction that the
caller mustn't continue to use any Session after Close() on the manager?  If so
then better to document that requirement on Close() itself.

http://codereview.chromium.org/7312013/diff/3011/remoting/protocol/session_ma...
remoting/protocol/session_manager.h:26: // SessionManager must outlive
SignalStrategy.
The last comment is exactly the opposite of what's required, isn't it?

[Wez, 2011-07-06 23:39:41]

http://codereview.chromium.org/7312013/diff/2001/remoting/protocol/jingle_ses...
File remoting/protocol/jingle_session.cc (right):

http://codereview.chromium.org/7312013/diff/2001/remoting/protocol/jingle_ses...
remoting/protocol/jingle_session.cc:162: Close();
On 2011/07/06 23:15:01, sergeyu wrote:
> On 2011/07/06 21:24:25, Wez wrote:
> > Is this safe?  Isn't it possible for |cricket_session_| to call back in to
> this
> > JingleSession after Terminate() has completed?
> It may. Added line that resets |state_change_callback_| so that we don't call
it
> while being destructed.
> 

Actually, I think the only way to be sure that you won't be called back by a
cricket::Session at some point after Terminate() is to Destroy() it.

[Sergey Ulanov, 2011-07-07 00:50:30]

http://codereview.chromium.org/7312013/diff/2001/remoting/protocol/jingle_ses...
File remoting/protocol/jingle_session.cc (right):

http://codereview.chromium.org/7312013/diff/2001/remoting/protocol/jingle_ses...
remoting/protocol/jingle_session.cc:162: Close();
On 2011/07/06 23:39:41, Wez wrote:
> On 2011/07/06 23:15:01, sergeyu wrote:
> > On 2011/07/06 21:24:25, Wez wrote:
> > > Is this safe?  Isn't it possible for |cricket_session_| to call back in to
> > this
> > > JingleSession after Terminate() has completed?
> > It may. Added line that resets |state_change_callback_| so that we don't
call
> it
> > while being destructed.
> > 
> 
> Actually, I think the only way to be sure that you won't be called back by a
> cricket::Session at some point after Terminate() is to Destroy() it.

We cannot destroy it because cricket::SessionManager owns it. I added code in
CloseInternal() that disconnects cricket::Session::SignalState signal, but it's
not strictly necessary because destruction of sigslot::has_slots<> would do it
anyway.

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

http://codereview.chromium.org/7312013/diff/3011/remoting/protocol/session_ma...
remoting/protocol/session_manager.h:12: // returned sessionion is initially in
INITIALIZING state. Later state is
On 2011/07/06 23:26:55, Wez wrote:
> nit: Old typo: "sessionion"

Done.

http://codereview.chromium.org/7312013/diff/3011/remoting/protocol/session_ma...
remoting/protocol/session_manager.h:24: // use. Caller owns Sessions created by
a SessionManager (except
On 2011/07/06 23:26:55, Wez wrote:
> The comment about closing makes the presence of a Close() call that "closes
all
> current sessions" confusing; are you trying to make the distinction that the
> caller mustn't continue to use any Session after Close() on the manager?  If
so
> then better to document that requirement on Close() itself.

Done.

http://codereview.chromium.org/7312013/diff/3011/remoting/protocol/session_ma...
remoting/protocol/session_manager.h:26: // SessionManager must outlive
SignalStrategy.
On 2011/07/06 23:26:55, Wez wrote:
> The last comment is exactly the opposite of what's required, isn't it?

Done.

[Wez, 2011-07-07 01:03:00]

On 2011/07/07 00:50:30, sergeyu wrote:
> On 2011/07/06 23:39:41, Wez wrote:
> > Actually, I think the only way to be sure that you won't be called back by a
> > cricket::Session at some point after Terminate() is to Destroy() it.
> 
> We cannot destroy it because cricket::SessionManager owns it. I added code in
> CloseInternal() that disconnects cricket::Session::SignalState signal, but
it's
> not strictly necessary because destruction of sigslot::has_slots<> would do it
> anyway.

Sorry, the call I have in mind is SessionManager::DestroySession(), which (I
think) tears down the Session immediately.  It's cleaner to Terminate() if we
can, though, so that we notify the peer.  We can support that in the "normal"
case, but still use Destroy in tear-down, by watching for OnSessionDestroy()
notifications after Terminate(), to know if/when the Session is actually
destroyed.