Do not time out a port if its role switched from controlled to controlling.

Do not time out a port if its role switched from controlled to controlling. Also fix some comments.
BUG=webrtc:5026
Committed: https://crrev.com/d0b3143f0e37f5f5e0578e88cf740dd839b50c24
Cr-Commit-Position: refs/heads/master@{#10122}

[honghaiz3, 2015-09-29 22:23:23]

Patchset #1 (id:1) has been deleted

[honghaiz3, 2015-09-29 22:27:01]

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

[pthatcher1, 2015-09-29 22:54:18]

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

https://codereview.webrtc.org/1376983002/diff/20001/webrtc/p2p/base/port.cc#n...
webrtc/p2p/base/port.cc:634: CheckTimeout();
Can you remove CheckTimeout and just put the logic here?

if (dead()) {
  Destroy();
}

https://codereview.webrtc.org/1376983002/diff/20001/webrtc/p2p/base/port.cc#n...
webrtc/p2p/base/port.cc:660: if (connections_.empty()) {
I think we should make the conditions the same inside of CheckTimeout as here:
"ice_role_ == ICEROLE_CONTROLLED && connections_.empty()". In fact, I think it
would make sense to make a helper method.  And MSG_DEAD would be a better name
for the message.

bool Port::dead() {
  if (ice_role_ == ICEROLE_CONTROLLED) {
    return connections_.empty();
  }
  return false;
}

if (dead()) {
  thread_->PostDelayed(timeout_delay_, this, MSG_DEAD);
}

[honghaiz3, 2015-09-30 15:58:59]

Patchset #3 (id:60001) has been deleted

[honghaiz3, 2015-09-30 15:59:06]

Patchset #2 (id:40001) has been deleted

[honghaiz3, 2015-09-30 15:59:39]

Addressed comments. PTAL.

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

https://codereview.webrtc.org/1376983002/diff/20001/webrtc/p2p/base/port.cc#n...
webrtc/p2p/base/port.cc:634: CheckTimeout();
On 2015/09/29 22:54:18, pthatcher1 wrote:
> Can you remove CheckTimeout and just put the logic here?
> 
> if (dead()) {
>   Destroy();
> }

Done.

https://codereview.webrtc.org/1376983002/diff/20001/webrtc/p2p/base/port.cc#n...
webrtc/p2p/base/port.cc:660: if (connections_.empty()) {
On 2015/09/29 22:54:18, pthatcher1 wrote:
> I think we should make the conditions the same inside of CheckTimeout as here:
> "ice_role_ == ICEROLE_CONTROLLED && connections_.empty()". In fact, I think it
> would make sense to make a helper method.  And MSG_DEAD would be a better name
> for the message.
> 
> bool Port::dead() {
>   if (ice_role_ == ICEROLE_CONTROLLED) {
>     return connections_.empty();
>   }
>   return false;
> }
> 
> if (dead()) {
>   thread_->PostDelayed(timeout_delay_, this, MSG_DEAD);
> }

Done. Except that using a single line for the dead method.

[pthatcher1, 2015-09-30 17:08:03]

lgtm

https://codereview.webrtc.org/1376983002/diff/80001/webrtc/p2p/base/port_unit...
File webrtc/p2p/base/port_unittest.cc (right):

https://codereview.webrtc.org/1376983002/diff/80001/webrtc/p2p/base/port_unit...
webrtc/p2p/base/port_unittest.cc:2415: // to controlling after all connections
fail, the port will not time out.
will not timeout => will not be destroyed

https://codereview.webrtc.org/1376983002/diff/80001/webrtc/p2p/base/port_unit...
webrtc/p2p/base/port_unittest.cc:2416: TEST_F(PortTest,
TestControlledToControllingNoTimeout) {
NoTimeout => NotDestroyed

[honghaiz3, 2015-09-30 17:15:13]

The CQ bit was checked by honghaiz@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2015-09-30 17:15:23]

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

[honghaiz3, 2015-09-30 17:17:53]

https://codereview.webrtc.org/1376983002/diff/80001/webrtc/p2p/base/port_unit...
File webrtc/p2p/base/port_unittest.cc (right):

https://codereview.webrtc.org/1376983002/diff/80001/webrtc/p2p/base/port_unit...
webrtc/p2p/base/port_unittest.cc:2415: // to controlling after all connections
fail, the port will not time out.
On 2015/09/30 17:08:03, pthatcher1 wrote:
> will not timeout => will not be destroyed

Done.

https://codereview.webrtc.org/1376983002/diff/80001/webrtc/p2p/base/port_unit...
webrtc/p2p/base/port_unittest.cc:2416: TEST_F(PortTest,
TestControlledToControllingNoTimeout) {
On 2015/09/30 17:08:03, pthatcher1 wrote:
> NoTimeout => NotDestroyed

Done.

[honghaiz3, 2015-09-30 17:18:01]

The CQ bit was checked by honghaiz@webrtc.org

[honghaiz3, 2015-09-30 17:18:01]

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/1376983002/#ps100001 (title:
" ")

[commit-bot: I haz the power, 2015-09-30 17:18:15]

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

[commit-bot: I haz the power, 2015-09-30 19:42:22]

Committed patchset #3 (id:100001)

[commit-bot: I haz the power, 2015-09-30 19:42:37]

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