chromeos: Make "mobile network" and "set time" dialogs work with mash

chromeos: Make "mobile network" and "set time" dialogs work with mash

Under mash code in chrome cannot directly access the ash aura window hierarchy.
Instead it must pass an ash window container id when a new window is being
opened.

Add ChooseMobileNetworkDialog::ShowDialogInContainer() factory method.

Refactor existing ShowWebDialogWithContainer method -- in this case the parent
NativeWindow can be null. Rather than splitting out a ShowWebDialogInParent
method instead fold the code into the existing ShowWebDialog implementation.

Also update "set time" dialog which uses the same SetWebDialog code path.

BUG=657021
TEST=open mobile network dialog at login screen and after login

Committed: https://crrev.com/1c6343d745687e1593c0eb8a0b3132aab81f5f42
Cr-Commit-Position: refs/heads/master@{#427383}

[James Cook, 2016-10-24 21:33:17]

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

[commit-bot: I haz the power, 2016-10-24 21:34:14]

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-24 21:36:38]

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

[commit-bot: I haz the power, 2016-10-24 21:37:02]

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-24 21:37:24]

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

[James Cook, 2016-10-24 21:37:25]

msw, please take a look.

There's a small amount of duplicated code between the two ShowWebDialog
implementations, but my attempts to share the code made it worse (the
WebDialogView must be created before the widget, and the widget must be
created before the ExtensionWebContentsObserver is attached, so it ends up
having to be split into 3 parts, and a lot of the duplicated stuff is pretty
common widget creation code).

[msw, 2016-10-24 22:18:09]

lgtm with minor comments/questions. It's a bit awkward, but I don't have any
brilliant suggestions. Perhaps aura as a mus client will help?

https://codereview.chromium.org/2446573003/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/ui/choose_mobile_network_dialog.cc (right):

https://codereview.chromium.org/2446573003/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/ui/choose_mobile_network_dialog.cc:36:
DCHECK_NE(container_id, ash::kShellWindowId_Invalid);
q: only a couple ids are really expected here, right? Check for those?

https://codereview.chromium.org/2446573003/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/ui/choose_mobile_network_dialog.h (right):

https://codereview.chromium.org/2446573003/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/ui/choose_mobile_network_dialog.h:17: // Shows the
dialog box. |owning_window| may be null.
nit: Maybe explain the significance of |owning_window| and what happens when
it's null?

https://codereview.chromium.org/2446573003/diff/20001/chrome/browser/ui/ash/w...
File chrome/browser/ui/ash/web_dialog_util.cc (right):

https://codereview.chromium.org/2446573003/diff/20001/chrome/browser/ui/ash/w...
chrome/browser/ui/ash/web_dialog_util.cc:30: using ui::mojom::WindowManager;
aside: this is a little odd; is it much worse to inline for the one use?

https://codereview.chromium.org/2446573003/diff/20001/chrome/browser/ui/ash/w...
File chrome/browser/ui/ash/web_dialog_util.h (right):

