GTK4: Fix event processing after closing dialogs

In GTK4, the GDestroyNotify that SelectFileDialogImplGtk used does not get fired
when the GtkFileChooser is closed (perhaps GTK is now doing some lazy
deletion?).  But the "destroy" signal does get fired, so move the event
re-enabling code there.

Also clean up some messy code.

BUG=876586
R=sky

Change-Id: I2d78ff292d167fe526bd5fb35aaf6296d32da1af
Reviewed-on: https://chromium-review.googlesource.com/1188585Reviewed-by: Scott Violet <sky@chromium.org>
Commit-Queue: Thomas Anderson <thomasanderson@chromium.org>
Cr-Commit-Position: refs/heads/master@{#586078}

chrome/browser/ui/libgtkui/gtk_util.cc

@@ -31,8 +31,6 @@
 
 namespace {
 
-const char kAuraTransientParent[] = "aura-transient-parent";
-
 void CommonInitFromCommandLine(const base::CommandLine& command_line) {
 #if GTK_CHECK_VERSION(3, 90, 0)
   gtk_init();
@@ -187,19 +185,6 @@ void SetGtkTransientForAura(GtkWidget* dialog, aura::Window* parent) {
   XSetTransientForHint(GDK_WINDOW_XDISPLAY(gdk_window),
                        GDK_WINDOW_XID(gdk_window),
                        parent->GetHost()->GetAcceleratedWidget());
-
-  // We also set the |parent| as a property of |dialog|, so that we can unlink
-  // the two later.
-  g_object_set_data(G_OBJECT(dialog), kAuraTransientParent, parent);
-}
-
-aura::Window* GetAuraTransientParent(GtkWidget* dialog) {
-  return reinterpret_cast<aura::Window*>(
-      g_object_get_data(G_OBJECT(dialog), kAuraTransientParent));
-}
-
-void ClearAuraTransientParent(GtkWidget* dialog) {
-  g_object_set_data(G_OBJECT(dialog), kAuraTransientParent, nullptr);
 }
 
 void ParseButtonLayout(const std::string& button_string,

chrome/browser/ui/libgtkui/gtk_util.h

@@ -75,12 +75,6 @@ void TurnButtonBlue(GtkWidget* button);
 // it above |parent|. Do nothing if |parent| is nullptr.
 void SetGtkTransientForAura(GtkWidget* dialog, aura::Window* parent);
 
-// Gets the transient parent aura window for |dialog|.
-aura::Window* GetAuraTransientParent(GtkWidget* dialog);
-
-// Clears the transient parent for |dialog|.
-void ClearAuraTransientParent(GtkWidget* dialog);
-
 // Parses |button_string| into |leading_buttons| and
 // |trailing_buttons|.  The string is of the format
 // "<button>*:<button*>", for example, "close:minimize:maximize".

chrome/browser/ui/libgtkui/print_dialog_gtk.cc

@@ -182,17 +182,15 @@ PrintDialogGtk::PrintDialogGtk(PrintingContextLinux* context)
       dialog_(nullptr),
       gtk_settings_(nullptr),
       page_setup_(nullptr),
-      printer_(nullptr) {}
+      printer_(nullptr),
+      parent_(nullptr) {}
 
 PrintDialogGtk::~PrintDialogGtk() {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
 
   if (dialog_) {
-    aura::Window* parent = libgtkui::GetAuraTransientParent(dialog_);
-    if (parent) {
-      parent->RemoveObserver(this);
-      libgtkui::ClearAuraTransientParent(dialog_);
-    }
+    if (parent_)
+      parent_->RemoveObserver(this);
     gtk_widget_destroy(dialog_);
     dialog_ = nullptr;
   }
@@ -325,6 +323,7 @@ void PrintDialogGtk::ShowDialog(
 
   dialog_ = gtk_print_unix_dialog_new(nullptr, nullptr);
   libgtkui::SetGtkTransientForAura(dialog_, parent_view);
+  parent_ = parent_view;
   if (parent_view)
     parent_view->AddObserver(this);
   g_signal_connect(dialog_, "delete-event",
@@ -537,9 +536,8 @@ void PrintDialogGtk::InitPrintSettings(PrintSettings* settings) {
 }
 
 void PrintDialogGtk::OnWindowDestroying(aura::Window* window) {
-  DCHECK_EQ(libgtkui::GetAuraTransientParent(dialog_), window);
-
-  libgtkui::ClearAuraTransientParent(dialog_);
+  DCHECK_EQ(parent_, window);
+  parent_ = nullptr;
   window->RemoveObserver(this);
   if (!callback_.is_null())
     std::move(callback_).Run(PrintingContextLinux::CANCEL);

chrome/browser/ui/libgtkui/print_dialog_gtk.h

@@ -84,6 +84,8 @@ class PrintDialogGtk : public printing::PrintDialogGtkInterface,
   GtkPageSetup* page_setup_;
   GtkPrinter* printer_;
 
+  aura::Window* parent_;
+
   base::FilePath path_to_pdf_;
 
   DISALLOW_COPY_AND_ASSIGN(PrintDialogGtk);

chrome/browser/ui/libgtkui/select_file_dialog_impl_gtk.cc

@@ -63,12 +63,6 @@ void OnFileFilterDataDestroyed(std::string* file_extension) {
   delete file_extension;
 }
 
-// Runs DesktopWindowTreeHostX11::EnableEventListening() when the file-picker
-// is closed.
-void OnFilePickerDestroy(base::Closure* callback) {
-  callback->Run();
-}
-
 }  // namespace
 
 namespace libgtkui {
@@ -93,17 +87,24 @@ SelectFileDialogImplGTK::SelectFileDialogImplGTK(
     : SelectFileDialogImpl(listener, std::move(policy)), preview_(nullptr) {}
 
 SelectFileDialogImplGTK::~SelectFileDialogImplGTK() {
-  for (std::set<aura::Window*>::iterator iter = parents_.begin();
-       iter != parents_.end(); ++iter) {
-    (*iter)->RemoveObserver(this);
-  }
-  while (dialogs_.begin() != dialogs_.end()) {
-    gtk_widget_destroy(*(dialogs_.begin()));
-  }
+  // gtk_widget_destroy() causes OnFileChooserDestroy() to run, which erases the
+  // dialog in |dialogs_|.  To prevent |dialogs_| from being modified while
+  // iterating over it, it is necessary to make a copy of its GtkWidgets.
+  std::vector<GtkWidget*> dialogs;
+  dialogs.reserve(dialogs_.size());
+  for (auto& pair : dialogs_)
+    dialogs.push_back(pair.first);
+  for (GtkWidget* dialog : dialogs)
+    gtk_widget_destroy(dialog);
+  DCHECK(dialogs_.empty());
 }
 
 bool SelectFileDialogImplGTK::IsRunning(gfx::NativeWindow parent_window) const {
-  return parents_.find(parent_window) != parents_.end();
+  for (auto& pair : dialogs_) {
+    if (pair.second->parent == parent_window)
+      return true;
+  }
+  return false;
 }
 
 bool SelectFileDialogImplGTK::HasMultipleFileTypeChoicesImpl() {
@@ -112,17 +113,11 @@ bool SelectFileDialogImplGTK::HasMultipleFileTypeChoicesImpl() {
 
 void SelectFileDialogImplGTK::OnWindowDestroying(aura::Window* window) {
   // Remove the |parent| property associated with the |dialog|.
-  for (std::set<GtkWidget*>::iterator it = dialogs_.begin();
-       it != dialogs_.end(); ++it) {
-    aura::Window* parent = GetAuraTransientParent(*it);
-    if (parent == window)
-      ClearAuraTransientParent(*it);
-  }
-
-  std::set<aura::Window*>::iterator iter = parents_.find(window);
-  if (iter != parents_.end()) {
-    (*iter)->RemoveObserver(this);
-    parents_.erase(iter);
+  for (auto& pair : dialogs_) {
+    if (pair.second->parent == window) {
+      pair.second->parent = nullptr;
+      window->RemoveObserver(this);
+    }
   }
 }
 
@@ -136,10 +131,12 @@ void SelectFileDialogImplGTK::SelectFileImpl(
     const base::FilePath::StringType& default_extension,
     gfx::NativeWindow owning_window,
     void* params) {
+  std::unique_ptr<WidgetData> widget_data = std::make_unique<WidgetData>();
+
   type_ = type;
   if (owning_window) {
     owning_window->AddObserver(this);
-    parents_.insert(owning_window);
+    widget_data->parent = owning_window;
   }
 
   std::string title_string = base::UTF16ToUTF8(title);
@@ -172,7 +169,6 @@ void SelectFileDialogImplGTK::SelectFileImpl(
   }
   g_signal_connect(dialog, "delete-event",
                    G_CALLBACK(gtk_widget_hide_on_delete), nullptr);
-  dialogs_.insert(dialog);
 
   preview_ = gtk_image_new();
   g_signal_connect(dialog, "destroy", G_CALLBACK(OnFileChooserDestroyThunk),
@@ -181,7 +177,7 @@ void SelectFileDialogImplGTK::SelectFileImpl(
                    this);
   gtk_file_chooser_set_preview_widget(GTK_FILE_CHOOSER(dialog), preview_);
 
-  params_map_[dialog] = params;
+  widget_data->params = params;
 
   // Disable input events handling in the host window to make this dialog modal.
   if (owning_window) {
@@ -191,20 +187,16 @@ void SelectFileDialogImplGTK::SelectFileImpl(
       // been captured and by turning off event listening, it is never
       // released. So we manually ensure there is no current capture.
       host->ReleaseCapture();
-      std::unique_ptr<base::Closure> callback =
+      widget_data->enable_event_listening =
           views::DesktopWindowTreeHostX11::GetHostForXID(
               host->GetAcceleratedWidget())
               ->DisableEventListening();
-      // OnFilePickerDestroy() is called when |dialog| destroyed, which allows
-      // to invoke the callback function to re-enable event handling on the
-      // owning window.
-      g_object_set_data_full(
-          G_OBJECT(dialog), "callback", callback.release(),
-          reinterpret_cast<GDestroyNotify>(OnFilePickerDestroy));
       gtk_window_set_modal(GTK_WINDOW(dialog), TRUE);
     }
   }
 
+  dialogs_[dialog] = std::move(widget_data);
+
 #if !GTK_CHECK_VERSION(3, 90, 0)
   gtk_widget_show_all(dialog);
 #endif
@@ -289,7 +281,7 @@ void SelectFileDialogImplGTK::FileSelected(GtkWidget* dialog,
     GSList* filters = gtk_file_chooser_list_filters(GTK_FILE_CHOOSER(dialog));
     int idx = g_slist_index(filters, selected_filter);
     g_slist_free(filters);
-    listener_->FileSelected(path, idx + 1, PopParamsForDialog(dialog));
+    listener_->FileSelected(path, idx + 1, GetParamsForDialog(dialog));
   }
   gtk_widget_destroy(dialog);
 }
@@ -300,12 +292,12 @@ void SelectFileDialogImplGTK::MultiFilesSelected(
   *last_opened_path_ = files[0].DirName();
 
   if (listener_)
-    listener_->MultiFilesSelected(files, PopParamsForDialog(dialog));
+    listener_->MultiFilesSelected(files, GetParamsForDialog(dialog));
   gtk_widget_destroy(dialog);
 }
 
 void SelectFileDialogImplGTK::FileNotSelected(GtkWidget* dialog) {
-  void* params = PopParamsForDialog(dialog);
+  void* params = GetParamsForDialog(dialog);
   if (listener_)
     listener_->FileSelectionCanceled(params);
   gtk_widget_destroy(dialog);
@@ -455,12 +447,9 @@ GtkWidget* SelectFileDialogImplGTK::CreateSaveAsDialog(
   return dialog;
 }
 
-void* SelectFileDialogImplGTK::PopParamsForDialog(GtkWidget* dialog) {
-  std::map<GtkWidget*, void*>::iterator iter = params_map_.find(dialog);
-  DCHECK(iter != params_map_.end());
-  void* params = iter->second;
-  params_map_.erase(iter);
-  return params;
+void* SelectFileDialogImplGTK::GetParamsForDialog(GtkWidget* dialog) {
+  DCHECK(dialogs_.find(dialog) != dialogs_.end());
+  return dialogs_[dialog]->params;
 }
 
 bool SelectFileDialogImplGTK::IsCancelResponse(gint response_id) {
@@ -544,20 +533,13 @@ void SelectFileDialogImplGTK::OnSelectMultiFileDialogResponse(GtkWidget* dialog,
 }
 
 void SelectFileDialogImplGTK::OnFileChooserDestroy(GtkWidget* dialog) {
-  dialogs_.erase(dialog);
-
   // |parent| can be nullptr when closing the host window
   // while opening the file-picker.
-  aura::Window* parent = GetAuraTransientParent(dialog);
-  if (!parent)
-    return;
-  std::set<aura::Window*>::iterator iter = parents_.find(parent);
-  if (iter != parents_.end()) {
-    (*iter)->RemoveObserver(this);
-    parents_.erase(iter);
-  } else {
-    NOTREACHED();
-  }
+  aura::Window* parent = dialogs_[dialog]->parent;
+  if (parent)
+    parent->RemoveObserver(this);
+  std::move(*dialogs_[dialog]->enable_event_listening).Run();
+  dialogs_.erase(dialog);
 }
 
 void SelectFileDialogImplGTK::OnUpdatePreview(GtkWidget* chooser) {
@@ -591,4 +573,8 @@ void SelectFileDialogImplGTK::OnUpdatePreview(GtkWidget* chooser) {
                                              pixbuf ? TRUE : FALSE);
 }
 
+SelectFileDialogImplGTK::WidgetData::WidgetData() {}
+
+SelectFileDialogImplGTK::WidgetData::~WidgetData() {}
+
 }  // namespace libgtkui

chrome/browser/ui/libgtkui/select_file_dialog_impl_gtk.h

@@ -5,6 +5,7 @@
 #ifndef CHROME_BROWSER_UI_LIBGTKUI_SELECT_FILE_DIALOG_IMPL_GTK_H_
 #define CHROME_BROWSER_UI_LIBGTKUI_SELECT_FILE_DIALOG_IMPL_GTK_H_
 
+#include "base/containers/flat_map.h"
 #include "base/macros.h"
 #include "chrome/browser/ui/libgtkui/gtk_signal.h"
 #include "chrome/browser/ui/libgtkui/gtk_util.h"
@@ -39,6 +40,20 @@ class SelectFileDialogImplGTK : public SelectFileDialogImpl,
 
  private:
   friend class FilePicker;
+
+  struct WidgetData {
+    WidgetData();
+    ~WidgetData();
+
+    // User data that we pass back to |listener_| once the result of the select
+    // file/folder action is known.
+    void* params = nullptr;
+
+    aura::Window* parent = nullptr;
+
+    std::unique_ptr<base::OnceClosure> enable_event_listening;
+  };
+
   bool HasMultipleFileTypeChoicesImpl() override;
 
   // Overridden from aura::WindowObserver:
@@ -76,9 +91,8 @@ class SelectFileDialogImplGTK : public SelectFileDialogImpl,
                                 const base::FilePath& default_path,
                                 gfx::NativeWindow parent);
 
-  // Removes and returns the |params| associated with |dialog| from
-  // |params_map_|.
-  void* PopParamsForDialog(GtkWidget* dialog);
+  // Returns the |params| associated with |dialog|.
+  void* GetParamsForDialog(GtkWidget* dialog);
 
   // Check whether response_id corresponds to the user cancelling/closing the
   // dialog. Used as a helper for the below callbacks.
@@ -119,17 +133,11 @@ class SelectFileDialogImplGTK : public SelectFileDialogImpl,
   // Callback for when we update the preview for the selection.
   CHROMEGTK_CALLBACK_0(SelectFileDialogImplGTK, void, OnUpdatePreview);
 
-  // A map from dialog windows to the |params| user data associated with them.
-  std::map<GtkWidget*, void*> params_map_;
-
   // The GtkImage widget for showing previews of selected images.
   GtkWidget* preview_;
 
   // All our dialogs.
-  std::set<GtkWidget*> dialogs_;
-
-  // The set of all parent windows for which we are currently running dialogs.
-  std::set<aura::Window*> parents_;
+  base::flat_map<GtkWidget*, std::unique_ptr<WidgetData>> dialogs_;
 
   DISALLOW_COPY_AND_ASSIGN(SelectFileDialogImplGTK);
 };

chrome/browser/ui/libgtkui/select_file_dialog_impl_gtk_unittest.cc

@@ -55,7 +55,7 @@ class FilePicker : public ui::SelectFileDialog::Listener {
         static_cast<SelectFileDialogImplGTK*>(select_file_dialog_.get());
 
     while (!file_dialog->dialogs_.empty())
-      gtk_widget_destroy(*(file_dialog->dialogs_.begin()));
+      gtk_widget_destroy(file_dialog->dialogs_.begin()->first);
 
     select_file_dialog_->ListenerDestroyed();
   }
@@ -85,7 +85,7 @@ class FilePicker : public ui::SelectFileDialog::Listener {
   GtkFileChooser* getChooser() {
     auto* dialog =
         static_cast<SelectFileDialogImplGTK*>(select_file_dialog_.get());
-    return GTK_FILE_CHOOSER(*(dialog->dialogs_.begin()));
+    return GTK_FILE_CHOOSER(dialog->dialogs_.begin()->first);
   }
 
   DISALLOW_COPY_AND_ASSIGN(FilePicker);

chrome/browser/ui/libgtkui/select_file_dialog_interactive_uitest.cc

@@ -51,7 +51,7 @@ class FilePicker : public ui::SelectFileDialog::Listener {
 
 
     while (!file_dialog->dialogs_.empty())
-      gtk_widget_destroy(*(file_dialog->dialogs_.begin()));
+      gtk_widget_destroy(file_dialog->dialogs_.begin()->first);
   }
 
   // SelectFileDialog::Listener implementation.

ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc

@@ -2362,7 +2362,7 @@ gfx::Rect DesktopWindowTreeHostX11::ToPixelRect(
   return gfx::ToEnclosingRect(rect_in_pixels);
 }
 
-std::unique_ptr<base::Closure>
+std::unique_ptr<base::OnceClosure>
 DesktopWindowTreeHostX11::DisableEventListening() {
   // Allows to open multiple file-pickers. See https://crbug.com/678982
   modal_dialog_counter_++;
@@ -2374,9 +2374,9 @@ DesktopWindowTreeHostX11::DisableEventListening() {
         std::unique_ptr<ui::EventTargeter>(new ui::NullEventTargeter)));
   }
 
-  return std::make_unique<base::Closure>(
-      base::Bind(&DesktopWindowTreeHostX11::EnableEventListening,
-                 weak_factory_.GetWeakPtr()));
+  return std::make_unique<base::OnceClosure>(
+      base::BindOnce(&DesktopWindowTreeHostX11::EnableEventListening,
+                     weak_factory_.GetWeakPtr()));
 }
 
 void DesktopWindowTreeHostX11::EnableEventListening() {

ui/views/widget/desktop_aura/desktop_window_tree_host_x11.h

@@ -86,7 +86,7 @@ class VIEWS_EXPORT DesktopWindowTreeHostX11
   static void CleanUpWindowList(void (*func)(aura::Window* window));
 
   // Disables event listening to make |dialog| modal.
-  std::unique_ptr<base::Closure> DisableEventListening();
+  std::unique_ptr<base::OnceClosure> DisableEventListening();
 
   // Returns a map of KeyboardEvent code to KeyboardEvent key values.
   base::flat_map<std::string, std::string> GetKeyboardLayoutMap() override;