Changes connection migration code to migrate on NetworkMadeDefault

Changes connection migration code to migrate on NetworkMadeDefault
instead of on SoonToDisconnect.

The NetworkMadeDefault notification is supposed to always be issued
before SoonToDisconnect, but there seems to be a bug in Android that
causes the order to sometimes be reversed. Current connection migration
looks for an alternate network on SoonToDisconnect, and this may not
work since there may be no alternate networks around. Since
NetworkMadeDefault guarantees that the new default network is around,
this CL changes connection migration code to migrate on
NetworkMadeDefault instead.

BUG=

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

[Jana, 2016-08-19 02:28:42]

jri@chromium.org changed reviewers:
+ pauljensen@chromium.org, rch@chromium.org

[pauljensen, 2016-08-19 11:10:47]

The idea sgtm; I have little knowledge of the code.

[Ryan Hamilton, 2016-08-19 20:05:02]

Looks good. A couple questions.

https://codereview.chromium.org/2258893004/diff/1/net/quic/chromium/quic_stre...
File net/quic/chromium/quic_stream_factory.cc (left):

https://codereview.chromium.org/2258893004/diff/1/net/quic/chromium/quic_stre...
net/quic/chromium/quic_stream_factory.cc:1526: if (force_close) {
This looks like a change in behavior, but I might be misunderstand it because of
the way the semantics have changed. Can you say more?

https://codereview.chromium.org/2258893004/diff/1/net/quic/chromium/quic_stre...
File net/quic/chromium/quic_stream_factory_test.cc (left):

https://codereview.chromium.org/2258893004/diff/1/net/quic/chromium/quic_stre...
net/quic/chromium/quic_stream_factory_test.cc:1644:
TEST_P(QuicStreamFactoryTest, OnNetworkChangeSoonToDisconnectNoNetworks) {
Presumably there is a test further down in which we test what happens when there
is no network after the current is disconnected?

[Jana, 2016-08-19 21:44:08]

Thanks, Ryan. PTAL

https://codereview.chromium.org/2258893004/diff/1/net/quic/chromium/quic_stre...
File net/quic/chromium/quic_stream_factory.cc (left):

https://codereview.chromium.org/2258893004/diff/1/net/quic/chromium/quic_stre...
net/quic/chromium/quic_stream_factory.cc:1526: if (force_close) {
On 2016/08/19 20:05:02, Ryan Hamilton wrote:
> This looks like a change in behavior, but I might be misunderstand it because
of
> the way the semantics have changed. Can you say more?

I've moved the code around a bit, hopefully it'll help explain. I've also
changed the comment in the header file to explain better.

https://codereview.chromium.org/2258893004/diff/1/net/quic/chromium/quic_stre...
File net/quic/chromium/quic_stream_factory_test.cc (left):

https://codereview.chromium.org/2258893004/diff/1/net/quic/chromium/quic_stre...
net/quic/chromium/quic_stream_factory_test.cc:1644:
TEST_P(QuicStreamFactoryTest, OnNetworkChangeSoonToDisconnectNoNetworks) {
On 2016/08/19 20:05:02, Ryan Hamilton wrote:
> Presumably there is a test further down in which we test what happens when
there
> is no network after the current is disconnected?

Yup -- "OnNetworkChangeDisconnectedNoNewNetwork"

[Ryan Hamilton, 2016-08-19 22:03:53]

Looks good mod one suggestion. thanks!

https://codereview.chromium.org/2258893004/diff/20001/net/quic/chromium/quic_...
File net/quic/chromium/quic_stream_factory.cc (right):

https://codereview.chromium.org/2258893004/diff/20001/net/quic/chromium/quic_...
net/quic/chromium/quic_stream_factory.cc:1498: while (it != all_sessions_.end())
{
nit: instead of having this if(...) clause outside the while loop (which
duplicates the while loop conditions before, can you move this if and body (w/o
the while) down to line 1514... inside the main while?

https://codereview.chromium.org/2258893004/diff/20001/net/quic/chromium/quic_...
net/quic/chromium/quic_stream_factory.cc:1541: // Close sessions where
connection migration is disabled.
nit: up to you, but I'd drop this comment and the one below on 1554

[Jana, 2016-08-20 01:48:32]

Thanks, Ryan. PTAL

https://codereview.chromium.org/2258893004/diff/20001/net/quic/chromium/quic_...
File net/quic/chromium/quic_stream_factory.cc (right):

https://codereview.chromium.org/2258893004/diff/20001/net/quic/chromium/quic_...
net/quic/chromium/quic_stream_factory.cc:1498: while (it != all_sessions_.end())
{
On 2016/08/19 22:03:53, Ryan Hamilton wrote:
> nit: instead of having this if(...) clause outside the while loop (which
> duplicates the while loop conditions before, can you move this if and body
(w/o
> the while) down to line 1514... inside the main while?

It used to be inside, but I thought it might be clearer this way ... clearly not
:-)  Done.

https://codereview.chromium.org/2258893004/diff/20001/net/quic/chromium/quic_...
net/quic/chromium/quic_stream_factory.cc:1541: // Close sessions where
connection migration is disabled.
On 2016/08/19 22:03:53, Ryan Hamilton wrote:
> nit: up to you, but I'd drop this comment and the one below on 1554

Done.

[Ryan Hamilton, 2016-08-20 04:21:14]

lgtm

[Jana, 2016-08-20 05:41:52]

The CQ bit was checked by jri@chromium.org

[commit-bot: I haz the power, 2016-08-20 05:42:05]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-20 07:03:19]

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

[commit-bot: I haz the power, 2016-08-20 07:03:20]

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

[Jana, 2016-08-20 07:45:41]

The CQ bit was checked by jri@chromium.org

[commit-bot: I haz the power, 2016-08-20 07:45:52]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-20 08:25:16]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-08-20 08:26:46]

Description was changed from

==========
Changes connection migration code to migrate on NetworkMadeDefault
instead of on SoonToDisconnect.

The NetworkMadeDefault notification is supposed to always be issued
before SoonToDisconnect, but there seems to be a bug in Android that
causes the order to sometimes be reversed. Current connection migration
looks for an alternate network on SoonToDisconnect, and this may not
work since there may be no alternate networks around. Since
NetworkMadeDefault guarantees that the new default network is around,
this CL changes connection migration code to migrate on
NetworkMadeDefault instead.

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

to

==========
Changes connection migration code to migrate on NetworkMadeDefault
instead of on SoonToDisconnect.

The NetworkMadeDefault notification is supposed to always be issued
before SoonToDisconnect, but there seems to be a bug in Android that
causes the order to sometimes be reversed. Current connection migration
looks for an alternate network on SoonToDisconnect, and this may not
work since there may be no alternate networks around. Since
NetworkMadeDefault guarantees that the new default network is around,
this CL changes connection migration code to migrate on
NetworkMadeDefault instead.

BUG=

Committed: https://crrev.com/e2661985a8fc716a61934a9c713ff94094155a99
Cr-Commit-Position: refs/heads/master@{#413327}
==========

[commit-bot: I haz the power, 2016-08-20 08:26:47]

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