Ping backup connection at a slower rate

Ping backup connection at a slower rate
and make it configurable from the app.
Changed the decision on whether a connection is pingable:
1.Check whether a connection is a backup connection. A connection is considered as a backup connection only if the channel is complete, the connection is active and it is not the best connection.
2. Ping a non-backup connection if it is active and for backup connection, ping it at a slower rate.
Note the default behavior is the same as before.

Also cached the channel state since we are accessing it more often.
BUG=webrtc:5034
R=pthatcher@webrtc.org

Committed: https://crrev.com/381b4217cb36f434c56e399a852a0a15522a9596
Cr-Commit-Position: refs/heads/master@{#10900}

[honghaiz3, 2015-11-19 23:36:45]

Patchset #1 (id:1) has been deleted

[honghaiz3, 2015-11-19 23:36:50]

Patchset #1 (id:20001) has been deleted

[honghaiz3, 2015-11-20 01:06:39]

Patchset #1 (id:40001) has been deleted

[honghaiz3, 2015-11-20 03:16:31]

Description was changed from

==========
Ping backup connection once every 25 seconds and make it
configurable from the java app.

BUG=
==========

to

==========
Ping backup connection once every 25 seconds 
And make it configurable from the java app.

BUG=
==========

[honghaiz3, 2015-11-20 03:16:37]

Patchset #1 (id:60001) has been deleted

[honghaiz3, 2015-11-20 03:17:55]

honghaiz@webrtc.org changed reviewers:
+ pthatcher@webrtc.org

[honghaiz3, 2015-11-20 03:23:04]

Patchset #1 (id:80001) has been deleted

[pthatcher1, 2015-11-20 05:34:38]

https://codereview.webrtc.org/1455033004/diff/100001/talk/app/webrtc/peerconn...
File talk/app/webrtc/peerconnectioninterface.h (right):

https://codereview.webrtc.org/1455033004/diff/100001/talk/app/webrtc/peerconn...
talk/app/webrtc/peerconnectioninterface.h:257: int
ice_backup_connection_ping_interval;
We need to specify the unit of measure.  For example having _ms at the end of
the name.  Or at least commenting on what the unit is.

https://codereview.webrtc.org/1455033004/diff/100001/talk/app/webrtc/webrtcse...
File talk/app/webrtc/webrtcsession.cc (right):

https://codereview.webrtc.org/1455033004/diff/100001/talk/app/webrtc/webrtcse...
talk/app/webrtc/webrtcsession.cc:1245: ice_config.receiving_timeout_ms =
config.ice_connection_receiving_timeout;
We should have made _ms on all of the places for the receiving timeout as well. 
It looks we only put it in half the places.

https://codereview.webrtc.org/1455033004/diff/100001/webrtc/p2p/base/p2ptrans...
File webrtc/p2p/base/p2ptransportchannel.cc (right):

https://codereview.webrtc.org/1455033004/diff/100001/webrtc/p2p/base/p2ptrans...
webrtc/p2p/base/p2ptransportchannel.cc:229:
backup_connection_ping_interval_(BACKUP_CONNECTION_PING_INTERVAL) {
We call one "delay" and the other "interval".  It sounds like it should be
'delay' everywhere or 'interval' everywhere.  I think 'interval' sounds better.

https://codereview.webrtc.org/1455033004/diff/100001/webrtc/p2p/base/p2ptrans...
webrtc/p2p/base/p2ptransportchannel.cc:305: return channel_state_;
Why not just state_?

https://codereview.webrtc.org/1455033004/diff/100001/webrtc/p2p/base/p2ptrans...
webrtc/p2p/base/p2ptransportchannel.cc:387: << backup_connection_ping_interval_
<< " milliseconds.";
You probably only want to log this it if it changes.

https://codereview.webrtc.org/1455033004/diff/100001/webrtc/p2p/base/p2ptrans...
webrtc/p2p/base/p2ptransportchannel.cc:399: << " milliseconds";
Same here.

https://codereview.webrtc.org/1455033004/diff/100001/webrtc/p2p/base/p2ptrans...
webrtc/p2p/base/p2ptransportchannel.cc:1068: channel_state_ =
GetStateInternal();
So what's the advantage of cacheing the state and possibly getting out of sync?

If there is one, we should definitely document what it is.

https://codereview.webrtc.org/1455033004/diff/100001/webrtc/p2p/base/p2ptrans...
webrtc/p2p/base/p2ptransportchannel.cc:1193: // If the channel is not COMPLETED,
Might be more readable as "If the channel is strongly connected but not yet
completed, ..."

https://codereview.webrtc.org/1455033004/diff/100001/webrtc/p2p/base/p2ptrans...
webrtc/p2p/base/p2ptransportchannel.cc:1194: // ping connections that have not
timed out on writing.
To match the code, you should say "ping all active connections"

https://codereview.webrtc.org/1455033004/diff/100001/webrtc/p2p/base/p2ptrans...
webrtc/p2p/base/p2ptransportchannel.cc:1199: // connection once every
|backup_connection_ping_interval_| milliseconds.
The comment says "ping the backup connection", but the code pings both the best
connection and then any connection that hasn't been pinged in the last
backup_connection_ping_interval_ milliseconds.

It seems like this would be more simple with the logic being "always ping all
active connections regardless of whether it is COMPLETED or not, but the
frequency of the ping changes depending on whether it's a backup connection or
not.  So the real logic is IsBackupConnection(connection).

So could we have something like this?

if (weak())) {
  return true;
}

if (IsBackupConnection(connection)) {
  return (conn->last_ping_response_received() &&
backup_connection_ping_interval_);
}

return conn->Active();

bool IsBackupConnection(connection) {
  return ((channel_state_ == STATE_COMPLETED) && (connection !=
best_connection_);
}

https://codereview.webrtc.org/1455033004/diff/100001/webrtc/p2p/base/p2ptrans...
webrtc/p2p/base/p2ptransportchannel.cc:1338: UpdateChannelState();
This kind of thing makes me worry that we won't remember to call it in all the
right places and end up out sync, introducing bugs.  Are you sure we need to
cache it?  If so, are you sure we call "update" in all of the right places?

https://codereview.webrtc.org/1455033004/diff/100001/webrtc/p2p/base/p2ptrans...
File webrtc/p2p/base/p2ptransportchannel_unittest.cc (right):

https://codereview.webrtc.org/1455033004/diff/100001/webrtc/p2p/base/p2ptrans...
webrtc/p2p/base/p2ptransportchannel_unittest.cc:1687: EXPECT_GE(time_elapsed,
backup_ping_interval);
Isn't this going to require the test to run for a long time?  I think we need to
either make the times a lot smaller or introduce a virtual clock, because this
is going to add a lot of time to how long it takes a unit test to run.

[juberti, 2015-11-20 06:09:47]

juberti@google.com changed reviewers:
+ juberti@google.com

[juberti, 2015-11-20 06:09:48]

I think this could definitely benefit from a longer CL description outlining the
overall approach.

[honghaiz3, 2015-11-20 19:47:02]

Description was changed from

==========
Ping backup connection once every 25 seconds 
And make it configurable from the java app.

BUG=
==========

to

==========
Ping backup connection at a slower rate 
and make it configurable from the app.
Changed the decision on whether a connection is pingable:
1.Check whether a connection is a backup connection. A connection is considered
as a backup connection only if the channel is complete, the connection is active
and it is not the best connection. 
2. Ping a non-backup connection if it is active and for backup connection, ping
it at a slower rate. 

Also cached the channel state since we are accessing it more often. 
BUG=webrtc:5034
==========

[honghaiz3, 2015-11-20 19:54:55]

Description was changed from

==========
Ping backup connection at a slower rate 
and make it configurable from the app.
Changed the decision on whether a connection is pingable:
1.Check whether a connection is a backup connection. A connection is considered
as a backup connection only if the channel is complete, the connection is active
and it is not the best connection. 
2. Ping a non-backup connection if it is active and for backup connection, ping
it at a slower rate. 

Also cached the channel state since we are accessing it more often. 
BUG=webrtc:5034
==========

to

==========
Ping backup connection at a slower rate 
and make it configurable from the app.
Changed the decision on whether a connection is pingable:
1.Check whether a connection is a backup connection. A connection is considered
as a backup connection only if the channel is complete, the connection is active
and it is not the best connection. 
2. Ping a non-backup connection if it is active and for backup connection, ping
it at a slower rate. 
Note the default behavior is the same as before. 

Also cached the channel state since we are accessing it more often. 
BUG=webrtc:5034
==========

[honghaiz3, 2015-11-20 20:02:09]

Patchset #2 (id:120001) has been deleted

[honghaiz3, 2015-11-20 20:02:15]

Patchset #2 (id:140001) has been deleted

[honghaiz3, 2015-11-20 20:02:21]

Patchset #2 (id:160001) has been deleted

[honghaiz3, 2015-11-20 20:10:06]

I changed the default behavior to be the same as before in consideration that
desktop chrome users may not care about a slower ping rate for backup
connections. 
Let me know if you prefer the default behavior of the first patch.

https://codereview.webrtc.org/1455033004/diff/100001/talk/app/webrtc/peerconn...
File talk/app/webrtc/peerconnectioninterface.h (right):

https://codereview.webrtc.org/1455033004/diff/100001/talk/app/webrtc/peerconn...
talk/app/webrtc/peerconnectioninterface.h:257: int
ice_backup_connection_ping_interval;
On 2015/11/20 05:34:37, pthatcher1 wrote:
> We need to specify the unit of measure.  For example having _ms at the end of
> the name.  Or at least commenting on what the unit is.  
I added comments because the variable name is already pretty long.

https://codereview.webrtc.org/1455033004/diff/100001/talk/app/webrtc/webrtcse...
File talk/app/webrtc/webrtcsession.cc (right):

https://codereview.webrtc.org/1455033004/diff/100001/talk/app/webrtc/webrtcse...
talk/app/webrtc/webrtcsession.cc:1245: ice_config.receiving_timeout_ms =
config.ice_connection_receiving_timeout;
On 2015/11/20 05:34:37, pthatcher1 wrote:
> We should have made _ms on all of the places for the receiving timeout as
well. 
> It looks we only put it in half the places.
I sort of did that intentionally because the variable name is already very long.
And I did not add _ms to backup_connection_ping_interval for the same reason.
Let me know if you prefer adding that regardless of the variable length.

https://codereview.webrtc.org/1455033004/diff/100001/webrtc/p2p/base/p2ptrans...
File webrtc/p2p/base/p2ptransportchannel.cc (right):

https://codereview.webrtc.org/1455033004/diff/100001/webrtc/p2p/base/p2ptrans...
webrtc/p2p/base/p2ptransportchannel.cc:229:
backup_connection_ping_interval_(BACKUP_CONNECTION_PING_INTERVAL) {
On 2015/11/20 05:34:38, pthatcher1 wrote:
> We call one "delay" and the other "interval".  It sounds like it should be
> 'delay' everywhere or 'interval' everywhere.  I think 'interval' sounds
better.
I agree that the interval sounds better. I will change the words in a subsequent
CL and put the TODO near the definition of this variable.

https://codereview.webrtc.org/1455033004/diff/100001/webrtc/p2p/base/p2ptrans...
webrtc/p2p/base/p2ptransportchannel.cc:305: return channel_state_;
On 2015/11/20 05:34:37, pthatcher1 wrote:
> Why not just state_?

Done.

https://codereview.webrtc.org/1455033004/diff/100001/webrtc/p2p/base/p2ptrans...
webrtc/p2p/base/p2ptransportchannel.cc:387: << backup_connection_ping_interval_
<< " milliseconds.";
On 2015/11/20 05:34:37, pthatcher1 wrote:
> You probably only want to log this it if it changes.

Done. Only set and log this if the value changes.

https://codereview.webrtc.org/1455033004/diff/100001/webrtc/p2p/base/p2ptrans...
webrtc/p2p/base/p2ptransportchannel.cc:399: << " milliseconds";
On 2015/11/20 05:34:38, pthatcher1 wrote:
> Same here.

Done.

https://codereview.webrtc.org/1455033004/diff/100001/webrtc/p2p/base/p2ptrans...
webrtc/p2p/base/p2ptransportchannel.cc:1068: channel_state_ =
GetStateInternal();
On 2015/11/20 05:34:37, pthatcher1 wrote:
> So what's the advantage of cacheing the state and possibly getting out of
sync?
> 
> If there is one, we should definitely document what it is.
Basically I want to avoid too much unnecessary computations to call GetState(). 
In TransportController, there are tons of places that will call this. 
Plus, I am adding more calls on this. we will be calling this once every 500ms,
or 50ms if the channel is weak. It could be more because we are calling this on
every connection (this can be reduced though).

https://codereview.webrtc.org/1455033004/diff/100001/webrtc/p2p/base/p2ptrans...
webrtc/p2p/base/p2ptransportchannel.cc:1193: // If the channel is not COMPLETED,
On 2015/11/20 05:34:37, pthatcher1 wrote:
> Might be more readable as "If the channel is strongly connected but not yet
> completed, ..."

Done.

https://codereview.webrtc.org/1455033004/diff/100001/webrtc/p2p/base/p2ptrans...
webrtc/p2p/base/p2ptransportchannel.cc:1194: // ping connections that have not
timed out on writing.
On 2015/11/20 05:34:38, pthatcher1 wrote:
> To match the code, you should say "ping all active connections"

Done.

https://codereview.webrtc.org/1455033004/diff/100001/webrtc/p2p/base/p2ptrans...
webrtc/p2p/base/p2ptransportchannel.cc:1199: // connection once every
|backup_connection_ping_interval_| milliseconds.
On 2015/11/20 05:34:38, pthatcher1 wrote:
> The comment says "ping the backup connection", but the code pings both the
best
> connection and then any connection that hasn't been pinged in the last
> backup_connection_ping_interval_ milliseconds.
> 
> It seems like this would be more simple with the logic being "always ping all
> active connections regardless of whether it is COMPLETED or not, but the
> frequency of the ping changes depending on whether it's a backup connection or
> not.  So the real logic is IsBackupConnection(connection).
> 
> So could we have something like this?
> 
> if (weak())) {
>   return true;
> }
> 
> if (IsBackupConnection(connection)) {
>   return (conn->last_ping_response_received() &&
> backup_connection_ping_interval_);
> }
> 
> return conn->Active();
> 
> bool IsBackupConnection(connection) {
>   return ((channel_state_ == STATE_COMPLETED) && (connection !=
> best_connection_);
> }

Thanks. made a small change, requiring that backup connections to be active.

https://codereview.webrtc.org/1455033004/diff/100001/webrtc/p2p/base/p2ptrans...
webrtc/p2p/base/p2ptransportchannel.cc:1338: UpdateChannelState();
On 2015/11/20 05:34:38, pthatcher1 wrote:
> This kind of thing makes me worry that we won't remember to call it in all the
> right places and end up out sync, introducing bugs.  Are you sure we need to
> cache it?  If so, are you sure we call "update" in all of the right places?
I am pretty sure this is the only place we need to add to make sure it syncs. 
GetStateInternal could only change the state when a connection is added,
removed, or a connection's write state has changed. Whenever a connection is
created, or a connection state has changed, it will sort connections. We
updateChannelState at the end of sorting. The only thing we miss is that when a
connection is deleted, we do not always UpdateChannelState. 
In the future, I think we should signal the channel state change event to
transportController instead of signal the write state change and connection
deletion event.

https://codereview.webrtc.org/1455033004/diff/100001/webrtc/p2p/base/p2ptrans...
File webrtc/p2p/base/p2ptransportchannel_unittest.cc (right):

https://codereview.webrtc.org/1455033004/diff/100001/webrtc/p2p/base/p2ptrans...
webrtc/p2p/base/p2ptransportchannel_unittest.cc:1687: EXPECT_GE(time_elapsed,
backup_ping_interval);
On 2015/11/20 05:34:38, pthatcher1 wrote:
> Isn't this going to require the test to run for a long time?  I think we need
to
> either make the times a lot smaller or introduce a virtual clock, because this
> is going to add a lot of time to how long it takes a unit test to run.
This currently takes about 3 seconds (About 2.5 seconds come from the backup
connection ping delay). Cannot make backup_ping interval to be shorter than 1
second because the original behavior is pinging the backup about once per
second. I agree we should switch to the fake/virtual clock, but maybe we should
do that independent of this. The whole p2ptransportchannel tests take about 210
seconds. Using virtual clock should reduce this a lot.

[pthatcher1, 2015-11-21 00:18:27]

This seems good, but I really want to make sure we don't mess up the connection
state.  Do you think we could make a little document showing all the things that
could cause the state to change (basically make the output of GetStateInternal()
change), and then we can make sure we cover all the places?  I know you said
it's only a few, but I want to make sure we are thorough with this.

https://codereview.webrtc.org/1455033004/diff/180001/webrtc/p2p/base/p2ptrans...
File webrtc/p2p/base/p2ptransportchannel.cc (right):

https://codereview.webrtc.org/1455033004/diff/180001/webrtc/p2p/base/p2ptrans...
webrtc/p2p/base/p2ptransportchannel.cc:300: TransportChannelState
P2PTransportChannel::GetState() const {
You can just call this method state().  And call GetStateInternal() something
like CalculateState() or just get rid of it and make it part of UpdateState().

https://codereview.webrtc.org/1455033004/diff/180001/webrtc/p2p/base/p2ptrans...
webrtc/p2p/base/p2ptransportchannel.cc:389: receiving_timeout_ !=
config.receiving_timeout_ms) {
If nothing else has _ms on the end, can you make a TODO to remove that one was
well?

[honghaiz3, 2015-12-01 18:05:36]

A doc is separately shared.

https://codereview.webrtc.org/1455033004/diff/180001/webrtc/p2p/base/p2ptrans...
File webrtc/p2p/base/p2ptransportchannel.cc (right):

https://codereview.webrtc.org/1455033004/diff/180001/webrtc/p2p/base/p2ptrans...
webrtc/p2p/base/p2ptransportchannel.cc:300: TransportChannelState
P2PTransportChannel::GetState() const {
On 2015/11/21 00:18:26, pthatcher1 wrote:
> You can just call this method state().  And call GetStateInternal() something
> like CalculateState() or just get rid of it and make it part of UpdateState().
Changed GetStateInternal to ComputeState(). 
GetState() is a virtual method. It cannot change (unless we change all methods
in the base and other subclasses.

https://codereview.webrtc.org/1455033004/diff/180001/webrtc/p2p/base/p2ptrans...
webrtc/p2p/base/p2ptransportchannel.cc:389: receiving_timeout_ !=
config.receiving_timeout_ms) {
On 2015/11/21 00:18:26, pthatcher1 wrote:
> If nothing else has _ms on the end, can you make a TODO to remove that one was
> well?

Done. TODO added in transport.cc

[pthatcher1, 2015-12-01 19:40:25]

Good write up.  

Please leave a nice comment on UpdateChannelState basically saying "Make sure
call this whenever X, but don't call it to much because Y.  For example, we
currently call it when Z."

https://codereview.webrtc.org/1455033004/diff/200001/talk/app/webrtc/java/jni...
File talk/app/webrtc/java/jni/peerconnection_jni.cc (right):

https://codereview.webrtc.org/1455033004/diff/200001/talk/app/webrtc/java/jni...
talk/app/webrtc/java/jni/peerconnection_jni.cc:1556: jni, j_rtc_config_class,
"iceBackupConnectionPingInterval", "I");
We should start using "candidate pair" instead of "connection", especially in
public APIs. So, iceBackupCandidatePairPingInterval

https://codereview.webrtc.org/1455033004/diff/200001/webrtc/p2p/base/p2ptrans...
File webrtc/p2p/base/p2ptransportchannel.cc (right):

https://codereview.webrtc.org/1455033004/diff/200001/webrtc/p2p/base/p2ptrans...
webrtc/p2p/base/p2ptransportchannel.cc:300: TransportChannelState
P2PTransportChannel::GetState() const {
Can we just change this to state()?

https://codereview.webrtc.org/1455033004/diff/200001/webrtc/p2p/base/p2ptrans...
webrtc/p2p/base/p2ptransportchannel.cc:1067: state_ = ComputeState();
Since ComputeState is never called anywhere else, would it be more readable to
just move that logic here into UpdateChannelState()?  

Also, can we rename it to UpdateState()?

https://codereview.webrtc.org/1455033004/diff/200001/webrtc/p2p/base/p2ptrans...
webrtc/p2p/base/p2ptransportchannel.cc:1169: // is complete, the connection is
not the best connection and it is active.
complete => completed

[honghaiz3, 2015-12-01 23:42:17]

Patchset #4 (id:220001) has been deleted

[honghaiz3, 2015-12-01 23:42:25]

Patchset #4 (id:240001) has been deleted

[honghaiz3, 2015-12-01 23:43:08]

https://codereview.webrtc.org/1455033004/diff/200001/talk/app/webrtc/java/jni...
File talk/app/webrtc/java/jni/peerconnection_jni.cc (right):

https://codereview.webrtc.org/1455033004/diff/200001/talk/app/webrtc/java/jni...
talk/app/webrtc/java/jni/peerconnection_jni.cc:1556: jni, j_rtc_config_class,
"iceBackupConnectionPingInterval", "I");
On 2015/12/01 19:40:25, pthatcher1 wrote:
> We should start using "candidate pair" instead of "connection", especially in
> public APIs. So, iceBackupCandidatePairPingInterval
Done. Also changed the one ice_backup_connection to ice_backup_candidate_pair to
be consistent. Kept the name backup_connection* in IceConfig.

https://codereview.webrtc.org/1455033004/diff/200001/webrtc/p2p/base/p2ptrans...
File webrtc/p2p/base/p2ptransportchannel.cc (right):

https://codereview.webrtc.org/1455033004/diff/200001/webrtc/p2p/base/p2ptrans...
webrtc/p2p/base/p2ptransportchannel.cc:300: TransportChannelState
P2PTransportChannel::GetState() const {
On 2015/12/01 19:40:25, pthatcher1 wrote:
> Can we just change this to state()?
This is a virtual method.

https://codereview.webrtc.org/1455033004/diff/200001/webrtc/p2p/base/p2ptrans...
webrtc/p2p/base/p2ptransportchannel.cc:1067: state_ = ComputeState();
On 2015/12/01 19:40:25, pthatcher1 wrote:
> Since ComputeState is never called anywhere else, would it be more readable to
> just move that logic here into UpdateChannelState()?  
> 
> Also, can we rename it to UpdateState()?  
Changed to UpdateState
ComputeState has a lot of early returns. 
Although we can keep the early return by moving the updating to the end of the
method, it will have issues that the transportcontroller will get an old state
of the channel.

https://codereview.webrtc.org/1455033004/diff/200001/webrtc/p2p/base/p2ptrans...
webrtc/p2p/base/p2ptransportchannel.cc:1169: // is complete, the connection is
not the best connection and it is active.
On 2015/12/01 19:40:25, pthatcher1 wrote:
> complete => completed

Done.

[pthatcher, 2015-12-02 19:18:23]

pthatcher@google.com changed reviewers:
+ pthatcher@google.com

[pthatcher, 2015-12-02 19:18:24]

lgtm

https://codereview.webrtc.org/1455033004/diff/260001/webrtc/p2p/base/p2ptrans...
File webrtc/p2p/base/p2ptransportchannel.cc (right):

https://codereview.webrtc.org/1455033004/diff/260001/webrtc/p2p/base/p2ptrans...
webrtc/p2p/base/p2ptransportchannel.cc:1071: // example, we call this at the end
of SortConnections.
Well written

[honghaiz3, 2015-12-02 19:43:57]

The CQ bit was checked by honghaiz@webrtc.org

[commit-bot: I haz the power, 2015-12-02 19:44:10]

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

[commit-bot: I haz the power, 2015-12-02 19:47:52]

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

[commit-bot: I haz the power, 2015-12-02 19:47:53]

Try jobs failed on following builders:
  presubmit on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/2121)

[juberti, 2015-12-02 19:59:10]

On 2015/12/02 19:47:53, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   presubmit on tryserver.webrtc (JOB_FAILED,
> http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/2121)

Hoping to take a look at this today, go ahead and submit if you don't hear from
me by tomorrow

[honghaiz3, 2015-12-02 20:06:30]

On 2015/12/02 19:59:10, juberti wrote:
> On 2015/12/02 19:47:53, commit-bot: I haz the power wrote:
> > Try jobs failed on following builders:
> >   presubmit on tryserver.webrtc (JOB_FAILED,
> > http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/2121)
> 
> Hoping to take a look at this today, go ahead and submit if you don't hear
from
> me by tomorrow
Ah, did not realize you may want to look at this. 
Will wait until tomorrow.

[honghaiz3, 2015-12-03 22:36:52]

honghaiz@webrtc.org changed reviewers:
+ tkchin@webrtc.org
- pthatcher@google.com

[honghaiz3, 2015-12-03 22:36:52]

Zeke, 

Can you take a look at this CL?

[honghaiz3, 2015-12-04 05:40:14]

Peter, 

I think this needs an LGTM from your webrtc account.

[honghaiz3, 2015-12-04 05:42:07]

honghaiz@webrtc.org changed reviewers:
- tkchin@webrtc.org

[tkchin_webrtc, 2015-12-04 18:52:47]

On 2015/12/04 05:40:14, honghaiz3 wrote:
> Peter, 
> 
> I think this needs an LGTM from your webrtc account.

lgtm

The presubmit script will complain that it's > 80 chars. Technically 100 chars
is allowable in ObjC. You can either trim to 80 chars where it's complaining, or
submit manually after try-jobs.

[pthatcher1, 2015-12-04 19:00:26]

On 2015/12/04 18:52:47, tkchin_webrtc wrote:
> On 2015/12/04 05:40:14, honghaiz3 wrote:
> > Peter, 
> > 
> > I think this needs an LGTM from your webrtc account.
> 
> lgtm
> 
> The presubmit script will complain that it's > 80 chars. Technically 100 chars
> is allowable in ObjC. You can either trim to 80 chars where it's complaining,
or
> submit manually after try-jobs.

lgtm

If I had a nickel for every time I do that :).

[honghaiz3, 2015-12-04 19:19:51]

The CQ bit was checked by honghaiz@webrtc.org

[commit-bot: I haz the power, 2015-12-04 19:19:58]

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

[commit-bot: I haz the power, 2015-12-04 19:24:11]

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

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

Try jobs failed on following builders:
  win_x64_gn_dbg on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_gn_dbg/builds/5697)

[honghaiz3, 2015-12-04 20:01:28]

On 2015/12/04 18:52:47, tkchin_webrtc wrote:
> On 2015/12/04 05:40:14, honghaiz3 wrote:
> > Peter, 
> > 
> > I think this needs an LGTM from your webrtc account.
> 
> lgtm
> 
> The presubmit script will complain that it's > 80 chars. Technically 100 chars
> is allowable in ObjC. You can either trim to 80 chars where it's complaining,
or
> submit manually after try-jobs.

Thanks!

[honghaiz3, 2015-12-04 20:03:08]

The CQ bit was checked by honghaiz@webrtc.org

[commit-bot: I haz the power, 2015-12-04 20:03:19]

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

[commit-bot: I haz the power, 2015-12-04 20:05:08]

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

[commit-bot: I haz the power, 2015-12-04 20:05:11]

Try jobs failed on following builders:
  presubmit on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/2200)

[honghaiz3, 2015-12-04 20:11:17]

The CQ bit was checked by honghaiz@webrtc.org

[honghaiz3, 2015-12-04 20:11:18]

The patchset sent to the CQ was uploaded after l-g-t-m from
pthatcher@google.com, pthatcher@webrtc.org, tkchin@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1455033004/#ps280001 (title:
"Merge with head")

[commit-bot: I haz the power, 2015-12-04 20:11:29]

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

[honghaiz3, 2015-12-04 20:24:17]

Description was changed from

==========
Ping backup connection at a slower rate 
and make it configurable from the app.
Changed the decision on whether a connection is pingable:
1.Check whether a connection is a backup connection. A connection is considered
as a backup connection only if the channel is complete, the connection is active
and it is not the best connection. 
2. Ping a non-backup connection if it is active and for backup connection, ping
it at a slower rate. 
Note the default behavior is the same as before. 

Also cached the channel state since we are accessing it more often. 
BUG=webrtc:5034
==========

to

==========
Ping backup connection at a slower rate
and make it configurable from the app.
Changed the decision on whether a connection is pingable:
1.Check whether a connection is a backup connection. A connection is considered
as a backup connection only if the channel is complete, the connection is active
and it is not the best connection.
2. Ping a non-backup connection if it is active and for backup connection, ping
it at a slower rate.
Note the default behavior is the same as before.

Also cached the channel state since we are accessing it more often.
BUG=webrtc:5034
R=pthatcher@webrtc.org

Committed:
https://chromium.googlesource.com/external/webrtc/+/381b4217cb36f434c56e399a8...
==========

[honghaiz3, 2015-12-04 20:24:19]

Committed patchset #5 (id:280001) manually as
381b4217cb36f434c56e399a852a0a15522a9596 (presubmit successful).

[commit-bot: I haz the power, 2015-12-04 20:24:19]

Description was changed from

==========
Ping backup connection at a slower rate
and make it configurable from the app.
Changed the decision on whether a connection is pingable:
1.Check whether a connection is a backup connection. A connection is considered
as a backup connection only if the channel is complete, the connection is active
and it is not the best connection.
2. Ping a non-backup connection if it is active and for backup connection, ping
it at a slower rate.
Note the default behavior is the same as before.

Also cached the channel state since we are accessing it more often.
BUG=webrtc:5034
R=pthatcher@webrtc.org

Committed:
https://chromium.googlesource.com/external/webrtc/+/381b4217cb36f434c56e399a8...
==========

to

==========
Ping backup connection at a slower rate
and make it configurable from the app.
Changed the decision on whether a connection is pingable:
1.Check whether a connection is a backup connection. A connection is considered
as a backup connection only if the channel is complete, the connection is active
and it is not the best connection.
2. Ping a non-backup connection if it is active and for backup connection, ping
it at a slower rate.
Note the default behavior is the same as before.

Also cached the channel state since we are accessing it more often.
BUG=webrtc:5034
R=pthatcher@webrtc.org

Committed: https://crrev.com/381b4217cb36f434c56e399a852a0a15522a9596
Cr-Commit-Position: refs/heads/master@{#10900}
==========

[commit-bot: I haz the power, 2015-12-04 20:24:21]

Patchset 5 (id:??) landed as
https://crrev.com/381b4217cb36f434c56e399a852a0a15522a9596
Cr-Commit-Position: refs/heads/master@{#10900}