Add Source property to networkingPrivate.getState

Add Source property to networkingPrivate.getState

BUG=454031
Committed: https://crrev.com/971fb92c8e09f93b8e06c5647784d3e4a3c9484f
Cr-Commit-Position: refs/heads/master@{#317174}

[stevenjb, 2015-02-12 02:13:53]

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

[pneubeck (no reviews), 2015-02-17 18:00:49]

https://codereview.chromium.org/917053002/diff/1/chrome/browser/resources/chr...
File chrome/browser/resources/chromeos/network_ui/network_ui.js (right):

https://codereview.chromium.org/917053002/diff/1/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/network_ui/network_ui.js:152: if
(guid.substring(0, 1) == '{')
can't you just remove all '{' and '}' or replace them by a valid character?
seems weird to handle such a specific case.

https://codereview.chromium.org/917053002/diff/1/chromeos/network/network_con...
File chromeos/network/network_configuration_handler.cc (right):

https://codereview.chromium.org/917053002/diff/1/chromeos/network/network_con...
chromeos/network/network_configuration_handler.cc:434:
properties.GetStringWithoutPathExpansion(::onc::network_config::kGUID, &guid);
can we remove this code eventually, when the guid is always set on first
creation?
if that's the goal, pls add a todo/bug for removal.

https://codereview.chromium.org/917053002/diff/1/chromeos/network/network_uti...
File chromeos/network/network_util.cc (right):

https://codereview.chromium.org/917053002/diff/1/chromeos/network/network_uti...
chromeos/network/network_util.cc:152:
shill_dictionary.SetStringWithoutPathExpansion(shill::kProfileProperty,
why not adding this in NetworkState::GetStateProperties ?
I thought that function was exactly meant to add exactly what should show up in
GetState/GetNetworks (this call is actually the only one to that function).

https://codereview.chromium.org/917053002/diff/1/chromeos/network/network_uti...
chromeos/network/network_util.cc:160: ::onc::ONCSource onc_source;
pls initialize explicitly to ONC_SOURCE_NONE to prevent non-deterministic
behavior in case of a bug.

https://codereview.chromium.org/917053002/diff/1/chromeos/network/network_uti...
chromeos/network/network_util.cc:182: for (auto state : network_states) {
optional nit: I usually prefer keeping the element type, const NetworkState*,
for clarity.

[stevenjb, 2015-02-17 18:23:31]

PTAL

https://codereview.chromium.org/917053002/diff/1/chrome/browser/resources/chr...
File chrome/browser/resources/chromeos/network_ui/network_ui.js (right):

https://codereview.chromium.org/917053002/diff/1/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/network_ui/network_ui.js:152: if
(guid.substring(0, 1) == '{')
On 2015/02/17 18:00:49, pneubeck wrote:
> can't you just remove all '{' and '}' or replace them by a valid character?
> seems weird to handle such a specific case.

Sure, that works also.

https://codereview.chromium.org/917053002/diff/1/chromeos/network/network_con...
File chromeos/network/network_configuration_handler.cc (right):

https://codereview.chromium.org/917053002/diff/1/chromeos/network/network_con...
chromeos/network/network_configuration_handler.cc:434:
properties.GetStringWithoutPathExpansion(::onc::network_config::kGUID, &guid);
On 2015/02/17 18:00:49, pneubeck wrote:
> can we remove this code eventually, when the guid is always set on first
> creation?
> if that's the goal, pls add a todo/bug for removal.

I am unaware of any plans to have Shill set a GUID, so visible networks will
always have a NSH assigned GUID that is not stored in Shill, until / unless they
are configured.

https://codereview.chromium.org/917053002/diff/1/chromeos/network/network_uti...
File chromeos/network/network_util.cc (right):

https://codereview.chromium.org/917053002/diff/1/chromeos/network/network_uti...
chromeos/network/network_util.cc:152:
shill_dictionary.SetStringWithoutPathExpansion(shill::kProfileProperty,
On 2015/02/17 18:00:49, pneubeck wrote:
> why not adding this in NetworkState::GetStateProperties ?
> I thought that function was exactly meant to add exactly what should show up
in
> GetState/GetNetworks (this call is actually the only one to that function).

Done.

https://codereview.chromium.org/917053002/diff/1/chromeos/network/network_uti...
chromeos/network/network_util.cc:160: ::onc::ONCSource onc_source;
On 2015/02/17 18:00:49, pneubeck wrote:
> pls initialize explicitly to ONC_SOURCE_NONE to prevent non-deterministic
> behavior in case of a bug.

Done.

https://codereview.chromium.org/917053002/diff/1/chromeos/network/network_uti...
chromeos/network/network_util.cc:182: for (auto state : network_states) {
On 2015/02/17 18:00:49, pneubeck wrote:
> optional nit: I usually prefer keeping the element type, const NetworkState*,
> for clarity.

Done.

[pneubeck (no reviews), 2015-02-17 18:27:57]

lgtm

https://codereview.chromium.org/917053002/diff/1/chromeos/network/network_con...
File chromeos/network/network_configuration_handler.cc (right):

https://codereview.chromium.org/917053002/diff/1/chromeos/network/network_con...
chromeos/network/network_configuration_handler.cc:434:
properties.GetStringWithoutPathExpansion(::onc::network_config::kGUID, &guid);
On 2015/02/17 18:23:30, stevenjb wrote:
> On 2015/02/17 18:00:49, pneubeck wrote:
> > can we remove this code eventually, when the guid is always set on first
> > creation?
> > if that's the goal, pls add a todo/bug for removal.
> 
> I am unaware of any plans to have Shill set a GUID, so visible networks will
> always have a NSH assigned GUID that is not stored in Shill, until / unless
they
> are configured.

ah, right I forgot that this code is executed also on visible but unconfigured
networks. hm.

but this code will be irrelevant if every body is calling MNCH::GetProperties
instead of this one?
There
(https://code.google.com/p/chromium/codesearch#chromium/src/chromeos/network/m...)
we have exactly the same handling already.

[stevenjb, 2015-02-17 18:34:52]

https://codereview.chromium.org/917053002/diff/1/chromeos/network/network_con...
File chromeos/network/network_configuration_handler.cc (right):

https://codereview.chromium.org/917053002/diff/1/chromeos/network/network_con...
chromeos/network/network_configuration_handler.cc:434:
properties.GetStringWithoutPathExpansion(::onc::network_config::kGUID, &guid);
On 2015/02/17 18:27:57, pneubeck wrote:
> On 2015/02/17 18:23:30, stevenjb wrote:
> > On 2015/02/17 18:00:49, pneubeck wrote:
> > > can we remove this code eventually, when the guid is always set on first
> > > creation?
> > > if that's the goal, pls add a todo/bug for removal.
> > 
> > I am unaware of any plans to have Shill set a GUID, so visible networks will
> > always have a NSH assigned GUID that is not stored in Shill, until / unless
> they
> > are configured.
> 
> ah, right I forgot that this code is executed also on visible but unconfigured
> networks. hm.
> 
> but this code will be irrelevant if every body is calling MNCH::GetProperties
> instead of this one?
> There
>
(https://code.google.com/p/chromium/codesearch#chromium/src/chromeos/network/m...)
> we have exactly the same handling already.

Yes, in theory if everything uses MNCH then we can deprecate this function
entirely :)

Currently however there is still a fair bit of code that uses Shill properties,
so I don't think that will be the case any time soon.

[stevenjb, 2015-02-17 18:35:26]

The CQ bit was checked by stevenjb@chromium.org

[commit-bot: I haz the power, 2015-02-17 18:37:09]

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

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

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

[commit-bot: I haz the power, 2015-02-17 19:41:16]

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

[stevenjb, 2015-02-18 01:02:12]

New patchsets have been uploaded after l-g-t-m from pneubeck@chromium.org

[stevenjb, 2015-02-18 01:02:20]

The CQ bit was checked by stevenjb@chromium.org

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

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

[commit-bot: I haz the power, 2015-02-18 02:34:39]

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

[commit-bot: I haz the power, 2015-02-18 02:34:40]

Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[stevenjb, 2015-02-19 21:23:35]

The CQ bit was checked by stevenjb@chromium.org

[commit-bot: I haz the power, 2015-02-19 21:24:56]

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

[commit-bot: I haz the power, 2015-02-19 23:45:38]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-02-19 23:46:12]

Patchset 5 (id:??) landed as
https://crrev.com/971fb92c8e09f93b8e06c5647784d3e4a3c9484f
Cr-Commit-Position: refs/heads/master@{#317174}