chromeos: Refactor system tray Wi-Fi connect dialog so it works with mash

chromeos: Refactor system tray Wi-Fi connect dialog so it works with mash

* Add ShowNetworkConnect method to SystemTrayClient mojom interface
* Add NetworkConnectDelegateMus to route calls over that interface
* Show the config dialog

TODO: Dialog is in the wrong parent window in mash, in particular on the lock
screen. Fixing this will require plumbing through src/ui dialog code, which
will happen in a subsequent CL.

BUG=644355
TEST=existing networking tests

Committed: https://crrev.com/5374fbd4595be5190f3b9f93bb5aaafbdaef426f
Cr-Commit-Position: refs/heads/master@{#425515}

[James Cook, 2016-10-14 17:52:00]

Description was changed from

==========
chromeos: Refactor system tray Wi-Fi connect dialog so it works with mash

move GetDialogParentContainerId to SystemTrayClient


Add NetworkConnectDelegateMus, add ShowNetworkConnect to mojom, logging

BUG=
==========

to

==========
chromeos: Refactor system tray Wi-Fi connect dialog so it works with mash

* Add ShowNetworkConnect method to SystemTrayClient mojom interface
* Add NetworkConnectDelegateMus to route calls over that interface
* Show the config dialog

TODO: Dialog is in the wrong parent window in mash, in particular on the lock
screen. Fixing this will require plumbing through src/ui dialog code, which
will happen in a subsequent CL.

BUG=644355
TEST=existing networking tests
==========

[James Cook, 2016-10-14 17:53:49]

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

[commit-bot: I haz the power, 2016-10-14 17:54:16]

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-14 17:59:51]

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

[commit-bot: I haz the power, 2016-10-14 18:00:33]

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-14 18:01:22]

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

[James Cook, 2016-10-14 18:01:22]

stevenjb, please take a look.

[commit-bot: I haz the power, 2016-10-14 19:05:55]

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

[commit-bot: I haz the power, 2016-10-14 19:05:56]

Dry run: Exceeded global retry quota

[James Cook, 2016-10-14 19:30:53]

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

[commit-bot: I haz the power, 2016-10-14 19:31:23]

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

[stevenjb, 2016-10-14 20:08:26]

https://codereview.chromium.org/2411223002/diff/60001/ash/mus/network_connect...
File ash/mus/network_connect_delegate_mus.h (right):

https://codereview.chromium.org/2411223002/diff/60001/ash/mus/network_connect...
ash/mus/network_connect_delegate_mus.h:17: // a mojo NetworkConfig service.
http://crbug.com/644355
This is a little different from what is so far discussed in
http://crbug.com/644355.

This delegate specifically exists to show UI elements and should be a separate
interface from any network configuration (which actually does the
configuration).

This may be easier to discuss in person.

https://codereview.chromium.org/2411223002/diff/60001/ash/mus/network_connect...
ash/mus/network_connect_delegate_mus.h:25: void ShowNetworkSettingsForGuid(const
std::string& network_id) override;
Ugh. I'd really kind of like to clean these up first, but we can live with it I
guess. Having both 'service_path' and 'network_id' (guid) in the interface is
super confusing.

[commit-bot: I haz the power, 2016-10-14 20:40:29]

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

[commit-bot: I haz the power, 2016-10-14 20:40:30]

Dry run: This issue passed the CQ dry run.

[James Cook, 2016-10-14 20:50:22]

https://codereview.chromium.org/2411223002/diff/60001/ash/mus/network_connect...
File ash/mus/network_connect_delegate_mus.h (right):

https://codereview.chromium.org/2411223002/diff/60001/ash/mus/network_connect...
ash/mus/network_connect_delegate_mus.h:17: // a mojo NetworkConfig service.
http://crbug.com/644355
On 2016/10/14 20:08:26, stevenjb wrote:
> This is a little different from what is so far discussed in
> http://crbug.com/644355.
> 
> This delegate specifically exists to show UI elements and should be a separate
> interface from any network configuration (which actually does the
> configuration).
> 
> This may be easier to discuss in person.

Yeah, it might. The issue is that the underlying network connect code will
occasionally need to ask for a piece of UI to be created. The typical way to do
this is to have an interface like:

interface NetworkConfig {
  SetClient(NetworkConfigClient client);

  TryToConnectToNetwork();
}

interface NetworkConfigClient {
  ShowDialogAskingForCredentials();
}

Ideally this service lives in //chromeos/network (but it can't right now due to
code in //ui/chromeos/network).

Then when chrome starts up it connects to the NetworkConfig service and sets
itself as a client.

So that's what I mean by "a client interface on a mojo NetworkConfig service".

Either way, though, I need to implement this delegate if I want to be able to
show a password input dialog.