https://codereview.chromium.org/2446573003/diff/20001/chrome/browser/ui/ash/w...
chrome/browser/ui/ash/web_dialog_util.h:24: void ShowWebDialogInContainer(int
container_id,
q: Should this live alongside chrome::ShowWebDialog?
(I guess that's awkward with cros-specific container ids)

[commit-bot: I haz the power, 2016-10-24 22:42:32]

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

[commit-bot: I haz the power, 2016-10-24 22:42:34]

Dry run: This issue passed the CQ dry run.

[James Cook, 2016-10-25 00:29:04]

Thanks for the quick review.

https://codereview.chromium.org/2446573003/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/ui/choose_mobile_network_dialog.cc (right):

https://codereview.chromium.org/2446573003/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/ui/choose_mobile_network_dialog.cc:36:
DCHECK_NE(container_id, ash::kShellWindowId_Invalid);
On 2016/10/24 22:18:09, msw wrote:
> q: only a couple ids are really expected here, right? Check for those?

Good idea. Done.

https://codereview.chromium.org/2446573003/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/ui/choose_mobile_network_dialog.h (right):

https://codereview.chromium.org/2446573003/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/ui/choose_mobile_network_dialog.h:17: // Shows the
dialog box. |owning_window| may be null.
On 2016/10/24 22:18:09, msw wrote:
> nit: Maybe explain the significance of |owning_window| and what happens when
> it's null?

Done (and renamed to parent).

https://codereview.chromium.org/2446573003/diff/20001/chrome/browser/ui/ash/w...
File chrome/browser/ui/ash/web_dialog_util.cc (right):

https://codereview.chromium.org/2446573003/diff/20001/chrome/browser/ui/ash/w...
chrome/browser/ui/ash/web_dialog_util.cc:30: using ui::mojom::WindowManager;
On 2016/10/24 22:18:09, msw wrote:
> aside: this is a little odd; is it much worse to inline for the one use?

I kept the "using" because otherwise you end up with 

  if (chrome::IsRunningInMash()) {
    params.mus_properties
        [ui::mojom::WindowManager::kInitialContainerId_Property] =
        mojo::ConvertTo<std::vector<uint8_t>>(container_id);
  } else {

which is odd also.

https://codereview.chromium.org/2446573003/diff/20001/chrome/browser/ui/ash/w...
File chrome/browser/ui/ash/web_dialog_util.h (right):

https://codereview.chromium.org/2446573003/diff/20001/chrome/browser/ui/ash/w...
chrome/browser/ui/ash/web_dialog_util.h:24: void ShowWebDialogInContainer(int
container_id,
On 2016/10/24 22:18:09, msw wrote:
> q: Should this live alongside chrome::ShowWebDialog?
> (I guess that's awkward with cros-specific container ids)

Moved it next to chrome::ShowWebDialog().

[James Cook, 2016-10-25 00:30:42]

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

[James Cook, 2016-10-25 00:30:43]

stevenjb, can I get OWNERS for chrome/browser/chromeos ?

[James Cook, 2016-10-25 00:39:08]

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

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

Dry run: 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-25 00:48:54]

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

[commit-bot: I haz the power, 2016-10-25 00:48:55]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
  win_clang on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)

[James Cook, 2016-10-25 15:43:13]

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

[commit-bot: I haz the power, 2016-10-25 15:43:36]

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-25 16:30:52]

Could you update the summary/description to account for changing the set time
dialog also?

Otherwise lgtm.

[commit-bot: I haz the power, 2016-10-25 16:52:17]

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

[commit-bot: I haz the power, 2016-10-25 16:52:18]

Dry run: This issue passed the CQ dry run.

[James Cook, 2016-10-25 16:52:50]

Description was changed from

==========
chromeos: Make "mobile network" dialog work with mash

Under mash code in chrome cannot directly access the ash aura window hierarchy.
Instead it must pass an ash window container id when a new window is being
opened.

Add ChooseMobileNetworkDialog::ShowDialogInContainer() factory method.

Refactor existing ShowWebDialogWithContainer method -- in this case the parent
NativeWindow can be null. Rather than splitting out a ShowWebDialogInParent
method instead fold the code into the existing ShowWebDialog implementation.

BUG=657021
TEST=open mobile network dialog at login screen and after login
==========

to

==========
chromeos: Make "mobile network" dialog work with mash

Under mash code in chrome cannot directly access the ash aura window hierarchy.
Instead it must pass an ash window container id when a new window is being
opened.

Add ChooseMobileNetworkDialog::ShowDialogInContainer() factory method.

Refactor existing ShowWebDialogWithContainer method -- in this case the parent
NativeWindow can be null. Rather than splitting out a ShowWebDialogInParent
method instead fold the code into the existing ShowWebDialog implementation.

Also update "set time" dialog which uses the same SetWebDialog code path.

BUG=657021
TEST=open mobile network dialog at login screen and after login
==========

[James Cook, 2016-10-25 16:53:37]

Description was changed from

==========
chromeos: Make "mobile network" dialog work with mash

Under mash code in chrome cannot directly access the ash aura window hierarchy.
Instead it must pass an ash window container id when a new window is being
opened.

Add ChooseMobileNetworkDialog::ShowDialogInContainer() factory method.

Refactor existing ShowWebDialogWithContainer method -- in this case the parent
NativeWindow can be null. Rather than splitting out a ShowWebDialogInParent
method instead fold the code into the existing ShowWebDialog implementation.

Also update "set time" dialog which uses the same SetWebDialog code path.

BUG=657021
TEST=open mobile network dialog at login screen and after login
==========

to

==========
chromeos: Make "mobile network" and "set time" dialogs work with mash

Under mash code in chrome cannot directly access the ash aura window hierarchy.
Instead it must pass an ash window container id when a new window is being
opened.

Add ChooseMobileNetworkDialog::ShowDialogInContainer() factory method.

Refactor existing ShowWebDialogWithContainer method -- in this case the parent
NativeWindow can be null. Rather than splitting out a ShowWebDialogInParent
method instead fold the code into the existing ShowWebDialog implementation.

Also update "set time" dialog which uses the same SetWebDialog code path.

BUG=657021
TEST=open mobile network dialog at login screen and after login
==========

[James Cook, 2016-10-25 16:53:54]

Updated CL title/description. Thanks for the review.

[James Cook, 2016-10-25 16:53:59]

The CQ bit was checked by jamescook@chromium.org

[James Cook, 2016-10-25 16:53:59]

The patchset sent to the CQ was uploaded after l-g-t-m from msw@chromium.org
Link to the patchset: https://codereview.chromium.org/2446573003/#ps80001
(title: "gn check")

[commit-bot: I haz the power, 2016-10-25 16:54:19]

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-25 16:59:18]

Description was changed from

==========
chromeos: Make "mobile network" and "set time" dialogs work with mash

Under mash code in chrome cannot directly access the ash aura window hierarchy.
Instead it must pass an ash window container id when a new window is being
opened.

Add ChooseMobileNetworkDialog::ShowDialogInContainer() factory method.

Refactor existing ShowWebDialogWithContainer method -- in this case the parent
NativeWindow can be null. Rather than splitting out a ShowWebDialogInParent
method instead fold the code into the existing ShowWebDialog implementation.

Also update "set time" dialog which uses the same SetWebDialog code path.

BUG=657021
TEST=open mobile network dialog at login screen and after login
==========

to

==========
chromeos: Make "mobile network" and "set time" dialogs work with mash

Under mash code in chrome cannot directly access the ash aura window hierarchy.
Instead it must pass an ash window container id when a new window is being
opened.

Add ChooseMobileNetworkDialog::ShowDialogInContainer() factory method.

Refactor existing ShowWebDialogWithContainer method -- in this case the parent
NativeWindow can be null. Rather than splitting out a ShowWebDialogInParent
method instead fold the code into the existing ShowWebDialog implementation.

Also update "set time" dialog which uses the same SetWebDialog code path.

BUG=657021
TEST=open mobile network dialog at login screen and after login
==========

[commit-bot: I haz the power, 2016-10-25 16:59:19]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-10-25 17:16:36]

