Add TransportContext class.

Add TransportContext class.

The new TransportContext is now used to store all parameters required
to initialize Transport objects and is applicable both to IceTransport
and WebrtcTransport. It also allowed to reduce amount of boilerplate
code when passing around these parameters.

BUG=547158
Committed: https://crrev.com/86030f38b4a7be9418c50da7e83ff55749438dba
Cr-Commit-Position: refs/heads/master@{#365649}

[Sergey Ulanov, 2015-12-14 20:03:59]

Patchset #1 (id:1) has been deleted

[Sergey Ulanov, 2015-12-14 20:11:52]

Description was changed from

==========
Add TransportContext class.

TransportContext is used to store all parameters required to initialize
any Transport objects and is applicable both to IceTransport and
WebrtcTransport. It also allowed to reduce amount of bouler-plate code
when passing around these parameters.

BUG=547158
==========

to

==========
Add TransportContext class.

The new TransportContext is now used to store all parameters required
to initialize Transport objects and is applicable both to IceTransport
and WebrtcTransport. It also allowed to reduce amount of boiler-plate
code when passing around these parameters.

BUG=547158
==========

[Sergey Ulanov, 2015-12-14 20:12:10]

Description was changed from

==========
Add TransportContext class.

The new TransportContext is now used to store all parameters required
to initialize Transport objects and is applicable both to IceTransport
and WebrtcTransport. It also allowed to reduce amount of boiler-plate
code when passing around these parameters.

BUG=547158
==========

to

==========
Add TransportContext class.

The new TransportContext is now used to store all parameters required
to initialize Transport objects and is applicable both to IceTransport
and WebrtcTransport. It also allowed to reduce amount of boilerplate
code when passing around these parameters.

BUG=547158
==========

[Sergey Ulanov, 2015-12-14 20:16:36]

Patchset #1 (id:20001) has been deleted

[Sergey Ulanov, 2015-12-14 20:17:30]

Patchset #1 (id:40001) has been deleted

[Sergey Ulanov, 2015-12-14 20:18:13]

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

[Jamie, 2015-12-14 23:29:05]

I've only looked at the new class so far. I'll hold off on the rest for now in
case the changes I've suggested result in a change to the API.

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

https://codereview.chromium.org/1521883006/diff/60001/remoting/protocol/trans...
remoting/protocol/transport_context.cc:45:
callback.Run(CreatePortAllocatorInternal());
In our JS code, we try to make sure that, even in the synchronous case, the
callback is not invoked until after the method responsible for calling it has
returned. Having the order non-deterministic can lead to hard-to-reproduce bugs
if one of the scenarios is rather rare and the caller doesn't handle it
properly. Could you explicitly post the task to the current run loop, or are
there already so many instances of this pattern that it's not worth fixing?

https://codereview.chromium.org/1521883006/diff/60001/remoting/protocol/trans...
remoting/protocol/transport_context.cc:52: if (!(network_settings_.flags &
stun_or_relay_flags) ||
I think an explicit ==0 would be clearer than a ! here, especially since the
conditional has an ||.

https://codereview.chromium.org/1521883006/diff/60001/remoting/protocol/trans...
remoting/protocol/transport_context.cc:75: if ((!relay_token.empty() &&
!relay_hosts.empty()) || !stun_hosts.empty())
If neither relay nor stun hosts were received, should we log an error? 
Similarly, does should we log an error if we receive relay hosts but no token,
or vice versa? Does it make sense to call the port allocator callbacks in that
case?

https://codereview.chromium.org/1521883006/diff/60001/remoting/protocol/trans...
remoting/protocol/transport_context.cc:76: last_jingle_info_update_time_ =
base::TimeTicks::Now();
Doesn't this belong in EnsureFreshJingleInfo (or maybe in both places)?
Otherwise, multiple rapid calls to the function will result in multiple requests
being sent.

https://codereview.chromium.org/1521883006/diff/60001/remoting/protocol/trans...
remoting/protocol/transport_context.cc:90: LOG(ERROR) << relay_hosts_.size();
Remove (or lower verbosity level).

https://codereview.chromium.org/1521883006/diff/60001/remoting/protocol/trans...
remoting/protocol/transport_context.cc:95: LOG(ERROR) << stun_hosts_.size();
Ditto

[Sergey Ulanov, 2015-12-15 18:52:15]

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

https://codereview.chromium.org/1521883006/diff/60001/remoting/protocol/trans...
remoting/protocol/transport_context.cc:45:
callback.Run(CreatePortAllocatorInternal());
On 2015/12/14 23:29:05, Jamie wrote:
> In our JS code, we try to make sure that, even in the synchronous case, the
> callback is not invoked until after the method responsible for calling it has
> returned. Having the order non-deterministic can lead to hard-to-reproduce
bugs
> if one of the scenarios is rather rare and the caller doesn't handle it
> properly. Could you explicitly post the task to the current run loop, or are
> there already so many instances of this pattern that it's not worth fixing?

We use the same pattern for many other asynchronous operations and I don't think
it causes any problems (e.g. see StreamChannelFactory::CreateChannel()).

https://codereview.chromium.org/1521883006/diff/60001/remoting/protocol/trans...
remoting/protocol/transport_context.cc:52: if (!(network_settings_.flags &
stun_or_relay_flags) ||
On 2015/12/14 23:29:05, Jamie wrote:
> I think an explicit ==0 would be clearer than a ! here, especially since the
> conditional has an ||.

Done.

https://codereview.chromium.org/1521883006/diff/60001/remoting/protocol/trans...
remoting/protocol/transport_context.cc:75: if ((!relay_token.empty() &&
!relay_hosts.empty()) || !stun_hosts.empty())
On 2015/12/14 23:29:05, Jamie wrote:
> If neither relay nor stun hosts were received, should we log an error? 
> Similarly, does should we log an error if we receive relay hosts but no token,
> or vice versa? Does it make sense to call the port allocator callbacks in that
> case?

This check is here to handle the case when the request has failed. In that case
JingleInfoRequest logs a warning and calls this callback with empty parameters.
PortAllocator is still created because the session may still connect
successfully without STUN and Relay.

If the server returned a response that had STUN servers, but no relay, or vise
versa then the response is still considered valid. I don't think it makes sense
to retry the request in that case. Also client already log results of jingleinfo
request so it's not worth logging a warning in that case.

There is also the case when we get a valid but empty response - it's handled the
same way as the error case here. This is not ideal, but I don't think it's worth
fixing now.

BTW we will need switch to standard TURN soon and will probably remove
JingleInfoRequest or replace it with something else.
Also note that this is not new logic - I'm just moving this code from
IceTransportFactory.

https://codereview.chromium.org/1521883006/diff/60001/remoting/protocol/trans...
remoting/protocol/transport_context.cc:76: last_jingle_info_update_time_ =
base::TimeTicks::Now();
On 2015/12/14 23:29:05, Jamie wrote:
> Doesn't this belong in EnsureFreshJingleInfo (or maybe in both places)?
> Otherwise, multiple rapid calls to the function will result in multiple
requests
> being sent.

EnsureFreshJingleInfo() makes a request only if it's not already pending, so I
don't think this is a problem. I've updated EnsureFreshJingleInfo() to make it
clearer.

https://codereview.chromium.org/1521883006/diff/60001/remoting/protocol/trans...
remoting/protocol/transport_context.cc:90: LOG(ERROR) << relay_hosts_.size();
On 2015/12/14 23:29:05, Jamie wrote:
> Remove (or lower verbosity level).

Sorry, I thought I removed these.

https://codereview.chromium.org/1521883006/diff/60001/remoting/protocol/trans...
remoting/protocol/transport_context.cc:95: LOG(ERROR) << stun_hosts_.size();
On 2015/12/14 23:29:05, Jamie wrote:
> Ditto

Done.

[Jamie, 2015-12-15 21:03:49]

lgtm

[Sergey Ulanov, 2015-12-15 23:23:08]

The CQ bit was checked by sergeyu@chromium.org

[commit-bot: I haz the power, 2015-12-15 23:25:21]

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

[commit-bot: I haz the power, 2015-12-15 23:29:46]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-12-15 23:29:47]

Try jobs failed on following builders:
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Sergey Ulanov, 2015-12-16 01:49:09]

The CQ bit was checked by sergeyu@chromium.org

[Sergey Ulanov, 2015-12-16 01:49:09]

The patchset sent to the CQ was uploaded after l-g-t-m from
jamiewalch@chromium.org
Link to the patchset: https://codereview.chromium.org/1521883006/#ps100001
(title: " ")

[commit-bot: I haz the power, 2015-12-16 01:51:05]

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

[commit-bot: I haz the power, 2015-12-16 02:21:41]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-12-16 02:21:42]

