NetworkConnect::Delegate: service_path -> network_id

NetworkConnect::Delegate: service_path -> network_id

This changes the NetworkConnect::Delegate interface to
consistently use network_id instead of service_path which is
a Shill implementation detail that should not be exposed to
the UI.

BUG=5
=657127

Committed: https://crrev.com/5a79452b66c4b0376c1abea3dc404a3fa0b87f10
Cr-Commit-Position: refs/heads/master@{#426527}

[stevenjb, 2016-10-18 23:08:30]

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

[stevenjb, 2016-10-18 23:08:31]

James, I know you're not super familiar with this code, but nobody really is at
this point.

We shouldn't be using |service_path| in the UI any more in general. We *really*
shouldn't be mixing it with |network_id| in a delegate.

This change adds an extra reverse lookup in a few places, but I think it is very
much worth it. Eventually the NetworkConnect code should get cleaned up to only
use network_id (AKA guid).

[James Cook, 2016-10-18 23:26:25]

LGTM with a nit. Thanks for cleaning this up -- I found the distinction
confusing.

https://codereview.chromium.org/2430003002/diff/1/chrome/browser/ui/ash/netwo...
File chrome/browser/ui/ash/network_connect_delegate_chromeos.cc (right):

https://codereview.chromium.org/2430003002/diff/1/chrome/browser/ui/ash/netwo...
chrome/browser/ui/ash/network_connect_delegate_chromeos.cc:40: int container_id
=
You may need to sync/rebase -- most of this function is gone in ToT.

https://codereview.chromium.org/2430003002/diff/1/chrome/browser/ui/ash/netwo...
chrome/browser/ui/ash/network_connect_delegate_chromeos.cc:49: std::string
GetServicePathFromNetworkId(const std::string& network_id) {
Can you put this in a static function in SystemTrayClient then call it from both
places?

https://codereview.chromium.org/2430003002/diff/1/chrome/browser/ui/ash/netwo...
chrome/browser/ui/ash/network_connect_delegate_chromeos.cc:55: return "";
nit: std::string()

[James Cook, 2016-10-18 23:32:57]

Also, just as FYI I have https://codereview.chromium.org/2426473009/ ready to
commit, which touches some code near this change. It's dependent on another
patch, though, so go ahead and land when you're ready and I'll rebase.

[stevenjb, 2016-10-19 17:06:44]

PTAL, I moved the lookup further down the stack and renamed
ShowNetworkSettingsForGuid in the Delegate just ShowNetworkSettings since the
Delegate is entirely network_id now.

https://codereview.chromium.org/2430003002/diff/1/chrome/browser/ui/ash/netwo...
File chrome/browser/ui/ash/network_connect_delegate_chromeos.cc (right):

https://codereview.chromium.org/2430003002/diff/1/chrome/browser/ui/ash/netwo...
chrome/browser/ui/ash/network_connect_delegate_chromeos.cc:40: int container_id
=
On 2016/10/18 23:26:25, James Cook wrote:
> You may need to sync/rebase -- most of this function is gone in ToT.

Done.

https://codereview.chromium.org/2430003002/diff/1/chrome/browser/ui/ash/netwo...
chrome/browser/ui/ash/network_connect_delegate_chromeos.cc:49: std::string
GetServicePathFromNetworkId(const std::string& network_id) {
On 2016/10/18 23:26:25, James Cook wrote:
> Can you put this in a static function in SystemTrayClient then call it from
both
> places?

Moved to the dialog code.

https://codereview.chromium.org/2430003002/diff/1/chrome/browser/ui/ash/netwo...
chrome/browser/ui/ash/network_connect_delegate_chromeos.cc:55: return "";
On 2016/10/18 23:26:25, James Cook wrote:
> nit: std::string()

Acknowledged.

[James Cook, 2016-10-19 18:10:15]

LGTM. Thanks for getting rid of ForGuid().

https://codereview.chromium.org/2430003002/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/options/network_config_view.h (right):

https://codereview.chromium.org/2430003002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/options/network_config_view.h:50: // Same as above but
tahes a network id (GUID) instead.
nit: tahes -> takes

[stevenjb, 2016-10-19 20:25:56]

The CQ bit was checked by stevenjb@chromium.org

[stevenjb, 2016-10-19 20:25:57]

The patchset sent to the CQ was uploaded after l-g-t-m from
jamescook@chromium.org
Link to the patchset: https://codereview.chromium.org/2430003002/#ps40001
(title: ".")

[commit-bot: I haz the power, 2016-10-19 20:26:35]

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, 2016-10-19 21:02:43]

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

[commit-bot: I haz the power, 2016-10-19 21:02:45]

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

[stevenjb, 2016-10-19 23:24:43]

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

[stevenjb, 2016-10-19 23:24:44]

+tsepez@ for OWNERS for 	ash/public/interfaces/system_tray.mojom

[Tom Sepez, 2016-10-20 16:33:09]

OWNER LGTM stamp for mojom.

[stevenjb, 2016-10-20 17:06:36]

The CQ bit was checked by stevenjb@chromium.org

[stevenjb, 2016-10-20 17:06:36]

The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org,
jamescook@chromium.org
Link to the patchset: https://codereview.chromium.org/2430003002/#ps60001
(title: "Rebase")

[commit-bot: I haz the power, 2016-10-20 17:06:57]

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, 2016-10-20 18:03:31]

Committed patchset #4 (id:60001)

[stevenjb, 2016-10-20 18:30:18]

Description was changed from

==========
NetworkConnect::Delegate: service_path -> network_id

This changes the NetworkConnect::Delegate interface to
consistently use network_id instead of service_path which is
a Shill implementation detail that should not be exposed to
the UI.

BUG=5=657127
==========

to

==========
NetworkConnect::Delegate: service_path -> network_id

This changes the NetworkConnect::Delegate interface to
consistently use network_id instead of service_path which is
a Shill implementation detail that should not be exposed to
the UI.

BUG=657127
==========

[commit-bot: I haz the power, 2016-10-21 13:20:04]

Description was changed from

==========
NetworkConnect::Delegate: service_path -> network_id

This changes the NetworkConnect::Delegate interface to
consistently use network_id instead of service_path which is
a Shill implementation detail that should not be exposed to
the UI.

BUG=657127
==========

to

==========
NetworkConnect::Delegate: service_path -> network_id

This changes the NetworkConnect::Delegate interface to
consistently use network_id instead of service_path which is
a Shill implementation detail that should not be exposed to
the UI.

BUG=5=657127

Committed: https://crrev.com/5a79452b66c4b0376c1abea3dc404a3fa0b87f10
Cr-Commit-Position: refs/heads/master@{#426527}
==========

[commit-bot: I haz the power, 2016-10-21 13:20:05]

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