Description was changed from

==========
chromeos: Make "mobile network" and "set time" dialogs work with mash

Under mash code in chrome cannot directly access the ash aura window hierarchy.
Instead it must pass an ash window container id when a new window is being
opened.

Add ChooseMobileNetworkDialog::ShowDialogInContainer() factory method.

Refactor existing ShowWebDialogWithContainer method -- in this case the parent
NativeWindow can be null. Rather than splitting out a ShowWebDialogInParent
method instead fold the code into the existing ShowWebDialog implementation.

Also update "set time" dialog which uses the same SetWebDialog code path.

BUG=657021
TEST=open mobile network dialog at login screen and after login
==========

to

==========
chromeos: Make "mobile network" and "set time" dialogs work with mash

Under mash code in chrome cannot directly access the ash aura window hierarchy.
Instead it must pass an ash window container id when a new window is being
opened.

Add ChooseMobileNetworkDialog::ShowDialogInContainer() factory method.

Refactor existing ShowWebDialogWithContainer method -- in this case the parent
NativeWindow can be null. Rather than splitting out a ShowWebDialogInParent
method instead fold the code into the existing ShowWebDialog implementation.

Also update "set time" dialog which uses the same SetWebDialog code path.

BUG=657021
TEST=open mobile network dialog at login screen and after login

Committed: https://crrev.com/1c6343d745687e1593c0eb8a0b3132aab81f5f42
Cr-Commit-Position: refs/heads/master@{#427383}
==========

[commit-bot: I haz the power, 2016-10-25 17:16:37]

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