Try jobs failed on following builders:
  win8_chromium_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_ng/...)

[Sergey Ulanov, 2015-12-16 18:37:34]

The CQ bit was checked by sergeyu@chromium.org

[Sergey Ulanov, 2015-12-16 18:37:34]

The patchset sent to the CQ was uploaded after l-g-t-m from
jamiewalch@chromium.org
Link to the patchset: https://codereview.chromium.org/1521883006/#ps120001
(title: " ")

[commit-bot: I haz the power, 2015-12-16 18:38:12]

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

[commit-bot: I haz the power, 2015-12-16 19:36:24]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-12-16 19:36:24]

Try jobs failed on following builders:
  android_chromium_gn_compile_rel on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_chro...)

[Sergey Ulanov, 2015-12-16 19:43:56]

The CQ bit was checked by sergeyu@chromium.org

[Sergey Ulanov, 2015-12-16 19:43:56]

The patchset sent to the CQ was uploaded after l-g-t-m from
jamiewalch@chromium.org
Link to the patchset: https://codereview.chromium.org/1521883006/#ps140001
(title: " ")

[commit-bot: I haz the power, 2015-12-16 19:44:14]

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

[commit-bot: I haz the power, 2015-12-16 20:23:51]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-12-16 20:23:52]

Try jobs failed on following builders:
  android_chromium_gn_compile_rel on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_chro...)

