Provide GUID in ONC for all networks in ManagedNetworkConfigurationHandler

Provide GUID in ONC for all networks in ManagedNetworkConfigurationHandler

BUG=448445
Committed: https://crrev.com/40d129f2149cc7fe0c23a0a916c2f5d1576a581b
Cr-Commit-Position: refs/heads/master@{#311747}

[stevenjb, 2015-01-13 19:13:02]

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

[pneubeck (no reviews), 2015-01-14 17:47:11]

lgtm with comments

https://codereview.chromium.org/821643005/diff/20001/chromeos/network/managed...
File chromeos/network/managed_network_configuration_handler_impl.cc (right):

https://codereview.chromium.org/821643005/diff/20001/chromeos/network/managed...
chromeos/network/managed_network_configuration_handler_impl.cc:750: guid =
state->guid();
nit: inlining this is clearer as the original value (empty guid from
shill_properties) is never used

https://codereview.chromium.org/821643005/diff/20001/chromeos/network/onc/onc...
File chromeos/network/onc/onc_merger.cc (right):

https://codereview.chromium.org/821643005/diff/20001/chromeos/network/onc/onc...
chromeos/network/onc/onc_merger.cc:385: if (!effective_value) {
please simplify/flatten the logic here:


if (effective_value) {
  // DCHECK that all provided GUIDs are identical.
  DCHECK(AllPresentValuesEqual(values, *effective_value));

  // Return the un-augmented GUID.
  return effective_value.Pass();
} else if (values.active_setting) {
  // Unmanaged networks have assigned (active) GUID values.
  return make_scoped_ptr(values.active_setting->DeepCopy());
} else {
  LOG(ERROR) << "GUID field has no effective or active value.";
  return nullptr;
}

[pneubeck (no reviews), 2015-01-14 17:48:05]

ah, and 
  ManagedNetworkConnectionHandler -> ManagedNetwork_Configuration_Handler

[stevenjb, 2015-01-14 18:07:44]

https://codereview.chromium.org/821643005/diff/20001/chromeos/network/managed...
File chromeos/network/managed_network_configuration_handler_impl.cc (right):

https://codereview.chromium.org/821643005/diff/20001/chromeos/network/managed...
chromeos/network/managed_network_configuration_handler_impl.cc:750: guid =
state->guid();
On 2015/01/14 17:47:10, pneubeck wrote:
> nit: inlining this is clearer as the original value (empty guid from
> shill_properties) is never used

I'm not sure I agree. Currently guid is unused, but it is easy to imagine code
down the road using guid since it is scoped outside of the if() (and would be
awkward to scope, I played with doing that).

https://codereview.chromium.org/821643005/diff/20001/chromeos/network/onc/onc...
File chromeos/network/onc/onc_merger.cc (right):

https://codereview.chromium.org/821643005/diff/20001/chromeos/network/onc/onc...
chromeos/network/onc/onc_merger.cc:385: if (!effective_value) {
On 2015/01/14 17:47:10, pneubeck wrote:
> please simplify/flatten the logic here:
> 
> 
> if (effective_value) {
>   // DCHECK that all provided GUIDs are identical.
>   DCHECK(AllPresentValuesEqual(values, *effective_value));
> 
>   // Return the un-augmented GUID.
>   return effective_value.Pass();
> } else if (values.active_setting) {
>   // Unmanaged networks have assigned (active) GUID values.
>   return make_scoped_ptr(values.active_setting->DeepCopy());
> } else {
>   LOG(ERROR) << "GUID field has no effective or active value.";
>   return nullptr;
> }

Done (without the else's)

[pneubeck (no reviews), 2015-01-15 09:45:12]

https://codereview.chromium.org/821643005/diff/20001/chromeos/network/managed...
File chromeos/network/managed_network_configuration_handler_impl.cc (right):

https://codereview.chromium.org/821643005/diff/20001/chromeos/network/managed...
chromeos/network/managed_network_configuration_handler_impl.cc:750: guid =
state->guid();
On 2015/01/14 18:07:44, stevenjb wrote:
> On 2015/01/14 17:47:10, pneubeck wrote:
> > nit: inlining this is clearer as the original value (empty guid from
> > shill_properties) is never used
> 
> I'm not sure I agree. Currently guid is unused, but it is easy to imagine code
> down the road using guid since it is scoped outside of the if() (and would be
> awkward to scope, I played with doing that).

sounds ok.

https://codereview.chromium.org/821643005/diff/20001/chromeos/network/onc/onc...
File chromeos/network/onc/onc_merger.cc (right):

https://codereview.chromium.org/821643005/diff/20001/chromeos/network/onc/onc...
chromeos/network/onc/onc_merger.cc:385: if (!effective_value) {
On 2015/01/14 18:07:44, stevenjb wrote:
> On 2015/01/14 17:47:10, pneubeck wrote:
> > please simplify/flatten the logic here:
> > 
> > 
> > if (effective_value) {
> >   // DCHECK that all provided GUIDs are identical.
> >   DCHECK(AllPresentValuesEqual(values, *effective_value));
> > 
> >   // Return the un-augmented GUID.
> >   return effective_value.Pass();
> > } else if (values.active_setting) {
> >   // Unmanaged networks have assigned (active) GUID values.
> >   return make_scoped_ptr(values.active_setting->DeepCopy());
> > } else {
> >   LOG(ERROR) << "GUID field has no effective or active value.";
> >   return nullptr;
> > }
> 
> Done (without the else's)

i think you didn't upload the latest patch set.

[stevenjb, 2015-01-15 17:00:48]

On 2015/01/15 09:45:12, pneubeck wrote:
>
https://codereview.chromium.org/821643005/diff/20001/chromeos/network/managed...
> File chromeos/network/managed_network_configuration_handler_impl.cc (right):
> 
>
https://codereview.chromium.org/821643005/diff/20001/chromeos/network/managed...
> chromeos/network/managed_network_configuration_handler_impl.cc:750: guid =
> state->guid();
> On 2015/01/14 18:07:44, stevenjb wrote:
> > On 2015/01/14 17:47:10, pneubeck wrote:
> > > nit: inlining this is clearer as the original value (empty guid from
> > > shill_properties) is never used
> > 
> > I'm not sure I agree. Currently guid is unused, but it is easy to imagine
code
> > down the road using guid since it is scoped outside of the if() (and would
be
> > awkward to scope, I played with doing that).
> 
> sounds ok.
> 
>
https://codereview.chromium.org/821643005/diff/20001/chromeos/network/onc/onc...
> File chromeos/network/onc/onc_merger.cc (right):
> 
>
https://codereview.chromium.org/821643005/diff/20001/chromeos/network/onc/onc...
> chromeos/network/onc/onc_merger.cc:385: if (!effective_value) {
> On 2015/01/14 18:07:44, stevenjb wrote:
> > On 2015/01/14 17:47:10, pneubeck wrote:
> > > please simplify/flatten the logic here:
> > > 
> > > 
> > > if (effective_value) {
> > >   // DCHECK that all provided GUIDs are identical.
> > >   DCHECK(AllPresentValuesEqual(values, *effective_value));
> > > 
> > >   // Return the un-augmented GUID.
> > >   return effective_value.Pass();
> > > } else if (values.active_setting) {
> > >   // Unmanaged networks have assigned (active) GUID values.
> > >   return make_scoped_ptr(values.active_setting->DeepCopy());
> > > } else {
> > >   LOG(ERROR) << "GUID field has no effective or active value.";
> > >   return nullptr;
> > > }
> > 
> > Done (without the else's)
> 
> i think you didn't upload the latest patch set.

I did not, I was waiting to test it and got distracted. Uploaded now.

[stevenjb, 2015-01-15 17:01:30]

The CQ bit was checked by stevenjb@chromium.org

[commit-bot: I haz the power, 2015-01-15 17:02:17]

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

[pneubeck (no reviews), 2015-01-15 17:02:20]

lgtm

[commit-bot: I haz the power, 2015-01-15 18:05:19]

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

[commit-bot: I haz the power, 2015-01-15 18:05:20]

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

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

[stevenjb, 2015-01-15 20:01:30]

The CQ bit was checked by stevenjb@chromium.org

[commit-bot: I haz the power, 2015-01-15 20:02:19]

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

[commit-bot: I haz the power, 2015-01-15 22:58:43]

Committed patchset #4 (id:60001)

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

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