Treat tethered networks as mobile networks

Treat tethered networks as mobile networks

This is additional CL to support update over cellular.
- add a property in NetworkState indicating tethering state.
- show a warning message in about Chrome OS page on tethered networks.

BUG=691108
Review-Url: https://codereview.chromium.org/2919683002
Cr-Commit-Position: refs/heads/master@{#476464}
Committed: https://chromium.googlesource.com/chromium/src/+/4432cf04a40645d3c8f2d972e0181ec7a1c6a093

[weidongg, 2017-06-01 01:06:48]

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

[commit-bot: I haz the power, 2017-06-01 01:07:45]

Dry run: CQ is trying da patch.

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

[weidongg, 2017-06-01 01:09:36]

weidongg@chromium.org changed reviewers:
+ stevenjb@chromium.org, xiyuan@chromium.org

[commit-bot: I haz the power, 2017-06-01 02:52:03]

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

[commit-bot: I haz the power, 2017-06-01 02:52:03]

Dry run: This issue passed the CQ dry run.

[stevenjb, 2017-06-01 16:03:36]

https://codereview.chromium.org/2919683002/diff/1/chromeos/network/network_st...
File chromeos/network/network_state.h (right):

https://codereview.chromium.org/2919683002/diff/1/chromeos/network/network_st...
chromeos/network/network_state.h:118: }
1. We shouldn't need a setter for this.
2. "tethering" is a bit different from type=Tether networks, so let's move
tethering_state() under the 'Cellular property accessors' section.
3. Since (for now anyway) we are always checking for "type==Cellular or
tethering==confirmed", instead of thethering_state() let's add a method here
"IsUsingMobileData()" which has that logic so that it is in one place.
4. We also want to add "type==Tether" to the IsUsingMobileData() method (another
good reason to put the logic in one place).

https://codereview.chromium.org/2919683002/diff/1/chromeos/network/network_st...
chromeos/network/network_state.h:242: std::string tethering_state_;
Blank line here (but also move this after line 232)

[weidongg, 2017-06-01 18:14:30]

https://codereview.chromium.org/2919683002/diff/1/chromeos/network/network_st...
File chromeos/network/network_state.h (right):

https://codereview.chromium.org/2919683002/diff/1/chromeos/network/network_st...
chromeos/network/network_state.h:118: }
On 2017/06/01 16:03:36, stevenjb wrote:
> 1. We shouldn't need a setter for this.
> 2. "tethering" is a bit different from type=Tether networks, so let's move
> tethering_state() under the 'Cellular property accessors' section.
> 3. Since (for now anyway) we are always checking for "type==Cellular or
> tethering==confirmed", instead of thethering_state() let's add a method here
> "IsUsingMobileData()" which has that logic so that it is in one place.
> 4. We also want to add "type==Tether" to the IsUsingMobileData() method
(another
> good reason to put the logic in one place).

Aha, that's much cleaner. Thanks. BTW, does it mean that when 'Magic Tether' is
implemented, we could get 'type==Tether' instead of 'tethering==confirmed'?

https://codereview.chromium.org/2919683002/diff/1/chromeos/network/network_st...
chromeos/network/network_state.h:242: std::string tethering_state_;
On 2017/06/01 16:03:36, stevenjb wrote:
> Blank line here (but also move this after line 232)

Done.

[stevenjb, 2017-06-01 20:03:00]

Thanks for the cleanup, lgtm w/ one nit/suggestion.

https://codereview.chromium.org/2919683002/diff/1/chromeos/network/network_st...
File chromeos/network/network_state.h (right):

https://codereview.chromium.org/2919683002/diff/1/chromeos/network/network_st...
chromeos/network/network_state.h:118: }
On 2017/06/01 18:14:29, weidongg wrote:
> On 2017/06/01 16:03:36, stevenjb wrote:
> > 1. We shouldn't need a setter for this.
> > 2. "tethering" is a bit different from type=Tether networks, so let's move
> > tethering_state() under the 'Cellular property accessors' section.
> > 3. Since (for now anyway) we are always checking for "type==Cellular or
> > tethering==confirmed", instead of thethering_state() let's add a method here
> > "IsUsingMobileData()" which has that logic so that it is in one place.
> > 4. We also want to add "type==Tether" to the IsUsingMobileData() method
> (another
> > good reason to put the logic in one place).
> 
> Aha, that's much cleaner. Thanks. BTW, does it mean that when 'Magic Tether'
is
> implemented, we could get 'type==Tether' instead of 'tethering==confirmed'?

