cros: Fix position of network connect dialog

Always place it in the center of the "root window for new windows", which
is the display where the user last interacted with a window.

This fixes a regression where the dialog could be centered over a tabbed
browser window. It also fixes a couple cases where adding a new Wi-Fi
network from the status tray menu on a secondary monitor would open the
dialog on the primary monitor.

Added some basic test coverage to NetworkConfigView.

Bug: 740316
Test: chrome browser_tests. Also manually add a new Wi-Fi network during OOBE, at the login screen, and after login on both primary and secondary displays.
Change-Id: I60f2070dea9f39da586c97ff8b178fc34c5fb325
Reviewed-on: https://chromium-review.googlesource.com/569025
Commit-Queue: James Cook <jamescook@chromium.org>
Reviewed-by: Steven Bennetts <stevenjb@chromium.org>
Cr-Commit-Position: refs/heads/master@{#486507}

chrome/browser/chromeos/options/network_config_view.cc

@@ -8,18 +8,12 @@
 
 #include "base/strings/string_util.h"
 #include "base/strings/utf_string_conversions.h"
-#include "chrome/browser/chromeos/login/ui/login_display_host.h"
 #include "chrome/browser/chromeos/options/network_property_ui_data.h"
 #include "chrome/browser/chromeos/options/vpn_config_view.h"
 #include "chrome/browser/chromeos/options/wifi_config_view.h"
 #include "chrome/browser/chromeos/options/wimax_config_view.h"
-#include "chrome/browser/profiles/profile_manager.h"
-#include "chrome/browser/ui/ash/ash_util.h"
 #include "chrome/browser/ui/ash/system_tray_client.h"
-#include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/browser_dialogs.h"
-#include "chrome/browser/ui/browser_finder.h"
-#include "chrome/browser/ui/browser_window.h"
 #include "chrome/grit/generated_resources.h"
 #include "chrome/grit/locale_settings.h"
 #include "chromeos/login/login_state.h"
@@ -50,22 +44,6 @@ namespace {
 // Used to check if a network config dialog is already showing.
 NetworkConfigView* g_instance = nullptr;
 
-gfx::NativeWindow GetParentForUnhostedDialog() {
-  if (LoginDisplayHost::default_host()) {
-    // TODO(jamescook): LoginDisplayHost has the wrong native window in mash.
-    // This will fix itself when mash converts from ui::Window to aura::Window.
-    // http://crbug.com/659155
-    if (!ash_util::IsRunningInMash())
-      return LoginDisplayHost::default_host()->GetNativeWindow();
-  } else {
-    Browser* browser = chrome::FindTabbedBrowser(
-        ProfileManager::GetPrimaryUserProfile(), true);
-    if (browser)
-      return browser->window()->GetNativeWindow();
-  }
-  return nullptr;
-}
-
 }  // namespace
 
 // static
@@ -116,17 +94,17 @@ NetworkConfigView::~NetworkConfigView() {
 }
 
 // static
-void NetworkConfigView::ShowForNetworkId(const std::string& network_id,
-                                         gfx::NativeWindow parent) {
+NetworkConfigView* NetworkConfigView::ShowForNetworkId(
+    const std::string& network_id) {
   if (g_instance)
-    return;
+    return g_instance;
   const NetworkState* network =
       NetworkHandler::Get()->network_state_handler()->GetNetworkStateFromGuid(
           network_id);
   if (!network) {
     LOG(ERROR)
         << "NetworkConfigView::ShowForNetworkId called with invalid network";
-    return;
+    return nullptr;
   }
   NetworkConfigView* view = new NetworkConfigView();
   if (!view->InitWithNetworkState(network)) {
@@ -134,26 +112,27 @@ void NetworkConfigView::ShowForNetworkId(const std::string& network_id,
                   "network type: "
                << network->type();
     delete view;
-    return;
+    return nullptr;
   }
   NET_LOG(USER) << "NetworkConfigView::ShowForNetworkId: " << network->path();
-  view->ShowDialog(parent);
+  view->ShowDialog();
+  return view;
 }
 
 // static
-void NetworkConfigView::ShowForType(const std::string& type,
-                                    gfx::NativeWindow parent) {
+NetworkConfigView* NetworkConfigView::ShowForType(const std::string& type) {
   if (g_instance)
-    return;
+    return g_instance;
   NetworkConfigView* view = new NetworkConfigView();
   if (!view->InitWithType(type)) {
     LOG(ERROR) << "NetworkConfigView::ShowForType called with invalid type: "
                << type;
     delete view;
-    return;
+    return nullptr;
   }
   NET_LOG(USER) << "NetworkConfigView::ShowForType: " << type;
-  view->ShowDialog(parent);
+  view->ShowDialog();
+  return view;
 }
 
 gfx::NativeWindow NetworkConfigView::GetNativeWindow() const {
@@ -279,19 +258,8 @@ void NetworkConfigView::ViewHierarchyChanged(
   }
 }
 
-void NetworkConfigView::ShowDialog(gfx::NativeWindow parent) {
-  // Attempt to find a fallback parent window.
-  if (parent == nullptr)
-    parent = GetParentForUnhostedDialog();
-
-  Widget* window = nullptr;
-  if (parent) {
-    // Create as a child of |parent|.
-    window = DialogDelegate::CreateDialogWidget(this, nullptr, parent);
-  } else {
-    // Fall back to default window container on primary display.
-    window = SystemTrayClient::CreateUnownedDialogWidget(this);
-  }
+void NetworkConfigView::ShowDialog() {
+  Widget* window = SystemTrayClient::CreateUnownedDialogWidget(this);
   window->SetAlwaysOnTop(true);
   window->Show();
 }

chrome/browser/chromeos/options/network_config_view.h

@@ -7,7 +7,6 @@
 
 #include <string>
 
-#include "base/compiler_specific.h"
 #include "base/macros.h"
 #include "base/strings/string16.h"
 #include "ui/gfx/native_widget_types.h"  // gfx::NativeWindow
@@ -41,16 +40,13 @@ class NetworkConfigView : public views::DialogDelegateView,
   };
 
   // Shows a network connection dialog if none is currently visible. The dialog
-  // will be a child of |parent| (e.g. the webui settings window) which ensures
-  // it appears on the display the user is looking at. If |parent| is null and
-  // no fallback parent can be found then the dialog will be placed on the
-  // primary display.
-  static void ShowForNetworkId(const std::string& network_id,
-                               gfx::NativeWindow parent);
+  // is placed on the default display for new windows. Returns the dialog or
+  // nullptr on error.
+  static NetworkConfigView* ShowForNetworkId(const std::string& network_id);
 
   // Shows a dialog to configure a new network. |type| must be a valid Shill
-  // 'Type' property value. See above regarding |parent|.
-  static void ShowForType(const std::string& type, gfx::NativeWindow parent);
+  // 'Type' property value. Returns the dialog or nullptr on error.
+  static NetworkConfigView* ShowForType(const std::string& type);
 
   // Returns corresponding native window.
   gfx::NativeWindow GetNativeWindow() const;
@@ -94,7 +90,7 @@ class NetworkConfigView : public views::DialogDelegateView,
   bool InitWithType(const std::string& type);
 
   // Creates and shows a dialog containing this view.
-  void ShowDialog(gfx::NativeWindow parent);
+  void ShowDialog();
 
   // Resets the underlying view to show advanced options.
   void ShowAdvancedView();

chrome/browser/chromeos/options/network_config_view_browsertest.cc

+// Copyright 2017 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "chrome/browser/chromeos/options/network_config_view.h"
+
+#include "chrome/test/base/in_process_browser_test.h"
+#include "third_party/cros_system_api/dbus/shill/dbus-constants.h"
+
+namespace chromeos {
+namespace {
+
+using NetworkConfigViewTest = InProcessBrowserTest;
+
+IN_PROC_BROWSER_TEST_F(NetworkConfigViewTest, ShowForType) {
+  // An invalid network type does not create a dialog.
+  EXPECT_FALSE(NetworkConfigView::ShowForType("invalid"));
+
+  // Showing the dialog creates an always-on-top widget.
+  NetworkConfigView* view = NetworkConfigView::ShowForType(shill::kTypeWifi);
+  ASSERT_TRUE(view->GetWidget());
+  EXPECT_TRUE(view->GetWidget()->IsVisible());
+  EXPECT_TRUE(view->GetWidget()->IsAlwaysOnTop());
+
+  // Showing again returns the same dialog.
+  NetworkConfigView* repeat = NetworkConfigView::ShowForType(shill::kTypeWifi);
+  EXPECT_EQ(view, repeat);
+
+  view->GetWidget()->CloseNow();
+}
+
+IN_PROC_BROWSER_TEST_F(NetworkConfigViewTest, ShowForNetworkId) {
+  // An invalid network ID does not create a dialog.
+  EXPECT_FALSE(NetworkConfigView::ShowForNetworkId("invalid"));
+
+  // Showing the dialog creates an always-on-top widget.
+  NetworkConfigView* view = NetworkConfigView::ShowForNetworkId("wifi1_guid");
+  ASSERT_TRUE(view->GetWidget());
+  EXPECT_TRUE(view->GetWidget()->IsVisible());
+  EXPECT_TRUE(view->GetWidget()->IsAlwaysOnTop());
+
+  // Showing again returns the same dialog.
+  NetworkConfigView* repeat = NetworkConfigView::ShowForNetworkId("wifi1_guid");
+  EXPECT_EQ(view, repeat);
+
+  view->GetWidget()->CloseNow();
+}
+
+}  // namespace
+}  // namespace chromeos

chrome/browser/chromeos/status/network_menu.cc

@@ -338,11 +338,13 @@ ui::MenuModelDelegate* NetworkMenuModel::GetMenuModelDelegate() const {
 // NetworkMenuModel, private methods:
 
 void NetworkMenuModel::ShowOther(const std::string& type) const {
-  gfx::NativeWindow native_window = owner_->delegate()->GetNativeWindow();
-  if (type == shill::kTypeCellular)
-    ChooseMobileNetworkDialog::ShowDialog(native_window);
-  else
-    NetworkConfigView::ShowForType(shill::kTypeWifi, native_window);
+  if (type == shill::kTypeCellular) {
+    // TODO(jamescook): This should not need a parent.
+    ChooseMobileNetworkDialog::ShowDialog(
+        owner_->delegate()->GetNativeWindow());
+  } else {
+    NetworkConfigView::ShowForType(shill::kTypeWifi);
+  }
 }
 
 ////////////////////////////////////////////////////////////////////////////////

chrome/browser/chromeos/ui/choose_mobile_network_dialog.h

@@ -16,6 +16,8 @@ class ChooseMobileNetworkDialog : public ui::WebDialogDelegate {
  public:
   // Shows the dialog box. The dialog is created as a child of |parent|. If the
   // |parent| is null the dialog is created in the default modal container.
+  // TODO(jamescook): This should not need a |parent|. See similar methods in
+  // NetworkConfigView.
   static void ShowDialog(gfx::NativeWindow parent);
 
   // Shows the dialog in an ash window container (which must be a system modal

chrome/browser/ui/ash/system_tray_client.cc

@@ -211,8 +211,8 @@ Widget* SystemTrayClient::CreateUnownedDialogWidget(
     params.mus_properties[WindowManager::kContainerId_InitProperty] =
         mojo::ConvertTo<std::vector<uint8_t>>(container_id);
   } else {
-    params.parent = ash::Shell::GetContainer(ash::Shell::GetPrimaryRootWindow(),
-                                             container_id);
+    params.parent = ash::Shell::GetContainer(
+        ash::Shell::GetRootWindowForNewWindows(), container_id);
   }
   Widget* widget = new Widget;  // Owned by native widget.
   widget->Init(params);
@@ -370,9 +370,7 @@ void SystemTrayClient::ShowNetworkConfigure(const std::string& network_id) {
     return;
   }
 
-  // Dialog will default to the primary display.
-  chromeos::NetworkConfigView::ShowForNetworkId(network_id,
-                                                nullptr /* parent */);
+  chromeos::NetworkConfigView::ShowForNetworkId(network_id);
 }
 
 void SystemTrayClient::ShowNetworkCreate(const std::string& type) {
@@ -381,7 +379,7 @@ void SystemTrayClient::ShowNetworkCreate(const std::string& type) {
     chromeos::ChooseMobileNetworkDialog::ShowDialogInContainer(container_id);
     return;
   }
-  chromeos::NetworkConfigView::ShowForType(type, nullptr /* parent */);
+  chromeos::NetworkConfigView::ShowForType(type);
 }
 
 void SystemTrayClient::ShowThirdPartyVpnCreate(

chrome/browser/ui/webui/chromeos/login/network_dropdown_handler.cc

@@ -79,8 +79,7 @@ void NetworkDropdownHandler::HandleLaunchProxySettingsDialog() {
 }
 
 void NetworkDropdownHandler::HandleLaunchAddWiFiNetworkDialog() {
-  gfx::NativeWindow native_window = GetNativeWindow();
-  NetworkConfigView::ShowForType(shill::kTypeWifi, native_window);
+  NetworkConfigView::ShowForType(shill::kTypeWifi);
 }
 
 void NetworkDropdownHandler::HandleLaunchAddMobileNetworkDialog() {

chrome/browser/ui/webui/chromeos/network_ui.cc

@@ -144,8 +144,7 @@ class NetworkConfigMessageHandler : public content::WebUIMessageHandler {
     std::string shill_type = (onc_type == ::onc::network_type::kVPN)
                                  ? shill::kTypeVPN
                                  : shill::kTypeWifi;
-    NetworkConfigView::ShowForType(
-        shill_type, web_ui()->GetWebContents()->GetTopLevelNativeWindow());
+    NetworkConfigView::ShowForType(shill_type);
   }
 
   base::WeakPtrFactory<NetworkConfigMessageHandler> weak_ptr_factory_;

chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc

@@ -183,7 +183,7 @@ const PrefService* InternetOptionsHandler::GetPrefs() const {
 void InternetOptionsHandler::AddVPNConnection(const base::ListValue* args) {
   if (args->empty()) {
     // Show the "add network" dialog for the built-in OpenVPN/L2TP provider.
-    NetworkConfigView::ShowForType(shill::kTypeVPN, GetNativeWindow());
+    NetworkConfigView::ShowForType(shill::kTypeVPN);
     return;
   }
 
@@ -206,7 +206,7 @@ void InternetOptionsHandler::AddNonVPNConnection(const base::ListValue* args) {
     return;
   }
   if (onc_type == ::onc::network_type::kWiFi) {
-    NetworkConfigView::ShowForType(shill::kTypeWifi, GetNativeWindow());
+    NetworkConfigView::ShowForType(shill::kTypeWifi);
   } else if (onc_type == ::onc::network_type::kCellular) {
     ChooseMobileNetworkDialog::ShowDialog(GetNativeWindow());
   } else {
@@ -246,7 +246,7 @@ void InternetOptionsHandler::ConfigureNetwork(const base::ListValue* args) {
     return;
   }
 
-  NetworkConfigView::ShowForNetworkId(guid, GetNativeWindow());
+  NetworkConfigView::ShowForNetworkId(guid);
 }
 
 }  // namespace options

chrome/browser/ui/webui/settings/chromeos/internet_handler.cc

@@ -74,7 +74,7 @@ void InternetHandler::AddNetwork(const base::ListValue* args) {
       args->GetString(1, &extension_id);
     if (extension_id.empty()) {
       // Show the "add network" dialog for the built-in OpenVPN/L2TP provider.
-      NetworkConfigView::ShowForType(shill::kTypeVPN, GetNativeWindow());
+      NetworkConfigView::ShowForType(shill::kTypeVPN);
       return;
     }
     // Request that the third-party VPN provider identified by |provider_id|
@@ -82,7 +82,7 @@ void InternetHandler::AddNetwork(const base::ListValue* args) {
     VpnServiceFactory::GetForBrowserContext(GetProfileForPrimaryUser())
         ->SendShowAddDialogToExtension(extension_id);
   } else if (onc_type == ::onc::network_type::kWiFi) {
-    NetworkConfigView::ShowForType(shill::kTypeWifi, GetNativeWindow());
+    NetworkConfigView::ShowForType(shill::kTypeWifi);
   } else if (onc_type == ::onc::network_type::kCellular) {
     ChooseMobileNetworkDialog::ShowDialog(GetNativeWindow());
   } else {
@@ -121,7 +121,7 @@ void InternetHandler::ConfigureNetwork(const base::ListValue* args) {
     return;
   }
 
-  NetworkConfigView::ShowForNetworkId(network->guid(), GetNativeWindow());
+  NetworkConfigView::ShowForNetworkId(network->guid());
 }
 
 gfx::NativeWindow InternetHandler::GetNativeWindow() const {

chrome/test/BUILD.gn

@@ -2274,6 +2274,7 @@ test("browser_tests") {
         "../browser/chromeos/login/webview_login_browsertest.cc",
         "../browser/chromeos/login/wizard_controller_browsertest.cc",
         "../browser/chromeos/net/network_portal_detector_impl_browsertest.cc",
+        "../browser/chromeos/options/network_config_view_browsertest.cc",
         "../browser/chromeos/policy/affiliation_test_helper.cc",
         "../browser/chromeos/policy/affiliation_test_helper.h",
         "../browser/chromeos/policy/blocking_login_browsertest.cc",