cros: Do not schedule attempt when behind captive portal with response 200 is detected

cros: Do not schedule attempt when behind captive portal with response 200 is detected

Changes:
(1) Do not schedule attempt if we are already behind captive portal with response 200, which can be either the result of NetworkPortalDetector or shill's detection. In test, this can reduce the call times of CaptivePortalDetector::DetectCaptivePortal from 6+ to 2~3 within 10s.
(2) modify tests to adapt this change

BUG=702273
TEST=If we log in CaptivePortalDetector::DetectCaptivePortal, we can see the attempts reduced to 2~3 for connecting to captive portal wifi.

Review-Url: https://codereview.chromium.org/2756643002
Cr-Commit-Position: refs/heads/master@{#457829}
Committed: https://chromium.googlesource.com/chromium/src/+/db14329b13d1109723acf5528a0253d6ee8c1651

[Qiang(Joe) Xu, 2017-03-16 04:07:27]

The CQ bit was checked by warx@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-16 04:07:50]

Dry run: 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, 2017-03-16 05:39:17]

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

[commit-bot: I haz the power, 2017-03-16 05:39:18]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Qiang(Joe) Xu, 2017-03-16 17:04:26]

Description was changed from

==========
cros: Do not schedule attempt when behind captive portal is detected

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

to

==========
cros: Do not schedule attempt when behind captive portal is detected

Changes:
Do not schedule attempt if we are already behind captive portal, which can be
either the result of NetworkPortalDetector or shill's detection. In test, this
can reduce the call times of CaptivePortalDetector::DetectCaptivePortal from 6+
to 2 within 10s.

BUG=702273
TEST=If we log in CaptivePortalDetector::DetectCaptivePortal, we can see the
attempts reduced to 2 for connecting to captive portal wifi.
==========

[Qiang(Joe) Xu, 2017-03-16 17:06:15]

warx@chromium.org changed reviewers:
+ stevenjb@chromium.org

[Qiang(Joe) Xu, 2017-03-16 17:07:20]

Hi Steven, PTAL, thanks!

[stevenjb, 2017-03-17 00:26:46]

Seems reasonable, lgtm, thanks!

https://codereview.chromium.org/2756643002/diff/1/chrome/browser/chromeos/net...
File chrome/browser/chromeos/net/network_portal_detector_impl.cc (right):

https://codereview.chromium.org/2756643002/diff/1/chrome/browser/chromeos/net...
chrome/browser/chromeos/net/network_portal_detector_impl.cc:554: // schedule new
attempt or we are already behind captive portal, which can be
nit: 'has already scheduled a new attempt', 'behind a captive portal'

[Qiang(Joe) Xu, 2017-03-17 02:17:23]

The CQ bit was checked by warx@chromium.org

[Qiang(Joe) Xu, 2017-03-17 02:17:23]

The patchset sent to the CQ was uploaded after l-g-t-m from
stevenjb@chromium.org
Link to the patchset: https://codereview.chromium.org/2756643002/#ps20001
(title: "nits")

[commit-bot: I haz the power, 2017-03-17 02:17:55]

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, 2017-03-17 02:58:03]

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

[commit-bot: I haz the power, 2017-03-17 02:58:04]

Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Qiang(Joe) Xu, 2017-03-17 04:37:12]

The CQ bit was checked by warx@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-17 04:37:28]

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

[Qiang(Joe) Xu, 2017-03-17 04:39:36]

Description was changed from

==========
cros: Do not schedule attempt when behind captive portal is detected

Changes:
Do not schedule attempt if we are already behind captive portal, which can be
either the result of NetworkPortalDetector or shill's detection. In test, this
can reduce the call times of CaptivePortalDetector::DetectCaptivePortal from 6+
to 2 within 10s.

BUG=702273
TEST=If we log in CaptivePortalDetector::DetectCaptivePortal, we can see the
attempts reduced to 2 for connecting to captive portal wifi.
==========

to

==========
cros: Do not schedule attempt when behind captive portal with response 200 is
detected

Changes:
(1) Do not schedule attempt if we are already behind captive portal with
response 200, which can be either the result of NetworkPortalDetector or shill's
detection. In test, this can reduce the call times of
CaptivePortalDetector::DetectCaptivePortal from 6+ to 2 within 10s.
(2) modify tests to adapt this change

BUG=702273
TEST=If we log in CaptivePortalDetector::DetectCaptivePortal, we can see the
attempts reduced to 2 for connecting to captive portal wifi.
==========

[Qiang(Joe) Xu, 2017-03-17 04:39:53]

Patchset #3 (id:40001) has been deleted

[commit-bot: I haz the power, 2017-03-17 04:40:24]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Qiang(Joe) Xu, 2017-03-17 04:42:12]

The CQ bit was checked by warx@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-17 04:42:29]

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

[Qiang(Joe) Xu, 2017-03-17 04:54:56]

Description was changed from

==========
cros: Do not schedule attempt when behind captive portal with response 200 is
detected

Changes:
(1) Do not schedule attempt if we are already behind captive portal with
response 200, which can be either the result of NetworkPortalDetector or shill's
detection. In test, this can reduce the call times of
CaptivePortalDetector::DetectCaptivePortal from 6+ to 2 within 10s.
(2) modify tests to adapt this change

