When doing DisableEquivalentPhases, exclude those AllocationSequences

When doing DisableEquivalentPhases, exclude those AllocationSequences
whose network has ever been removed. It is unlikely the sockets/ports/candidates created from
those AllocationSequences will still be valid.

BUG=

Committed: https://crrev.com/8c404fab8d9ce5ef87046fb57ec8ac2c90fad9ac
Cr-Commit-Position: refs/heads/master@{#10093}

[honghaiz3, 2015-09-24 15:28:46]

Patchset #1 (id:1) has been deleted

[honghaiz3, 2015-09-24 15:28:55]

Patchset #1 (id:20001) has been deleted

[honghaiz3, 2015-09-24 15:29:01]

Patchset #1 (id:40001) has been deleted

[honghaiz3, 2015-09-24 15:30:49]

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

[honghaiz3, 2015-09-24 15:30:49]

Please take a look at the CL.

[honghaiz3, 2015-09-24 16:42:42]

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

[commit-bot: I haz the power, 2015-09-24 16:42:50]

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

[commit-bot: I haz the power, 2015-09-24 16:47:10]

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

[commit-bot: I haz the power, 2015-09-24 16:47:11]

Dry run: Try jobs failed on following builders:
  win_compile_dbg on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/win_compile_dbg/builds/104)

[pthatcher1, 2015-09-24 17:11:44]

I need a little more clarity on the difference between stopped, terminated,
clear(), and terminate().  

Also, I think we need a flag to control whether this behavior happens or not.  I
don't think we can just turn this on by default.

https://codereview.webrtc.org/1361183004/diff/60001/webrtc/p2p/client/basicpo...
File webrtc/p2p/client/basicportallocator.cc (right):