[Sergey Ulanov, 2015-12-16 21:44:04]

The CQ bit was checked by sergeyu@chromium.org

[Sergey Ulanov, 2015-12-16 21:44:05]

The patchset sent to the CQ was uploaded after l-g-t-m from
jamiewalch@chromium.org
Link to the patchset: https://codereview.chromium.org/1521883006/#ps160001
(title: " ")

[commit-bot: I haz the power, 2015-12-16 21:46:24]

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

[commit-bot: I haz the power, 2015-12-16 22:45:08]

Description was changed from

==========
Add TransportContext class.

The new TransportContext is now used to store all parameters required
to initialize Transport objects and is applicable both to IceTransport
and WebrtcTransport. It also allowed to reduce amount of boilerplate
code when passing around these parameters.

BUG=547158
==========

to

==========
Add TransportContext class.

The new TransportContext is now used to store all parameters required
to initialize Transport objects and is applicable both to IceTransport
and WebrtcTransport. It also allowed to reduce amount of boilerplate
code when passing around these parameters.

BUG=547158
==========

[commit-bot: I haz the power, 2015-12-16 22:45:09]

Committed patchset #6 (id:160001)

[commit-bot: I haz the power, 2015-12-16 22:46:26]

Description was changed from

==========
Add TransportContext class.

The new TransportContext is now used to store all parameters required
to initialize Transport objects and is applicable both to IceTransport
and WebrtcTransport. It also allowed to reduce amount of boilerplate
code when passing around these parameters.

BUG=547158
==========

to

==========
Add TransportContext class.

The new TransportContext is now used to store all parameters required
to initialize Transport objects and is applicable both to IceTransport
and WebrtcTransport. It also allowed to reduce amount of boilerplate
code when passing around these parameters.

BUG=547158

Committed: https://crrev.com/86030f38b4a7be9418c50da7e83ff55749438dba
Cr-Commit-Position: refs/heads/master@{#365649}
==========

[commit-bot: I haz the power, 2015-12-16 22:46:27]

Patchset 6 (id:??) landed as
https://crrev.com/86030f38b4a7be9418c50da7e83ff55749438dba
Cr-Commit-Position: refs/heads/master@{#365649}