chromeos: Convert "show other network" dialog to work with mash

chrome/browser/chromeos/options/network_config_view.cc

 // Copyright (c) 2012 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 <algorithm>
 
 #include "ash/public/cpp/shell_window_ids.h"
 #include "ash/shell.h"
@@ skipping 31 matching lines @@
 #include "ui/views/layout/fill_layout.h"
 #include "ui/views/layout/layout_constants.h"
 #include "ui/views/widget/widget.h"
 
 using views::Widget;
 
 namespace chromeos {
 
 namespace {
 
+// Used to check if a network config dialog is already showing.
+NetworkConfigView* g_instance = nullptr;
+
 gfx::NativeWindow GetParentForUnhostedDialog() {
   if (LoginDisplayHost::default_host()) {
     return LoginDisplayHost::default_host()->GetNativeWindow();
   } else {
     Browser* browser = chrome::FindTabbedBrowser(
         ProfileManager::GetPrimaryUserProfile(), true);
     if (browser)
       return browser->window()->GetNativeWindow();
   }
   return nullptr;
 }
 
-// Avoid global static initializer.
-NetworkConfigView** GetActiveDialogPointer() {
-  static NetworkConfigView* active_dialog = nullptr;

[James Cook, 2016/10/24 20:14:37]

I don't think this actually saved anything, so I switched to g_instance since I
was in the file.

[stevenjb, 2016/10/24 20:20:51]

It should be valid, but g_instace is simpler.

-  return &active_dialog;
-}
-
-NetworkConfigView* GetActiveDialog() {
-  return *(GetActiveDialogPointer());
-}
-
-void SetActiveDialog(NetworkConfigView* dialog) {
-  *(GetActiveDialogPointer()) = dialog;
-}
-
 }  // namespace
 
 // static
 const int ChildNetworkConfigView::kInputFieldMinWidth = 270;
 
 NetworkConfigView::NetworkConfigView()
     : child_config_view_(nullptr),
       delegate_(nullptr),
       advanced_button_(nullptr) {
-  DCHECK(GetActiveDialog() == nullptr);
-  SetActiveDialog(this);
+  DCHECK(!g_instance);
+  g_instance = this;
 }
 
 bool NetworkConfigView::InitWithNetworkState(const NetworkState* network) {
   DCHECK(network);
   std::string service_path = network->path();
   if (network->type() == shill::kTypeWifi ||
       network->type() == shill::kTypeEthernet) {
     child_config_view_ = new WifiConfigView(this, service_path, false);
   } else if (network->type() == shill::kTypeWimax) {
     child_config_view_ = new WimaxConfigView(this, service_path);
@@ skipping 12 matching lines @@
         this, l10n_util::GetStringUTF16(
                   IDS_OPTIONS_SETTINGS_INTERNET_OPTIONS_ADVANCED_BUTTON));
   } else if (type == shill::kTypeVPN) {
     child_config_view_ = new VPNConfigView(this,
                                            "" /* service_path */);
   }
   return child_config_view_ != nullptr;
 }
 
 NetworkConfigView::~NetworkConfigView() {
-  DCHECK(GetActiveDialog() == this);
-  SetActiveDialog(nullptr);
+  DCHECK_EQ(g_instance, this);
+  g_instance = nullptr;
 }
 
 // static
 void NetworkConfigView::ShowInParent(const std::string& network_id,
                                      gfx::NativeWindow parent) {
   DCHECK(parent);
-  ShowImpl(network_id, parent, ash::kShellWindowId_Invalid);
+  NetworkConfigView* view = CreateForNetworkId(network_id);
+  if (view)
+    view->ShowDialog(parent);
 }
 
 // static
 void NetworkConfigView::ShowInContainer(const std::string& network_id,
                                         int container_id) {
   DCHECK_NE(container_id, ash::kShellWindowId_Invalid);
-  ShowImpl(network_id, nullptr, container_id);
+  NetworkConfigView* view = CreateForNetworkId(network_id);
+  if (view)
+    view->ShowDialogInContainer(container_id);
 }
 
 // static
