Enable TURN on the host when using WebRTC.

Enable TURN on the host when using WebRTC.

BUG=577954
Committed: https://crrev.com/e8e7cb43534bb63ce2972cc0a51f614cda15db3f
Cr-Commit-Position: refs/heads/master@{#381520}

[Sergey Ulanov, 2016-03-14 23:36:18]

Patchset #1 (id:1) has been deleted

[Sergey Ulanov, 2016-03-14 23:36:52]

sergeyu@chromium.org changed reviewers:
+ garykac@chromium.org

[garykac, 2016-03-15 00:13:16]

https://codereview.chromium.org/1800893002/diff/20001/remoting/host/chromotin...
File remoting/host/chromoting_host_unittest.cc (right):

https://codereview.chromium.org/1800893002/diff/20001/remoting/host/chromotin...
remoting/host/chromoting_host_unittest.cc:69: nullptr,
Can you make this non-null and add some tests here?

https://codereview.chromium.org/1800893002/diff/20001/remoting/host/it2me/it2...
File remoting/host/it2me/it2me_host.cc (right):

https://codereview.chromium.org/1800893002/diff/20001/remoting/host/it2me/it2...
remoting/host/it2me/it2me_host.cc:239: scoped_refptr<protocol::TransportContext>
transport_context =
For consistency, this should be 'ice_transport_context'.

https://codereview.chromium.org/1800893002/diff/20001/remoting/test/protocol_...
File remoting/test/protocol_perftest.cc (right):

https://codereview.chromium.org/1800893002/diff/20001/remoting/test/protocol_...
remoting/test/protocol_perftest.cc:336: transport_context, transport_context,
host_thread_.task_runner(),
Shouldn't there be separate ice_ and _webrtc_ transport_contexts here?

And do we run separate ice and webrtc perf tests?

[Sergey Ulanov, 2016-03-15 20:17:31]

In the new patchset I implemented a different approach that I think is better.
Now the same TransportContext can be used for both TURN and GTURN mode and it
allows switching between the two modes dynamically.

https://codereview.chromium.org/1800893002/diff/20001/remoting/host/chromotin...
File remoting/host/chromoting_host_unittest.cc (right):

https://codereview.chromium.org/1800893002/diff/20001/remoting/host/chromotin...
remoting/host/chromoting_host_unittest.cc:69: nullptr,
On 2016/03/15 00:13:16, garykac wrote:
> Can you make this non-null and add some tests here?

Removed this parameter

https://codereview.chromium.org/1800893002/diff/20001/remoting/host/it2me/it2...
File remoting/host/it2me/it2me_host.cc (right):

https://codereview.chromium.org/1800893002/diff/20001/remoting/host/it2me/it2...
remoting/host/it2me/it2me_host.cc:239: scoped_refptr<protocol::TransportContext>
transport_context =
On 2016/03/15 00:13:16, garykac wrote:
> For consistency, this should be 'ice_transport_context'.

removed webrtc_transport_context

https://codereview.chromium.org/1800893002/diff/20001/remoting/test/protocol_...
File remoting/test/protocol_perftest.cc (right):

https://codereview.chromium.org/1800893002/diff/20001/remoting/test/protocol_...
remoting/test/protocol_perftest.cc:336: transport_context, transport_context,
host_thread_.task_runner(),
On 2016/03/15 00:13:16, garykac wrote:
> Shouldn't there be separate ice_ and _webrtc_ transport_contexts here?

Reverted this. There is no reason to use a different transport_context for
webrtc protocol.

> 
> And do we run separate ice and webrtc perf tests?

There is TotalLatencyWebrtc that measures WebRTC protocol perf.

[garykac, 2016-03-15 20:39:00]

https://codereview.chromium.org/1800893002/diff/40001/remoting/protocol/trans...
File remoting/protocol/transport_context.cc (right):

https://codereview.chromium.org/1800893002/diff/40001/remoting/protocol/trans...
remoting/protocol/transport_context.cc:87: LOG(WARNING) << "ice_config_url isn't
set.";
Should this be an error? If the config url is not set, then it doesn't seem like
it's ensuring a fresh ice config.

https://codereview.chromium.org/1800893002/diff/40001/remoting/protocol/trans...
File remoting/protocol/transport_context.h (right):

https://codereview.chromium.org/1800893002/diff/40001/remoting/protocol/trans...
remoting/protocol/transport_context.h:39: static const int kNumRelayModes =
RelayMode::TURN + 1;
The enum should end with MAX_RELAYMODE (or something like that), and we should
use that value instead of TURN+1.

If someone adds a new RelayMode at the end of the enum, then this code will
break.

[Sergey Ulanov, 2016-03-15 21:54:53]

https://codereview.chromium.org/1800893002/diff/40001/remoting/protocol/trans...
File remoting/protocol/transport_context.cc (right):

https://codereview.chromium.org/1800893002/diff/40001/remoting/protocol/trans...
remoting/protocol/transport_context.cc:87: LOG(WARNING) << "ice_config_url isn't
set.";
On 2016/03/15 20:38:59, garykac wrote:
> Should this be an error? If the config url is not set, then it doesn't seem
like
> it's ensuring a fresh ice config.

There are cases when this URL won't be set and it's safe to ignore this issue.
Particularly for tests it's not set, but tests also don't need to use STUN or
TURN.

https://codereview.chromium.org/1800893002/diff/40001/remoting/protocol/trans...
File remoting/protocol/transport_context.h (right):

https://codereview.chromium.org/1800893002/diff/40001/remoting/protocol/trans...
remoting/protocol/transport_context.h:39: static const int kNumRelayModes =
RelayMode::TURN + 1;
On 2016/03/15 20:38:59, garykac wrote:
> The enum should end with MAX_RELAYMODE (or something like that), and we should
> use that value instead of TURN+1.
> 
> If someone adds a new RelayMode at the end of the enum, then this code will
> break.

That would make the compiler complain that MAX_RELAYMODE is not handled in
switch statement in TransportContext::EnsureFreshIceConfig(). It's rather
obvious that this value need to be updated if a new mode needs to be added, but
it's also unlikely that we will ever need to add another value here.

[garykac, 2016-03-16 00:58:21]

https://codereview.chromium.org/1800893002/diff/40001/remoting/protocol/trans...
File remoting/protocol/transport_context.cc (right):

https://codereview.chromium.org/1800893002/diff/40001/remoting/protocol/trans...
remoting/protocol/transport_context.cc:87: LOG(WARNING) << "ice_config_url isn't
set.";
On 2016/03/15 21:54:53, Sergey Ulanov wrote:
> On 2016/03/15 20:38:59, garykac wrote:
> > Should this be an error? If the config url is not set, then it doesn't seem
> like
> > it's ensuring a fresh ice config.
> 
> There are cases when this URL won't be set and it's safe to ignore this issue.
> Particularly for tests it's not set, but tests also don't need to use STUN or
> TURN.

Acknowledged.

https://codereview.chromium.org/1800893002/diff/40001/remoting/protocol/trans...
File remoting/protocol/transport_context.h (right):

https://codereview.chromium.org/1800893002/diff/40001/remoting/protocol/trans...
remoting/protocol/transport_context.h:39: static const int kNumRelayModes =
RelayMode::TURN + 1;
On 2016/03/15 21:54:53, Sergey Ulanov wrote:
> On 2016/03/15 20:38:59, garykac wrote:
> > The enum should end with MAX_RELAYMODE (or something like that), and we
should
> > use that value instead of TURN+1.
> > 
> > If someone adds a new RelayMode at the end of the enum, then this code will
> > break.
> 
> That would make the compiler complain that MAX_RELAYMODE is not handled in
> switch statement in TransportContext::EnsureFreshIceConfig(). It's rather
> obvious that this value need to be updated if a new mode needs to be added,
but
> it's also unlikely that we will ever need to add another value here. 

enum RelayMode {
  GTURN,
  TURN,
  LAST_RELAYMODE = TURN
};
static count int kNumRelayModes = RelayMode::LAST_RELAYMODE + 1;

[Sergey Ulanov, 2016-03-16 18:34:14]

https://codereview.chromium.org/1800893002/diff/40001/remoting/protocol/trans...
File remoting/protocol/transport_context.h (right):

https://codereview.chromium.org/1800893002/diff/40001/remoting/protocol/trans...
remoting/protocol/transport_context.h:39: static const int kNumRelayModes =
RelayMode::TURN + 1;
On 2016/03/16 00:58:21, garykac wrote:
> On 2016/03/15 21:54:53, Sergey Ulanov wrote:
> > On 2016/03/15 20:38:59, garykac wrote:
> > > The enum should end with MAX_RELAYMODE (or something like that), and we
> should
> > > use that value instead of TURN+1.
> > > 
> > > If someone adds a new RelayMode at the end of the enum, then this code
will
> > > break.
> > 
> > That would make the compiler complain that MAX_RELAYMODE is not handled in
> > switch statement in TransportContext::EnsureFreshIceConfig(). It's rather
> > obvious that this value need to be updated if a new mode needs to be added,
> but
> > it's also unlikely that we will ever need to add another value here. 
> 
> enum RelayMode {
>   GTURN,
>   TURN,
>   LAST_RELAYMODE = TURN
> };
> static count int kNumRelayModes = RelayMode::LAST_RELAYMODE + 1;

Done.

[garykac, 2016-03-16 18:40:14]

lgtm

[Sergey Ulanov, 2016-03-16 18:40:32]

The CQ bit was checked by sergeyu@chromium.org

[commit-bot: I haz the power, 2016-03-16 18:41:00]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1800893002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1800893002/60001

[commit-bot: I haz the power, 2016-03-16 20:41:13]

Committed patchset #3 (id:60001)

[commit-bot: I haz the power, 2016-03-16 20:42:18]

Description was changed from

==========
Enable TURN on the host when using WebRTC.

BUG=577954
==========

to

==========
Enable TURN on the host when using WebRTC.

BUG=577954

Committed: https://crrev.com/e8e7cb43534bb63ce2972cc0a51f614cda15db3f
Cr-Commit-Position: refs/heads/master@{#381520}
==========

[commit-bot: I haz the power, 2016-03-16 20:42:19]

Patchset 3 (id:??) landed as
https://crrev.com/e8e7cb43534bb63ce2972cc0a51f614cda15db3f
Cr-Commit-Position: refs/heads/master@{#381520}