Appropriate handling for Cellular First devices.

Appropriate handling for Cellular First devices.

"Cellular First" devices rely on cellular telphone data networks as their
primary means of connecting to the internet.
Supplying the --cellular-first command line flag has two consequences:
1. Cellular data roaming will be enabled by default.
2. UpdateEngine will be instructed to allow updates over cellular connections.

BUG=640721
TEST=browser test
CQ-DEPEND=2729433004

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

[xiyuan, 2017-02-14 17:06:09]

xiyuan@chromium.org changed reviewers:
+ xiyuan@chromium.org

[xiyuan, 2017-02-14 17:06:10]

The approach looks good.

https://codereview.chromium.org/2694923005/diff/1/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/wizard_controller.cc (right):

https://codereview.chromium.org/2694923005/diff/1/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/wizard_controller.cc:895: if
(base::CommandLine::ForCurrentProcess()->HasSwitch(
nit: Move this block into a help function (e.g. MaybeAllowUpdateOverCellular) in
anonymous namespace and call it here.

https://codereview.chromium.org/2694923005/diff/1/chrome/browser/ui/webui/hel...
File chrome/browser/ui/webui/help/help_utils_chromeos.cc (right):

https://codereview.chromium.org/2694923005/diff/1/chrome/browser/ui/webui/hel...
chrome/browser/ui/webui/help/help_utils_chromeos.cc:24: const bool
kDefaultUpdateOverCellularAllowed = false;
We can get rid of this now since the default can change based on whether we have
--celluar-first switch.

https://codereview.chromium.org/2694923005/diff/1/chrome/browser/ui/webui/hel...
chrome/browser/ui/webui/help/help_utils_chromeos.cc:31: bool
updateOverCellularAllowed = kDefaultUpdateOverCellularAllowed;
updateOverCellularAllowed -> default_update_over_cellular_allowed

[kumarniranjan, 2017-02-28 19:01:39]

https://codereview.chromium.org/2694923005/diff/1/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/wizard_controller.cc (right):

https://codereview.chromium.org/2694923005/diff/1/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/wizard_controller.cc:895: if
(base::CommandLine::ForCurrentProcess()->HasSwitch(
On 2017/02/14 17:06:10, xiyuan wrote:
> nit: Move this block into a help function (e.g. MaybeAllowUpdateOverCellular)
in
> anonymous namespace and call it here.

I decided to name it IsCellularFirstDevice because that is most descriptive of
what it is doing. Please let me know if you don't think it is a good name.

https://codereview.chromium.org/2694923005/diff/1/chrome/browser/ui/webui/hel...
File chrome/browser/ui/webui/help/help_utils_chromeos.cc (right):

https://codereview.chromium.org/2694923005/diff/1/chrome/browser/ui/webui/hel...
chrome/browser/ui/webui/help/help_utils_chromeos.cc:24: const bool
kDefaultUpdateOverCellularAllowed = false;
On 2017/02/14 17:06:10, xiyuan wrote:
> We can get rid of this now since the default can change based on whether we
have
> --celluar-first switch.

Done.

https://codereview.chromium.org/2694923005/diff/1/chrome/browser/ui/webui/hel...
chrome/browser/ui/webui/help/help_utils_chromeos.cc:31: bool
updateOverCellularAllowed = kDefaultUpdateOverCellularAllowed;
On 2017/02/14 17:06:10, xiyuan wrote:
> updateOverCellularAllowed -> default_update_over_cellular_allowed

Done.

[xiyuan, 2017-02-28 20:26:42]

https://codereview.chromium.org/2694923005/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/wizard_controller.cc (right):

https://codereview.chromium.org/2694923005/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/login/wizard_controller.cc:187: bool
IsCellularFirstDevice() {
Maybe we should move this into chromeos_swithces.h/cc to share with other places
that check the switch directly.

https://codereview.chromium.org/2694923005/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/wizard_controller_browsertest.cc (right):

https://codereview.chromium.org/2694923005/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/login/wizard_controller_browsertest.cc:1316:
IN_PROC_BROWSER_TEST_F(WizardControllerCellularFirstTest, CellularFirstFlow) {
I am not sure how this test differs from ControlFlowMain. The test case below
looks the same to me.

https://codereview.chromium.org/2694923005/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/policy/device_network_configuration_updater.cc
(right):

https://codereview.chromium.org/2694923005/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/device_network_configuration_updater.cc:65:
chromeos::switches::kCellularFirst)) {
What would be the expected behavior if policy says no data roaming but we have
the switch? If the switch should win, then we probably should move the logic
into OnDataRoamingSettingChanged() to ignore policy value. If policy should win,
we should keep the !IsEnterpriseManaged() test here.

https://codereview.chromium.org/2694923005/diff/40001/chromeos/dbus/update_en...
File chromeos/dbus/update_engine_client.cc (right):

https://codereview.chromium.org/2694923005/diff/40001/chromeos/dbus/update_en...
chromeos/dbus/update_engine_client.cc:417: std::string failure_message_stub =
nit: std::string failure_message_stub =
-> constexpr char kFailureMessageStub[] =

[kumarniranjan, 2017-02-28 21:49:34]

https://codereview.chromium.org/2694923005/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/wizard_controller.cc (right):

https://codereview.chromium.org/2694923005/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/login/wizard_controller.cc:187: bool
IsCellularFirstDevice() {
On 2017/02/28 20:26:41, xiyuan wrote:
> Maybe we should move this into chromeos_swithces.h/cc to share with other
places
> that check the switch directly.

Done.

https://codereview.chromium.org/2694923005/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/wizard_controller_browsertest.cc (right):

https://codereview.chromium.org/2694923005/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/login/wizard_controller_browsertest.cc:1316:
IN_PROC_BROWSER_TEST_F(WizardControllerCellularFirstTest, CellularFirstFlow) {
On 2017/02/28 20:26:41, xiyuan wrote:
> I am not sure how this test differs from ControlFlowMain. The test case below
> looks the same to me.

As we discussed, it is the same test, and it has been refactored to avoid code
duplication.

https://codereview.chromium.org/2694923005/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/policy/device_network_configuration_updater.cc
(right):

https://codereview.chromium.org/2694923005/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/device_network_configuration_updater.cc:65:
chromeos::switches::kCellularFirst)) {
On 2017/02/28 20:26:41, xiyuan wrote:
> What would be the expected behavior if policy says no data roaming but we have
> the switch? If the switch should win, then we probably should move the logic
> into OnDataRoamingSettingChanged() to ignore policy value. If policy should
win,
> we should keep the !IsEnterpriseManaged() test here.

Policy should win, so I kept the !IsEnterpriseManaged() test

https://codereview.chromium.org/2694923005/diff/40001/chromeos/dbus/update_en...
File chromeos/dbus/update_engine_client.cc (right):

https://codereview.chromium.org/2694923005/diff/40001/chromeos/dbus/update_en...
chromeos/dbus/update_engine_client.cc:417: std::string failure_message_stub =
On 2017/02/28 20:26:41, xiyuan wrote:
> nit: std::string failure_message_stub =
> -> constexpr char kFailureMessageStub[] =

Done.

[xiyuan, 2017-02-28 22:29:42]

lgtm

You still need owner review for //DEPS and //chromeos/* files.

https://codereview.chromium.org/2694923005/diff/60001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/help/help_utils_chromeos.cc (right):

https://codereview.chromium.org/2694923005/diff/60001/chrome/browser/ui/webui...
chrome/browser/ui/webui/help/help_utils_chromeos.cc:29:
chromeos::switches::kCellularFirst);
nit: use IsCellularFirstDevice() and get rid of #include "command_line.h".

https://codereview.chromium.org/2694923005/diff/60001/chromeos/dbus/update_en...
File chromeos/dbus/update_engine_client.cc (right):

https://codereview.chromium.org/2694923005/diff/60001/chromeos/dbus/update_en...
chromeos/dbus/update_engine_client.cc:51: "Failed to set UpdateEngine to allow
updates over cellular: ";
nit: you can define a const close to where they are used if it only used from
one place.

[kumarniranjan, 2017-02-28 22:52:21]

kumarniranjan@google.com changed reviewers:
+ achuith@chromium.org, brettw@chromium.org

[kumarniranjan, 2017-02-28 22:52:21]

https://codereview.chromium.org/2694923005/diff/60001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/help/help_utils_chromeos.cc (right):

https://codereview.chromium.org/2694923005/diff/60001/chrome/browser/ui/webui...
chrome/browser/ui/webui/help/help_utils_chromeos.cc:29:
chromeos::switches::kCellularFirst);
On 2017/02/28 22:29:42, xiyuan wrote:
> nit: use IsCellularFirstDevice() and get rid of #include "command_line.h".

Done.

https://codereview.chromium.org/2694923005/diff/60001/chromeos/dbus/update_en...
File chromeos/dbus/update_engine_client.cc (right):

https://codereview.chromium.org/2694923005/diff/60001/chromeos/dbus/update_en...
chromeos/dbus/update_engine_client.cc:51: "Failed to set UpdateEngine to allow
updates over cellular: ";
On 2017/02/28 22:29:42, xiyuan wrote:
> nit: you can define a const close to where they are used if it only used from
> one place.

Done.

[kumarniranjan, 2017-03-01 19:18:02]

Description was changed from

==========
Appropriate handling for Cellular First devices.

"Cellular First" devices rely on cellular telphone data networks as their
primary means of connecting to the internet.
Supplying the --cellular-first command line flag has two consequences:
1. Cellular data roaming will be enabled by default.
2. UpdateEngine will be instructed to allow updates over cellular connections.

BUG=640721
TEST=none
==========

to

==========
Appropriate handling for Cellular First devices.

"Cellular First" devices rely on cellular telphone data networks as their
primary means of connecting to the internet.
Supplying the --cellular-first command line flag has two consequences:
1. Cellular data roaming will be enabled by default.
2. UpdateEngine will be instructed to allow updates over cellular connections.

BUG=640721
TEST=none
==========

[kumarniranjan, 2017-03-01 19:18:02]

kumarniranjan@google.com changed reviewers:
+ stevenjb@chromium.org

[stevenjb, 2017-03-01 23:11:56]

https://codereview.chromium.org/2694923005/diff/80001/DEPS
File DEPS (right):

https://codereview.chromium.org/2694923005/diff/80001/DEPS#newcode407
DEPS:407: Var('chromium_git') + '/chromiumos/platform/system_api.git' + '@' +
'c08ae470458b06cf23c1907817490d2fc917ac29',
Please make the DEPS roll a separate CL.

https://codereview.chromium.org/2694923005/diff/80001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/wizard_controller.cc (right):

https://codereview.chromium.org/2694923005/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/login/wizard_controller.cc:907: VLOG(1) <<
"InitiateOOBEUpdate";
We should make this method more descriptive (e.g. StartOOBEUpdate) and update
the VLOG.

https://codereview.chromium.org/2694923005/diff/80001/chromeos/dbus/update_en...
File chromeos/dbus/update_engine_client.cc (right):

https://codereview.chromium.org/2694923005/diff/80001/chromeos/dbus/update_en...
chromeos/dbus/update_engine_client.cc:418: "Failed to set UpdateEngine to allow
updates over cellular: ";
nit: elim "Stub" from name, I'm not sure what it's intended to mean.

[kumarniranjan, 2017-03-02 00:06:09]

Description was changed from

==========
Appropriate handling for Cellular First devices.

"Cellular First" devices rely on cellular telphone data networks as their
primary means of connecting to the internet.
Supplying the --cellular-first command line flag has two consequences:
1. Cellular data roaming will be enabled by default.
2. UpdateEngine will be instructed to allow updates over cellular connections.

BUG=640721
TEST=none
==========

to

==========
Appropriate handling for Cellular First devices.

"Cellular First" devices rely on cellular telphone data networks as their
primary means of connecting to the internet.
Supplying the --cellular-first command line flag has two consequences:
1. Cellular data roaming will be enabled by default.
2. UpdateEngine will be instructed to allow updates over cellular connections.

BUG=640721
TEST=browser test
CQ-DEPEND=2729433004
==========

[kumarniranjan, 2017-03-02 00:29:33]

kumarniranjan@google.com changed reviewers:
- achuith@chromium.org

[kumarniranjan, 2017-03-02 00:29:34]

https://codereview.chromium.org/2694923005/diff/80001/DEPS
File DEPS (right):

https://codereview.chromium.org/2694923005/diff/80001/DEPS#newcode407
DEPS:407: Var('chromium_git') + '/chromiumos/platform/system_api.git' + '@' +
'c08ae470458b06cf23c1907817490d2fc917ac29',
On 2017/03/01 23:11:56, stevenjb wrote:
> Please make the DEPS roll a separate CL.

Done.

https://codereview.chromium.org/2694923005/diff/80001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/wizard_controller.cc (right):

https://codereview.chromium.org/2694923005/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/login/wizard_controller.cc:907: VLOG(1) <<
"InitiateOOBEUpdate";
On 2017/03/01 23:11:56, stevenjb wrote:
> We should make this method more descriptive (e.g. StartOOBEUpdate) and update
> the VLOG.

Done.

https://codereview.chromium.org/2694923005/diff/80001/chromeos/dbus/update_en...
File chromeos/dbus/update_engine_client.cc (right):

https://codereview.chromium.org/2694923005/diff/80001/chromeos/dbus/update_en...
chromeos/dbus/update_engine_client.cc:418: "Failed to set UpdateEngine to allow
updates over cellular: ";
On 2017/03/01 23:11:56, stevenjb wrote:
> nit: elim "Stub" from name, I'm not sure what it's intended to mean.

Done.

[stevenjb, 2017-03-02 00:30:49]

lgtm

[kumarniranjan, 2017-03-02 17:55:59]

Description was changed from

==========
Appropriate handling for Cellular First devices.

"Cellular First" devices rely on cellular telphone data networks as their
primary means of connecting to the internet.
Supplying the --cellular-first command line flag has two consequences:
1. Cellular data roaming will be enabled by default.
2. UpdateEngine will be instructed to allow updates over cellular connections.

BUG=640721
TEST=browser test
CQ-DEPEND=2729433004
==========

to

==========
Appropriate handling for Cellular First devices.

"Cellular First" devices rely on cellular telphone data networks as their
primary means of connecting to the internet.
Supplying the --cellular-first command line flag has two consequences:
1. Cellular data roaming will be enabled by default.
2. UpdateEngine will be instructed to allow updates over cellular connections.

BUG=640721
TEST=browser test
CQ-DEPEND=2729433004
==========

[kumarniranjan, 2017-03-02 17:55:59]

kumarniranjan@google.com changed reviewers:
- brettw@chromium.org

[kumarniranjan, 2017-03-02 17:58:40]

On 2017/03/02 00:30:49, stevenjb wrote:
> lgtm

Steven, I added you as a reviewer since you were an owner of //chromeos/* but
you also reviewed //DEPS
Do you think I need another reviewer, or is this ready for CQ?

[kumarniranjan, 2017-03-02 18:35:58]

The CQ bit was checked by kumarniranjan@google.com

[kumarniranjan, 2017-03-02 18:35:58]

The patchset sent to the CQ was uploaded after l-g-t-m from xiyuan@chromium.org
Link to the patchset: https://codereview.chromium.org/2694923005/#ps100001
(title: "changes regarding DEPS")

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

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-02 18:38:52]

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

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

Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[kumarniranjan, 2017-03-02 20:58:54]

The CQ bit was checked by kumarniranjan@google.com

[kumarniranjan, 2017-03-02 20:58:55]

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/2694923005/#ps120001
(title: "rebased")

[commit-bot: I haz the power, 2017-03-02 20:59: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-03-02 21:42:29]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1488488334979500,
"parent_rev": "3b151bc48c69b73a3cc2005570ef8d1cdf42df70", "commit_rev":
"b9afef64d11bfc58e7611620c528b2cb5f766748"}

[commit-bot: I haz the power, 2017-03-02 21:43:15]

Description was changed from

==========
Appropriate handling for Cellular First devices.

"Cellular First" devices rely on cellular telphone data networks as their
primary means of connecting to the internet.
Supplying the --cellular-first command line flag has two consequences:
1. Cellular data roaming will be enabled by default.
2. UpdateEngine will be instructed to allow updates over cellular connections.

BUG=640721
TEST=browser test
CQ-DEPEND=2729433004
==========

to

==========
Appropriate handling for Cellular First devices.

"Cellular First" devices rely on cellular telphone data networks as their
primary means of connecting to the internet.
Supplying the --cellular-first command line flag has two consequences:
1. Cellular data roaming will be enabled by default.
2. UpdateEngine will be instructed to allow updates over cellular connections.

BUG=640721
TEST=browser test
CQ-DEPEND=2729433004

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

[commit-bot: I haz the power, 2017-03-02 21:43:19]

Committed patchset #7 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/b9afef64d11bfc58e7611620c528...