Skip to content

Commit b4b0927

Browse files
SergeyUlanovCommit bot
SergeyUlanov
authored and
Commit bot
committed
Use standard ICE in Chromoting.
Previously we were using legacy, non-standard version of ICE. This change adds ICE version negotiation and enabled standard ICE by default, when both peers support it. BUG=473758 Committed: https://crrev.com/5a5854ee3e1c5760b422f26d31909bfb5dca631f Cr-Commit-Position: refs/heads/master@{#326560} Review URL: https://codereview.chromium.org/1085703003 Cr-Commit-Position: refs/heads/master@{#326600}
1 parent 616c2c3 commit b4b0927

27 files changed

+744
-328
lines changed

‎remoting/client/jni/chromoting_jni_instance.cc

+2-1
Original file line numberDiff line numberDiff line change
@@ -434,7 +434,8 @@ void ChromotingJniInstance::ConnectToHostOnNetworkThread() {
434434

435435
scoped_ptr<protocol::TransportFactory> transport_factory(
436436
new protocol::LibjingleTransportFactory(
437-
signaling_.get(), port_allocator.Pass(), network_settings));
437+
signaling_.get(), port_allocator.Pass(), network_settings,
438+
protocol::TransportRole::CLIENT));
438439

439440
client_->Start(signaling_.get(), authenticator_.Pass(),
440441
transport_factory.Pass(), host_jid_, capabilities_);

‎remoting/client/plugin/chromoting_instance.cc

+3-3
Original file line numberDiff line numberDiff line change
@@ -712,10 +712,10 @@ void ChromotingInstance::HandleConnect(const base::DictionaryValue& data) {
712712
// Create TransportFactory.
713713
scoped_ptr<protocol::TransportFactory> transport_factory(
714714
new protocol::LibjingleTransportFactory(
715-
signal_strategy_.get(),
716-
PepperPortAllocator::Create(this).Pass(),
715+
signal_strategy_.get(), PepperPortAllocator::Create(this).Pass(),
717716
protocol::NetworkSettings(
718-
protocol::NetworkSettings::NAT_TRAVERSAL_FULL)));
717+
protocol::NetworkSettings::NAT_TRAVERSAL_FULL),
718+
protocol::TransportRole::CLIENT));
719719

720720
// Create Authenticator.
721721
scoped_ptr<protocol::ThirdPartyClientAuthenticator::TokenFetcher>

‎remoting/host/chromoting_host.cc

+5-3
Original file line numberDiff line numberDiff line change
@@ -285,15 +285,17 @@ void ChromotingHost::OnIncomingSession(
285285
return;
286286
}
287287