-void NetworkConfigView::ShowImpl(const std::string& network_id,
-                                 gfx::NativeWindow parent,
-                                 int container_id) {
-  DCHECK(parent || container_id != ash::kShellWindowId_Invalid);
-  if (GetActiveDialog() != nullptr)
-    return;
+NetworkConfigView* NetworkConfigView::CreateForNetworkId(
+    const std::string& network_id) {
+  if (g_instance)
+    return nullptr;
   const NetworkState* network =
       NetworkHandler::Get()->network_state_handler()->GetNetworkStateFromGuid(
           network_id);
   if (!network) {
-    LOG(ERROR) << "NetworkConfigView::Show called with invalid network";
-    return;
+    LOG(ERROR)
+        << "NetworkConfigView::CreateForNetworkId called with invalid network";
+    return nullptr;
   }
   NetworkConfigView* view = new NetworkConfigView();
   if (!view->InitWithNetworkState(network)) {
-    LOG(ERROR) << "NetworkConfigView::Show called with invalid network type: "
+    LOG(ERROR) << "NetworkConfigView::CreateForNetworkId called with invalid "
+                  "network type: "
                << network->type();
     delete view;
-    return;
+    return nullptr;
   }
-  NET_LOG(USER) << "NetworkConfigView::Show: " << network->path();
-  if (parent)
-    view->ShowDialog(parent);
-  else
-    view->ShowDialogInContainer(container_id);
+  NET_LOG(USER) << "NetworkConfigView::CreateForNetworkId: " << network->path();
+  return view;
 }
 
 // static
 void NetworkConfigView::ShowForType(const std::string& type,
                                     gfx::NativeWindow parent) {
-  if (GetActiveDialog() != nullptr)
-    return;
+  // |parent| may be null.
+  NetworkConfigView* view = CreateForType(type);
+  if (view)
+    view->ShowDialog(parent);
+}
+
+// static
+void NetworkConfigView::ShowForTypeInContainer(const std::string& type,
+                                               int container_id) {
+  DCHECK_NE(container_id, ash::kShellWindowId_Invalid);
+  NetworkConfigView* view = CreateForType(type);
+  if (view)
+    view->ShowDialogInContainer(container_id);
+}
+
+// static
+NetworkConfigView* NetworkConfigView::CreateForType(const std::string& type) {

[James Cook, 2016/10/24 20:14:37]

I introduced a Create method instead of ShowForTypeImpl() because |parent| can
be null in ShowForType(), so ShowForTypeImpl() would not be able to test if
(parent) {} else {} to decide whether to place in a container. I converted the
other Show code path for consistency.

+  if (g_instance)
+    return nullptr;
   NetworkConfigView* view = new NetworkConfigView();
   if (!view->InitWithType(type)) {
-    LOG(ERROR) << "NetworkConfigView::ShowForType called with invalid type: "
+    LOG(ERROR) << "NetworkConfigView::CreateForType called with invalid type: "
                << type;
     delete view;
-    return;
+    return nullptr;
   }
-  NET_LOG(USER) << "NetworkConfigView::ShowForType: " << type;
-  view->ShowDialog(parent);
+  NET_LOG(USER) << "NetworkConfigView::CreateForType: " << type;
+  return view;
 }
 
 gfx::NativeWindow NetworkConfigView::GetNativeWindow() const {
   return GetWidget()->GetNativeWindow();
 }
 
 base::string16 NetworkConfigView::GetDialogButtonLabel(
     ui::DialogButton button) const {
   if (button == ui::DIALOG_BUTTON_OK) {
     if (child_config_view_->IsConfigureDialog())
@@ skipping 179 matching lines @@
 }
 
 ControlledSettingIndicatorView::~ControlledSettingIndicatorView() {}
 
 gfx::Size ControlledSettingIndicatorView::GetPreferredSize() const {
   return (managed_ && visible()) ? image_view_->GetPreferredSize()
                                  : gfx::Size();
 }
 
 }  // namespace chromeos