Use GUID instead of servicePath in network settings

Use GUID instead of servicePath in network settings

* Uses guid instead of service path in the Internet Settings UI
* Uses chrome://settings/internet?guid= instead of ?servicePath=
* Uses chrome.networkingPrivate.getManagedProperties and setProperties directly in Internet Settings
* Makes callback parameter optional in networkingPrivate for setters
* Removes the difficult to maintain (and only partially implemented) Error translation from internet setigns, adding a general TODO for translating ONC properties in the UI.
* Replaces ShowNetworkSettings with ShowNetworkSettingsForGuid in NetworkConnect::Delegate and ash::SystemTrayDelegate
* Uses guid instead of service path in UIProxyConfigService

BUG=467275
For chrome/browser/ui/ash:
TBR=oshima@chromium.org

Committed: https://crrev.com/82dbc53092955f8609b402be88183693d4e19718
Cr-Commit-Position: refs/heads/master@{#320662}

[stevenjb, 2015-03-05 19:05:31]

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

[stevenjb, 2015-03-05 19:05:32]

A few more nails in the 'service path' coffin.

[stevenjb, 2015-03-05 19:17:43]

Updated description

[pneubeck (no reviews), 2015-03-12 20:53:58]

https://codereview.chromium.org/983823002/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/ui_proxy_config_service.cc (right):

https://codereview.chromium.org/983823002/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/ui_proxy_config_service.cc:82: const NetworkState*
network = NULL;
indentation

https://codereview.chromium.org/983823002/diff/40001/chrome/browser/resources...
File chrome/browser/resources/options/chromeos/internet_detail.js (right):

https://codereview.chromium.org/983823002/diff/40001/chrome/browser/resources...
chrome/browser/resources/options/chromeos/internet_detail.js:216: if (!guid ||
!guid.length)
on the C++ side, this is actually also called without guid (previously without
service_path).
Does that really work?

https://codereview.chromium.org/983823002/diff/40001/chrome/browser/resources...
chrome/browser/resources/options/chromeos/internet_detail.js:220:
DetailsInternetPage.initializeDetailsPage(oncProperties);
instead of the anonymous function you could use
  DetailsInternetPage.initializeDetailsPage.bind(DetailsInternetPage)

(or without .bind() if it doesn't need a specific |this|)

https://codereview.chromium.org/983823002/diff/40001/chrome/browser/resources...
File chrome/browser/resources/options/chromeos/network_list.js (right):

https://codereview.chromium.org/983823002/diff/40001/chrome/browser/resources...
chrome/browser/resources/options/chromeos/network_list.js:167: guid,
function(oncProperties) {
nit: as before, anonymous function not required

https://codereview.chromium.org/983823002/diff/40001/chrome/browser/resources...
chrome/browser/resources/options/chromeos/network_list.js:1035: var
ethernetOptions = function() {
nit: ... = showDetails.bind(null, ethernetConnection.GUID);

https://codereview.chromium.org/983823002/diff/40001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc
(right):

https://codereview.chromium.org/983823002/diff/40001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc:137:
std::string ServicePathFromGuid(const std::string& guid) {
to consider:
should this handle an empty |guid|?

https://codereview.chromium.org/983823002/diff/40001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc:422:
return;
is that such a good idea to send all network properties on every state or
properties change of any network to JS?

https://codereview.chromium.org/983823002/diff/40001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc:426: void
InternetOptionsHandler::NetworkPropertiesUpdated(
this is triggered for every signalStrength change of any network

[pneubeck (no reviews), 2015-03-12 20:54:27]

The one concern I have is about the details_path_ that you removed from the
internet_options_handler.cc

[stevenjb, 2015-03-13 01:20:14]

https://codereview.chromium.org/983823002/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/ui_proxy_config_service.cc (right):

https://codereview.chromium.org/983823002/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/ui_proxy_config_service.cc:82: const NetworkState*
network = NULL;
On 2015/03/12 20:53:57, pneubeck wrote:
> indentation

Done.

https://codereview.chromium.org/983823002/diff/40001/chrome/browser/resources...
File chrome/browser/resources/options/chromeos/internet_detail.js (right):

https://codereview.chromium.org/983823002/diff/40001/chrome/browser/resources...
chrome/browser/resources/options/chromeos/internet_detail.js:216: if (!guid ||
!guid.length)
On 2015/03/12 20:53:57, pneubeck wrote:
> on the C++ side, this is actually also called without guid (previously without
> service_path).
> Does that really work?
Yes. initializePage always gets called, and if the parameter is provided it
shows the details page (from the callback), otherwise it does not.

https://codereview.chromium.org/983823002/diff/40001/chrome/browser/resources...
chrome/browser/resources/options/chromeos/internet_detail.js:220:
DetailsInternetPage.initializeDetailsPage(oncProperties);
On 2015/03/12 20:53:57, pneubeck wrote:
> instead of the anonymous function you could use
>   DetailsInternetPage.initializeDetailsPage.bind(DetailsInternetPage)
> 
> (or without .bind() if it doesn't need a specific |this|)

Done.

https://codereview.chromium.org/983823002/diff/40001/chrome/browser/resources...
File chrome/browser/resources/options/chromeos/network_list.js (right):

https://codereview.chromium.org/983823002/diff/40001/chrome/browser/resources...
chrome/browser/resources/options/chromeos/network_list.js:167: guid,
function(oncProperties) {
On 2015/03/12 20:53:58, pneubeck wrote:
> nit: as before, anonymous function not required

Done.

https://codereview.chromium.org/983823002/diff/40001/chrome/browser/resources...
chrome/browser/resources/options/chromeos/network_list.js:1035: var
ethernetOptions = function() {
On 2015/03/12 20:53:58, pneubeck wrote:
> nit: ... = showDetails.bind(null, ethernetConnection.GUID);

Done.

https://codereview.chromium.org/983823002/diff/40001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc
(right):

https://codereview.chromium.org/983823002/diff/40001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc:137:
std::string ServicePathFromGuid(const std::string& guid) {
On 2015/03/12 20:53:58, pneubeck wrote:
> to consider:
> should this handle an empty |guid|?

Do you mean log an error? It will already return an empty service path if no
networks match. I think it is fine as-is, it will be handled the same as for an
invalid guid.

https://codereview.chromium.org/983823002/diff/40001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc:422:
return;
On 2015/03/12 20:53:58, pneubeck wrote:
> is that such a good idea to send all network properties on every state or
> properties change of any network to JS?

The JS already ignores updates to other networks. This is how the
networkingPrivate events will work when we switch to using those in the JS
instead of forwarding them here. If that overhead is a problem then we have to
rethink our approach, but it shouldn't be. These events occur with a frequency
that is O(seconds) not milliseconds.

https://codereview.chromium.org/983823002/diff/40001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc:426: void
InternetOptionsHandler::NetworkPropertiesUpdated(
On 2015/03/12 20:53:58, pneubeck wrote:
> this is triggered for every signalStrength change of any network

Still infrequent as these things go. Previously we were sending entire icon
updates with every signal strength change.

[pneubeck (no reviews), 2015-03-13 09:54:20]

lgtm

https://codereview.chromium.org/983823002/diff/40001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc
(right):

https://codereview.chromium.org/983823002/diff/40001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc:422:
return;
On 2015/03/13 01:20:14, stevenjb wrote:
> On 2015/03/12 20:53:58, pneubeck wrote:
> > is that such a good idea to send all network properties on every state or
> > properties change of any network to JS?
> 
> The JS already ignores updates to other networks. This is how the
> networkingPrivate events will work when we switch to using those in the JS
> instead of forwarding them here. If that overhead is a problem then we have to
> rethink our approach, but it shouldn't be.
That's not what the API currently exposes.
All events send lists of GUIDs (which we considered extending to list of state
properties) but because of the overhead not all configuration properties.
JS will have to filter from the GUIDs and request state properties or all
properties, depending on what it needs.

> These events occur with a frequency
> that is O(seconds) not milliseconds.
I remember someone caring about hundreds of Wi-Fi networks in range...
If that isn't a problem, it's fine with me.

https://codereview.chromium.org/983823002/diff/40001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc:426: void
InternetOptionsHandler::NetworkPropertiesUpdated(
On 2015/03/13 01:20:14, stevenjb wrote:
> On 2015/03/12 20:53:58, pneubeck wrote:
> > this is triggered for every signalStrength change of any network
> 
> Still infrequent as these things go. Previously we were sending entire icon
> updates with every signal strength change.

fair enough.

https://codereview.chromium.org/983823002/diff/60001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/options/chromeos/internet_options_handler.h
(right):

https://codereview.chromium.org/983823002/diff/60001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/chromeos/internet_options_handler.h:118:
std::string details_guid_;
I guess you tested something here.
Not used.

[stevenjb, 2015-03-13 17:01:14]

OK, I added a setNetworkGuid signal between the JS and IneternetOptionsHandler
for now. LMKWYT

https://codereview.chromium.org/983823002/diff/40001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc
(right):

https://codereview.chromium.org/983823002/diff/40001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc:422:
return;
On 2015/03/13 09:54:20, pneubeck wrote:
> On 2015/03/13 01:20:14, stevenjb wrote:
> > On 2015/03/12 20:53:58, pneubeck wrote:
> > > is that such a good idea to send all network properties on every state or
> > > properties change of any network to JS?
> > 
> > The JS already ignores updates to other networks. This is how the
> > networkingPrivate events will work when we switch to using those in the JS
> > instead of forwarding them here. If that overhead is a problem then we have
to
> > rethink our approach, but it shouldn't be.
> That's not what the API currently exposes.
> All events send lists of GUIDs (which we considered extending to list of state
> properties) but because of the overhead not all configuration properties.
> JS will have to filter from the GUIDs and request state properties or all
> properties, depending on what it needs.
> 
> > These events occur with a frequency
> > that is O(seconds) not milliseconds.
> I remember someone caring about hundreds of Wi-Fi networks in range...
> If that isn't a problem, it's fine with me.

Sure, that's fair, this will send more data than just the guids. And we do have
to be aware of ~100 networks in range, but in practice Shill limits the number
of networks it will send, e.g. strength updates for.

By eliminating GetManagedPropertiesCallback we lost our convenient way to track
the guid, but I could track it explicitly to alleviate that concern.

https://codereview.chromium.org/983823002/diff/60001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/options/chromeos/internet_options_handler.h
(right):

https://codereview.chromium.org/983823002/diff/60001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/chromeos/internet_options_handler.h:118:
std::string details_guid_;
On 2015/03/13 09:54:20, pneubeck wrote:
> I guess you tested something here.
> Not used.

Oops, thanks. I thought I could do this easily using
CoreChromeOSOptionsHandler::SelectNetworkCallback, but I couldn't find any
convenient links between the classes.

[pneubeck (no reviews), 2015-03-13 17:33:03]

yeah, with that approach we'll be on the safe side.
lgtm modulo making it compile :-)

[stevenjb, 2015-03-13 19:15:03]

The CQ bit was checked by stevenjb@chromium.org

[stevenjb, 2015-03-13 19:15:04]

The patchset sent to the CQ was uploaded after l-g-t-m from
pneubeck@chromium.org
Link to the patchset: https://codereview.chromium.org/983823002/#ps100001
(title: "Compile fixes")

[commit-bot: I haz the power, 2015-03-13 19:15:17]

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

[commit-bot: I haz the power, 2015-03-13 19:36:49]

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

[commit-bot: I haz the power, 2015-03-13 19:36:51]

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

[stevenjb, 2015-03-14 03:08:02]

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

[stevenjb, 2015-03-14 17:14:46]

The CQ bit was checked by stevenjb@chromium.org

[stevenjb, 2015-03-14 17:14:47]

The patchset sent to the CQ was uploaded after l-g-t-m from
pneubeck@chromium.org
Link to the patchset: https://codereview.chromium.org/983823002/#ps120001
(title: "Use guid instead of service_path for proxy prefs")

[commit-bot: I haz the power, 2015-03-14 17:14:53]

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

[commit-bot: I haz the power, 2015-03-14 18:32:33]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-03-14 18:33:19]

Patchset 7 (id:??) landed as
https://crrev.com/82dbc53092955f8609b402be88183693d4e19718
Cr-Commit-Position: refs/heads/master@{#320662}