SystemWebDialogDelegate: Track instances

This CL tracks SystemWebDialogDelegate instances so that derived
classes can check for an existing instance and focus that instead
of opening a new instance.

Bug: 855344
Change-Id: I688f2e09bd280a511d4ab53463beccfdd10f5371
Reviewed-on: https://chromium-review.googlesource.com/c/1366793Reviewed-by: James Cook <jamescook@chromium.org>
Reviewed-by: Toni Baržić <tbarzic@chromium.org>
Commit-Queue: Steven Bennetts <stevenjb@chromium.org>
Cr-Commit-Position: refs/heads/master@{#616061}

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

@@ -28,7 +28,9 @@ void AddBluetoothStrings(content::WebUIDataSource* html_source) {
     const char* name;
     int id;
   } localized_strings[] = {
-      {"ok", IDS_OK}, {"cancel", IDS_CANCEL}, {"close", IDS_CLOSE},
+      {"ok", IDS_OK},
+      {"cancel", IDS_CANCEL},
+      {"close", IDS_CLOSE},
   };
   for (const auto& entry : localized_strings)
     html_source->AddLocalizedString(entry.name, entry.id);
@@ -38,7 +40,7 @@ void AddBluetoothStrings(content::WebUIDataSource* html_source) {
 }  // namespace
 
 // static
-BluetoothPairingDialog* BluetoothPairingDialog::ShowDialog(
+SystemWebDialogDelegate* BluetoothPairingDialog::ShowDialog(
     const std::string& address,
     const base::string16& name_for_display,
     bool paired,
@@ -49,6 +51,12 @@ BluetoothPairingDialog* BluetoothPairingDialog::ShowDialog(
     LOG(ERROR) << "BluetoothPairingDialog: Invalid address: " << address;
     return nullptr;
   }
+  auto* instance = SystemWebDialogDelegate::FindInstance(cannonical_address);
+  if (instance) {
+    instance->Focus();
+    return instance;
+  }
+
   BluetoothPairingDialog* dialog = new BluetoothPairingDialog(
       cannonical_address, name_for_display, paired, connected);
   dialog->ShowSystemDialog();
@@ -62,7 +70,8 @@ BluetoothPairingDialog::BluetoothPairingDialog(
     bool connected)
     : SystemWebDialogDelegate(
           GURL(chrome::kChromeUIBluetoothPairingURL),
-          l10n_util::GetStringUTF16(IDS_SETTINGS_BLUETOOTH_PAIR_DEVICE_TITLE)) {
+          l10n_util::GetStringUTF16(IDS_SETTINGS_BLUETOOTH_PAIR_DEVICE_TITLE)),
+      address_(address) {
   device_data_.SetString("address", address);
   device_data_.SetString("name", name_for_display);
   device_data_.SetBoolean("paired", paired);
@@ -71,6 +80,10 @@ BluetoothPairingDialog::BluetoothPairingDialog(
 
 BluetoothPairingDialog::~BluetoothPairingDialog() = default;
 
+const std::string& BluetoothPairingDialog::Id() {
+  return address_;
+}
+
 void BluetoothPairingDialog::GetDialogSize(gfx::Size* size) const {
   size->SetSize(SystemWebDialogDelegate::kDialogWidth,
                 kBluetoothPairingDialogHeight);
@@ -95,9 +108,9 @@ BluetoothPairingDialogUI::BluetoothPairingDialogUI(content::WebUI* web_ui)
 #if BUILDFLAG(OPTIMIZE_WEBUI)
   source->UseGzip();
   source->SetDefaultResource(
-      base::FeatureList::IsEnabled(features::kWebUIPolymer2) ?
-          IDR_BLUETOOTH_PAIRING_DIALOG_VULCANIZED_P2_HTML :
-          IDR_BLUETOOTH_PAIRING_DIALOG_VULCANIZED_HTML);
+      base::FeatureList::IsEnabled(features::kWebUIPolymer2)
+          ? IDR_BLUETOOTH_PAIRING_DIALOG_VULCANIZED_P2_HTML
+          : IDR_BLUETOOTH_PAIRING_DIALOG_VULCANIZED_HTML);
   source->AddResourcePath("crisper.js",
                           IDR_BLUETOOTH_PAIRING_DIALOG_CRISPER_JS);
 #else

chrome/browser/ui/webui/chromeos/bluetooth_pairing_dialog.h

@@ -15,7 +15,7 @@ namespace chromeos {
 class BluetoothPairingDialog : public SystemWebDialogDelegate {
  public:
   // Shows a bluetooth pairing dialog. The dialog is returned for testing.
-  static BluetoothPairingDialog* ShowDialog(
+  static SystemWebDialogDelegate* ShowDialog(
       const std::string& address,
       const base::string16& name_for_display,
       bool paired,
@@ -28,11 +28,15 @@ class BluetoothPairingDialog : public SystemWebDialogDelegate {
                          bool connected);
   ~BluetoothPairingDialog() override;
 
+  // SystemWebDialogDelegate
+  const std::string& Id() override;
+
   // ui::WebDialogDelegate
   void GetDialogSize(gfx::Size* size) const override;
   std::string GetDialogArgs() const override;
 
  private:
+  std::string address_;
   base::DictionaryValue device_data_;
 
   DISALLOW_COPY_AND_ASSIGN(BluetoothPairingDialog);

chrome/browser/ui/webui/chromeos/bluetooth_pairing_dialog_browsertest-inl.h

@@ -51,7 +51,7 @@ void BluetoothPairingDialogTest::ShowDialog() {
   EXPECT_CALL(*mock_adapter_, GetDevice(testing::_))
       .WillOnce(testing::Return(mock_device_.get()));
 
-  chromeos::BluetoothPairingDialog* dialog =
+  chromeos::SystemWebDialogDelegate* dialog =
       chromeos::BluetoothPairingDialog::ShowDialog(
           mock_device_->GetAddress(), mock_device_->GetNameForDisplay(),
           mock_device_->IsPaired(), mock_device_->IsConnected());

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

@@ -48,6 +48,16 @@ void AddInternetStrings(content::WebUIDataSource* html_source) {
     html_source->AddLocalizedString(entry.name, entry.id);
 }
 
+std::string GetId(const std::string& network_type,
+                  const std::string& network_id) {
+  std::string result = chrome::kChromeUIIntenetConfigDialogURL + network_type;
+  if (!network_id.empty()) {
+    result += ".";
+    result += network_id;
+  }
+  return result;
+}
+
 }  // namespace
 
 // static
@@ -62,26 +72,46 @@ void InternetConfigDialog::ShowDialogForNetworkId(
   }
   std::string network_type =
       chromeos::network_util::TranslateShillTypeToONC(network_state->type());
+  std::string id = GetId(network_type, network_id);
+  auto* instance = SystemWebDialogDelegate::FindInstance(id);
+  if (instance) {
+    instance->Focus();
+    return;
+  }
+
   InternetConfigDialog* dialog =
-      new InternetConfigDialog(network_type, network_id);
+      new InternetConfigDialog(id, network_type, network_id);
   dialog->ShowSystemDialog();
 }
 
 // static
 void InternetConfigDialog::ShowDialogForNetworkType(
     const std::string& network_type) {
-  InternetConfigDialog* dialog = new InternetConfigDialog(network_type, "");
+  std::string id = GetId(network_type, "");
+  auto* instance = SystemWebDialogDelegate::FindInstance(id);
+  if (instance) {
+    instance->Focus();
+    return;
+  }
+
+  InternetConfigDialog* dialog = new InternetConfigDialog(id, network_type, "");
   dialog->ShowSystemDialog();
 }
 
-InternetConfigDialog::InternetConfigDialog(const std::string& network_type,
+InternetConfigDialog::InternetConfigDialog(const std::string& dialog_id,
+                                           const std::string& network_type,
                                            const std::string& network_id)
     : SystemWebDialogDelegate(GURL(chrome::kChromeUIIntenetConfigDialogURL),
                               base::string16() /* title */),
+      dialog_id_(dialog_id),
       network_type_(network_type),
       network_id_(network_id) {}
 
-InternetConfigDialog::~InternetConfigDialog() {}
+InternetConfigDialog::~InternetConfigDialog() = default;
+
+const std::string& InternetConfigDialog::Id() {
+  return dialog_id_;
+}
 
 void InternetConfigDialog::GetDialogSize(gfx::Size* size) const {
   const NetworkState* network =
@@ -117,9 +147,9 @@ InternetConfigDialogUI::InternetConfigDialogUI(content::WebUI* web_ui)
 #if BUILDFLAG(OPTIMIZE_WEBUI)
   source->UseGzip();
   source->SetDefaultResource(
-      base::FeatureList::IsEnabled(features::kWebUIPolymer2) ?
-          IDR_INTERNET_CONFIG_DIALOG_VULCANIZED_P2_HTML :
-          IDR_INTERNET_CONFIG_DIALOG_VULCANIZED_HTML);
+      base::FeatureList::IsEnabled(features::kWebUIPolymer2)
+          ? IDR_INTERNET_CONFIG_DIALOG_VULCANIZED_P2_HTML
+          : IDR_INTERNET_CONFIG_DIALOG_VULCANIZED_HTML);
   source->AddResourcePath("crisper.js", IDR_INTERNET_CONFIG_DIALOG_CRISPER_JS);
 #else
   source->SetDefaultResource(IDR_INTERNET_CONFIG_DIALOG_HTML);

chrome/browser/ui/webui/chromeos/internet_config_dialog.h

@@ -25,15 +25,22 @@ class InternetConfigDialog : public SystemWebDialogDelegate {
   static void ShowDialogForNetworkType(const std::string& network_type);
 
  protected:
-  InternetConfigDialog(const std::string& network_type,
+  // |dialog_id| provides a pre-calculated identifier for the dialog based on
+  // the network type and the network id.
+  InternetConfigDialog(const std::string& dialog_id,
+                       const std::string& network_type,
                        const std::string& network_id);
   ~InternetConfigDialog() override;
 
+  // SystemWebDialogDelegate
+  const std::string& Id() override;
+
   // ui::WebDialogDelegate
   void GetDialogSize(gfx::Size* size) const override;
   std::string GetDialogArgs() const override;
 
  private:
+  std::string dialog_id_;
   std::string network_type_;
   std::string network_id_;
 

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

@@ -88,6 +88,12 @@ void InternetDetailDialog::ShowDialog(const std::string& network_id) {
     LOG(ERROR) << "Network not found: " << network_id;
     return;
   }
+  auto* instance = SystemWebDialogDelegate::FindInstance(network->guid());
+  if (instance) {
+    instance->Focus();
+    return;
+  }
+
   InternetDetailDialog* dialog = new InternetDetailDialog(*network);
   dialog->ShowSystemDialog();
 }
@@ -105,6 +111,10 @@ InternetDetailDialog::~InternetDetailDialog() {
   --s_internet_detail_dialog_count;
 }
 
+const std::string& InternetDetailDialog::Id() {
+  return network_id_;
+}
+
 void InternetDetailDialog::GetDialogSize(gfx::Size* size) const {
   size->SetSize(kInternetDetailDialogWidth,
                 SystemWebDialogDelegate::kDialogHeight);
@@ -133,9 +143,9 @@ InternetDetailDialogUI::InternetDetailDialogUI(content::WebUI* web_ui)
 #if BUILDFLAG(OPTIMIZE_WEBUI)
   source->UseGzip();
   source->SetDefaultResource(
-      base::FeatureList::IsEnabled(features::kWebUIPolymer2) ?
-          IDR_INTERNET_DETAIL_DIALOG_VULCANIZED_P2_HTML :
-          IDR_INTERNET_DETAIL_DIALOG_VULCANIZED_HTML);
+      base::FeatureList::IsEnabled(features::kWebUIPolymer2)
+          ? IDR_INTERNET_DETAIL_DIALOG_VULCANIZED_P2_HTML
+          : IDR_INTERNET_DETAIL_DIALOG_VULCANIZED_HTML);
   source->AddResourcePath("crisper.js", IDR_INTERNET_DETAIL_DIALOG_CRISPER_JS);
 #else
   source->SetDefaultResource(IDR_INTERNET_DETAIL_DIALOG_HTML);

chrome/browser/ui/webui/chromeos/internet_detail_dialog.h

@@ -26,6 +26,9 @@ class InternetDetailDialog : public SystemWebDialogDelegate {
   explicit InternetDetailDialog(const NetworkState& network);
   ~InternetDetailDialog() override;
 
+  // SystemWebDialogDelegate
+  const std::string& Id() override;
+
   // ui::WebDialogDelegate
   void GetDialogSize(gfx::Size* size) const override;
   std::string GetDialogArgs() const override;

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

@@ -9,6 +9,7 @@
 #include "chrome/browser/chromeos/login/login_manager_test.h"
 #include "chrome/browser/chromeos/login/startup_utils.h"
 #include "chrome/common/webui_url_constants.h"
+#include "chrome/test/base/in_process_browser_test.h"
 #include "components/account_id/account_id.h"
 #include "content/public/common/service_manager_connection.h"
 #include "services/service_manager/public/cpp/connector.h"
@@ -41,45 +42,50 @@ bool IsSystemModalWindowOpen() {
   return modal_open;
 }
 
-class SystemWebDialogTest : public LoginManagerTest {
- public:
-  SystemWebDialogTest()
-      : LoginManagerTest(false, true /* should_initialize_webui */) {}
-  ~SystemWebDialogTest() override = default;
-
- private:
-  DISALLOW_COPY_AND_ASSIGN(SystemWebDialogTest);
-};
-
 class MockSystemWebDialog : public SystemWebDialogDelegate {
  public:
-  MockSystemWebDialog()
+  explicit MockSystemWebDialog(const char* id = nullptr)
       : SystemWebDialogDelegate(GURL(chrome::kChromeUIVersionURL),
-                                base::string16()) {}
+                                base::string16()) {
+    if (id)
+      id_ = std::string(id);
+  }
   ~MockSystemWebDialog() override = default;
 
+  const std::string& Id() override { return id_; }
   std::string GetDialogArgs() const override { return std::string(); }
 
  private:
+  std::string id_;
   DISALLOW_COPY_AND_ASSIGN(MockSystemWebDialog);
 };
 
 }  // namespace
 
+class SystemWebDialogLoginTest : public LoginManagerTest {
+ public:
+  SystemWebDialogLoginTest()
+      : LoginManagerTest(false, true /* should_initialize_webui */) {}
+  ~SystemWebDialogLoginTest() override = default;
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(SystemWebDialogLoginTest);
+};
+
 // Verifies that system dialogs are modal before login (e.g. during OOBE).
-IN_PROC_BROWSER_TEST_F(SystemWebDialogTest, ModalTest) {
+IN_PROC_BROWSER_TEST_F(SystemWebDialogLoginTest, ModalTest) {
   auto* dialog = new MockSystemWebDialog();
   dialog->ShowSystemDialog();
   EXPECT_TRUE(IsSystemModalWindowOpen());
 }
 
-IN_PROC_BROWSER_TEST_F(SystemWebDialogTest, PRE_NonModalTest) {
+IN_PROC_BROWSER_TEST_F(SystemWebDialogLoginTest, PRE_NonModalTest) {
   RegisterUser(AccountId::FromUserEmailGaiaId(kTestUser, kTestUserGaiaId));
   StartupUtils::MarkOobeCompleted();
 }
 
 // Verifies that system dialogs are not modal and always-on-top after login.
-IN_PROC_BROWSER_TEST_F(SystemWebDialogTest, NonModalTest) {
+IN_PROC_BROWSER_TEST_F(SystemWebDialogLoginTest, NonModalTest) {
   LoginUser(AccountId::FromUserEmailGaiaId(kTestUser, kTestUserGaiaId));
   auto* dialog = new MockSystemWebDialog();
   dialog->ShowSystemDialog();
@@ -91,4 +97,18 @@ IN_PROC_BROWSER_TEST_F(SystemWebDialogTest, NonModalTest) {
   EXPECT_TRUE(window_to_test->GetProperty(aura::client::kAlwaysOnTopKey));
 }
 
+using SystemWebDialogTest = InProcessBrowserTest;
+
+IN_PROC_BROWSER_TEST_F(SystemWebDialogTest, InstanceTest) {
+  const char* kDialogId = "dialog_id";
+  SystemWebDialogDelegate* dialog = new MockSystemWebDialog(kDialogId);
+  dialog->ShowSystemDialog();
+  SystemWebDialogDelegate* found_dialog =
+      SystemWebDialogDelegate::FindInstance(kDialogId);
+  EXPECT_EQ(dialog, found_dialog);
+  // Closing (deleting) the dialog causes a crash in WebDialogView when the main
+  // loop is run. TODO(stevenjb): Investigate, fix, and test closing the dialog.
+  // https://crbug.com/855344.
+}
+
 }  // namespace chromeos

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

@@ -3,14 +3,40 @@
 // found in the LICENSE file.
 
 #include "chrome/browser/ui/webui/chromeos/system_web_dialog_delegate.h"
+
+#include <list>
+
 #include "ash/public/cpp/shell_window_ids.h"
+#include "base/no_destructor.h"
+#include "base/stl_util.h"
 #include "chrome/browser/profiles/profile_manager.h"
 #include "chrome/browser/ui/views/chrome_web_dialog_view.h"
 #include "components/session_manager/core/session_manager.h"
 #include "content/public/browser/browser_context.h"
+#include "ui/aura/window.h"
 
 namespace chromeos {
 
+namespace {
+
+// Track all open system web dialog instances. This should be a small list.
+std::list<SystemWebDialogDelegate*>* GetInstances() {
+  static base::NoDestructor<std::list<SystemWebDialogDelegate*>> instances;
+  return instances.get();
+}
+
+}  // namespace
+
+// static
+SystemWebDialogDelegate* SystemWebDialogDelegate::FindInstance(
+    const std::string& id) {
+  auto* instances = GetInstances();
+  auto iter =
+      std::find_if(instances->begin(), instances->end(),
+                   [id](SystemWebDialogDelegate* i) { return i->Id() == id; });
+  return iter == instances->end() ? nullptr : *iter;
+}
+
 SystemWebDialogDelegate::SystemWebDialogDelegate(const GURL& gurl,
                                                  const base::string16& title)
     : gurl_(gurl), title_(title), modal_type_(ui::MODAL_TYPE_NONE) {
@@ -28,9 +54,26 @@ SystemWebDialogDelegate::SystemWebDialogDelegate(const GURL& gurl,
       modal_type_ = ui::MODAL_TYPE_SYSTEM;
       break;
   }
+  GetInstances()->push_back(this);
+}
+
+SystemWebDialogDelegate::~SystemWebDialogDelegate() {
+  base::EraseIf(*GetInstances(),
+                [this](SystemWebDialogDelegate* i) { return i == this; });
 }
 
-SystemWebDialogDelegate::~SystemWebDialogDelegate() {}
+const std::string& SystemWebDialogDelegate::Id() {
+  return gurl_.spec();
+}
+
+void SystemWebDialogDelegate::Focus() {
+  // Focusing a modal dialog does not make it the topmost dialog and does not
+  // enable interaction. It does however remove focus from the current dialog,
+  // preventing interaction with any dialog. TODO(stevenjb): Investigate and
+  // fix, https://crbug.com/914133.
+  if (modal_type_ == ui::MODAL_TYPE_NONE)
+    dialog_window()->Focus();
+}
 
 ui::ModalType SystemWebDialogDelegate::GetDialogModalType() const {
   return modal_type_;

chrome/browser/ui/webui/chromeos/system_web_dialog_delegate.h

@@ -24,11 +24,24 @@ namespace chromeos {
 
 class SystemWebDialogDelegate : public ui::WebDialogDelegate {
  public:
+  // Returns the instance whose Id() matches |id|. If more than one instance
+  // matches, the first matching instance created is returned.
+  static SystemWebDialogDelegate* FindInstance(const std::string& id);
+
   // |gurl| is the HTML file path for the dialog content and must be set.
   // |title| may be empty in which case ShouldShowDialogTitle() returns false.
   SystemWebDialogDelegate(const GURL& gurl, const base::string16& title);
   ~SystemWebDialogDelegate() override;
 
+  // Returns an identifier used for matching an instance in FindInstance.
+  // By default returns gurl_.spec() which should be sufficient for dialogs
+  // that only support a single instance.
+  virtual const std::string& Id();
+
+  // Focuses the dialog window. Note: No-op for modal dialogs, see
+  // implementation for details.
+  void Focus();
+
   // ui::WebDialogDelegate
   ui::ModalType GetDialogModalType() const override;
   base::string16 GetDialogTitle() const override;
@@ -58,7 +71,7 @@ class SystemWebDialogDelegate : public ui::WebDialogDelegate {
   static constexpr int kDialogHeight = 480;
 
  protected:
-  FRIEND_TEST_ALL_PREFIXES(SystemWebDialogTest, NonModalTest);
+  FRIEND_TEST_ALL_PREFIXES(SystemWebDialogLoginTest, NonModalTest);
   gfx::NativeWindow dialog_window() const { return dialog_window_; }
 
  private: