New issue
Advanced search Search tips

Issue 657021 link

Starred by 2 users

Issue metadata

Status: Archived
Owner:
Closed: Jan 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 2
Type: Bug

Blocking:
issue 647412



Sign in to add a comment

mash: Fix window parenting for networking dialogs

Project Member Reported by jamescook@chromium.org, Oct 18 2016

Issue description

Networking dialogs can spawn at arbitrary times. For example, if the user attempts to connect to a Wi-Fi network the network may respond with an authentication error several seconds later. The password dialog needs to place itself in the correct window container when it opens. This depends on login state, whether network settings UI is already open, etc.

 
This is going to be a general problem for mash. For example, I've been doing some reviews for  issue 612884  ( issue 612886  specifically) where smart cards need to be able to request a dialog. There has been discussion about this needing to be available during login (although I'm not sure how extensions/apps work in that context). Regardless, this seems like a service that will eventually need to be provided by ash.

Blocking: 647412
Labels: -Pri-3 Pri-2
Owner: jamescook@chromium.org
Status: Assigned (was: Untriaged)
Status: Started (was: Assigned)
Project Member

Comment 5 by bugdroid1@chromium.org, Oct 20 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/7b9cc810cd7d5b8288e66654041cbbd2beee29e0

commit 7b9cc810cd7d5b8288e66654041cbbd2beee29e0
Author: jamescook <jamescook@chromium.org>
Date: Thu Oct 20 22:03:25 2016

mash: Place views Wi-Fi network config dialogs in correct window parent

This is necessary to show them at the login screen, where they must appear in
the lock-screen-modal container.

* Convert chromeos::NetworkConfigView to take either a native window parent
or a container id.
* Check login status on mash to compute proper container.

TODO: Convert other networking dialogs, like cellular and VPN

BUG= 657021 
TEST=ash_unittests + manual, spawn wifi password dialog before and after login

Review-Url: https://chromiumcodereview.appspot.com/2426473009
Cr-Commit-Position: refs/heads/master@{#426613}

[modify] https://crrev.com/7b9cc810cd7d5b8288e66654041cbbd2beee29e0/ash/common/wm/system_modal_container_layout_manager.cc
[modify] https://crrev.com/7b9cc810cd7d5b8288e66654041cbbd2beee29e0/chrome/browser/chromeos/options/network_config_view.cc
[modify] https://crrev.com/7b9cc810cd7d5b8288e66654041cbbd2beee29e0/chrome/browser/chromeos/options/network_config_view.h
[modify] https://crrev.com/7b9cc810cd7d5b8288e66654041cbbd2beee29e0/chrome/browser/ui/ash/network_connect_delegate_chromeos.cc
[modify] https://crrev.com/7b9cc810cd7d5b8288e66654041cbbd2beee29e0/chrome/browser/ui/ash/system_tray_client.cc
[modify] https://crrev.com/7b9cc810cd7d5b8288e66654041cbbd2beee29e0/chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc
[modify] https://crrev.com/7b9cc810cd7d5b8288e66654041cbbd2beee29e0/chrome/browser/ui/webui/settings/chromeos/internet_handler.cc

Project Member

Comment 6 by bugdroid1@chromium.org, Oct 22 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/e36a90e2b15864b46c1d4ab862fd38590877b8d0

commit e36a90e2b15864b46c1d4ab862fd38590877b8d0
Author: jamescook <jamescook@chromium.org>
Date: Sat Oct 22 02:22:04 2016

mash: Introduce ShowWebDialogWithContainer for webui dialogs

Several webui dialogs spawned by the ash system tray can be shown either as
children of the general settings webui window or by themselves at the login
screen. In the latter case we need to place them in the right ash window
container. Under mash, chrome is not aware of the ash window hierarchy, so
it needs to use a container ID rather than a native window parent.

Wire up the "set time" dialog as an example.

BUG= 657021 
TEST=manual, spawn set time dialog from settings and from lock screen
TBR=stevenjb@chromium.org for function rename touching c/b/ui/webui/chromeos

Review-Url: https://chromiumcodereview.appspot.com/2441133002
Cr-Commit-Position: refs/heads/master@{#426953}

[modify] https://crrev.com/e36a90e2b15864b46c1d4ab862fd38590877b8d0/chrome/browser/chromeos/set_time_dialog.cc
[modify] https://crrev.com/e36a90e2b15864b46c1d4ab862fd38590877b8d0/chrome/browser/chromeos/set_time_dialog.h
[modify] https://crrev.com/e36a90e2b15864b46c1d4ab862fd38590877b8d0/chrome/browser/ui/BUILD.gn
[modify] https://crrev.com/e36a90e2b15864b46c1d4ab862fd38590877b8d0/chrome/browser/ui/ash/system_tray_delegate_chromeos.cc
[add] https://crrev.com/e36a90e2b15864b46c1d4ab862fd38590877b8d0/chrome/browser/ui/ash/web_dialog_util.cc
[add] https://crrev.com/e36a90e2b15864b46c1d4ab862fd38590877b8d0/chrome/browser/ui/ash/web_dialog_util.h
[modify] https://crrev.com/e36a90e2b15864b46c1d4ab862fd38590877b8d0/chrome/browser/ui/webui/options/chromeos/date_time_options_handler.cc

Project Member

Comment 7 by bugdroid1@chromium.org, Oct 24 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/4f08d17c7ae7f28f341428276acab0f1aa3a0078

commit 4f08d17c7ae7f28f341428276acab0f1aa3a0078
Author: jamescook <jamescook@chromium.org>
Date: Mon Oct 24 21:42:44 2016

chromeos: Convert "show other network" dialog to 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.

Create a new NetworkConfigView::ShowForType to allow passing a container id.

Also clean up some unnecessary code around static initializers.

BUG= 657021 
TEST=open "join other" from system tray menu at login screen and after login

Review-Url: https://codereview.chromium.org/2445843002
Cr-Commit-Position: refs/heads/master@{#427160}

[modify] https://crrev.com/4f08d17c7ae7f28f341428276acab0f1aa3a0078/chrome/browser/chromeos/options/network_config_view.cc
[modify] https://crrev.com/4f08d17c7ae7f28f341428276acab0f1aa3a0078/chrome/browser/chromeos/options/network_config_view.h
[modify] https://crrev.com/4f08d17c7ae7f28f341428276acab0f1aa3a0078/chrome/browser/ui/ash/system_tray_delegate_chromeos.cc

Project Member

Comment 8 by bugdroid1@chromium.org, Oct 25 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/1c6343d745687e1593c0eb8a0b3132aab81f5f42

commit 1c6343d745687e1593c0eb8a0b3132aab81f5f42
Author: jamescook <jamescook@chromium.org>
Date: Tue Oct 25 16:58:50 2016

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

Review-Url: https://codereview.chromium.org/2446573003
Cr-Commit-Position: refs/heads/master@{#427383}

[modify] https://crrev.com/1c6343d745687e1593c0eb8a0b3132aab81f5f42/chrome/browser/chromeos/set_time_dialog.cc
[modify] https://crrev.com/1c6343d745687e1593c0eb8a0b3132aab81f5f42/chrome/browser/chromeos/ui/choose_mobile_network_dialog.cc
[modify] https://crrev.com/1c6343d745687e1593c0eb8a0b3132aab81f5f42/chrome/browser/chromeos/ui/choose_mobile_network_dialog.h
[modify] https://crrev.com/1c6343d745687e1593c0eb8a0b3132aab81f5f42/chrome/browser/ui/BUILD.gn
[modify] https://crrev.com/1c6343d745687e1593c0eb8a0b3132aab81f5f42/chrome/browser/ui/ash/system_tray_delegate_chromeos.cc
[delete] https://crrev.com/4bdb40f764d2fc3ac7f4e34da68424e84b89968f/chrome/browser/ui/ash/web_dialog_util.cc
[delete] https://crrev.com/4bdb40f764d2fc3ac7f4e34da68424e84b89968f/chrome/browser/ui/ash/web_dialog_util.h
[modify] https://crrev.com/1c6343d745687e1593c0eb8a0b3132aab81f5f42/chrome/browser/ui/browser_dialogs.h
[modify] https://crrev.com/1c6343d745687e1593c0eb8a0b3132aab81f5f42/chrome/browser/ui/views/chrome_web_dialog_view.cc

Project Member

Comment 9 by bugdroid1@chromium.org, Oct 26 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/aa0f21bffad98b4b27d6ffff0879d2e13c0ae3c5

commit aa0f21bffad98b4b27d6ffff0879d2e13c0ae3c5
Author: jamescook <jamescook@chromium.org>
Date: Wed Oct 26 00:27:44 2016

chromeos: Convert bluetooth pairing dialog to work with mash, part 1

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.

Migrate some UMA statistics from //ash to //chrome to eliminate a couple
references to ash::WmShell inside chrome.

BUG= 657021 
TEST=chrome browser_tests, also open bluetooth pairing dialog from system tray
menu at login screen

Review-Url: https://codereview.chromium.org/2446403003
Cr-Commit-Position: refs/heads/master@{#427549}

[modify] https://crrev.com/aa0f21bffad98b4b27d6ffff0879d2e13c0ae3c5/ash/common/metrics/user_metrics_action.h
[modify] https://crrev.com/aa0f21bffad98b4b27d6ffff0879d2e13c0ae3c5/ash/metrics/user_metrics_recorder.cc
[modify] https://crrev.com/aa0f21bffad98b4b27d6ffff0879d2e13c0ae3c5/chrome/browser/chromeos/bluetooth/bluetooth_pairing_dialog.cc
[modify] https://crrev.com/aa0f21bffad98b4b27d6ffff0879d2e13c0ae3c5/chrome/browser/chromeos/bluetooth/bluetooth_pairing_dialog.h
[modify] https://crrev.com/aa0f21bffad98b4b27d6ffff0879d2e13c0ae3c5/chrome/browser/ui/ash/system_tray_delegate_chromeos.cc
[modify] https://crrev.com/aa0f21bffad98b4b27d6ffff0879d2e13c0ae3c5/chrome/browser/ui/webui/chromeos/bluetooth_pairing_ui_browsertest-inl.h

Project Member

Comment 10 by bugdroid1@chromium.org, Oct 29 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/f44dbc1714dc8e15a394c6eac2fbbfddf47dfb52

commit f44dbc1714dc8e15a394c6eac2fbbfddf47dfb52
Author: jamescook <jamescook@chromium.org>
Date: Sat Oct 29 00:51:41 2016

chromeos: Make network enrollment and SIM unlock dialogs work with mash

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

* Convert dialogs to use container id when no explicit parent is available
* Remove ash dependencies from NetworkConnectDelegateChromeos, including
GetNativeWindow()
* Add DEPS to restrict access to ash from chrome/browser/chromeos/net

Also do some cleanup:
* Add class comments for captive portal and HaTS dialogs, which I audited.
They do not need to be fixed for mash because they are always displayed in the
default container.
* Fix include-what-you-use.

BUG= 657021 
TEST=chrome browser_tests

Review-Url: https://codereview.chromium.org/2452283003
Cr-Commit-Position: refs/heads/master@{#428569}

[modify] https://crrev.com/f44dbc1714dc8e15a394c6eac2fbbfddf47dfb52/chrome/browser/chromeos/enrollment_dialog_view.cc
[modify] https://crrev.com/f44dbc1714dc8e15a394c6eac2fbbfddf47dfb52/chrome/browser/chromeos/enrollment_dialog_view.h
[modify] https://crrev.com/f44dbc1714dc8e15a394c6eac2fbbfddf47dfb52/chrome/browser/chromeos/hats/hats_dialog.h
[modify] https://crrev.com/f44dbc1714dc8e15a394c6eac2fbbfddf47dfb52/chrome/browser/chromeos/net/DEPS
[modify] https://crrev.com/f44dbc1714dc8e15a394c6eac2fbbfddf47dfb52/chrome/browser/chromeos/net/network_connect_delegate_chromeos.cc
[modify] https://crrev.com/f44dbc1714dc8e15a394c6eac2fbbfddf47dfb52/chrome/browser/chromeos/net/network_portal_notification_controller.h
[modify] https://crrev.com/f44dbc1714dc8e15a394c6eac2fbbfddf47dfb52/chrome/browser/chromeos/sim_dialog_delegate.cc
[modify] https://crrev.com/f44dbc1714dc8e15a394c6eac2fbbfddf47dfb52/chrome/browser/chromeos/sim_dialog_delegate.h
[modify] https://crrev.com/f44dbc1714dc8e15a394c6eac2fbbfddf47dfb52/chrome/browser/ui/ash/system_tray_client.cc
[modify] https://crrev.com/f44dbc1714dc8e15a394c6eac2fbbfddf47dfb52/chrome/browser/ui/ash/system_tray_client.h

Project Member

Comment 11 by bugdroid1@chromium.org, Oct 31 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/f3e88cbf92c8965319eef3a20e6073c8fe4bed6f

commit f3e88cbf92c8965319eef3a20e6073c8fe4bed6f
Author: jamescook <jamescook@chromium.org>
Date: Mon Oct 31 16:21:43 2016

chromeos: Refactor NetworkConfigView to use shared widget parent code for mash

Now that SystemTrayClient::CreateUnhostedDialogWidget exists for both mash and
classic ash, convert NetworkConfigView to use it.

This exposes the fact that LoginDisplayHost::GetNativeWindow does not return a
valid mus window (aka ui::Window) parent. However, mash is converting soon to
use aura::Window natively, so that problem will go away. Also, if the user is
at a login screen then CreateUnhostedDialogWidget should do the right thing
anyway.

BUG= 657021 
TEST=open wifi password dialogs, both after and before login, on both mash and
classic ash

Review-Url: https://codereview.chromium.org/2458883002
Cr-Commit-Position: refs/heads/master@{#428731}

[modify] https://crrev.com/f3e88cbf92c8965319eef3a20e6073c8fe4bed6f/chrome/browser/chromeos/options/network_config_view.cc
[modify] https://crrev.com/f3e88cbf92c8965319eef3a20e6073c8fe4bed6f/chrome/browser/chromeos/options/network_config_view.h
[modify] https://crrev.com/f3e88cbf92c8965319eef3a20e6073c8fe4bed6f/chrome/browser/ui/ash/system_tray_client.cc
[modify] https://crrev.com/f3e88cbf92c8965319eef3a20e6073c8fe4bed6f/chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc
[modify] https://crrev.com/f3e88cbf92c8965319eef3a20e6073c8fe4bed6f/chrome/browser/ui/webui/settings/chromeos/internet_handler.cc

Status: Fixed (was: Started)
All the major ones are done. I'll file separate bugs for any others as they come up.

Comment 13 by dchan@google.com, Mar 4 2017

Labels: VerifyIn-58

Comment 14 by dchan@google.com, Apr 17 2017

Labels: VerifyIn-59

Comment 15 by dchan@google.com, May 30 2017

Labels: VerifyIn-60
Labels: VerifyIn-61

Comment 17 by dchan@chromium.org, Oct 14 2017

Status: Archived (was: Fixed)
Components: -Internals>MUS Internals>Services>WindowService

Sign in to add a comment