https://codereview.chromium.org/2411223002/diff/60001/ash/mus/network_connect...
ash/mus/network_connect_delegate_mus.h:25: void ShowNetworkSettingsForGuid(const
std::string& network_id) override;
On 2016/10/14 20:08:26, stevenjb wrote:
> Ugh. I'd really kind of like to clean these up first, but we can live with it
I
> guess. Having both 'service_path' and 'network_id' (guid) in the interface is
> super confusing.

Yeah. Frankly, I don't really know much about when you would have one over the
other. It's hard to tell when running on desktop if the stub ids are
representative of what's on the device (for example, the "ForGuid" method
receives strings that are not guids when running on desktop). So I just kept
what we had.

[stevenjb, 2016-10-14 22:33:39]

lgtm

https://codereview.chromium.org/2411223002/diff/60001/ash/mus/network_connect...
File ash/mus/network_connect_delegate_mus.h (right):

https://codereview.chromium.org/2411223002/diff/60001/ash/mus/network_connect...
ash/mus/network_connect_delegate_mus.h:17: // a mojo NetworkConfig service.
http://crbug.com/644355
On 2016/10/14 20:50:22, James Cook wrote:
> On 2016/10/14 20:08:26, stevenjb wrote:
> > This is a little different from what is so far discussed in
> > http://crbug.com/644355.
> > 
> > This delegate specifically exists to show UI elements and should be a
separate
> > interface from any network configuration (which actually does the
> > configuration).
> > 
> > This may be easier to discuss in person.
> 
> Yeah, it might. The issue is that the underlying network connect code will
> occasionally need to ask for a piece of UI to be created. The typical way to
do
> this is to have an interface like:
> 
> interface NetworkConfig {
>   SetClient(NetworkConfigClient client);
> 
>   TryToConnectToNetwork();
> }
> 
> interface NetworkConfigClient {
>   ShowDialogAskingForCredentials();
> }
> 
> Ideally this service lives in //chromeos/network (but it can't right now due
to
> code in //ui/chromeos/network).
> 
> Then when chrome starts up it connects to the NetworkConfig service and sets
> itself as a client.
> 
> So that's what I mean by "a client interface on a mojo NetworkConfig service".
> 
> Either way, though, I need to implement this delegate if I want to be able to
> show a password input dialog.

Maybe just for now word this something like like:
// TODO(mash): Replace this with a mojo service to handle network config UI
requests. See crbug.com/644355.

https://codereview.chromium.org/2411223002/diff/60001/ash/mus/network_connect...
ash/mus/network_connect_delegate_mus.h:25: void ShowNetworkSettingsForGuid(const
std::string& network_id) override;
On 2016/10/14 20:50:22, James Cook wrote:
> On 2016/10/14 20:08:26, stevenjb wrote:
> > Ugh. I'd really kind of like to clean these up first, but we can live with
it
> I
> > guess. Having both 'service_path' and 'network_id' (guid) in the interface
is
> > super confusing.
> 
> Yeah. Frankly, I don't really know much about when you would have one over the
> other. It's hard to tell when running on desktop if the stub ids are
> representative of what's on the device (for example, the "ForGuid" method
> receives strings that are not guids when running on desktop). So I just kept
> what we had.

service_path is the Shill name for the network. The UI code shouldn't care about
that detail. 'network_id' is the unique identifier for the network and is what
the UI should use.

I'll plan to clean this up in the delegate. Fixing these at the same time will
be no big deal.

[James Cook, 2016-10-14 22:49:18]

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

[James Cook, 2016-10-14 22:49:19]

tsepez, can you take a look at ash/public/interfaces/system_tray.mojom ?

Thanks.

[Tom Sepez, 2016-10-14 23:11:24]

lgtm

[James Cook, 2016-10-15 00:08:18]

The CQ bit was checked by jamescook@chromium.org

[commit-bot: I haz the power, 2016-10-15 00:08:41]

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-15 00:17:00]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-10-15 00:20:10]

Description was changed from

==========
chromeos: Refactor system tray Wi-Fi connect dialog so it works with mash

* Add ShowNetworkConnect method to SystemTrayClient mojom interface
* Add NetworkConnectDelegateMus to route calls over that interface
* Show the config dialog

TODO: Dialog is in the wrong parent window in mash, in particular on the lock
screen. Fixing this will require plumbing through src/ui dialog code, which
will happen in a subsequent CL.

BUG=644355
TEST=existing networking tests
==========

to

==========
chromeos: Refactor system tray Wi-Fi connect dialog so it works with mash

* Add ShowNetworkConnect method to SystemTrayClient mojom interface
* Add NetworkConnectDelegateMus to route calls over that interface
* Show the config dialog

TODO: Dialog is in the wrong parent window in mash, in particular on the lock
screen. Fixing this will require plumbing through src/ui dialog code, which
will happen in a subsequent CL.

BUG=644355
TEST=existing networking tests

Committed: https://crrev.com/5374fbd4595be5190f3b9f93bb5aaafbdaef426f
Cr-Commit-Position: refs/heads/master@{#425515}
==========

[commit-bot: I haz the power, 2016-10-15 00:20:11]

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