cros: no active portal check when successfully behind portal

cros: no active portal check when successfully behind portal

Changes:
- Mark the state so that if we are behind captive portal with response
200, do not actively schedule attempts by self.
- If captive portal session expires on portal side, this can happen when
protal provider disconnects us. Under this case, shill's online state is
not in-line with the actual portal state. Thus, scheduling chrome's
portal detection.

R=stevenjb@chromium.org, cernekee@chromium.org
BUG=702273
TEST=device test: the attempts reduced to 3 from 6+ for connecting to
captive portal wifi.
Also tested on portal session expiration, crbug.com/705813 does not
happen.

Review-Url: https://codereview.chromium.org/2923343004
Cr-Commit-Position: refs/heads/master@{#479732}
Committed: https://chromium.googlesource.com/chromium/src/+/72881ff9907aac8d2df53327ded8af9f6db3d48c

[Qiang(Joe) Xu, 2017-06-07 22:50:24]

Hi Steven PTAL thanks

[stevenjb, 2017-06-07 23:00:23]

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

[stevenjb, 2017-06-07 23:00:23]

+cernekee@ to help review the captive portal logic (which I am not very familiar
with).

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

https://codereview.chromium.org/2923343004/diff/1/chrome/browser/chromeos/net...
chrome/browser/chromeos/net/network_portal_detector_impl.cc:510: state_ =
STATE_BEHIND_PORTAL_IDLE;
Combine the logic so that this is:

if (result == captive_portal::RESULT_BEHIND_CAPTIVE_PORTAL &&
    response_code == 200 &&
    (!network || network->connection_state() != shill::kStateOnline) {
  state_ = STATE_BEHIND_PORTAL_IDLE;
} else {
  state_ = STATE_IDLE;
}

When we combine it like that I'm not 100% certain the logic is correct, or at
least, the comments are not entirely clear, so maybe try to clarify the logic a
little?

[Qiang(Joe) Xu, 2017-06-07 23:18:00]

Description was changed from

==========
cros: no active portal check when successfully behind portal

Changes:
- Mark the state so that if we are behind captive portal with response
200, do not actively schedule attempts by self.
- If captive portal session expires on portal side, this can happen when
protal provider disconnects us. Under this case, shill's online state is
not in-line with the actual portal state. Thus, scheduling chrome's
portal detection.

R=stevenjb@chromium.org
BUG=702273
TEST=device test: the attempts reduced to 3 from 6+ for connecting to
captive portal wifi.
Also tested on portal session expiration, crbug.com/705813 does not
happen.
==========

to

==========
cros: no active portal check when successfully behind portal

Changes:
- Mark the state so that if we are behind captive portal with response
200, do not actively schedule attempts by self.
- If captive portal session expires on portal side, this can happen when
protal provider disconnects us. Under this case, shill's online state is
not in-line with the actual portal state. Thus, scheduling chrome's
portal detection.

R=stevenjb@chromium.org, cernekee@chromium.org
BUG=702273
TEST=device test: the attempts reduced to 3 from 6+ for connecting to
captive portal wifi.
Also tested on portal session expiration, crbug.com/705813 does not
happen.
==========

[Qiang(Joe) Xu, 2017-06-07 23:19:13]

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

https://codereview.chromium.org/2923343004/diff/1/chrome/browser/chromeos/net...
chrome/browser/chromeos/net/network_portal_detector_impl.cc:510: state_ =
STATE_BEHIND_PORTAL_IDLE;
On 2017/06/07 23:00:23, stevenjb wrote:
> Combine the logic so that this is:
> 
> if (result == captive_portal::RESULT_BEHIND_CAPTIVE_PORTAL &&
>     response_code == 200 &&
>     (!network || network->connection_state() != shill::kStateOnline) {
>   state_ = STATE_BEHIND_PORTAL_IDLE;
> } else {
>   state_ = STATE_IDLE;
> }
> 
> When we combine it like that I'm not 100% certain the logic is correct, or at
> least, the comments are not entirely clear, so maybe try to clarify the logic
a
> little?

Done.

[stevenjb, 2017-06-07 23:46:14]

lgtm

[Qiang(Joe) Xu, 2017-06-09 18:16:00]

On 2017/06/07 23:46:14, stevenjb wrote:
> lgtm

ping: Kevin could you also take a look? Thanks!

[Kevin Cernekee, 2017-06-15 15:37:58]

lgtm

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

The CQ bit was checked by warx@chromium.org

[commit-bot: I haz the power, 2017-06-15 16:04:31]

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-06-15 16:37:37]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1497542655153810,
"parent_rev": "909b2948037f5d7bbc042449cdb9da61f25b15c4", "commit_rev":
"72881ff9907aac8d2df53327ded8af9f6db3d48c"}

[commit-bot: I haz the power, 2017-06-15 16:37:51]

Description was changed from

==========
cros: no active portal check when successfully behind portal

Changes:
- Mark the state so that if we are behind captive portal with response
200, do not actively schedule attempts by self.
- If captive portal session expires on portal side, this can happen when
protal provider disconnects us. Under this case, shill's online state is
not in-line with the actual portal state. Thus, scheduling chrome's
portal detection.

R=stevenjb@chromium.org, cernekee@chromium.org
BUG=702273
TEST=device test: the attempts reduced to 3 from 6+ for connecting to
captive portal wifi.
Also tested on portal session expiration, crbug.com/705813 does not
happen.
==========

to

==========
cros: no active portal check when successfully behind portal

Changes:
- Mark the state so that if we are behind captive portal with response
200, do not actively schedule attempts by self.
- If captive portal session expires on portal side, this can happen when
protal provider disconnects us. Under this case, shill's online state is
not in-line with the actual portal state. Thus, scheduling chrome's
portal detection.

R=stevenjb@chromium.org, cernekee@chromium.org
BUG=702273
TEST=device test: the attempts reduced to 3 from 6+ for connecting to
captive portal wifi.
Also tested on portal session expiration, crbug.com/705813 does not
happen.

Review-Url: https://codereview.chromium.org/2923343004
Cr-Commit-Position: refs/heads/master@{#479732}
Committed:
https://chromium.googlesource.com/chromium/src/+/72881ff9907aac8d2df53327ded8...
==========

[commit-bot: I haz the power, 2017-06-15 16:37:52]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/72881ff9907aac8d2df53327ded8...