They are different things. You can still connect to a phone hotspot without
using Magic Tether.

https://codereview.chromium.org/2919683002/diff/20001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/settings/about_handler.cc (right):

https://codereview.chromium.org/2919683002/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/about_handler.cc:106: const bool cellular =
nit: we should rename this mobile_data. (We can leave the IDS strings as they
are, but this one is easy to change and makes the code more clear).

[xiyuan, 2017-06-01 20:18:28]

lgtm

https://codereview.chromium.org/2919683002/diff/20001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/help/help_utils_chromeos.h (right):

https://codereview.chromium.org/2919683002/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/help/help_utils_chromeos.h:8: #include <string>
nit: this can be removed now.

https://codereview.chromium.org/2919683002/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/help/help_utils_chromeos.h:11: #include
"chromeos/network/network_state.h"
nit: forward decl chromeos::NetworkState and move include to cc

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

https://codereview.chromium.org/2919683002/diff/20001/chromeos/network/networ...
chromeos/network/network_state.h:104: bool IsUsingMobileData() const;
nit: This is not an accessor. Move it up ?

[weidongg, 2017-06-01 20:41:00]

Thanks for the review!

https://codereview.chromium.org/2919683002/diff/20001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/help/help_utils_chromeos.h (right):

https://codereview.chromium.org/2919683002/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/help/help_utils_chromeos.h:8: #include <string>
On 2017/06/01 20:18:27, xiyuan wrote:
> nit: this can be removed now.

Done.

https://codereview.chromium.org/2919683002/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/help/help_utils_chromeos.h:11: #include
"chromeos/network/network_state.h"
On 2017/06/01 20:18:27, xiyuan wrote:
> nit: forward decl chromeos::NetworkState and move include to cc

Done.

https://codereview.chromium.org/2919683002/diff/20001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/settings/about_handler.cc (right):

https://codereview.chromium.org/2919683002/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/about_handler.cc:106: const bool cellular =
On 2017/06/01 20:03:00, stevenjb wrote:
> nit: we should rename this mobile_data. (We can leave the IDS strings as they
> are, but this one is easy to change and makes the code more clear).

Done.

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

https://codereview.chromium.org/2919683002/diff/20001/chromeos/network/networ...
chromeos/network/network_state.h:104: bool IsUsingMobileData() const;
On 2017/06/01 20:18:27, xiyuan wrote:
> nit: This is not an accessor. Move it up ?

Done.

[weidongg, 2017-06-01 21:27:39]

The CQ bit was checked by weidongg@chromium.org

[weidongg, 2017-06-01 21:27:40]

The patchset sent to the CQ was uploaded after l-g-t-m from xiyuan@chromium.org,
stevenjb@chromium.org
Link to the patchset: https://codereview.chromium.org/2919683002/#ps40001
(title: "Clean up code")

[commit-bot: I haz the power, 2017-06-01 21:28:29]

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-01 22:44:56]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1496352459415230,
"parent_rev": "40f734ce03b4693acb36388b7892dc67f863b65b", "commit_rev":
"4432cf04a40645d3c8f2d972e0181ec7a1c6a093"}

[commit-bot: I haz the power, 2017-06-01 22:45:12]

Description was changed from

==========
Treat tethered networks as mobile networks

This is additional CL to support update over cellular.
- add a property in NetworkState indicating tethering state.
- show a warning message in about Chrome OS page on tethered networks.

BUG=691108
==========

to

==========
Treat tethered networks as mobile networks

This is additional CL to support update over cellular.
- add a property in NetworkState indicating tethering state.
- show a warning message in about Chrome OS page on tethered networks.

BUG=691108

Review-Url: https://codereview.chromium.org/2919683002
Cr-Commit-Position: refs/heads/master@{#476464}
Committed:
https://chromium.googlesource.com/chromium/src/+/4432cf04a40645d3c8f2d972e018...
==========

[commit-bot: I haz the power, 2017-06-01 22:45:14]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/4432cf04a40645d3c8f2d972e018...