Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2426473009:
mash: Place views Wi-Fi network config dialogs in correct window parent (Closed)
|
Created:
4 years, 2 months ago by James Cook Modified:
4 years, 2 months ago CC:
chromium-reviews, dbeam+watch-options_chromium.org, alemate+watch_chromium.org, sadrul, michaelpg+watch-md-settings_chromium.org, tfarina, achuith+watch_chromium.org, dcheng, michaelpg+watch-options_chromium.org, oshima+watch_chromium.org, kalyank, dbeam+watch-settings_chromium.org, stevenjb+watch-md-settings_chromium.org, davemoore+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptionmash: 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
Committed: https://crrev.com/7b9cc810cd7d5b8288e66654041cbbd2beee29e0
Cr-Commit-Position: refs/heads/master@{#426613}
Patch Set 1 #Patch Set 2 : cleanup #
Total comments: 3
Patch Set 3 : sync to parent, nit #Patch Set 4 : rebase #
Total comments: 3
Patch Set 5 : Simplify, eliminate WidgetParent #Patch Set 6 : rebase, Show -> ShowByServicePath #Patch Set 7 : typo #
Total comments: 8
Patch Set 8 : review comments #
Messages
Total messages: 51 (37 generated)
The CQ bit was checked by jamescook@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by jamescook@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
jamescook@chromium.org changed reviewers: + sky@chromium.org
sky, please take a look. https://codereview.chromium.org/2426473009/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/options/network_config_view.cc (right): https://codereview.chromium.org/2426473009/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/options/network_config_view.cc:340: } In this case I was able to configure the dialog widget parent without delegating from src/ui back into src/chrome. I suspect we'll be able to avoid delegating most (all?) of the time. However, I also expect we'll eventually want to consolidate this "choose-parent-or-fallback" code in a central location. I'm waiting to try to centralize it until I see how often this pattern emerges. https://codereview.chromium.org/2426473009/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/options/chromeos/internet_options_handler.h (right): https://codereview.chromium.org/2426473009/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/chromeos/internet_options_handler.h:49: gfx::NativeWindow GetNativeWindow() const; These will go away after a couple other dialog types get converted.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
LGTM https://codereview.chromium.org/2426473009/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/options/network_config_view.cc (right): https://codereview.chromium.org/2426473009/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/options/network_config_view.cc:301: if (params.parent_mus) { no {}
The CQ bit was checked by jamescook@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by jamescook@chromium.org to run a CQ dry run
The CQ bit was unchecked by jamescook@chromium.org
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by jamescook@chromium.org to run a CQ dry run
jamescook@chromium.org changed reviewers: + stevenjb@chromium.org
stevenjb, can I get OWNERS for chrome/browser/ui/webui/options/chromeos/ ?
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2426473009/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/options/network_config_view.cc (right): https://codereview.chromium.org/2426473009/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/options/network_config_view.cc:339: } This doesn't feel like it belongs here. I'm sure we have other dialogs that will need to do the same thing?
https://codereview.chromium.org/2426473009/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/options/network_config_view.cc (right): https://codereview.chromium.org/2426473009/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/options/network_config_view.cc:339: } On 2016/10/19 17:40:07, stevenjb wrote: > This doesn't feel like it belongs here. I'm sure we have other dialogs that will > need to do the same thing? Yeah, I know. Copy/pasting what I said to sky in the initial review: "In this case I was able to configure the dialog widget parent without delegating from src/ui back into src/chrome. I suspect we'll be able to avoid delegating most (all?) of the time. However, I also expect we'll eventually want to consolidate this "choose-parent-or-fallback" code in a central location. I'm waiting to try to centralize it until I see how often this pattern emerges." I'm working on the next one now.
https://codereview.chromium.org/2426473009/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/options/network_config_view.cc (right): https://codereview.chromium.org/2426473009/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/options/network_config_view.cc:339: } On 2016/10/19 18:05:27, James Cook wrote: > On 2016/10/19 17:40:07, stevenjb wrote: > > This doesn't feel like it belongs here. I'm sure we have other dialogs that > will > > need to do the same thing? > > Yeah, I know. Copy/pasting what I said to sky in the initial review: > > "In this case I was able to configure the dialog widget parent without > delegating > from src/ui back into src/chrome. I suspect we'll be able to avoid delegating > most (all?) of the time. However, I also expect we'll eventually want to > consolidate this "choose-parent-or-fallback" code in a central location. I'm > waiting to try to centralize it until I see how often this pattern emerges." > > I'm working on the next one now. Hrmm. Can we at least put this in an anonymous namespace instead of making it a member? I don't see any member dependencies here.
The CQ bit was checked by jamescook@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
jamescook@chromium.org changed reviewers: - sky@chromium.org
stevenjb, please take another look. I cut back this CL significantly to just do the container_id placement in the case I needed. I think if NetworkConfigView::Show* gets any more complicated we're going to need to introduce an InitParams. -sky because this no longer introduces WidgetParent / touches ui/views.
The CQ bit was checked by jamescook@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2426473009/diff/120001/chrome/browser/chromeo... File chrome/browser/chromeos/options/network_config_view.h (right): https://codereview.chromium.org/2426473009/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/options/network_config_view.h:47: // Shows a network connection dialog if none is currently visible. Maybe add 'TODO: deprecate' or explain why we use |parent| here and |container_id| below. (Or have both take parent and container id, even though we happen to only use one or the other). https://codereview.chromium.org/2426473009/diff/120001/chrome/browser/ui/ash/... File chrome/browser/ui/ash/system_tray_client.cc (right): https://codereview.chromium.org/2426473009/diff/120001/chrome/browser/ui/ash/... chrome/browser/ui/ash/system_tray_client.cc:110: return ash::kShellWindowId_LockSystemModalContainer; nit: {} https://codereview.chromium.org/2426473009/diff/120001/chrome/browser/ui/ash/... chrome/browser/ui/ash/system_tray_client.cc:208: return; {} https://codereview.chromium.org/2426473009/diff/120001/chrome/browser/ui/webu... File chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc (right): https://codereview.chromium.org/2426473009/diff/120001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc:266: NetworkConfigView::ShowByServicePath(service_path, GetNativeWindow()); If this and settings are the only places where we use ShowByServicePath, maybe use network->guid() in both of these instead so that we are only differentiating how we specify the container. (I should have audited this in my cleanup pass, sorry).
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by jamescook@chromium.org to run a CQ dry run
stevenjb, thanks for the quick review. Please take another look. https://codereview.chromium.org/2426473009/diff/120001/chrome/browser/chromeo... File chrome/browser/chromeos/options/network_config_view.h (right): https://codereview.chromium.org/2426473009/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/options/network_config_view.h:47: // Shows a network connection dialog if none is currently visible. On 2016/10/20 19:42:40, stevenjb wrote: > Maybe add 'TODO: deprecate' or explain why we use |parent| here and > |container_id| below. (Or have both take parent and container id, even though we > happen to only use one or the other). > Done. I kept it as two functions so the caller doesn't have to reason about what happens if you specify both (or none). https://codereview.chromium.org/2426473009/diff/120001/chrome/browser/ui/ash/... File chrome/browser/ui/ash/system_tray_client.cc (right): https://codereview.chromium.org/2426473009/diff/120001/chrome/browser/ui/ash/... chrome/browser/ui/ash/system_tray_client.cc:110: return ash::kShellWindowId_LockSystemModalContainer; On 2016/10/20 19:42:40, stevenjb wrote: > nit: {} Done. https://codereview.chromium.org/2426473009/diff/120001/chrome/browser/ui/ash/... chrome/browser/ui/ash/system_tray_client.cc:208: return; On 2016/10/20 19:42:40, stevenjb wrote: > {} Done. https://codereview.chromium.org/2426473009/diff/120001/chrome/browser/ui/webu... File chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc (right): https://codereview.chromium.org/2426473009/diff/120001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc:266: NetworkConfigView::ShowByServicePath(service_path, GetNativeWindow()); On 2016/10/20 19:42:40, stevenjb wrote: > If this and settings are the only places where we use ShowByServicePath, maybe > use network->guid() in both of these instead so that we are only differentiating > how we specify the container. (I should have audited this in my cleanup pass, > sorry). > Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
thanks, lgtm!
The CQ bit was checked by jamescook@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org Link to the patchset: https://codereview.chromium.org/2426473009/#ps140001 (title: "review comments")
The CQ bit was unchecked by jamescook@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== 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. * Introduce views::WidgetParent to allow specifying a parent by either native window or container id. * Convert chromeos::NetworkConfigView to use WidgetParent. TODO: Support display ID in WidgetParent. TODO: Convert other networking dialogs, like cellular and VPN BUG=657021 TEST=ash_unittests + manual, spawn wifi password dialog before and after login ========== to ========== 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 ==========
The CQ bit was checked by jamescook@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== 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 ========== to ========== 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 ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== 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 ========== to ========== 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 Committed: https://crrev.com/7b9cc810cd7d5b8288e66654041cbbd2beee29e0 Cr-Commit-Position: refs/heads/master@{#426613} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/7b9cc810cd7d5b8288e66654041cbbd2beee29e0 Cr-Commit-Position: refs/heads/master@{#426613} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
