Replace readable with receiving where receiving means receiving anything (stun ping, response or da…

Replace readable with receiving where receiving means receiving anything (stun ping, response or data packet).
If a connection does not receive for 30 seconds, it will be deleted.
BUG=

Committed: https://crrev.com/ae16f8547d3b447f62f6660f13688585c6c3de15
Cr-Commit-Position: refs/heads/master@{#10001}

[honghaiz3, 2015-09-17 19:39:49]

Patchset #1 (id:1) has been deleted

[honghaiz3, 2015-09-17 19:39:54]

Patchset #1 (id:20001) has been deleted

[honghaiz3, 2015-09-17 19:40:01]

Patchset #1 (id:40001) has been deleted

[honghaiz3, 2015-09-17 19:40:09]

Patchset #1 (id:60001) has been deleted

[honghaiz3, 2015-09-17 19:48:49]

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

[honghaiz3, 2015-09-17 19:48:50]

This is a breakup CL from 
https://codereview.webrtc.org/1311433009/

[pthatcher1, 2015-09-17 21:45:12]

Mostly nits

https://codereview.webrtc.org/1351673003/diff/100001/webrtc/p2p/base/dtlstran...
File webrtc/p2p/base/dtlstransportchannel_unittest.cc (right):

https://codereview.webrtc.org/1351673003/diff/100001/webrtc/p2p/base/dtlstran...
webrtc/p2p/base/dtlstransportchannel_unittest.cc:202: bool writable() const {
return transport_->any_channels_writable(); }
Shouldn't that be all_channels_writable()?

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

https://codereview.webrtc.org/1351673003/diff/100001/webrtc/p2p/base/p2ptrans...
webrtc/p2p/base/p2ptransportchannel.cc:852: if (connection->pruned() &&
!connection->receiving()) {
It might be nice to have a method connection->pinging() ==
!connection->pruned().

Then we could write this:

if (!connection->pinging() && !connection->receiving()) {
  continue;
}

https://codereview.webrtc.org/1351673003/diff/100001/webrtc/p2p/base/p2ptrans...
webrtc/p2p/base/p2ptransportchannel.cc:1049: // TODO(honghaiz): channel
receiving state is set in the OnCheckReceiving.
channel => The channel
in the OnCheckingReceiving => in OnCheckingReceiving.

https://codereview.webrtc.org/1351673003/diff/100001/webrtc/p2p/base/p2ptrans...
webrtc/p2p/base/p2ptransportchannel.cc:1175: return !writable() ||
conn->write_state() != Connection::STATE_WRITE_TIMEOUT;
The comment seems to indicate the code should be:

return !writable() || !conn->pruned();


Or this:

return conn->pinging() || !writable();

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

https://codereview.webrtc.org/1351673003/diff/100001/webrtc/p2p/base/p2ptrans...
webrtc/p2p/base/p2ptransportchannel_unittest.cc:1716: // Wait for conn1 being
pruned.
being pruned => to be pruned

https://codereview.webrtc.org/1351673003/diff/100001/webrtc/p2p/base/port.cc
File webrtc/p2p/base/port.cc (right):

https://codereview.webrtc.org/1351673003/diff/100001/webrtc/p2p/base/port.cc#...
webrtc/p2p/base/port.cc:907: if (last_ping_received_ != 0 ||
last_ping_response_received_ != 0) {
Why do we only do this if we've received a ping or ping response?  Why not
always do it?

https://codereview.webrtc.org/1351673003/diff/100001/webrtc/p2p/base/port.cc#...
webrtc/p2p/base/port.cc:1101: if (!receiving) {
Please use an early return

if (receiving) {
  return;
}

https://codereview.webrtc.org/1351673003/diff/100001/webrtc/p2p/base/port.cc#...
webrtc/p2p/base/port.cc:1104: last_recv_time = std::max(last_recv_time,
connection_created_);
I wouldn't re-assign to last_recv_time.  I would do something like:

uint32 receive_deadline = std::max(last_recv_time, connection_created_) +
DEAD_CONNECTION_RECEIVE_TIMEOUT;
if (now > receive_deadline) {
  Destroy();
}

https://codereview.webrtc.org/1351673003/diff/100001/webrtc/p2p/base/port.h
File webrtc/p2p/base/port.h (right):

https://codereview.webrtc.org/1351673003/diff/100001/webrtc/p2p/base/port.h#n...
webrtc/p2p/base/port.h:54: // DEAD.
DEAD => dead.

https://codereview.webrtc.org/1351673003/diff/100001/webrtc/p2p/base/port.h#n...
webrtc/p2p/base/port.h:596: uint32 connection_created_;
If this is a timestamp, call it something like creation_time_ms_.

[honghaiz3, 2015-09-18 00:33:32]

Patchset #3 (id:120001) has been deleted

[honghaiz3, 2015-09-18 00:33:42]

Patchset #3 (id:140001) has been deleted

[honghaiz3, 2015-09-18 00:33:50]

Patchset #3 (id:160001) has been deleted

[honghaiz3, 2015-09-18 02:25:16]

Patchset #3 (id:180001) has been deleted

[honghaiz3, 2015-09-18 02:35:47]

https://codereview.webrtc.org/1351673003/diff/100001/webrtc/p2p/base/dtlstran...
File webrtc/p2p/base/dtlstransportchannel_unittest.cc (right):

https://codereview.webrtc.org/1351673003/diff/100001/webrtc/p2p/base/dtlstran...
webrtc/p2p/base/dtlstransportchannel_unittest.cc:202: bool writable() const {
return transport_->any_channels_writable(); }
On 2015/09/17 21:45:11, pthatcher1 wrote:
> Shouldn't that be all_channels_writable()?

No. See the deleted writable() in transport.h.

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

https://codereview.webrtc.org/1351673003/diff/100001/webrtc/p2p/base/p2ptrans...
webrtc/p2p/base/p2ptransportchannel.cc:852: if (connection->pruned() &&
!connection->receiving()) {
On 2015/09/17 21:45:11, pthatcher1 wrote:
> It might be nice to have a method connection->pinging() ==
> !connection->pruned().
> 
> Then we could write this:
> 
> if (!connection->pinging() && !connection->receiving()) {
>   continue;
> }

Having one method just returning the negative of another method looks redundant
(and may cause confusing). Since it will be used only once here, it might be
sufficient to just add some comments.  If you want to replace pruned with
!pinging everywhere, maybe we can do it in the next CL. WDYT?

https://codereview.webrtc.org/1351673003/diff/100001/webrtc/p2p/base/p2ptrans...
webrtc/p2p/base/p2ptransportchannel.cc:1049: // TODO(honghaiz): channel
receiving state is set in the OnCheckReceiving.
On 2015/09/17 21:45:11, pthatcher1 wrote:
> channel => The channel
> in the OnCheckingReceiving => in OnCheckingReceiving.

Done.

https://codereview.webrtc.org/1351673003/diff/100001/webrtc/p2p/base/p2ptrans...
webrtc/p2p/base/p2ptransportchannel.cc:1175: return !writable() ||
conn->write_state() != Connection::STATE_WRITE_TIMEOUT;
On 2015/09/17 21:45:11, pthatcher1 wrote:
> The comment seems to indicate the code should be:
> 
> return !writable() || !conn->pruned();
> 
> 
> Or this:
> 
> return conn->pinging() || !writable();
The current code and comments tried to be similar to the original code.
When Prune is called, it simply set the connection state to WRITE_TIMEOUT. 
The difference is that the write_state may still come out of the WRITE_TIMEOUT
but pruned will never. Plus, if a high-priority connection has timed out on
writing, it does not need to be pinged unless the channel is not writable. So I
prefer the existing code but change the comments. 
It probably does not matter that much considering we are timing out connections
and deleting them.

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

https://codereview.webrtc.org/1351673003/diff/100001/webrtc/p2p/base/p2ptrans...
webrtc/p2p/base/p2ptransportchannel_unittest.cc:1716: // Wait for conn1 being
pruned.
On 2015/09/17 21:45:11, pthatcher1 wrote:
> being pruned => to be pruned

Done.

https://codereview.webrtc.org/1351673003/diff/100001/webrtc/p2p/base/port.cc
File webrtc/p2p/base/port.cc (right):

https://codereview.webrtc.org/1351673003/diff/100001/webrtc/p2p/base/port.cc#...
webrtc/p2p/base/port.cc:907: if (last_ping_received_ != 0 ||
last_ping_response_received_ != 0) {
On 2015/09/17 21:45:11, pthatcher1 wrote:
> Why do we only do this if we've received a ping or ping response?  Why not
> always do it?
> 
Basically I am set_receiving at the same time when setting one of the 
 last_data_received, last_ping_received, last_ping_response_received, so that
receiving state will remain consistent. 
If I always do it here (set receiving), but did not update the
last_data_received, then, after a short while, when the channel checks the
connection state, it may become not receiving (because last_data_received was
not set).

https://codereview.webrtc.org/1351673003/diff/100001/webrtc/p2p/base/port.cc#...
webrtc/p2p/base/port.cc:1101: if (!receiving) {
On 2015/09/17 21:45:11, pthatcher1 wrote:
> Please use an early return
> 
> if (receiving) {
>   return;
> }

Done.

https://codereview.webrtc.org/1351673003/diff/100001/webrtc/p2p/base/port.cc#...
webrtc/p2p/base/port.cc:1104: last_recv_time = std::max(last_recv_time,
connection_created_);
On 2015/09/17 21:45:12, pthatcher1 wrote:
> I wouldn't re-assign to last_recv_time.  I would do something like:
> 
> uint32 receive_deadline = std::max(last_recv_time, connection_created_) +
> DEAD_CONNECTION_RECEIVE_TIMEOUT;
> if (now > receive_deadline) {
>   Destroy();
> }

Just remove the temporary var.

https://codereview.webrtc.org/1351673003/diff/100001/webrtc/p2p/base/port.h
File webrtc/p2p/base/port.h (right):

https://codereview.webrtc.org/1351673003/diff/100001/webrtc/p2p/base/port.h#n...
webrtc/p2p/base/port.h:54: // DEAD.
On 2015/09/17 21:45:12, pthatcher1 wrote:
> DEAD => dead.

Done.

https://codereview.webrtc.org/1351673003/diff/100001/webrtc/p2p/base/port.h#n...
webrtc/p2p/base/port.h:596: uint32 connection_created_;
On 2015/09/17 21:45:12, pthatcher1 wrote:
> If this is a timestamp, call it something like creation_time_ms_.

Use name time_created, similar to the style of last_ping_sent, etc.

[pthatcher1, 2015-09-18 17:54:42]

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

https://codereview.webrtc.org/1351673003/diff/100001/webrtc/p2p/base/p2ptrans...
webrtc/p2p/base/p2ptransportchannel.cc:852: if (connection->pruned() &&
!connection->receiving()) {
On 2015/09/18 02:35:46, honghaiz3 wrote:
> On 2015/09/17 21:45:11, pthatcher1 wrote:
> > It might be nice to have a method connection->pinging() ==
> > !connection->pruned().
> > 
> > Then we could write this:
> > 
> > if (!connection->pinging() && !connection->receiving()) {
> >   continue;
> > }
> 
> Having one method just returning the negative of another method looks
redundant
> (and may cause confusing). Since it will be used only once here, it might be
> sufficient to just add some comments.  If you want to replace pruned with
> !pinging everywhere, maybe we can do it in the next CL. WDYT?

It's OK.  We can just leave it pruned() and remember that pruned() == !pinging. 
Actually, can you just leave a nice comment above the pruned() method that says
"This means we're not pinging."?

https://codereview.webrtc.org/1351673003/diff/100001/webrtc/p2p/base/p2ptrans...
webrtc/p2p/base/p2ptransportchannel.cc:1175: return !writable() ||
conn->write_state() != Connection::STATE_WRITE_TIMEOUT;
On 2015/09/18 02:35:46, honghaiz3 wrote:
> On 2015/09/17 21:45:11, pthatcher1 wrote:
> > The comment seems to indicate the code should be:
> > 
> > return !writable() || !conn->pruned();
> > 
> > 
> > Or this:
> > 
> > return conn->pinging() || !writable();
> The current code and comments tried to be similar to the original code.
> When Prune is called, it simply set the connection state to WRITE_TIMEOUT. 
> The difference is that the write_state may still come out of the WRITE_TIMEOUT
> but pruned will never. Plus, if a high-priority connection has timed out on
> writing, it does not need to be pinged unless the channel is not writable. So
I
> prefer the existing code but change the comments. 
> It probably does not matter that much considering we are timing out
connections
> and deleting them.  

How does a connection go from WRITE_TIMEOUT to WRITABLE if it's pruned?  If it's
pruned, then it's not pinging.  If it's not pinging, then it can't get out of
WRITE_TIMEOUT.  

I think your original comment and "!writable() || !conn->pruned();" is more
readable than the new comment and "!writable() || conn->write_state() !=
Connection::STATE_WRITE_TIMEOUT;".

If they are equivalent, then we should go with the more readable one.  If they
aren't, then we should go with the correct one.

So the big question is: Under what conditions are they different, if any?  I
don't see one, but perhaps you do.  If you do, could you please explain it?

https://codereview.webrtc.org/1351673003/diff/100001/webrtc/p2p/base/port.cc
File webrtc/p2p/base/port.cc (right):

https://codereview.webrtc.org/1351673003/diff/100001/webrtc/p2p/base/port.cc#...
webrtc/p2p/base/port.cc:907: if (last_ping_received_ != 0 ||
last_ping_response_received_ != 0) {
On 2015/09/18 02:35:46, honghaiz3 wrote:
> On 2015/09/17 21:45:11, pthatcher1 wrote:
> > Why do we only do this if we've received a ping or ping response?  Why not
> > always do it?
> > 
> Basically I am set_receiving at the same time when setting one of the 
>  last_data_received, last_ping_received, last_ping_response_received, so that
> receiving state will remain consistent. 
> If I always do it here (set receiving), but did not update the
> last_data_received, then, after a short while, when the channel checks the
> connection state, it may become not receiving (because last_data_received was
> not set).  

Sorry, when I said "why no always do it", I meant the whole block of code, not
just set_receiving(true).  Why don't we just remove this if statement and run
all the code below it every time we get a packet?

[pthatcher1, 2015-09-18 17:56:55]

https://codereview.webrtc.org/1351673003/diff/200001/webrtc/p2p/base/dtlstran...
File webrtc/p2p/base/dtlstransportchannel_unittest.cc (right):

https://codereview.webrtc.org/1351673003/diff/200001/webrtc/p2p/base/dtlstran...
webrtc/p2p/base/dtlstransportchannel_unittest.cc:202: bool writable() const {
return transport_->any_channels_writable(); }
Shouldn't this be all_channels_writable()?  

And what happened to transport_->writable()?  Did that go away?

[pthatcher1, 2015-09-18 18:00:04]

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

https://codereview.webrtc.org/1351673003/diff/200001/webrtc/p2p/base/p2ptrans...
webrtc/p2p/base/p2ptransportchannel.cc:1150: if (best_connection_ &&
best_connection_->recv_total_bytes() > 0) {
What changed that made it so we don't need this any more?  Did we not need it
before either?

[pthatcher1, 2015-09-18 19:19:18]

On 2015/09/18 18:00:04, pthatcher1 wrote:
>
https://codereview.webrtc.org/1351673003/diff/200001/webrtc/p2p/base/p2ptrans...
> File webrtc/p2p/base/p2ptransportchannel.cc (left):
> 
>
https://codereview.webrtc.org/1351673003/diff/200001/webrtc/p2p/base/p2ptrans...
> webrtc/p2p/base/p2ptransportchannel.cc:1150: if (best_connection_ &&
> best_connection_->recv_total_bytes() > 0) {
> What changed that made it so we don't need this any more?  Did we not need it
> before either?

As we discussed in person, let's move the pruning behavior changes to the second
CL and just make the rule for this CL be "when we prune or hit WRITE_TIMEOUT,
delete the connection;  don't check readable/receiving state".

[honghaiz3, 2015-09-18 21:12:24]

Patchset #4 (id:220001) has been deleted

[honghaiz3, 2015-09-18 21:12:32]

Patchset #4 (id:230001) has been deleted

[honghaiz3, 2015-09-18 21:35:04]

Patchset #4 (id:250001) has been deleted

[honghaiz3, 2015-09-18 21:39:30]

Resynced and rebased and addressed the comments. PTAL.

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

https://codereview.webrtc.org/1351673003/diff/100001/webrtc/p2p/base/p2ptrans...
webrtc/p2p/base/p2ptransportchannel.cc:852: if (connection->pruned() &&
!connection->receiving()) {
On 2015/09/18 17:54:42, pthatcher1 wrote:
> On 2015/09/18 02:35:46, honghaiz3 wrote:
> > On 2015/09/17 21:45:11, pthatcher1 wrote:
> > > It might be nice to have a method connection->pinging() ==
> > > !connection->pruned().
> > > 
> > > Then we could write this:
> > > 
> > > if (!connection->pinging() && !connection->receiving()) {
> > >   continue;
> > > }
> > 
> > Having one method just returning the negative of another method looks
> redundant
> > (and may cause confusing). Since it will be used only once here, it might be
> > sufficient to just add some comments.  If you want to replace pruned with
> > !pinging everywhere, maybe we can do it in the next CL. WDYT?
> 
> It's OK.  We can just leave it pruned() and remember that pruned() ==
!pinging. 
> Actually, can you just leave a nice comment above the pruned() method that
says
> "This means we're not pinging."?
In fact we don't need this for this CL now since we are deleting them when
pruning them (Previously it kept more connections causing test failures).

https://codereview.webrtc.org/1351673003/diff/100001/webrtc/p2p/base/p2ptrans...
webrtc/p2p/base/p2ptransportchannel.cc:1175: return !writable() ||
conn->write_state() != Connection::STATE_WRITE_TIMEOUT;
Plus, write_out may be caused by just writing failures. 
So don't need to ping some high-priority connections which cannot write all the
time even if they are not pruned. 
On 2015/09/18 17:54:42, pthatcher1 wrote:
> On 2015/09/18 02:35:46, honghaiz3 wrote:
> > On 2015/09/17 21:45:11, pthatcher1 wrote:
> > > The comment seems to indicate the code should be:
> > > 
> > > return !writable() || !conn->pruned();
> > > 
> > > 
> > > Or this:
> > > 
> > > return conn->pinging() || !writable();
> > The current code and comments tried to be similar to the original code.
> > When Prune is called, it simply set the connection state to WRITE_TIMEOUT. 
> > The difference is that the write_state may still come out of the
WRITE_TIMEOUT
> > but pruned will never. Plus, if a high-priority connection has timed out on
> > writing, it does not need to be pinged unless the channel is not writable.
So
> I
> > prefer the existing code but change the comments. 
> > It probably does not matter that much considering we are timing out
> connections
> > and deleting them.  
> 
> How does a connection go from WRITE_TIMEOUT to WRITABLE if it's pruned?  If
it's
> pruned, then it's not pinging.  If it's not pinging, then it can't get out of
> WRITE_TIMEOUT.  
> 
> I think your original comment and "!writable() || !conn->pruned();" is more
> readable than the new comment and "!writable() || conn->write_state() !=
> Connection::STATE_WRITE_TIMEOUT;".
> 
> If they are equivalent, then we should go with the more readable one.  If they
> aren't, then we should go with the correct one.
> 
> So the big question is: Under what conditions are they different, if any?  I
> don't see one, but perhaps you do.  If you do, could you please explain it?
pruned is not always equal to write_timeout. 
When pruning, it will set write state to write_timeout. 
pruned state will never change back. 
But in the state of write_timeout may change back to writable (if the transport
channel becomes not writable, it will try to ping everything, so a previous
write_timeout connection may become writable again, although the chance is
small.

https://codereview.webrtc.org/1351673003/diff/100001/webrtc/p2p/base/port.cc
File webrtc/p2p/base/port.cc (right):

https://codereview.webrtc.org/1351673003/diff/100001/webrtc/p2p/base/port.cc#...
webrtc/p2p/base/port.cc:907: if (last_ping_received_ != 0 ||
last_ping_response_received_ != 0) {
On 2015/09/18 17:54:42, pthatcher1 wrote:
> On 2015/09/18 02:35:46, honghaiz3 wrote:
> > On 2015/09/17 21:45:11, pthatcher1 wrote:
> > > Why do we only do this if we've received a ping or ping response?  Why not
> > > always do it?
> > > 
> > Basically I am set_receiving at the same time when setting one of the 
> >  last_data_received, last_ping_received, last_ping_response_received, so
that
> > receiving state will remain consistent. 
> > If I always do it here (set receiving), but did not update the
> > last_data_received, then, after a short while, when the channel checks the
> > connection state, it may become not receiving (because last_data_received
was
> > not set).  
> 
> Sorry, when I said "why no always do it", I meant the whole block of code, not
> just set_receiving(true).  Why don't we just remove this if statement and run
> all the code below it every time we get a packet?
OK. Just remove the condition.

https://codereview.webrtc.org/1351673003/diff/200001/webrtc/p2p/base/dtlstran...
File webrtc/p2p/base/dtlstransportchannel_unittest.cc (right):

https://codereview.webrtc.org/1351673003/diff/200001/webrtc/p2p/base/dtlstran...
webrtc/p2p/base/dtlstransportchannel_unittest.cc:202: bool writable() const {
return transport_->any_channels_writable(); }
On 2015/09/18 17:56:54, pthatcher1 wrote:
> Shouldn't this be all_channels_writable()?  
> 
> And what happened to transport_->writable()?  Did that go away?
I tried to removed it. 
But to be safe, I revert it and leave that in a separate CL.

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

https://codereview.webrtc.org/1351673003/diff/200001/webrtc/p2p/base/p2ptrans...
webrtc/p2p/base/p2ptransportchannel.cc:1150: if (best_connection_ &&
best_connection_->recv_total_bytes() > 0) {
On 2015/09/18 18:00:04, pthatcher1 wrote:
> What changed that made it so we don't need this any more?  Did we not need it
> before either?
Changed this to fix a unittest. In unittest, we sometimes do not send data. 
It was not absolutely needed before.

[pthatcher1, 2015-09-18 21:51:33]

https://codereview.webrtc.org/1351673003/diff/270001/webrtc/p2p/base/port.cc
File webrtc/p2p/base/port.cc (right):

https://codereview.webrtc.org/1351673003/diff/270001/webrtc/p2p/base/port.cc#...
webrtc/p2p/base/port.cc:916: set_write_state(STATE_WRITE_INIT);
Interesting.  This basically means "ping every connection for which we are
receiving data".  That's kind of like "unprune if we receive data".  We should
remember to address this in your second CL when we change the definition of
"pruned".

https://codereview.webrtc.org/1351673003/diff/270001/webrtc/p2p/base/port.cc#...
webrtc/p2p/base/port.cc:1102: }
Doesn't CheckTimeout() handle this?  I think we can leave this part off (and
time_created_ also).

[honghaiz3, 2015-09-18 22:01:49]

Patchset #5 (id:250002) has been deleted

[honghaiz3, 2015-09-18 22:03:02]

https://codereview.webrtc.org/1351673003/diff/270001/webrtc/p2p/base/port.cc
File webrtc/p2p/base/port.cc (right):

https://codereview.webrtc.org/1351673003/diff/270001/webrtc/p2p/base/port.cc#...
webrtc/p2p/base/port.cc:916: set_write_state(STATE_WRITE_INIT);
On 2015/09/18 21:51:33, pthatcher1 wrote:
> Interesting.  This basically means "ping every connection for which we are
> receiving data".  That's kind of like "unprune if we receive data".  We should
> remember to address this in your second CL when we change the definition of
> "pruned".
We need to un-prune sometimes. We also need this in other cases.

https://codereview.webrtc.org/1351673003/diff/270001/webrtc/p2p/base/port.cc#...
webrtc/p2p/base/port.cc:1102: }
On 2015/09/18 21:51:33, pthatcher1 wrote:
> Doesn't CheckTimeout() handle this?  I think we can leave this part off (and
> time_created_ also).

Done.

[pthatcher1, 2015-09-18 22:44:02]

lgtm

[honghaiz3, 2015-09-21 13:53:01]

The CQ bit was checked by honghaiz@webrtc.org

[honghaiz3, 2015-09-21 13:53:02]

The patchset sent to the CQ was uploaded after l-g-t-m from pthatcher@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1351673003/#ps340001 (title:
"Removed a comment in transport.cc")

[commit-bot: I haz the power, 2015-09-21 13:53:13]

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

[commit-bot: I haz the power, 2015-09-21 13:54:19]

Committed patchset #7 (id:340001)

[commit-bot: I haz the power, 2015-09-21 13:54:29]

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

[tommi, 2015-09-21 14:20:16]

A revert of this CL (patchset #7 id:340001) has been created in
https://codereview.webrtc.org/1356103002/ by tommi@webrtc.org.

The reason for reverting is: Broke the Windows build:

[226/365] LINK_EMBED cc_perftests.exe
FAILED: ninja -t msvc -e environment.x86 -- E:\b\build\goma/gomacc
"E:\b\depot_tools\win_toolchain\vs2013_files\VC\bin\amd64_x86\cl.exe" /nologo
/showIncludes /FC
@obj\remoting\protocol\remoting_unittests.channel_socket_adapter_unittest.obj.rsp
/c ..\..\remoting\protocol\channel_socket_adapter_unittest.cc
/Foobj\remoting\protocol\remoting_unittests.channel_socket_adapter_unittest.obj
/Fdobj\remoting\remoting_unittests.cc.pdb 
e:\b\build\slave\win\build\src\remoting\protocol\channel_socket_adapter_unittest.cc(36)
: error C3861: 'set_readable': identifier not found
ninja: build stopped: subcommand failed..

[tommi, 2015-09-21 14:22:34]

On 2015/09/21 14:20:16, tommi (webrtc) wrote:
> A revert of this CL (patchset #7 id:340001) has been created in
> https://codereview.webrtc.org/1356103002/ by mailto:tommi@webrtc.org.
> 
> The reason for reverting is: Broke the Windows build:
> 
> [226/365] LINK_EMBED cc_perftests.exe
> FAILED: ninja -t msvc -e environment.x86 -- E:\b\build\goma/gomacc
> "E:\b\depot_tools\win_toolchain\vs2013_files\VC\bin\amd64_x86\cl.exe" /nologo
> /showIncludes /FC
>
@obj\remoting\protocol\remoting_unittests.channel_socket_adapter_unittest.obj.rsp
> /c ..\..\remoting\protocol\channel_socket_adapter_unittest.cc
>
/Foobj\remoting\protocol\remoting_unittests.channel_socket_adapter_unittest.obj
> /Fdobj\remoting\remoting_unittests.cc.pdb 
>
e:\b\build\slave\win\build\src\remoting\protocol\channel_socket_adapter_unittest.cc(36)
> : error C3861: 'set_readable': identifier not found
> ninja: build stopped: subcommand failed..

Same issue on other bots now (e.g. Mac)