Remove the need for ReplayPortAllocator.

BUG=none
TEST=still connects.

Review URL: http://codereview.chromium.org/6603003

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@76849 0039d316-1c4b-4281-b951-d872f2087c98

4 files changed, 92 insertions, 105 deletions

diff --git a/remoting/jingle_glue/jingle_client.cc b/remoting/jingle_glue/jingle_client.cc
index 02593ed..085e47c 100644
--- a/remoting/jingle_glue/jingle_client.cc
+++ b/remoting/jingle_glue/jingle_client.cc
@@ -8,13 +8,14 @@
 #include "base/message_loop.h"
 #include "jingle/notifier/communicator/gaia_token_pre_xmpp_auth.h"
 #include "remoting/jingle_glue/iq_request.h"
+#include "remoting/jingle_glue/jingle_info_task.h"
 #include "remoting/jingle_glue/jingle_thread.h"
-#include "remoting/jingle_glue/relay_port_allocator.h"
 #include "remoting/jingle_glue/xmpp_socket_adapter.h"
 #include "third_party/libjingle/source/talk/base/asyncsocket.h"
 #include "third_party/libjingle/source/talk/base/ssladapter.h"
 #include "third_party/libjingle/source/talk/p2p/base/sessionmanager.h"
 #include "third_party/libjingle/source/talk/p2p/base/transport.h"
+#include "third_party/libjingle/source/talk/p2p/client/httpportallocator.h"
 #include "third_party/libjingle/source/talk/p2p/client/sessionmanagertask.h"
 #include "third_party/libjingle/source/talk/session/tunnel/tunnelsessionclient.h"
 #include "third_party/libjingle/source/talk/xmpp/prexmppauth.h"
