Skip to content

Commit

Permalink
More cleanups in JingleSessionManager interface.
Browse files Browse the repository at this point in the history 
1. Removed Close() method.
2. Replaced Init() method with AcceptIncoming() that's called only
   on the host.

Review URL: https://codereview.chromium.org/1530523002

Cr-Commit-Position: refs/heads/master@{#365702}
  • Loading branch information
@SergeyUlanov
SergeyUlanov authored and Commit bot committed Dec 17, 2015
1 parent 6a12034 commit 1b03268
Show file tree
Hide file tree
Showing 13 changed files with 94 additions and 165 deletions.
7 changes: 2 additions & 5 deletions remoting/host/chromoting_host.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,6 @@ const net::BackoffEntry::Policy kDefaultBackoffPolicy = {
} // namespace

ChromotingHost::ChromotingHost(
SignalStrategy* signal_strategy,
DesktopEnvironmentFactory* desktop_environment_factory,
scoped_ptr<protocol::SessionManager> session_manager,
scoped_refptr<base::SingleThreadTaskRunner> audio_task_runner,
Expand All @@ -77,13 +76,11 @@ ChromotingHost::ChromotingHost(
video_encode_task_runner_(video_encode_task_runner),
network_task_runner_(network_task_runner),
ui_task_runner_(ui_task_runner),
signal_strategy_(signal_strategy),
started_(false),
login_backoff_(&kDefaultBackoffPolicy),
enable_curtaining_(false),
weak_factory_(this) {
DCHECK(network_task_runner_->BelongsToCurrentThread());
DCHECK(signal_strategy);

jingle_glue::JingleThreadWrapper::EnsureForCurrentMessageLoop();
}
Expand Down Expand Up @@ -114,8 +111,8 @@ void ChromotingHost::Start(const std::string& host_owner_email) {
FOR_EACH_OBSERVER(HostStatusObserver, status_observers_,
OnStart(host_owner_email));

// Start the SessionManager, supplying this ChromotingHost as the listener.
session_manager_->Init(signal_strategy_, this);
session_manager_->AcceptIncoming(
base::Bind(&ChromotingHost::OnIncomingSession, base::Unretained(this)));
}

void ChromotingHost::AddStatusObserver(HostStatusObserver* observer) {
Expand Down
12 changes: 3 additions & 9 deletions remoting/host/chromoting_host.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -62,13 +62,10 @@ class DesktopEnvironmentFactory;
// incoming connection.
class ChromotingHost : public base::NonThreadSafe,
public ClientSession::EventHandler,
public protocol::SessionManager::Listener,
public HostStatusMonitor {
public:
// Both |signal_strategy| and |desktop_environment_factory| should outlive
// this object.
// |desktop_environment_factory| must outlive this object.
ChromotingHost(
SignalStrategy* signal_strategy,
DesktopEnvironmentFactory* desktop_environment_factory,
scoped_ptr<protocol::SessionManager> session_manager,
scoped_refptr<base::SingleThreadTaskRunner> audio_task_runner,
Expand Down Expand Up @@ -122,10 +119,10 @@ class ChromotingHost : public base::NonThreadSafe,
const std::string& channel_name,
const protocol::TransportRoute& route) override;

// SessionManager::Listener implementation.
// Callback for SessionManager to accept incoming sessions.
void OnIncomingSession(
protocol::Session* session,
protocol::SessionManager::IncomingSessionResponse* response) override;
protocol::SessionManager::IncomingSessionResponse* response);

// The host uses a pairing registry to generate and store pairing information
// for clients for PIN-less authentication.
Expand Down Expand Up @@ -165,9 +162,6 @@ class ChromotingHost : public base::NonThreadSafe,
scoped_refptr<base::SingleThreadTaskRunner> network_task_runner_;
scoped_refptr<base::SingleThreadTaskRunner> ui_task_runner_;

// Connection objects.
SignalStrategy* signal_strategy_;

// Must be used on the network thread only.
base::ObserverList<HostStatusObserver> status_observers_;

Expand Down
59 changes: 22 additions & 37 deletions remoting/host/chromoting_host_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@
#include "remoting/protocol/fake_desktop_capturer.h"
#include "remoting/protocol/protocol_mock_objects.h"
#include "remoting/protocol/session_config.h"
#include "remoting/signaling/mock_signal_strategy.h"
#include "testing/gmock/include/gmock/gmock.h"
#include "testing/gmock_mutant.h"
#include "testing/gtest/include/gtest/gtest.h"
Expand Down Expand Up @@ -61,16 +60,14 @@ class ChromotingHostTest : public testing::Test {
desktop_environment_factory_.reset(new FakeDesktopEnvironmentFactory());
session_manager_ = new protocol::MockSessionManager();

host_.reset(new ChromotingHost(
&signal_strategy_,
desktop_environment_factory_.get(),
make_scoped_ptr(session_manager_),
task_runner_, // Audio
task_runner_, // Input
task_runner_, // Video capture
task_runner_, // Video encode
task_runner_, // Network
task_runner_)); // UI
host_.reset(new ChromotingHost(desktop_environment_factory_.get(),
make_scoped_ptr(session_manager_),
task_runner_, // Audio
task_runner_, // Input
task_runner_, // Video capture
task_runner_, // Video encode
task_runner_, // Network
task_runner_)); // UI
host_->AddStatusObserver(&host_status_observer_);

xmpp_login_ = "host@domain";
Expand Down Expand Up @@ -189,11 +186,11 @@ class ChromotingHostTest : public testing::Test {
desktop_environment_factory_.reset();
}

// Expect the host and session manager to start, and return the expectation
// that the session manager has started.
Expectation ExpectHostAndSessionManagerStart() {
// Starts the host.
void StartHost() {
EXPECT_CALL(host_status_observer_, OnStart(xmpp_login_));
return EXPECT_CALL(*session_manager_, Init(_, host_.get()));
EXPECT_CALL(*session_manager_, AcceptIncoming(_));
host_->Start(xmpp_login_);
}

// Expect a client to connect.
Expand All @@ -219,7 +216,6 @@ class ChromotingHostTest : public testing::Test {
base::MessageLoop message_loop_;
scoped_refptr<AutoThreadTaskRunner> task_runner_;
MockConnectionToClientEventHandler handler_;
MockSignalStrategy signal_strategy_;
scoped_ptr<FakeDesktopEnvironmentFactory> desktop_environment_factory_;
MockHostStatusObserver host_status_observer_;
scoped_ptr<ChromotingHost> host_;
Expand Down Expand Up @@ -268,30 +264,26 @@ class ChromotingHostTest : public testing::Test {
};

TEST_F(ChromotingHostTest, StartAndShutdown) {
ExpectHostAndSessionManagerStart();
host_->Start(xmpp_login_);
StartHost();
}

TEST_F(ChromotingHostTest, Connect) {
ExpectHostAndSessionManagerStart();
host_->Start(xmpp_login_);
StartHost();

// Shut down the host when the first video packet is received.
ExpectClientConnected(0);
SimulateClientConnection(0, true, false);
}

TEST_F(ChromotingHostTest, AuthenticationFailed) {
ExpectHostAndSessionManagerStart();
host_->Start(xmpp_login_);
StartHost();

EXPECT_CALL(host_status_observer_, OnAccessDenied(session_jid1_));
SimulateClientConnection(0, false, false);
}

TEST_F(ChromotingHostTest, Reconnect) {
ExpectHostAndSessionManagerStart();
host_->Start(xmpp_login_);
StartHost();

// Connect first client.
ExpectClientConnected(0);
Expand All @@ -311,8 +303,7 @@ TEST_F(ChromotingHostTest, Reconnect) {
}

TEST_F(ChromotingHostTest, ConnectWhenAnotherClientIsConnected) {
ExpectHostAndSessionManagerStart();
host_->Start(xmpp_login_);
StartHost();

// Connect first client.
ExpectClientConnected(0);
Expand All @@ -332,8 +323,7 @@ TEST_F(ChromotingHostTest, ConnectWhenAnotherClientIsConnected) {
}

TEST_F(ChromotingHostTest, IncomingSessionAccepted) {
ExpectHostAndSessionManagerStart();
host_->Start(xmpp_login_);
StartHost();

MockSession* session = session_unowned1_.get();
protocol::SessionManager::IncomingSessionResponse response =
Expand All @@ -347,8 +337,7 @@ TEST_F(ChromotingHostTest, IncomingSessionAccepted) {
}

TEST_F(ChromotingHostTest, LoginBackOffUponConnection) {
ExpectHostAndSessionManagerStart();
host_->Start(xmpp_login_);
StartHost();

protocol::SessionManager::IncomingSessionResponse response =
protocol::SessionManager::DECLINE;
Expand All @@ -364,16 +353,14 @@ TEST_F(ChromotingHostTest, LoginBackOffUponConnection) {
}

TEST_F(ChromotingHostTest, LoginBackOffUponAuthenticating) {
ExpectHostAndSessionManagerStart();
StartHost();

EXPECT_CALL(*session_unowned1_, Close(_)).WillOnce(
InvokeWithoutArgs(this, &ChromotingHostTest::NotifyConnectionClosed1));

EXPECT_CALL(*session_unowned2_, Close(_)).WillOnce(
InvokeWithoutArgs(this, &ChromotingHostTest::NotifyConnectionClosed2));

host_->Start(xmpp_login_);

protocol::SessionManager::IncomingSessionResponse response =
protocol::SessionManager::DECLINE;

Expand All @@ -394,8 +381,7 @@ TEST_F(ChromotingHostTest, LoginBackOffUponAuthenticating) {
}

TEST_F(ChromotingHostTest, OnSessionRouteChange) {
ExpectHostAndSessionManagerStart();
host_->Start(xmpp_login_);
StartHost();


ExpectClientConnected(0);
Expand All @@ -409,8 +395,7 @@ TEST_F(ChromotingHostTest, OnSessionRouteChange) {
}

TEST_F(ChromotingHostTest, DisconnectAllClients) {
ExpectHostAndSessionManagerStart();
host_->Start(xmpp_login_);
StartHost();

ExpectClientConnected(0);
SimulateClientConnection(0, true, false);
Expand Down
5 changes: 3 additions & 2 deletions remoting/host/it2me/it2me_host.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -239,7 +239,8 @@ void It2MeHost::FinishConnect() {
network_settings, protocol::TransportRole::SERVER)));

scoped_ptr<protocol::SessionManager> session_manager(
new protocol::JingleSessionManager(transport_factory.Pass()));
new protocol::JingleSessionManager(transport_factory.Pass(),
signal_strategy.get()));

scoped_ptr<protocol::CandidateSessionConfig> protocol_config =
protocol::CandidateSessionConfig::CreateDefault();
Expand All @@ -250,7 +251,7 @@ void It2MeHost::FinishConnect() {

// Create the host.
host_.reset(new ChromotingHost(
signal_strategy_.get(), desktop_environment_factory_.get(),
desktop_environment_factory_.get(),
session_manager.Pass(), host_context_->audio_task_runner(),
host_context_->input_task_runner(),
host_context_->video_capture_task_runner(),
Expand Down
5 changes: 3 additions & 2 deletions remoting/host/remoting_me2me_host.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1524,7 +1524,8 @@ void HostProcess::StartHost() {
new protocol::IceTransportFactory(transport_context));
}
scoped_ptr<protocol::SessionManager> session_manager(
new protocol::JingleSessionManager(transport_factory.Pass()));
new protocol::JingleSessionManager(transport_factory.Pass(),
signal_strategy_.get()));

scoped_ptr<protocol::CandidateSessionConfig> protocol_config =
protocol::CandidateSessionConfig::CreateDefault();
Expand All @@ -1540,7 +1541,7 @@ void HostProcess::StartHost() {
session_manager->set_protocol_config(protocol_config.Pass());

host_.reset(new ChromotingHost(
signal_strategy_.get(), desktop_environment_factory_.get(),
desktop_environment_factory_.get(),
session_manager.Pass(), context_->audio_task_runner(),
context_->input_task_runner(), context_->video_capture_task_runner(),
context_->video_encode_task_runner(), context_->network_task_runner(),
Expand Down
12 changes: 2 additions & 10 deletions remoting/protocol/connection_to_host_impl.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -94,9 +94,9 @@ void ConnectionToHostImpl::Connect(
signal_strategy_->AddListener(this);

session_manager_.reset(new JingleSessionManager(
make_scoped_ptr(new IceTransportFactory(transport_context))));
make_scoped_ptr(new IceTransportFactory(transport_context)),
signal_strategy));
session_manager_->set_protocol_config(candidate_config_->Clone());
session_manager_->Init(signal_strategy_, this);

SetState(CONNECTING, OK);

Expand Down Expand Up @@ -188,14 +188,6 @@ bool ConnectionToHostImpl::OnSignalStrategyIncomingStanza(
return false;
}

void ConnectionToHostImpl::OnIncomingSession(
Session* session,
SessionManager::IncomingSessionResponse* response) {
DCHECK(CalledOnValidThread());
// Client always rejects incoming sessions.
*response = SessionManager::DECLINE;
}

void ConnectionToHostImpl::OnSessionStateChange(Session::State state) {
DCHECK(CalledOnValidThread());
DCHECK(event_callback_);
Expand Down
6 changes: 0 additions & 6 deletions remoting/protocol/connection_to_host_impl.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,6 @@ class ClientVideoDispatcher;

class ConnectionToHostImpl : public ConnectionToHost,
public SignalStrategy::Listener,
public SessionManager::Listener,
public Session::EventHandler,
public ChannelDispatcherBase::EventHandler,
public base::NonThreadSafe {
Expand Down Expand Up @@ -71,11 +70,6 @@ class ConnectionToHostImpl : public ConnectionToHost,
void OnSignalStrategyStateChange(SignalStrategy::State state) override;
bool OnSignalStrategyIncomingStanza(const buzz::XmlElement* stanza) override;

// SessionManager::Listener interface.
void OnIncomingSession(
Session* session,
SessionManager::IncomingSessionResponse* response) override;

// Session::EventHandler interface.
void OnSessionStateChange(Session::State state) override;
void OnSessionRouteChange(const std::string& channel_name,
Expand Down
41 changes: 15 additions & 26 deletions remoting/protocol/jingle_session_manager.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,22 +21,24 @@ namespace remoting {
namespace protocol {

JingleSessionManager::JingleSessionManager(
scoped_ptr<TransportFactory> transport_factory)
: protocol_config_(CandidateSessionConfig::CreateDefault()),
transport_factory_(transport_factory.Pass()) {}
scoped_ptr<TransportFactory> transport_factory,
SignalStrategy* signal_strategy)
: transport_factory_(transport_factory.Pass()),
signal_strategy_(signal_strategy),
protocol_config_(CandidateSessionConfig::CreateDefault()),
iq_sender_(new IqSender(signal_strategy_)) {
signal_strategy_->AddListener(this);
}

JingleSessionManager::~JingleSessionManager() {
Close();
DCHECK(sessions_.empty());
signal_strategy_->RemoveListener(this);
}

void JingleSessionManager::Init(
SignalStrategy* signal_strategy,
SessionManager::Listener* listener) {
listener_ = listener;
signal_strategy_ = signal_strategy;
iq_sender_.reset(new IqSender(signal_strategy_));
void JingleSessionManager::AcceptIncoming(
const IncomingSessionCallback& incoming_session_callback) {
incoming_session_callback_ = incoming_session_callback;

signal_strategy_->AddListener(this);
}

void JingleSessionManager::set_protocol_config(
Expand All @@ -53,20 +55,6 @@ scoped_ptr<Session> JingleSessionManager::Connect(
return session.Pass();
}

void JingleSessionManager::Close() {
DCHECK(CalledOnValidThread());

// Close() can be called only after all sessions are destroyed.
DCHECK(sessions_.empty());

listener_ = nullptr;

if (signal_strategy_) {
signal_strategy_->RemoveListener(this);
signal_strategy_ = nullptr;
}
}

void JingleSessionManager::set_authenticator_factory(
scoped_ptr<AuthenticatorFactory> authenticator_factory) {
DCHECK(CalledOnValidThread());
Expand Down Expand Up @@ -111,7 +99,8 @@ bool JingleSessionManager::OnSignalStrategyIncomingStanza(
}

IncomingSessionResponse response = SessionManager::DECLINE;
listener_->OnIncomingSession(session, &response);
if (!incoming_session_callback_.is_null())
incoming_session_callback_.Run(session, &response);

if (response == SessionManager::ACCEPT) {
session->AcceptIncomingConnection(message);
Expand Down
Loading

0 comments on commit 1b03268

Please sign in to comment.