chromeos: Refactor system tray ShowNetworkSettingsForGuid for mash

chromeos: Refactor system tray ShowNetworkSettingsForGuid for mash

* Add to ash::mojom::SystemTrayClient interface
* Move implementation in chrome browser into SystemTrayClient
* Rename to ShowNetworkSettings with |network_id| because the parameter
|guid| isn't always a GUID.

This unfortunately has to check IsRunningInMash to access the session
state delegate. xiyuan@ is working on a system to replace that delegate,
but for now this is the simplest way.

BUG=647412
TEST=browser_tests, manually open network settings from system tray, both
via the Settings button and by clicking on the connected network

Committed: https://crrev.com/b9b36ee098ef29022e8e24c2f6faab21dcde5ec5
Cr-Commit-Position: refs/heads/master@{#423600}

[James Cook, 2016-10-05 22:10:25]

The CQ bit was checked by jamescook@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-05 22:10:52]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[James Cook, 2016-10-05 22:13:43]

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

[James Cook, 2016-10-05 22:13:44]

msw, please take a look.

[commit-bot: I haz the power, 2016-10-05 22:51:34]

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

[commit-bot: I haz the power, 2016-10-05 22:51:35]

Dry run: This issue passed the CQ dry run.

[msw, 2016-10-05 22:56:36]

lgtm with minor optional nits

https://codereview.chromium.org/2396863004/diff/1/ash/common/system/chromeos/...
File ash/common/system/chromeos/network/network_state_list_detailed_view.cc
(right):

https://codereview.chromium.org/2396863004/diff/1/ash/common/system/chromeos/...
ash/common/system/chromeos/network/network_state_list_detailed_view.cc:439:
network ? network->guid() : "");
nit: s/""/std::string()/

https://codereview.chromium.org/2396863004/diff/1/ash/common/system/chromeos/...
ash/common/system/chromeos/network/network_state_list_detailed_view.cc:516:
WmShell::Get()->system_tray_controller()->ShowNetworkSettings("");
nit: s/""/std::string()/

https://codereview.chromium.org/2396863004/diff/1/chrome/browser/ui/ash/syste...
File chrome/browser/ui/ash/system_tray_client.cc (right):

https://codereview.chromium.org/2396863004/diff/1/chrome/browser/ui/ash/syste...
chrome/browser/ui/ash/system_tray_client.cc:151: // TODO(mash): Need replacement
for SessionStateDelegate. crbug.com/648964
bummer, but yeah...

[James Cook, 2016-10-05 23:02:24]

The CQ bit was checked by jamescook@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-05 23:03:00]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[James Cook, 2016-10-05 23:03:51]

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

[James Cook, 2016-10-05 23:03:52]

tsepez, can I get OWNERS for //ash/public/interfaces/system_tray.mojom? (This is
the networking "guid" I chatted with you about -- this interface is restricted
by capabilities to the ash process.)

https://codereview.chromium.org/2396863004/diff/1/ash/common/system/chromeos/...
File ash/common/system/chromeos/network/network_state_list_detailed_view.cc
(right):

https://codereview.chromium.org/2396863004/diff/1/ash/common/system/chromeos/...
ash/common/system/chromeos/network/network_state_list_detailed_view.cc:439:
network ? network->guid() : "");
On 2016/10/05 22:56:36, msw wrote:
> nit: s/""/std::string()/

Done.

https://codereview.chromium.org/2396863004/diff/1/ash/common/system/chromeos/...
ash/common/system/chromeos/network/network_state_list_detailed_view.cc:516:
WmShell::Get()->system_tray_controller()->ShowNetworkSettings("");
On 2016/10/05 22:56:36, msw wrote:
> nit: s/""/std::string()/

Done.

[commit-bot: I haz the power, 2016-10-05 23:40:44]

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

[commit-bot: I haz the power, 2016-10-05 23:40:45]

Dry run: This issue passed the CQ dry run.

