Introduce NetworkState::is_captive_portal()

Introduce NetworkState::is_captive_portal()

For c/b/ui/webui/chromeos/login/network_state_informer.cc
TBR=nkostylov@chromium.org
BUG=450364
Committed: https://crrev.com/4262bb6a7dc62913cecba1e9b888fc000c96a507
Cr-Commit-Position: refs/heads/master@{#313311}

[stevenjb, 2015-01-24 00:14:48]

stevenjb@chromium.org changed reviewers:
+ pneubeck@chromium.org, pstew@chromium.org

[stevenjb, 2015-01-24 00:14:48]

Does this look remotely reasonable / correct?

[Paul Stewart, 2015-01-24 00:35:34]

Looks reasonably correct.  You could think about other bits of messaging you
could send the user about their general network health for other portal states,
but at least this version doesn't ask the user to perform a remediation that's
not actually possible / necessary.

[pneubeck (no reviews), 2015-01-24 10:06:57]

lgtm after addressing the comments.

https://codereview.chromium.org/873713004/diff/20001/chromeos/network/network...
File chromeos/network/network_state.cc (right):

https://codereview.chromium.org/873713004/diff/20001/chromeos/network/network...
chromeos/network/network_state.cc:35: if (state == shill::kStatePortal)
>>BUG<<
shouldn't this be negated?

if (state != kStatePortal)
  return false;

https://codereview.chromium.org/873713004/diff/20001/chromeos/network/network...
chromeos/network/network_state.cc:48: bool is_captive_portal =
please add a comment what the rational of this is, e.g.:

"Shill reports in which phase it reached conclusion that it believes the device
behind a captive portal. We want to rely only on incorrect content being
returned and ignore other reasons."

https://codereview.chromium.org/873713004/diff/20001/chromeos/network/network...
chromeos/network/network_state.cc:58: if (portal_detection_status !=
shill::kPortalDetectionStatusFailure ||
nit: condition can be replaced by !is_captive_portal.

https://codereview.chromium.org/873713004/diff/20001/chromeos/network/network...
File chromeos/network/network_state.h (right):

https://codereview.chromium.org/873713004/diff/20001/chromeos/network/network...
chromeos/network/network_state.h:168: bool connectable_;
is this a pure wireless state? i guessed it would be useable for VPN as well
(e.g. !connectable if not connect with any other network)

[pneubeck (no reviews), 2015-01-24 10:10:58]

can you add a case to the unit test for this?

[stevenjb, 2015-01-26 20:16:30]

Unit test added.
PTAL

https://codereview.chromium.org/873713004/diff/20001/chromeos/network/network...
File chromeos/network/network_state.cc (right):

https://codereview.chromium.org/873713004/diff/20001/chromeos/network/network...
chromeos/network/network_state.cc:35: if (state == shill::kStatePortal)
On 2015/01/24 10:06:56, pneubeck wrote:
> >>BUG<<
> shouldn't this be negated?
> 
> if (state != kStatePortal)
>   return false;

Yes, thanks. Too many balls in the air.

https://codereview.chromium.org/873713004/diff/20001/chromeos/network/network...
chromeos/network/network_state.cc:48: bool is_captive_portal =
On 2015/01/24 10:06:56, pneubeck wrote:
> please add a comment what the rational of this is, e.g.:
> 
> "Shill reports in which phase it reached conclusion that it believes the
device
> behind a captive portal. We want to rely only on incorrect content being
> returned and ignore other reasons."

Done.

https://codereview.chromium.org/873713004/diff/20001/chromeos/network/network...
chromeos/network/network_state.cc:58: if (portal_detection_status !=
shill::kPortalDetectionStatusFailure ||
On 2015/01/24 10:06:56, pneubeck wrote:
> nit: condition can be replaced by !is_captive_portal.

Ditto, thanks, done.

https://codereview.chromium.org/873713004/diff/20001/chromeos/network/network...
File chromeos/network/network_state.h (right):

https://codereview.chromium.org/873713004/diff/20001/chromeos/network/network...
chromeos/network/network_state.h:168: bool connectable_;
On 2015/01/24 10:06:56, pneubeck wrote:
> is this a pure wireless state? i guessed it would be useable for VPN as well
> (e.g. !connectable if not connect with any other network)

Hmm, interesting point. I moved this to match the logic above and in the .cc
file.

service-api.txt doesn't say anything about VPN, and I don't see any indication
that we set this for VPN in the Shill code, so I will move this to the
'Wireless' section for now to match the .cc file.

[pneubeck (no reviews), 2015-01-27 08:16:08]

lgtm

[stevenjb, 2015-01-27 16:53:07]

The CQ bit was checked by stevenjb@chromium.org

[commit-bot: I haz the power, 2015-01-27 16:53:40]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/873713004/40001

[commit-bot: I haz the power, 2015-01-27 16:59:13]

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

[commit-bot: I haz the power, 2015-01-27 16:59:14]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[stevenjb, 2015-01-27 17:39:07]

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

[stevenjb, 2015-01-27 17:39:13]

The CQ bit was checked by stevenjb@chromium.org

[commit-bot: I haz the power, 2015-01-27 17:39:42]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/873713004/40001

[commit-bot: I haz the power, 2015-01-27 17:40:09]

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

[commit-bot: I haz the power, 2015-01-27 17:40:10]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[stevenjb, 2015-01-27 18:22:27]

The CQ bit was checked by stevenjb@chromium.org

[commit-bot: I haz the power, 2015-01-27 18:23:00]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/873713004/40001

[commit-bot: I haz the power, 2015-01-27 18:25:19]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-01-27 18:27:25]

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