Better support for Cellular roaming state in ONC

Better support for Cellular roaming state in ONC

This CL does the following:
* Eliminates "ProviderRequiresRoaming" from ONC
* Adds a "required" state to "RoamingState" in ONC when ProviderRequiresRoaming is true
* Adds "ONC State" to chrome://network for testing networkingPrivate.getState()
* Adds support for shill_stub=roaming=[home|roaming|required] to facilitate testing on Linux

BUG=461163
Committed: https://crrev.com/00f2e618c1f54ba2403aec209ef3a5431e71bc7b
Cr-Commit-Position: refs/heads/master@{#318113}

[stevenjb, 2015-02-24 01:45:43]

stevenjb@chromium.org changed reviewers:
+ armansito@chromium.org, benchan@chromium.org, pneubeck@chromium.org

[armansito, 2015-02-24 03:05:03]

https://codereview.chromium.org/948943003/diff/1/chromeos/dbus/fake_shill_man...
File chromeos/dbus/fake_shill_manager_client.cc (right):

https://codereview.chromium.org/948943003/diff/1/chromeos/dbus/fake_shill_man...
chromeos/dbus/fake_shill_manager_client.cc:112: const char* kRoamingRequired =
"required";
nit: The commit message calls this state "always", so update the commit message
to say "required".

P.S.: Now that I've looked at the rest of the CL, is this a fake-only parameter?

https://codereview.chromium.org/948943003/diff/1/chromeos/dbus/fake_shill_man...
chromeos/dbus/fake_shill_manager_client.cc:814: roaming_state = roaming_state_;
I would add a comment above saying that values of |roaming_state_| other than
kRoamingRequired are identical to the corresponding shill properties so it's OK
to directly assign.

https://codereview.chromium.org/948943003/diff/1/chromeos/network/network_sta...
File chromeos/network/network_state.cc (right):

https://codereview.chromium.org/948943003/diff/1/chromeos/network/network_sta...
chromeos/network/network_state.cc:247: // the Shill -> ONC translation can
provide 'ProviderRequiresRoaming'
nit: Is there an additional indentation here?

https://codereview.chromium.org/948943003/diff/1/components/onc/docs/onc_spec...
File components/onc/docs/onc_spec.html (right):

https://codereview.chromium.org/948943003/diff/1/components/onc/docs/onc_spec...
components/onc/docs/onc_spec.html:2298: <span class="value">Always</span>,
"Required" or "Always"?

[stevenjb, 2015-02-24 16:31:10]

Patchset #2 (id:20001) has been deleted

[stevenjb, 2015-02-24 16:31:18]

ptal

https://codereview.chromium.org/948943003/diff/1/chromeos/dbus/fake_shill_man...
File chromeos/dbus/fake_shill_manager_client.cc (right):

https://codereview.chromium.org/948943003/diff/1/chromeos/dbus/fake_shill_man...
chromeos/dbus/fake_shill_manager_client.cc:112: const char* kRoamingRequired =
"required";
On 2015/02/24 03:05:03, armansito wrote:
> nit: The commit message calls this state "always", so update the commit
message
> to say "required".
> 
> P.S.: Now that I've looked at the rest of the CL, is this a fake-only
parameter?

Yes, this is only for the fake implementation. Changed the ONC state to better
match the meaning.

https://codereview.chromium.org/948943003/diff/1/chromeos/dbus/fake_shill_man...
chromeos/dbus/fake_shill_manager_client.cc:814: roaming_state = roaming_state_;
On 2015/02/24 03:05:03, armansito wrote:
> I would add a comment above saying that values of |roaming_state_| other than
> kRoamingRequired are identical to the corresponding shill properties so it's
OK
> to directly assign. 

Done.

https://codereview.chromium.org/948943003/diff/1/chromeos/network/network_sta...
File chromeos/network/network_state.cc (right):

https://codereview.chromium.org/948943003/diff/1/chromeos/network/network_sta...
chromeos/network/network_state.cc:247: // the Shill -> ONC translation can
provide 'ProviderRequiresRoaming'
On 2015/02/24 03:05:03, armansito wrote:
> nit: Is there an additional indentation here?

Done.

https://codereview.chromium.org/948943003/diff/1/components/onc/docs/onc_spec...
File components/onc/docs/onc_spec.html (right):

https://codereview.chromium.org/948943003/diff/1/components/onc/docs/onc_spec...
components/onc/docs/onc_spec.html:2298: <span class="value">Always</span>,
On 2015/02/24 03:05:03, armansito wrote:
> "Required" or "Always"?

Switched to Required

[pneubeck (no reviews), 2015-02-25 09:57:04]

please update the commit message

lgtm

https://codereview.chromium.org/948943003/diff/40001/chromeos/dbus/fake_shill...
File chromeos/dbus/fake_shill_manager_client.cc (right):

https://codereview.chromium.org/948943003/diff/40001/chromeos/dbus/fake_shill...
chromeos/dbus/fake_shill_manager_client.cc:808: std::string roaming_state;
to distinguish this from the member, maybe call it one of
{shill,cellular,service}_roaming_state

https://codereview.chromium.org/948943003/diff/40001/chromeos/dbus/fake_shill...
chromeos/dbus/fake_shill_manager_client.cc:813: else  // Expected to be a valid
Shill state.
|roaming_state| is expected to ...

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

https://codereview.chromium.org/948943003/diff/40001/chromeos/network/network...
chromeos/network/network_state.cc:250:
NetworkHandler::Get()->network_state_handler()->GetDeviceState(
instead of adding this cyclic dependency on NSH, how about calling
GetDeviceState at the call-site of GetStateProperties / 
TranslateNetworkStateToONC? There are only two, one in internet_options_handler
and one in networking_private.

https://codereview.chromium.org/948943003/diff/40001/components/onc/docs/onc_...
File components/onc/docs/onc_spec.html (right):

https://codereview.chromium.org/948943003/diff/40001/components/onc/docs/onc_...
components/onc/docs/onc_spec.html:2298: <span class="value">Always</span>,
Always -> Required

please add explanations for each value.

[stevenjb, 2015-02-25 19:16:25]

PTAL

https://codereview.chromium.org/948943003/diff/40001/chromeos/dbus/fake_shill...
File chromeos/dbus/fake_shill_manager_client.cc (right):

https://codereview.chromium.org/948943003/diff/40001/chromeos/dbus/fake_shill...
chromeos/dbus/fake_shill_manager_client.cc:808: std::string roaming_state;
On 2015/02/25 09:57:04, pneubeck wrote:
> to distinguish this from the member, maybe call it one of
> {shill,cellular,service}_roaming_state 

Done.

https://codereview.chromium.org/948943003/diff/40001/chromeos/dbus/fake_shill...
chromeos/dbus/fake_shill_manager_client.cc:813: else  // Expected to be a valid
Shill state.
On 2015/02/25 09:57:04, pneubeck wrote:
> |roaming_state| is expected to ...

Done.

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

https://codereview.chromium.org/948943003/diff/40001/chromeos/network/network...
chromeos/network/network_state.cc:250:
NetworkHandler::Get()->network_state_handler()->GetDeviceState(
On 2015/02/25 09:57:04, pneubeck wrote:
> instead of adding this cyclic dependency on NSH, how about calling
> GetDeviceState at the call-site of GetStateProperties / 
> TranslateNetworkStateToONC? There are only two, one in
internet_options_handler
> and one in networking_private.

Moved to TranslateNetworkStateToONC. Was on the fence when I did this, thought
this might be better but don't feel strongly about that.

https://codereview.chromium.org/948943003/diff/40001/components/onc/docs/onc_...
File components/onc/docs/onc_spec.html (right):

https://codereview.chromium.org/948943003/diff/40001/components/onc/docs/onc_...
components/onc/docs/onc_spec.html:2298: <span class="value">Always</span>,
On 2015/02/25 09:57:04, pneubeck wrote:
> Always -> Required
> 
> please add explanations for each value.

Provided an explanation for 'Required'; the others are either self explanatory,
or beyond my knowledge to explain.

[armansito, 2015-02-25 19:44:46]

lgtm with nits

https://codereview.chromium.org/948943003/diff/60001/chromeos/network/network...
File chromeos/network/network_util.cc (right):

https://codereview.chromium.org/948943003/diff/60001/chromeos/network/network...
chromeos/network/network_util.cc:156: // Celluler[RoamingState] can be set
correctly for badging network icons.
nit: s/Celluler/Cellular/

https://codereview.chromium.org/948943003/diff/60001/chromeos/network/onc/onc...
File chromeos/network/onc/onc_translation_tables.cc (right):

https://codereview.chromium.org/948943003/diff/60001/chromeos/network/onc/onc...
chromeos/network/onc/onc_translation_tables.cc:323:
{::onc::cellular::kRoamingHome, shill::kRoamingStateHome},
nit: it looks like there is a space after the opening brace in all other
declarations, I think it's better to be consistent.

[stevenjb, 2015-02-25 20:09:50]

https://codereview.chromium.org/948943003/diff/60001/chromeos/network/network...
File chromeos/network/network_util.cc (right):

https://codereview.chromium.org/948943003/diff/60001/chromeos/network/network...
chromeos/network/network_util.cc:156: // Celluler[RoamingState] can be set
correctly for badging network icons.
On 2015/02/25 19:44:46, armansito wrote:
> nit: s/Celluler/Cellular/

Done.

https://codereview.chromium.org/948943003/diff/60001/chromeos/network/onc/onc...
File chromeos/network/onc/onc_translation_tables.cc (right):

https://codereview.chromium.org/948943003/diff/60001/chromeos/network/onc/onc...
chromeos/network/onc/onc_translation_tables.cc:323:
{::onc::cellular::kRoamingHome, shill::kRoamingStateHome},
On 2015/02/25 19:44:46, armansito wrote:
> nit: it looks like there is a space after the opening brace in all other
> declarations, I think it's better to be consistent.

Done (although clang complains about this; some day we should clang format this
directory as a separate CL).

[stevenjb, 2015-02-25 20:09:54]

The CQ bit was checked by stevenjb@chromium.org

[stevenjb, 2015-02-25 20:09:54]

The patchset sent to the CQ was uploaded after l-g-t-m from
pneubeck@chromium.org, armansito@chromium.org
Link to the patchset: https://codereview.chromium.org/948943003/#ps80001 (title:
"Nits")

[commit-bot: I haz the power, 2015-02-25 20:10:36]

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

[commit-bot: I haz the power, 2015-02-25 21:06:16]

Committed patchset #4 (id:80001)

[commit-bot: I haz the power, 2015-02-25 21:06:42]

Patchset 4 (id:??) landed as
https://crrev.com/00f2e618c1f54ba2403aec209ef3a5431e71bc7b
Cr-Commit-Position: refs/heads/master@{#318113}