BUG=702273
TEST=If we log in CaptivePortalDetector::DetectCaptivePortal, we can see the
attempts reduced to 2 for connecting to captive portal wifi.
==========

to

==========
cros: Do not schedule attempt when behind captive portal with response 200 is
detected

Changes:
(1) Do not schedule attempt if we are already behind captive portal with
response 200, which can be either the result of NetworkPortalDetector or shill's
detection. In test, this can reduce the call times of
CaptivePortalDetector::DetectCaptivePortal from 6+ to 2~3 within 10s.
(2) modify tests to adapt this change

BUG=702273
TEST=If we log in CaptivePortalDetector::DetectCaptivePortal, we can see the
attempts reduced to 2~3 for connecting to captive portal wifi.
==========

[Qiang(Joe) Xu, 2017-03-17 04:58:04]

Hi Steven, please take another look, thanks!

The flaky tests hide the NetworkPortalDetectorImplTest failures. I fixed them in
this patch.

Changes:
(1) check now becomes beind captive portal and response code 200, so that any
-1, 204, 3XX, 5XX response code will still schedule attempts. It is inspired by
the test coverage.

(2) modify tests that when behind captive portal with response 200, detector
state should be idle.

[commit-bot: I haz the power, 2017-03-17 05:12:04]

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

[commit-bot: I haz the power, 2017-03-17 05:12:05]

Dry run: This issue passed the CQ dry run.

[stevenjb, 2017-03-17 16:53:01]

https://codereview.chromium.org/2756643002/diff/60001/chrome/browser/chromeos...
File chrome/browser/chromeos/net/network_portal_detector_impl.cc (right):

https://codereview.chromium.org/2756643002/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/net/network_portal_detector_impl.cc:560: }
This logic got a bit complicated, let's break it up:

if (!is_idle())
  return;  // Attempt already scheduled

// If behind a captive portal and the response code was 200 (OK), do
// not schedule a new attempt.
if (state.status == CAPTIVE_PORTAL_STATUS_PORTAL &&
    response_code == 200) {
  return;
}

ScheduleAttempt(results.retry_after_delta);

[Qiang(Joe) Xu, 2017-03-17 17:23:55]

Updated, PTAL, thanks

https://codereview.chromium.org/2756643002/diff/60001/chrome/browser/chromeos...
File chrome/browser/chromeos/net/network_portal_detector_impl.cc (right):

https://codereview.chromium.org/2756643002/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/net/network_portal_detector_impl.cc:560: }
On 2017/03/17 16:53:01, stevenjb wrote:
> This logic got a bit complicated, let's break it up:
> 
> if (!is_idle())
>   return;  // Attempt already scheduled
> 
> // If behind a captive portal and the response code was 200 (OK), do
> // not schedule a new attempt.
> if (state.status == CAPTIVE_PORTAL_STATUS_PORTAL &&
>     response_code == 200) {
>   return;
> }
> 
> ScheduleAttempt(results.retry_after_delta);

agree. Done, thanks!

[stevenjb, 2017-03-17 17:52:43]

lgtm

[Qiang(Joe) Xu, 2017-03-17 17:55:20]

The CQ bit was checked by warx@chromium.org

[commit-bot: I haz the power, 2017-03-17 17:56:02]

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, 2017-03-17 18:42:27]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1489773320336220,
"parent_rev": "19de954fc750d30bbc2ff6229c6d1b3fb5746d05", "commit_rev":
"db14329b13d1109723acf5528a0253d6ee8c1651"}

[commit-bot: I haz the power, 2017-03-17 18:47:04]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1489773320336220,
"parent_rev": "19de954fc750d30bbc2ff6229c6d1b3fb5746d05", "commit_rev":
"db14329b13d1109723acf5528a0253d6ee8c1651"}

[commit-bot: I haz the power, 2017-03-17 18:47:09]

Description was changed from

==========
cros: Do not schedule attempt when behind captive portal with response 200 is
detected

Changes:
(1) Do not schedule attempt if we are already behind captive portal with
response 200, which can be either the result of NetworkPortalDetector or shill's
detection. In test, this can reduce the call times of
CaptivePortalDetector::DetectCaptivePortal from 6+ to 2~3 within 10s.
(2) modify tests to adapt this change

BUG=702273
TEST=If we log in CaptivePortalDetector::DetectCaptivePortal, we can see the
attempts reduced to 2~3 for connecting to captive portal wifi.
==========

to

==========
cros: Do not schedule attempt when behind captive portal with response 200 is
detected

Changes:
(1) Do not schedule attempt if we are already behind captive portal with
response 200, which can be either the result of NetworkPortalDetector or shill's
detection. In test, this can reduce the call times of
CaptivePortalDetector::DetectCaptivePortal from 6+ to 2~3 within 10s.
(2) modify tests to adapt this change

BUG=702273
TEST=If we log in CaptivePortalDetector::DetectCaptivePortal, we can see the
attempts reduced to 2~3 for connecting to captive portal wifi.

Review-Url: https://codereview.chromium.org/2756643002
Cr-Commit-Position: refs/heads/master@{#457829}
Committed:
https://chromium.googlesource.com/chromium/src/+/db14329b13d1109723acf5528a02...
==========

[commit-bot: I haz the power, 2017-03-17 18:47:10]

Committed patchset #5 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/db14329b13d1109723acf5528a02...