288-
protocol::SessionConfig config;
289-
if (!protocol_config_->Select(session->candidate_config(), &config)) {
288+
scoped_ptr<protocol::SessionConfig> config =
289+
protocol::SessionConfig::SelectCommon(session->candidate_config(),
290+
protocol_config_.get());
291+
if (!config) {
290292
LOG(WARNING) << "Rejecting connection from " << session->jid()
291293
<< " because no compatible configuration has been found.";
292294
*response = protocol::SessionManager::INCOMPATIBLE;
293295
return;
294296
}
295297

296-
session->set_config(config);
298+
session->set_config(config.Pass());
297299

298300
*response = protocol::SessionManager::ACCEPT;
299301

‎remoting/host/chromoting_host_unittest.cc

+8-12
Original file line numberDiff line numberDiff line change
@@ -108,9 +108,7 @@ class ChromotingHostTest : public testing::Test {
108108
session_jid1_ = "user@domain/rest-of-jid";
109109
session_config2_ = SessionConfig::ForTest();
110110
session_jid2_ = "user2@domain/rest-of-jid";
111-
session_unowned_config1_ = SessionConfig::ForTest();
112111
session_unowned_jid1_ = "user3@doman/rest-of-jid";
113-
session_unowned_config2_ = SessionConfig::ForTest();
114112
session_unowned_jid2_ = "user4@doman/rest-of-jid";
115113

116114
EXPECT_CALL(*session1_, jid())
@@ -132,9 +130,9 @@ class ChromotingHostTest : public testing::Test {
132130
.Times(AnyNumber())
133131
.WillRepeatedly(SaveArg<0>(&session_unowned2_event_handler_));
134132
EXPECT_CALL(*session1_, config())
135-
.WillRepeatedly(ReturnRef(session_config1_));
133+
.WillRepeatedly(ReturnRef(*session_config1_));
136134
EXPECT_CALL(*session2_, config())
137-
.WillRepeatedly(ReturnRef(session_config2_));
135+
.WillRepeatedly(ReturnRef(*session_config2_));
138136

139137
owned_connection1_.reset(new MockConnectionToClient(session1_,
140138
&host_stub1_));
@@ -421,7 +419,7 @@ class ChromotingHostTest : public testing::Test {
421419
ClientSession* client1_;
422420
std::string session_jid1_;
423421
MockSession* session1_; // Owned by |connection_|.
424-
SessionConfig session_config1_;
422+
scoped_ptr<SessionConfig> session_config1_;
425423
MockVideoStub video_stub1_;
426424
MockClientStub client_stub1_;
427425
MockHostStub host_stub1_;
@@ -430,15 +428,13 @@ class ChromotingHostTest : public testing::Test {
430428
ClientSession* client2_;
431429
std::string session_jid2_;
432430
MockSession* session2_; // Owned by |connection2_|.
433-
SessionConfig session_config2_;
431+
scoped_ptr<SessionConfig> session_config2_;
434432
MockVideoStub video_stub2_;
435433
MockClientStub client_stub2_;
436434
MockHostStub host_stub2_;
437435
scoped_ptr<MockSession> session_unowned1_; // Not owned by a connection.
438-
SessionConfig session_unowned_config1_;
439436
std::string session_unowned_jid1_;
440437
scoped_ptr<MockSession> session_unowned2_; // Not owned by a connection.
441-
SessionConfig session_unowned_config2_;
442438
std::string session_unowned_jid2_;
443439
protocol::Session::EventHandler* session_unowned1_event_handler_;
444440
protocol::Session::EventHandler* session_unowned2_event_handler_;
@@ -599,7 +595,7 @@ TEST_F(ChromotingHostTest, IncomingSessionAccepted) {
599595
ExpectHostAndSessionManagerStart();
600596
EXPECT_CALL(*session_unowned1_, candidate_config()).WillOnce(Return(
601597
default_candidate_config_.get()));
602-
EXPECT_CALL(*session_unowned1_, set_config(_));
598+
EXPECT_CALL(*session_unowned1_, set_config_ptr(_));
603599
EXPECT_CALL(*session_unowned1_, Close()).WillOnce(InvokeWithoutArgs(
604600
this, &ChromotingHostTest::NotifyConnectionClosed1));
605601
EXPECT_CALL(host_status_observer_, OnAccessDenied(_));
@@ -620,7 +616,7 @@ TEST_F(ChromotingHostTest, LoginBackOffUponConnection) {
620616
ExpectHostAndSessionManagerStart();
621617
EXPECT_CALL(*session_unowned1_, candidate_config()).WillOnce(
622618
Return(default_candidate_config_.get()));
623-
EXPECT_CALL(*session_unowned1_, set_config(_));
619+
EXPECT_CALL(*session_unowned1_, set_config_ptr(_));
624620
EXPECT_CALL(*session_unowned1_, Close()).WillOnce(
625621
InvokeWithoutArgs(this, &ChromotingHostTest::NotifyConnectionClosed1));
626622
EXPECT_CALL(host_status_observer_, OnAccessDenied(_));
@@ -646,13 +642,13 @@ TEST_F(ChromotingHostTest, LoginBackOffUponAuthenticating) {
646642
Expectation start = ExpectHostAndSessionManagerStart();
647643
EXPECT_CALL(*session_unowned1_, candidate_config()).WillOnce(
648644
Return(default_candidate_config_.get()));
649-
EXPECT_CALL(*session_unowned1_, set_config(_));
645+
EXPECT_CALL(*session_unowned1_, set_config_ptr(_));
650646
EXPECT_CALL(*session_unowned1_, Close()).WillOnce(
651647
InvokeWithoutArgs(this, &ChromotingHostTest::NotifyConnectionClosed1));
652648

653649
EXPECT_CALL(*session_unowned2_, candidate_config()).WillOnce(
654650
Return(default_candidate_config_.get()));
655-
EXPECT_CALL(*session_unowned2_, set_config(_));
651+
EXPECT_CALL(*session_unowned2_, set_config_ptr(_));
656652
EXPECT_CALL(*session_unowned2_, Close()).WillOnce(
657653
InvokeWithoutArgs(this, &ChromotingHostTest::NotifyConnectionClosed2));
658654

‎remoting/host/client_session_unittest.cc

+2-2
Original file line numberDiff line numberDiff line change
@@ -178,7 +178,7 @@ class ClientSessionTest : public testing::Test {
178178
MockClientSessionEventHandler session_event_handler_;
179179

180180
// Storage for values to be returned by the protocol::Session mock.
181-
SessionConfig session_config_;
181+
scoped_ptr<SessionConfig> session_config_;
182182
const std::string client_jid_;
183183

184184
// Stubs returned to |client_session_| components by |connection_|.
@@ -224,7 +224,7 @@ void ClientSessionTest::TearDown() {
224224
void ClientSessionTest::CreateClientSession() {
225225
// Mock protocol::Session APIs called directly by ClientSession.
226226
protocol::MockSession* session = new MockSession();
227-
EXPECT_CALL(*session, config()).WillRepeatedly(ReturnRef(session_config_));
227+
EXPECT_CALL(*session, config()).WillRepeatedly(ReturnRef(*session_config_));
228228
EXPECT_CALL(*session, jid()).WillRepeatedly(ReturnRef(client_jid_));
229229
EXPECT_CALL(*session, SetEventHandler(_));
230230

‎remoting/host/session_manager_factory.cc

+2-1
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,8 @@ scoped_ptr<protocol::SessionManager> CreateHostSessionManager(
2525

2626
scoped_ptr<protocol::TransportFactory> transport_factory(
2727
new protocol::LibjingleTransportFactory(
28-
signal_strategy, port_allocator.Pass(), network_settings));
28+
signal_strategy, port_allocator.Pass(), network_settings,
29+
protocol::TransportRole::SERVER));
2930

3031
scoped_ptr<protocol::JingleSessionManager> session_manager(
3132
new protocol::JingleSessionManager(transport_factory.Pass()));

‎remoting/protocol/content_description.cc

+19-23
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,7 @@ const char kDefaultNs[] = "";
2525

2626
// Following constants are used to format session description in XML.
2727
const char kDescriptionTag[] = "description";
28+
const char kStandardIceTag[] = "standard-ice";
2829
const char kControlTag[] = "control";
2930
const char kEventTag[] = "event";
3031
const char kVideoTag[] = "video";
@@ -121,17 +122,10 @@ ContentDescription::ContentDescription(
121122

122123
ContentDescription::~ContentDescription() { }
123124

124-
ContentDescription* ContentDescription::Copy() const {
125-
if (!candidate_config_.get() || !authenticator_message_.get()) {
126-
return nullptr;
127-
}
128-
scoped_ptr<XmlElement> message(new XmlElement(*authenticator_message_));
129-
return new ContentDescription(candidate_config_->Clone(), message.Pass());
130-
}
131-
132125
// ToXml() creates content description for chromoting session. The
133126
// description looks as follows:
134127
// <description xmlns="google:remoting">
128+
// <standard-ice/>
135129
// <control transport="stream" version="1" />
136130
// <event transport="datagram" version="1" />
137131
// <video transport="stream" codec="vp8" version="1" />
@@ -145,27 +139,25 @@ XmlElement* ContentDescription::ToXml() const {
145139
XmlElement* root = new XmlElement(
146140
QName(kChromotingXmlNamespace, kDescriptionTag), true);
147141

148-
std::list<ChannelConfig>::const_iterator it;
142+
if (config()->standard_ice()) {
143+
root->AddElement(
144+
new buzz::XmlElement(QName(kChromotingXmlNamespace, kStandardIceTag)));
145+
}
149146

150-
for (it = config()->control_configs().begin();
151-
it != config()->control_configs().end(); ++it) {
152-
root->AddElement(FormatChannelConfig(*it, kControlTag));
147+
for (const ChannelConfig& channel_config : config()->control_configs()) {
148+
root->AddElement(FormatChannelConfig(channel_config, kControlTag));
153149
}
154150

155-
for (it = config()->event_configs().begin();
156-
it != config()->event_configs().end(); ++it) {
157-
root->AddElement(FormatChannelConfig(*it, kEventTag));
151+
for (const ChannelConfig& channel_config : config()->event_configs()) {
152+
root->AddElement(FormatChannelConfig(channel_config, kEventTag));
158153
}
159154

160-
for (it = config()->video_configs().begin();
161-
it != config()->video_configs().end(); ++it) {
162-
root->AddElement(FormatChannelConfig(*it, kVideoTag));
155+
for (const ChannelConfig& channel_config : config()->video_configs()) {
156+
root->AddElement(FormatChannelConfig(channel_config, kVideoTag));
163157
}
164158

165-
for (it = config()->audio_configs().begin();
166-
it != config()->audio_configs().end(); ++it) {
167-
ChannelConfig config = *it;
168-
root->AddElement(FormatChannelConfig(config, kAudioTag));
159+
for (const ChannelConfig& channel_config : config()->audio_configs()) {
160+
root->AddElement(FormatChannelConfig(channel_config, kAudioTag));
169161
}
170162

171163
// Older endpoints require an initial-resolution tag, but otherwise ignore it.
@@ -192,7 +184,6 @@ bool ContentDescription::ParseChannelConfigs(
192184
bool codec_required,
193185
bool optional,
194186
std::list<ChannelConfig>* const configs) {
195-
196187
QName tag(kChromotingXmlNamespace, tag_name);
197188
const XmlElement* child = element->FirstNamed(tag);
198189
while (child) {
@@ -218,6 +209,11 @@ scoped_ptr<ContentDescription> ContentDescription::ParseXml(
218209
}
219210
scoped_ptr<CandidateSessionConfig> config(
220211
CandidateSessionConfig::CreateEmpty());
212+
213+
config->set_standard_ice(
214+
element->FirstNamed(QName(kChromotingXmlNamespace, kStandardIceTag)) !=
215+
nullptr);
216+
221217
if (!ParseChannelConfigs(element, kControlTag, false, false,
222218
config->mutable_control_configs()) ||
223219
!ParseChannelConfigs(element, kEventTag, false, false,

‎remoting/protocol/content_description.h

+2-5
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,6 @@
1010
#include "base/memory/ref_counted.h"
1111
#include "base/memory/scoped_ptr.h"
1212
#include "remoting/protocol/session_config.h"
13-
#include "third_party/webrtc/p2p/base/sessiondescription.h"
1413

1514
namespace buzz {
1615
class XmlElement;
@@ -24,15 +23,13 @@ namespace protocol {
2423
//
2524
// This class also provides a type abstraction so that the Chromotocol Session
2625
// interface does not need to depend on libjingle.
27-
class ContentDescription : public cricket::ContentDescription {
26+
class ContentDescription {
2827
public:
2928
static const char kChromotingContentName[];
3029

3130
ContentDescription(scoped_ptr<CandidateSessionConfig> config,
3231
scoped_ptr<buzz::XmlElement> authenticator_message);
33-
~ContentDescription() override;
34-
35-
ContentDescription* Copy() const override;
32+
~ContentDescription();
3633

3734
const CandidateSessionConfig* config() const {
3835
return candidate_config_.get();

‎remoting/protocol/fake_connection_to_host.cc

+1-1
Original file line numberDiff line numberDiff line change
@@ -85,7 +85,7 @@ void FakeConnectionToHost::SignalConnectionReady(bool ready) {
8585
}
8686

8787
const protocol::SessionConfig& FakeConnectionToHost::config() {
88-
return session_config_;
88+
return *session_config_;
8989
}
9090

9191
protocol::ClipboardStub* FakeConnectionToHost::clipboard_forwarder() {

‎remoting/protocol/fake_connection_to_host.h

+1-1
Original file line numberDiff line numberDiff line change
@@ -54,7 +54,7 @@ class FakeConnectionToHost : public protocol::ConnectionToHost {
5454
testing::NiceMock<protocol::MockClipboardStub> mock_clipboard_stub_;
5555
testing::NiceMock<protocol::MockHostStub> mock_host_stub_;
5656
testing::NiceMock<protocol::MockInputStub> mock_input_stub_;
57-
protocol::SessionConfig session_config_;
57+
scoped_ptr<protocol::SessionConfig> session_config_;
5858

5959
DISALLOW_COPY_AND_ASSIGN(FakeConnectionToHost);
6060
};

‎remoting/protocol/fake_session.cc

+3-3
Original file line numberDiff line numberDiff line change
@@ -36,11 +36,11 @@ const CandidateSessionConfig* FakeSession::candidate_config() {
3636
}
3737

3838
const SessionConfig& FakeSession::config() {
39-
return config_;
39+
return *config_;
4040
}
4141

42-
void FakeSession::set_config(const SessionConfig& config) {
43-
config_ = config;
42+
void FakeSession::set_config(scoped_ptr<SessionConfig> config) {
43+
config_ = config.Pass();
4444
}
4545

4646
StreamChannelFactory* FakeSession::GetTransportChannelFactory() {

‎remoting/protocol/fake_session.h

+2-2
Original file line numberDiff line numberDiff line change
@@ -39,15 +39,15 @@ class FakeSession : public Session {
3939
const std::string& jid() override;
4040
const CandidateSessionConfig* candidate_config() override;
4141
const SessionConfig& config() override;
42-
void set_config(const SessionConfig& config) override;
42+
void set_config(scoped_ptr<SessionConfig> config) override;
4343
StreamChannelFactory* GetTransportChannelFactory() override;
4444
StreamChannelFactory* GetMultiplexedChannelFactory() override;
4545
void Close() override;
4646

4747
public:
4848
EventHandler* event_handler_;
4949
scoped_ptr<const CandidateSessionConfig> candidate_config_;
50-
SessionConfig config_;
50+
scoped_ptr<SessionConfig> config_;
5151

5252
FakeStreamChannelFactory channel_factory_;
5353

0 commit comments

Comments
 (0)