[Tom Sepez, 2016-10-06 15:59:42]

mojom LGTM given manifest constraints.

[James Cook, 2016-10-06 18:01:07]

The CQ bit was checked by jamescook@chromium.org

[James Cook, 2016-10-06 18:01:08]

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

[commit-bot: I haz the power, 2016-10-06 18:02:07]

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-06 18:37:01]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-10-06 18:40:08]

Description was changed from

==========
chromeos: Refactor system tray ShowNetworkSettingsForGuid for mash

* Add to ash::mojom::SystemTrayClient interface
* Move implementation in chrome browser into SystemTrayClient
* Rename to ShowNetworkSettings with |network_id| because the parameter
|guid| isn't always a GUID.

This unfortunately has to check IsRunningInMash to access the session
state delegate. xiyuan@ is working on a system to replace that delegate,
but for now this is the simplest way.

BUG=647412
TEST=browser_tests, manually open network settings from system tray, both
via the Settings button and by clicking on the connected network
==========

to

==========
chromeos: Refactor system tray ShowNetworkSettingsForGuid for mash

* Add to ash::mojom::SystemTrayClient interface
* Move implementation in chrome browser into SystemTrayClient
* Rename to ShowNetworkSettings with |network_id| because the parameter
|guid| isn't always a GUID.

This unfortunately has to check IsRunningInMash to access the session
state delegate. xiyuan@ is working on a system to replace that delegate,
but for now this is the simplest way.

BUG=647412
TEST=browser_tests, manually open network settings from system tray, both
via the Settings button and by clicking on the connected network

Committed: https://crrev.com/b9b36ee098ef29022e8e24c2f6faab21dcde5ec5
Cr-Commit-Position: refs/heads/master@{#423600}
==========

[commit-bot: I haz the power, 2016-10-06 18:40:09]

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

[stevenjb, 2016-10-13 22:03:09]

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

[stevenjb, 2016-10-13 22:03:10]

Good cleanup, thanks. I'm a little concerned about confusion between
"network_id" and "service_path" on Chrome OS, so I upped the priority (a little)
on https://bugs.chromium.org/p/chromium/issues/detail?id=457855.

e.g.:

https://cs.chromium.org/chromium/src/ash/common/system/chromeos/network/netwo...

The comment you added should suffice though.

LGTM

https://codereview.chromium.org/2396863004/diff/60001/ash/public/interfaces/s...
File ash/public/interfaces/system_tray.mojom (right):

https://codereview.chromium.org/2396863004/diff/60001/ash/public/interfaces/s...
ash/public/interfaces/system_tray.mojom:61: ShowNetworkSettings(string
network_id);
const string& ?

[James Cook, 2016-10-13 22:16:07]

stevenjb - thanks for taking a look.

https://codereview.chromium.org/2396863004/diff/60001/ash/public/interfaces/s...
File ash/public/interfaces/system_tray.mojom (right):

https://codereview.chromium.org/2396863004/diff/60001/ash/public/interfaces/s...
ash/public/interfaces/system_tray.mojom:61: ShowNetworkSettings(string
network_id);
On 2016/10/13 22:03:09, stevenjb wrote:
> const string& ?

mojom files use "string", which is translated to "const string&" in the
generated C++ code

[stevenjb, 2016-10-13 23:04:38]

https://codereview.chromium.org/2396863004/diff/60001/ash/public/interfaces/s...
File ash/public/interfaces/system_tray.mojom (right):

https://codereview.chromium.org/2396863004/diff/60001/ash/public/interfaces/s...
ash/public/interfaces/system_tray.mojom:61: ShowNetworkSettings(string
network_id);
On 2016/10/13 22:16:07, James Cook wrote:
> On 2016/10/13 22:03:09, stevenjb wrote:
> > const string& ?
> 
> mojom files use "string", which is translated to "const string&" in the
> generated C++ code

AH, doh, I didn't notice this was a .mojom file.