@@ -35,7 +36,8 @@ XmppSignalStrategy::XmppSignalStrategy(JingleThread* jingle_thread,
      auth_token_(auth_token),
      auth_token_service_(auth_token_service),
      xmpp_client_(NULL),
-     observer_(NULL) {
+     observer_(NULL),
+     port_allocator_(NULL) {
 }
 
 XmppSignalStrategy::~XmppSignalStrategy() {
@@ -62,17 +64,20 @@ void XmppSignalStrategy::Init(StatusObserver* observer) {
   xmpp_client_->SignalStateChange.connect(
       this, &XmppSignalStrategy::OnConnectionStateChanged);
   xmpp_client_->Start();
-
-  // Setup the port allocation based on jingle connections.
-  network_manager_.reset(new talk_base::NetworkManager());
-  RelayPortAllocator* port_allocator =
-      new RelayPortAllocator(network_manager_.get(), "transp2");
-  port_allocator->SetJingleInfo(xmpp_client_);
-  port_allocator_.reset(port_allocator);
 }
 
-cricket::BasicPortAllocator* XmppSignalStrategy::port_allocator() {
-  return port_allocator_.get();
+void XmppSignalStrategy::ConfigureAllocator(
+    cricket::HttpPortAllocator* port_allocator, Task* done) {
+  // TODO(ajwong): There are 2 races on destruction here. First, port_allocator
+  // by be destroyed before the OnJingleInfo is run.  Second,
+  // XmppSignalStrategy itself may be destroyed.  Fix later.
+  port_allocator_ = port_allocator;
+  allocator_config_cb_.reset(done);
+
+  JingleInfoTask* jit = new JingleInfoTask(xmpp_client_);
+  jit->SignalJingleInfo.connect(this, &XmppSignalStrategy::OnJingleInfo);
+  jit->Start();
+  jit->RefreshJingleInfoNow();
 }
 
 void XmppSignalStrategy::StartSession(
@@ -120,6 +125,23 @@ void XmppSignalStrategy::OnConnectionStateChanged(
   }
 }
 
+void XmppSignalStrategy::OnJingleInfo(
+    const std::string& token,
+    const std::vector<std::string>& relay_hosts,
+    const std::vector<talk_base::SocketAddress>& stun_hosts) {
+  // TODO(ajwong): Log that we found the stun/turn servers.
+  if (port_allocator_) {
+    port_allocator_->SetRelayToken(token);
+    port_allocator_->SetStunHosts(stun_hosts);
+    port_allocator_->SetRelayHosts(relay_hosts);
+  }
+
+  if (allocator_config_cb_.get()) {
+    allocator_config_cb_->Run();
+    allocator_config_cb_.reset();
+  }
+}
+
 buzz::PreXmppAuth* XmppSignalStrategy::CreatePreXmppAuth(
     const buzz::XmppClientSettings& settings) {
   buzz::Jid jid(settings.user(), settings.host(), buzz::STR_EMPTY);
@@ -138,9 +160,11 @@ void JavascriptSignalStrategy::Init(StatusObserver* observer) {
   NOTIMPLEMENTED();
 }
 
-cricket::BasicPortAllocator* JavascriptSignalStrategy::port_allocator() {
+void JavascriptSignalStrategy::ConfigureAllocator(
+    cricket::HttpPortAllocator* port_allocator, Task* done) {
   NOTIMPLEMENTED();
-  return NULL;
+  done->Run();
+  delete done;
 }
 
 void JavascriptSignalStrategy::StartSession(
@@ -186,12 +210,33 @@ void JingleClient::Init() {
 void JingleClient::DoInitialize() {
   DCHECK_EQ(message_loop(), MessageLoop::current());
 
+  network_manager_.reset(new talk_base::NetworkManager());
+  port_allocator_.reset(
+      new cricket::HttpPortAllocator(network_manager_.get(), "transp2"));
+  // TODO(ajwong): The strategy needs a "start" command or something.  Right
+  // now, Init() implicitly starts processing events.  Thus, we must have the
+  // other fields of JingleClient initialized first, otherwise the state-change
+  // may occur and callback into class before we're done initializing.
   signal_strategy_->Init(this);
+  signal_strategy_->ConfigureAllocator(
+      port_allocator_.get(),
+      NewRunnableMethod(this, &JingleClient::DoStartSession));
+}
 
+void JingleClient::DoStartSession() {
   session_manager_.reset(
-      new cricket::SessionManager(signal_strategy_->port_allocator()));
-
+      new cricket::SessionManager(port_allocator_.get()));
   signal_strategy_->StartSession(session_manager_.get());
+
+  // TODO(ajwong): Major hack to synchronize state change logic.  Since the Xmpp
+  // connection starts first, it move the state to CONNECTED before we've gotten
+  // the jingle stun and relay information. Thus, we have to delay signaling
+  // until now.  There is a parallel if to disable signaling in the
+  // OnStateChange logic.
+  initialized_finished_ = true;
+  if (!closed_ && state_ == CONNECTED) {
+    callback_->OnStateChange(this, state_);
+  }
 }
 
 void JingleClient::Close() {
@@ -255,8 +300,15 @@ void JingleClient::OnStateChange(State new_state) {
       // We have to have the lock held, otherwise we cannot be sure that
       // the client hasn't been closed when we call the callback.
       base::AutoLock auto_lock(state_lock_);
-      if (!closed_)
-        callback_->OnStateChange(this, new_state);
+      if (!closed_) {
+        // TODO(ajwong): HACK! remove this.  See DoStartSession() for details.
+        //
+        // If state is connected, only signal if initialized_finished_ is also
+        // finished.
+        if (state_ != CONNECTED || initialized_finished_) {
+          callback_->OnStateChange(this, new_state);
+        }
+      }
     }
   }
 }
diff --git a/remoting/jingle_glue/jingle_client.h b/remoting/jingle_glue/jingle_client.h
index f60e029..ba759b9 100644
--- a/remoting/jingle_glue/jingle_client.h
+++ b/remoting/jingle_glue/jingle_client.h
@@ -24,6 +24,10 @@ class PreXmppAuth;
 }  // namespace buzz
 
 namespace cricket {
+class HttpPortAllocator;
+}  // namespace cricket
+
+namespace cricket {
 class BasicPortAllocator;
 class SessionManager;
 class TunnelSessionClient;
@@ -57,7 +61,8 @@ class SignalStrategy {
   SignalStrategy() {}
   virtual ~SignalStrategy() {}
   virtual void Init(StatusObserver* observer) = 0;
-  virtual cricket::BasicPortAllocator* port_allocator() = 0;
+  virtual void ConfigureAllocator(
+      cricket::HttpPortAllocator* port_allocator, Task* done) = 0;
   virtual void StartSession(cricket::SessionManager* session_manager) = 0;
   virtual void EndSession() = 0;
   virtual IqRequest* CreateIqRequest() = 0;
@@ -75,13 +80,19 @@ class XmppSignalStrategy : public SignalStrategy, public sigslot::has_slots<> {
   virtual ~XmppSignalStrategy();
 
   virtual void Init(StatusObserver* observer);
-  virtual cricket::BasicPortAllocator* port_allocator();
+  virtual void ConfigureAllocator(
+      cricket::HttpPortAllocator* port_allocator, Task* done);
   virtual void StartSession(cricket::SessionManager* session_manager);
   virtual void EndSession();
   virtual IqRequest* CreateIqRequest();
 
  private:
+  friend class JingleClientTest;
+
   void OnConnectionStateChanged(buzz::XmppEngine::State state);
+  void OnJingleInfo(const std::string& token,
+                    const std::vector<std::string>& relay_hosts,
+                    const std::vector<talk_base::SocketAddress>& stun_hosts);
   static buzz::PreXmppAuth* CreatePreXmppAuth(
       const buzz::XmppClientSettings& settings);
 
@@ -93,10 +104,9 @@ class XmppSignalStrategy : public SignalStrategy, public sigslot::has_slots<> {
   buzz::XmppClient* xmpp_client_;
   StatusObserver* observer_;
   scoped_ptr<talk_base::NetworkManager> network_manager_;
-  scoped_ptr<cricket::BasicPortAllocator> port_allocator_;
 
- private:
-  friend class JingleClientTest;
+  cricket::HttpPortAllocator* port_allocator_;
+  scoped_ptr<Task> allocator_config_cb_;
 
   DISALLOW_COPY_AND_ASSIGN(XmppSignalStrategy);
 };
@@ -107,7 +117,8 @@ class JavascriptSignalStrategy : public SignalStrategy {
   virtual ~JavascriptSignalStrategy();
 
   virtual void Init(StatusObserver* observer);
-  virtual cricket::BasicPortAllocator* port_allocator();
+  virtual void ConfigureAllocator(
+      cricket::HttpPortAllocator* port_allocator, Task* done);
   virtual void StartSession(cricket::SessionManager* session_manager);
   virtual void EndSession();
   virtual IqRequest* CreateIqRequest();
@@ -162,6 +173,7 @@ class JingleClient : public base::RefCountedThreadSafe<JingleClient>,
   friend class JingleClientTest;
 
   void DoInitialize();
+  void DoStartSession();
   void DoClose();
 
   // Updates current state of the connection. Must be called only in
@@ -182,6 +194,9 @@ class JingleClient : public base::RefCountedThreadSafe<JingleClient>,
   bool closed_;
   scoped_ptr<Task> closed_task_;
 
+  // TODO(ajwong): HACK! remove this.  See DoStartSession() for details.
+  bool initialized_finished_;
+
   // We need a separate lock for the jid since the |state_lock_| may be held
   // over a callback which can end up having a double lock.
   //
@@ -194,6 +209,8 @@ class JingleClient : public base::RefCountedThreadSafe<JingleClient>,
   Callback* callback_;
 
   SignalStrategy* signal_strategy_;
+  scoped_ptr<talk_base::NetworkManager> network_manager_;
+  scoped_ptr<cricket::HttpPortAllocator> port_allocator_;
   scoped_ptr<cricket::SessionManager> session_manager_;
 
   DISALLOW_COPY_AND_ASSIGN(JingleClient);
diff --git a/remoting/jingle_glue/relay_port_allocator.cc b/remoting/jingle_glue/relay_port_allocator.cc
deleted file mode 100644
index 98af60a..0000000
--- a/remoting/jingle_glue/relay_port_allocator.cc
+++ /dev/null
@@ -1,39 +0,0 @@
-// Copyright (c) 2010 The Chromium Authors. All rights reserved.
-// Use of this source code is governed by a BSD-style license that can be
-// found in the LICENSE file.
-
-#include "remoting/jingle_glue/relay_port_allocator.h"
-
-#include <vector>
-
-#include "remoting/jingle_glue/jingle_info_task.h"
-#include "third_party/libjingle/source/talk/xmpp/xmppclient.h"
-
-namespace remoting {
-
-RelayPortAllocator::RelayPortAllocator(
-    talk_base::NetworkManager* network_manager,
-    const std::string& user_agent)
-    : cricket::HttpPortAllocator(network_manager, user_agent) {
-}
-
-RelayPortAllocator::~RelayPortAllocator() {}
-
-void RelayPortAllocator::OnJingleInfo(
-    const std::string& token,
-    const std::vector<std::string>& relay_hosts,
-    const std::vector<talk_base::SocketAddress>& stun_hosts) {
-  this->SetRelayToken(token);
-  this->SetStunHosts(stun_hosts);
-  this->SetRelayHosts(relay_hosts);
-}
-
-void RelayPortAllocator::SetJingleInfo(buzz::XmppClient* client) {
-  // The JingleInfoTask is freed by the task-runner.
-  JingleInfoTask* jit = new JingleInfoTask(client);
-  jit->SignalJingleInfo.connect(this, &RelayPortAllocator::OnJingleInfo);
-  jit->Start();
-  jit->RefreshJingleInfoNow();
-}
-
-}  // namespace remoting
diff --git a/remoting/jingle_glue/relay_port_allocator.h b/remoting/jingle_glue/relay_port_allocator.h
deleted file mode 100644
index 0e8f25d..0000000
--- a/remoting/jingle_glue/relay_port_allocator.h
+++ /dev/null
@@ -1,43 +0,0 @@
-// Copyright (c) 2010 The Chromium Authors. All rights reserved.
-// Use of this source code is governed by a BSD-style license that can be
-// found in the LICENSE file.
-
-#ifndef REMOTING_JINGLE_GLUE_RELAY_PORT_ALLOCATOR_H_
-#define REMOTING_JINGLE_GLUE_RELAY_PORT_ALLOCATOR_H_
-
-#include <string>
-#include <vector>
-
-#include "third_party/libjingle/source/talk/base/sigslot.h"
-#include "third_party/libjingle/source/talk/p2p/client/httpportallocator.h"
-
-namespace buzz {
-class XmppClient;
-}  // namespace buzz
-
-namespace remoting {
-
-class RelayPortAllocator : public cricket::HttpPortAllocator,
-                           public sigslot::has_slots<> {
- public:
-  // Caller keeps ownership of |network_manager|. |network_manager| must not
-  // be destroyed before RelayPortAllocator.
-  RelayPortAllocator(talk_base::NetworkManager* network_manager,
-                     const std::string& user_agent);
-  virtual ~RelayPortAllocator();
-
-  void OnJingleInfo(const std::string& token,
-                    const std::vector<std::string>& relay_hosts,
-                    const std::vector<talk_base::SocketAddress>& stun_hosts);
-
-  // Starts JingleInfoTask to get new relay information. Caller keeps ownership
-  // of |client|.
-  void SetJingleInfo(buzz::XmppClient* client);
-
- private:
-  DISALLOW_COPY_AND_ASSIGN(RelayPortAllocator);
-};
-
-}  // namespace remoting
-
-#endif  // REMOTING_JINGLE_GLUE_RELAY_PORT_ALLOCATOR_H_