https://codereview.webrtc.org/1361183004/diff/60001/webrtc/p2p/client/basicpo...
webrtc/p2p/client/basicportallocator.cc:408: if (!sequence->IsTerminated()) {
Can we just make DisableEquivalentPhases check to see if it's terminated and
then no-op?  Then we don't have to check it at every call site.

https://codereview.webrtc.org/1361183004/diff/60001/webrtc/p2p/client/basicpo...
webrtc/p2p/client/basicportallocator.cc:735: void
AllocationSequence::Terminate() {
How is this much different than calling Clear()?  Why do we have both?

https://codereview.webrtc.org/1361183004/diff/60001/webrtc/p2p/client/basicpo...
File webrtc/p2p/client/basicportallocator.h (right):

https://codereview.webrtc.org/1361183004/diff/60001/webrtc/p2p/client/basicpo...
webrtc/p2p/client/basicportallocator.h:255: // it depends on is removed
(disconnected).
How is that different than kStopped?

https://codereview.webrtc.org/1361183004/diff/60001/webrtc/p2p/client/basicpo...
webrtc/p2p/client/basicportallocator.h:269: bool IsTerminated() { return state_
== kTerminated; }
Is this needed when you can just do state() == kTerminated?

https://codereview.webrtc.org/1361183004/diff/60001/webrtc/p2p/client/portall...
File webrtc/p2p/client/portallocator_unittest.cc (right):

https://codereview.webrtc.org/1361183004/diff/60001/webrtc/p2p/client/portall...
webrtc/p2p/client/portallocator_unittest.cc:489: // generated.
You mean should not be generated?

[honghaiz3, 2015-09-24 19:12:26]

Patchset #2 (id:80001) has been deleted

[honghaiz3, 2015-09-24 19:16:38]

Patchset #2 (id:100001) has been deleted

[honghaiz3, 2015-09-24 19:27:00]

Made some revisions. Removed the state kTerminated. Instead just set the
network_ to be null. That would have the same effect. 
I feel we may not need a flag to control this because
1. This is sort of a bug. Previously when a network interface switched off and
on again, the old allocation seq still contains the once-switched-off network
and the new allocation sequence will not be started because it think it is
equivalent to an existing seq. But since the network has been down and up again.
All the port/socket/connections in the previous seq is not valid any more. 
2. The behavior is still controlled by the whether we call StopGettingPorts. If
we have called StopGettingPorts (in p2ptransportchannel), then the seq won't be
started. That is the place we will need to control.

https://codereview.webrtc.org/1361183004/diff/60001/webrtc/p2p/client/basicpo...
File webrtc/p2p/client/basicportallocator.cc (right):

https://codereview.webrtc.org/1361183004/diff/60001/webrtc/p2p/client/basicpo...
webrtc/p2p/client/basicportallocator.cc:408: if (!sequence->IsTerminated()) {
On 2015/09/24 17:11:43, pthatcher1 wrote:
> Can we just make DisableEquivalentPhases check to see if it's terminated and
> then no-op?  Then we don't have to check it at every call site.

Done.

https://codereview.webrtc.org/1361183004/diff/60001/webrtc/p2p/client/basicpo...
webrtc/p2p/client/basicportallocator.cc:735: void
AllocationSequence::Terminate() {
On 2015/09/24 17:11:43, pthatcher1 wrote:
> How is this much different than calling Clear()?  Why do we have both?

Removed the state.

https://codereview.webrtc.org/1361183004/diff/60001/webrtc/p2p/client/basicpo...
File webrtc/p2p/client/basicportallocator.h (right):

https://codereview.webrtc.org/1361183004/diff/60001/webrtc/p2p/client/basicpo...
webrtc/p2p/client/basicportallocator.h:255: // it depends on is removed
(disconnected).
On 2015/09/24 17:11:43, pthatcher1 wrote:
> How is that different than kStopped?

Removed the state.

https://codereview.webrtc.org/1361183004/diff/60001/webrtc/p2p/client/basicpo...
webrtc/p2p/client/basicportallocator.h:269: bool IsTerminated() { return state_
== kTerminated; }
On 2015/09/24 17:11:43, pthatcher1 wrote:
> Is this needed when you can just do state() == kTerminated?
Removed the new state.

https://codereview.webrtc.org/1361183004/diff/60001/webrtc/p2p/client/portall...
File webrtc/p2p/client/portallocator_unittest.cc (right):

https://codereview.webrtc.org/1361183004/diff/60001/webrtc/p2p/client/portall...
webrtc/p2p/client/portallocator_unittest.cc:489: // generated.
On 2015/09/24 17:11:43, pthatcher1 wrote:
> You mean should not be generated?

Done.

[honghaiz3, 2015-09-24 19:29:07]

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

[commit-bot: I haz the power, 2015-09-24 19:29:19]

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

[commit-bot: I haz the power, 2015-09-24 21:30:01]

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

[commit-bot: I haz the power, 2015-09-24 21:30:02]

Dry run: Try jobs failed on following builders:
  android_arm64_rel on tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  android_dbg on tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  android_rel on tryserver.webrtc (JOB_TIMED_OUT, no build URL)

[pthatcher1, 2015-09-24 21:41:43]

https://codereview.webrtc.org/1361183004/diff/120001/webrtc/p2p/client/basicp...
File webrtc/p2p/client/basicportallocator.cc (right):

https://codereview.webrtc.org/1361183004/diff/120001/webrtc/p2p/client/basicp...
webrtc/p2p/client/basicportallocator.cc:396: }
Wouldn't this be the same as:

if (sequence->state() != kStopped && 
    std::find(networks.begin(), networks.end(), sequence->network()) ==
              networks.end()) {
  sequence->Stop();
}

?

https://codereview.webrtc.org/1361183004/diff/120001/webrtc/p2p/client/basicp...
webrtc/p2p/client/basicportallocator.cc:736: network_ = nullptr;
There's lot of code in this file that expects network_ not not be null.   Will
none of that ever run?  If not, what's the point of setting network_ = null?

[honghaiz3, 2015-09-24 22:38:41]

Patchset #3 (id:140001) has been deleted

[honghaiz3, 2015-09-24 22:41:28]

Revised based on offline discussion.

https://codereview.webrtc.org/1361183004/diff/120001/webrtc/p2p/client/basicp...
File webrtc/p2p/client/basicportallocator.cc (right):

https://codereview.webrtc.org/1361183004/diff/120001/webrtc/p2p/client/basicp...
webrtc/p2p/client/basicportallocator.cc:396: }
On 2015/09/24 21:41:43, pthatcher1 wrote:
> Wouldn't this be the same as:
> 
> if (sequence->state() != kStopped && 
>     std::find(networks.begin(), networks.end(), sequence->network()) ==
>               networks.end()) {
>   sequence->Stop();
> }
> 
> ?
Not exactly as we discussed.

https://codereview.webrtc.org/1361183004/diff/120001/webrtc/p2p/client/basicp...
webrtc/p2p/client/basicportallocator.cc:736: network_ = nullptr;
On 2015/09/24 21:41:43, pthatcher1 wrote:
> There's lot of code in this file that expects network_ not not be null.   Will
> none of that ever run?  If not, what's the point of setting network_ = null?
Changed to use a binary variable.

[honghaiz3, 2015-09-24 22:42:17]

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

[commit-bot: I haz the power, 2015-09-24 22:42:35]

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

[commit-bot: I haz the power, 2015-09-25 00:42:45]

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

[commit-bot: I haz the power, 2015-09-25 00:42:46]

Dry run: Try jobs failed on following builders:
  android_arm64_rel on tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  android_rel on tryserver.webrtc (JOB_TIMED_OUT, no build URL)

[pthatcher1, 2015-09-25 23:49:10]

lgtm, with nits

https://codereview.webrtc.org/1361183004/diff/160001/webrtc/p2p/client/basicp...
File webrtc/p2p/client/basicportallocator.cc (right):

https://codereview.webrtc.org/1361183004/diff/160001/webrtc/p2p/client/basicp...
webrtc/p2p/client/basicportallocator.cc:733: void
AllocationSequence::RemoveNetwork() {
I'd call this OnNetworkRemoved.

https://codereview.webrtc.org/1361183004/diff/160001/webrtc/p2p/client/basicp...
webrtc/p2p/client/basicportallocator.cc:747: // be equivalent to the new
network.
has ever gone => has ever gone away

[honghaiz3, 2015-09-25 23:59:40]

https://codereview.webrtc.org/1361183004/diff/160001/webrtc/p2p/client/basicp...
File webrtc/p2p/client/basicportallocator.cc (right):

https://codereview.webrtc.org/1361183004/diff/160001/webrtc/p2p/client/basicp...
webrtc/p2p/client/basicportallocator.cc:733: void
AllocationSequence::RemoveNetwork() {
On 2015/09/25 23:49:10, pthatcher1 wrote:
> I'd call this OnNetworkRemoved.

Done.

https://codereview.webrtc.org/1361183004/diff/160001/webrtc/p2p/client/basicp...
webrtc/p2p/client/basicportallocator.cc:747: // be equivalent to the new
network.
On 2015/09/25 23:49:10, pthatcher1 wrote:
> has ever gone => has ever gone away

Done.

[honghaiz3, 2015-09-26 00:01:10]

The CQ bit was checked by honghaiz@webrtc.org

[honghaiz3, 2015-09-26 00:01:11]

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/1361183004/#ps180001 (title:
" ")

[commit-bot: I haz the power, 2015-09-26 00:01:24]

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

[commit-bot: I haz the power, 2015-09-26 02:01:36]

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

[commit-bot: I haz the power, 2015-09-26 02:01:37]

Try jobs failed on following builders:
  android_arm64_rel on tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  android_dbg on tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  android_rel on tryserver.webrtc (JOB_TIMED_OUT, no build URL)

[honghaiz3, 2015-09-28 02:50:39]

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

[commit-bot: I haz the power, 2015-09-28 02:50:53]

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

[commit-bot: I haz the power, 2015-09-28 03:59:13]

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

[commit-bot: I haz the power, 2015-09-28 03:59:14]

Dry run: This issue passed the CQ dry run.

[honghaiz3, 2015-09-28 14:58:34]

The CQ bit was checked by honghaiz@webrtc.org

[commit-bot: I haz the power, 2015-09-28 14:58:41]

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

[commit-bot: I haz the power, 2015-09-28 14:59:46]

Committed patchset #4 (id:180001)

[commit-bot: I haz the power, 2015-09-28 14:59:57]

Patchset 4 (id:??) landed as
https://crrev.com/8c404fab8d9ce5ef87046fb57ec8ac2c90fad9ac
Cr-Commit-Position: refs/heads/master@{#10093}