Minor cleanups in JingleSession and JingleSessionManager.

remoting/protocol/session_manager.h

Index: remoting/protocol/session_manager.h
diff --git a/remoting/protocol/session_manager.h b/remoting/protocol/session_manager.h
index 376559d6808b1996f1dd32474f3355a7e10976c3..6c8a605fe1574f6d890d6f99029fda8328a862fc 100644
--- a/remoting/protocol/session_manager.h
+++ b/remoting/protocol/session_manager.h
@@ -19,12 +19,12 @@
 // rejected.
 //
 // SESSION OWNERSHIP AND SHUTDOWN
-// SessionManager owns all Chromotocol Session it creates. The server
-// must not be closed while sessions created by the server are still in use.
-// When shutting down the Close() method for the sessionion and the server
-// objects must be called in the following order: Session,
-// SessionManager, JingleClient. The same order must be followed in the case
-// of rejected and failed sessions.
+// SessionManager owns all sessions it creates. The server must not be

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

nit: "sessions" -> "Sessions".
nit: "server" -> "manager".

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

Done.

+// closed while sessions created by the server are still in use. When
+// shutting down object must be destroyed in the following order:

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

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.

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

Done.

+// Session, SessionManager, JingleClient. The same order must be
+// followed in the case of failed sessions. Rejected sessions are
+// destroyed by the session manager.
 //
 // PROTOCOL VERSION NEGOTIATION
 // When client connects to a host it sends a session-initiate stanza with list
@@ -88,18 +88,16 @@ class SessionManager : public base::NonThreadSafe {
   // has incompatible configuration, and cannot be accepted.  If the
   // callback accepts session then it must also set configuration for
   // the new session using Session::set_config(). The callback must
-  // take ownership of the session if it accepts connection.
+  // take ownership of the session if it ACCEPTs connection.

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

nit: "connection" -> "it" (the callback is ACCEPTing the session).

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

Done.

   typedef Callback2<Session*, IncomingSessionResponse*>::Type
       IncomingSessionCallback;
 
   // Initializes the session client. Doesn't accept ownership of the
-  // |signal_strategy|. Close() must be called _before_ the |session_manager|
-  // is destroyed.
-  // If this object is used in server mode, then |private_key| and
-  // |certificate| are used to establish a secured communication with the
-  // client. It will also take ownership of these objects.
-  // In case this is used in client mode, pass in NULL for both private key and
-  // certificate.
+  // |signal_strategy|. If this object is used in server mode, then
+  // |private_key| and |certificate| are used to establish a secured
+  // communication with the client. It will also take ownership of
+  // these objects. In case this is used in client mode, pass in NULL
+  // for both private key and certificate.

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

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]

Done
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.

   virtual void Init(const std::string& local_jid,
                     SignalStrategy* signal_strategy,
                     IncomingSessionCallback* incoming_session_callback,
@@ -114,9 +112,6 @@ class SessionManager : public base::NonThreadSafe {
   // |config| contains the session configurations that the client supports.
   // |state_change_callback| is called when the connection state changes.
   //
-  // This function may be called from any thread. The |state_change_callback|
-  // is invoked on the network thread.
-  //
   // Ownership of the |config| is passed to the new session.
   virtual Session* Connect(
       const std::string& host_jid,