lacros: Fix window stacking for select file dialogs

Select file dialogs are implemented in ash-chrome via
SelectFileDialogExtension, which requires an aura::Window owning window
for the dialog.

Top-level windows in lacros-chrome are represented in ash-chrome via
ShellSurface objects. Pass an ID for the shell surface over mojo from
lacros-chrome to ash-chrome and use it for window parenting.

The ID is a string: "org.chromium.lacros.<unguessable-token>", e.g.
"org.chromium.lacros.8B93BD96131616984961C7790B5E9A62". Lacros creates
an ID for each WaylandToplevelWindow and uses xdg_shell's set_app_id
method to set it. This is similar to how ARC++ encodes an Android
task ID in the app ID, e.g. "org.chromium.arc.12345".

The ID is sent as a string to provide future flexibility (in case we
need to encode different window types, like "lacros_popup") and because
it felt like an appropriate return type for the cross-platform method
ui::PlatformWindow::GetInternalWindowId().

Feature design doc at go/lacros-select-file

Bug: 1090587
Test: multiple simultaneous file open dialogs spawn/stack/close correctly
Change-Id: I7b2cdb934480f72f5dbb3523922d30cf5df0376a
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2301119
Commit-Queue: James Cook <jamescook@chromium.org>
Reviewed-by: Sadrul Chowdhury <sadrul@chromium.org>
Reviewed-by: Robert Kroeger <rjkroege@chromium.org>
Reviewed-by: Mitsuru Oshima <oshima@chromium.org>
Reviewed-by: Luciano Pacheco <lucmult@chromium.org>
Reviewed-by: Erik Chen <erikchen@chromium.org>
Reviewed-by: Jorge Lucangeli Obes <jorgelo@chromium.org>
Cr-Commit-Position: refs/heads/master@{#791992}

chrome/browser/chromeos/crosapi/select_file_crosapi.cc

@@ -9,7 +9,6 @@
 
 #include "ash/public/cpp/shell_window_ids.h"
 #include "ash/shell.h"
-#include "ash/wm/desks/desks_util.h"
 #include "base/files/file_path.h"
 #include "base/memory/ref_counted.h"
 #include "base/numerics/ranges.h"
@@ -19,14 +18,10 @@
 #include "ui/shell_dialogs/select_file_dialog.h"
 #include "ui/shell_dialogs/select_file_policy.h"
 #include "ui/shell_dialogs/selected_file_info.h"
-#include "ui/wm/public/activation_client.h"
 #include "url/gurl.h"
 
 namespace {
 
-// TODO(https://crbug.com/1090587): Replace with window ID from Wayland client.
-int g_next_window_id = 0;
-
 ui::SelectFileDialog::Type GetUiType(
     crosapi::mojom::SelectFileDialogType type) {
   switch (type) {
@@ -57,36 +52,50 @@ ui::SelectFileDialog::FileTypeInfo::AllowedPaths GetUiAllowedPaths(
   }
 }
 
-// TODO(https://crbug.com/1090587): Parent to the ShellSurface that spawned
-// the dialog. For now, parent to the active window, which in practice should be
-// the spawning window.
-aura::Window* GetOwnerWindow() {
-  aura::Window* root = ash::Shell::GetRootWindowForNewWindows();
-  aura::Window* active = ::wm::GetActivationClient(root)->GetActiveWindow();
-  // Check that the active window is still a ShellSurface window.
-  if (active && exo::GetShellSurfaceBaseForWindow(active))
-    return active;
-  // Fallback to the active virtual desk.
-  return ash::Shell::GetContainer(root,
-                                  ash::desks_util::GetActiveDeskContainerId());
+// Performs a depth-first search for a window with a given exo ShellSurface
+// |app_id| starting at |root|.
+aura::Window* FindWindowWithShellAppId(aura::Window* root,
+                                       const std::string& app_id) {
+  const std::string* id = exo::GetShellApplicationId(root);
+  if (id && *id == app_id)
+    return root;
+  for (aura::Window* child : root->children()) {
+    aura::Window* found = FindWindowWithShellAppId(child, app_id);
+    if (found)
+      return found;
+  }
+  return nullptr;
+}
+
+// Searches all displays for a ShellSurfaceBase with |app_id| and
+// returns its aura::Window. Returns null if no such shell surface exists.
+aura::Window* GetShellSurfaceWindow(const std::string& app_id) {
+  for (aura::Window* display_root : ash::Shell::GetAllRootWindows()) {
+    aura::Window* window = FindWindowWithShellAppId(display_root, app_id);
+    if (window)
+      return window;
+  }
+  return nullptr;
 }
 
 // Manages a single open/save dialog. There may be multiple dialogs showing at
 // the same time. Deletes itself when the dialog is closed.
 class SelectFileDialogHolder : public ui::SelectFileDialog::Listener {
  public:
-  SelectFileDialogHolder(crosapi::mojom::SelectFileOptionsPtr options,
+  SelectFileDialogHolder(aura::Window* shell_surface_window,
+                         crosapi::mojom::SelectFileOptionsPtr options,
                          crosapi::mojom::SelectFile::SelectCallback callback)
       : select_callback_(std::move(callback)) {
+    DCHECK(shell_surface_window);
     // Policy is null because showing the file-dialog-blocked infobar is handled
     // client-side in lacros-chrome.
     select_file_dialog_ =
         SelectFileDialogExtension::Create(this, /*policy=*/nullptr);
 
     SelectFileDialogExtension::Owner owner;
-    owner.window = GetOwnerWindow();
-    // TODO(https://crbug.com/1090587): Replace with ID from Wayland client.
-    owner.lacros_window_id = g_next_window_id++;
+    // Parent to the ShellSurface window that spawned the dialog.
+    owner.window = shell_surface_window;
+    owner.lacros_window_id = options->owning_shell_window_id;
 
     int file_type_index = 0;
     if (options->file_types) {
@@ -191,6 +200,15 @@ SelectFileCrosapi::~SelectFileCrosapi() = default;
 
 void SelectFileCrosapi::Select(crosapi::mojom::SelectFileOptionsPtr options,
                                SelectCallback callback) {
+  aura::Window* shell_surface_window =
+      GetShellSurfaceWindow(options->owning_shell_window_id);
+  // Bail out if the shell surface doesn't exist any more.
+  if (!shell_surface_window) {
+    std::move(callback).Run(
+        crosapi::mojom::SelectFileResult::kInvalidShellWindow, {}, 0);
+    return;
+  }
   // Deletes itself when the dialog closes.
-  new SelectFileDialogHolder(std::move(options), std::move(callback));
+  new SelectFileDialogHolder(shell_surface_window, std::move(options),
+                             std::move(callback));
 }

chrome/browser/ui/views/select_file_dialog_extension.cc

@@ -169,8 +169,9 @@ SelectFileDialogExtension::RoutingID GetRoutingID(
   if (owner.android_task_id.has_value())
     return base::StringPrintf("android.%d", *owner.android_task_id);
 
+  // Lacros ids are already prefixed with "lacros".
   if (owner.lacros_window_id.has_value())
-    return base::StringPrintf("lacros.%d", *owner.lacros_window_id);
+    return *owner.lacros_window_id;
 
   if (web_contents) {
     return base::StringPrintf(

chrome/browser/ui/views/select_file_dialog_extension.h

@@ -80,7 +80,7 @@ class SelectFileDialogExtension
     // Android task ID if the owner window is an Android app.
     base::Optional<int> android_task_id;
     // Lacros window ID if the owner window is a Lacros browser.
-    base::Optional<int> lacros_window_id;
+    base::Optional<std::string> lacros_window_id;
   };
   void SelectFileWithFileManagerParams(Type type,
                                        const base::string16& title,

chromeos/crosapi/mojom/select_file.mojom

@@ -91,6 +91,9 @@ struct SelectFileOptions {
   // Optional file type configuration. In the common case for file open (Ctrl-O)
   // this is null.
   SelectFileTypeInfo? file_types;
+
+  // Internal window ID for the top-level shell window that owns the dialog.
+  string owning_shell_window_id;
 };
 
 // Result of the select file dialog.
@@ -99,7 +102,8 @@ enum SelectFileResult {
   // Everything worked.
   kSuccess = 0,
 
-  // TODO(jamescook): Error handling.
+  // Failed because the dialog couldn't find the owning top-level window.
+  kInvalidShellWindow = 1,
 };
 
 // Information about the selected file or files.

components/exo/shell_surface_util.cc

@@ -34,7 +34,9 @@ namespace {
 
 DEFINE_UI_CLASS_PROPERTY_KEY(Surface*, kMainSurfaceKey, nullptr)
 
-// Application Id set by the client.
+// Application Id set by the client. For example:
+// "org.chromium.arc.<task-id>" for ARC++ shell surfaces.
+// "org.chromium.lacros.<window-id>" for Lacros browser shell surfaces.
 DEFINE_OWNED_UI_CLASS_PROPERTY_KEY(std::string, kApplicationIdKey, nullptr)
 
 // Startup Id set by the client.

ui/aura/window_tree_host_platform.h

@@ -49,6 +49,11 @@ class AURA_EXPORT WindowTreeHostPlatform : public WindowTreeHost,
       const gfx::Point& location_in_pixels) override;
   void OnCursorVisibilityChangedNative(bool show) override;
 
+  ui::PlatformWindow* platform_window() { return platform_window_.get(); }
+  const ui::PlatformWindow* platform_window() const {
+    return platform_window_.get();
+  }
+
  protected:
   // NOTE: this does not call CreateCompositor(); subclasses must call
   // CreateCompositor() at the appropriate time.
@@ -59,10 +64,6 @@ class AURA_EXPORT WindowTreeHostPlatform : public WindowTreeHost,
   void CreateAndSetPlatformWindow(ui::PlatformWindowInitProperties properties);
 
   void SetPlatformWindow(std::unique_ptr<ui::PlatformWindow> window);
-  ui::PlatformWindow* platform_window() { return platform_window_.get(); }
-  const ui::PlatformWindow* platform_window() const {
-    return platform_window_.get();
-  }
 
   // ui::PlatformWindowDelegate:
   void OnBoundsChanged(const gfx::Rect& new_bounds) override;

ui/ozone/platform/wayland/BUILD.gn

@@ -138,6 +138,7 @@ source_set("wayland") {
 
   deps = [
     "//base",
+    "//build:lacros_buildflags",
     "//build/config/linux/libdrm",
     "//mojo/public/cpp/bindings",
     "//mojo/public/cpp/system",

ui/ozone/platform/wayland/host/wayland_toplevel_window.cc

@@ -5,6 +5,8 @@
 #include "ui/ozone/platform/wayland/host/wayland_toplevel_window.h"
 
 #include "base/run_loop.h"
+#include "base/unguessable_token.h"
+#include "build/lacros_buildflags.h"
 #include "ui/base/dragdrop/drag_drop_types.h"
 #include "ui/base/dragdrop/os_exchange_data.h"
 #include "ui/base/hit_test.h"
@@ -48,7 +50,11 @@ bool WaylandToplevelWindow::CreateShellSurface() {
     return false;
   }
 
-  shell_surface_->SetAppId(app_id_);
+#if BUILDFLAG(IS_LACROS)
+  shell_surface_->SetAppId(window_unique_id_);
+#else
+  shell_surface_->SetAppId(wm_class_class_);
+#endif
   shell_surface_->SetTitle(window_title_);
   SetSizeConstraints();
   TriggerStateChanges();
@@ -200,6 +206,14 @@ void WaylandToplevelWindow::SizeConstraintsChanged() {
   SetSizeConstraints();
 }
 
+std::string WaylandToplevelWindow::GetWindowUniqueId() const {
+#if BUILDFLAG(IS_LACROS)
+  return window_unique_id_;
+#else
+  return std::string();
+#endif
+}
+
 void WaylandToplevelWindow::HandleSurfaceConfigure(int32_t width,
                                                    int32_t height,
                                                    bool is_maximized,
@@ -318,7 +332,12 @@ void WaylandToplevelWindow::OnDragSessionClose(uint32_t dnd_action) {
 
 bool WaylandToplevelWindow::OnInitialize(
     PlatformWindowInitProperties properties) {
-  app_id_ = properties.wm_class_class;
+#if BUILDFLAG(IS_LACROS)
+  auto token = base::UnguessableToken::Create();
+  window_unique_id_ = "org.chromium.lacros." + token.ToString();
+#else
+  wm_class_class_ = properties.wm_class_class;
+#endif
   SetWmMoveLoopHandler(this, static_cast<WmMoveLoopHandler*>(this));
   return true;
 }

ui/ozone/platform/wayland/host/wayland_toplevel_window.h

@@ -5,6 +5,7 @@
 #ifndef UI_OZONE_PLATFORM_WAYLAND_HOST_WAYLAND_TOPLEVEL_WINDOW_H_
 #define UI_OZONE_PLATFORM_WAYLAND_HOST_WAYLAND_TOPLEVEL_WINDOW_H_
 
+#include "build/lacros_buildflags.h"
 #include "ui/gfx/geometry/vector2d.h"
 #include "ui/ozone/platform/wayland/host/wayland_window.h"
 #include "ui/platform_window/platform_window_handler/wm_drag_handler.h"
@@ -56,6 +57,7 @@ class WaylandToplevelWindow : public WaylandWindow,
   void Restore() override;
   PlatformWindowState GetPlatformWindowState() const override;
   void SizeConstraintsChanged() override;
+  std::string GetWindowUniqueId() const override;
 
  private:
   // WaylandWindow overrides:
@@ -114,11 +116,17 @@ class WaylandToplevelWindow : public WaylandWindow,
 
   bool is_active_ = false;
 
+#if BUILDFLAG(IS_LACROS)
+  // Unique ID for this window. May be shared over non-Wayland IPC transports
+  // (e.g. mojo) to identify the window.
+  std::string window_unique_id_;
+#else
   // Id of the chromium app passed through
   // PlatformWindowInitProperties::wm_class_class. This is used by Wayland
   // compositor to identify the app, unite it's windows into the same stack of
   // windows and find *.desktop file to set various preferences including icons.
-  std::string app_id_;
+  std::string wm_class_class_;
+#endif
 
   // Title of the ShellSurface.
   base::string16 window_title_;

ui/platform_window/platform_window.cc

@@ -4,6 +4,8 @@
 
 #include "ui/platform_window/platform_window.h"
 
+#include <string>
+
 #include "ui/gfx/geometry/rect.h"
 
 namespace ui {
@@ -48,4 +50,8 @@ void PlatformWindow::SetOpacity(float opacity) {}
 
 void PlatformWindow::SetVisibilityChangedAnimationsEnabled(bool enabled) {}
 
+std::string PlatformWindow::GetWindowUniqueId() const {
+  return std::string();
+}
+
 }  // namespace ui

ui/platform_window/platform_window.h

@@ -6,6 +6,7 @@
 #define UI_PLATFORM_WINDOW_PLATFORM_WINDOW_H_
 
 #include <memory>
+#include <string>
 
 #include "base/component_export.h"
 #include "base/strings/string16.h"
@@ -140,6 +141,10 @@ class COMPONENT_EXPORT(PLATFORM_WINDOW) PlatformWindow
   // Enables or disables platform provided animations of the PlatformWindow.
   // If |enabled| is set to false, animations are disabled.
   virtual void SetVisibilityChangedAnimationsEnabled(bool enabled);
+
+  // Returns a unique ID for the window. The interpretation of the ID is
+  // platform specific. Overriding this method is optional.
+  virtual std::string GetWindowUniqueId() const;
 };
 
 }  // namespace ui

ui/shell_dialogs/BUILD.gn

@@ -118,6 +118,7 @@ jumbo_component("shell_dialogs") {
       "//chromeos/crosapi/mojom",
       "//chromeos/lacros",
       "//mojo/public/cpp/bindings",
+      "//ui/platform_window",
     ]
   }
 }

ui/shell_dialogs/DEPS

@@ -8,5 +8,6 @@ include_rules = [
   "+ui/aura",
   "+ui/base",
   "+ui/gfx",
+  "+ui/platform_window",
   "+ui/strings/grit/ui_strings.h",
 ]

ui/shell_dialogs/select_file_dialog_lacros.cc

@@ -10,6 +10,9 @@
 #include "base/notreached.h"
 #include "chromeos/crosapi/mojom/select_file.mojom.h"
 #include "chromeos/lacros/lacros_chrome_service_impl.h"
+#include "ui/aura/window.h"
+#include "ui/aura/window_tree_host_platform.h"
+#include "ui/platform_window/platform_window.h"
 #include "ui/shell_dialogs/select_file_policy.h"
 #include "ui/shell_dialogs/selected_file_info.h"
 
@@ -58,6 +61,18 @@ SelectedFileInfo ConvertSelectedFileInfo(
   return file;
 }
 
+// Returns the ID of the Wayland shell surface that contains |window|.
+std::string GetShellWindowUniqueId(aura::Window* window) {
+  // On desktop aura there is one WindowTreeHost per top-level window.
+  aura::WindowTreeHost* window_tree_host = window->GetRootWindow()->GetHost();
+  DCHECK(window_tree_host);
+  // Lacros is based on Ozone/Wayland, which uses PlatformWindow and
+  // aura::WindowTreeHostPlatform.
+  aura::WindowTreeHostPlatform* window_tree_host_platform =
+      static_cast<aura::WindowTreeHostPlatform*>(window_tree_host);
+  return window_tree_host_platform->platform_window()->GetWindowUniqueId();
+}
+
 }  // namespace
 
 SelectFileDialogLacros::Factory::Factory() = default;
@@ -111,6 +126,7 @@ void SelectFileDialogLacros::SelectFileImpl(
     options->file_types->allowed_paths =
         GetMojoAllowedPaths(file_types->allowed_paths);
   }
+  options->owning_shell_window_id = GetShellWindowUniqueId(owning_window);
 
   // Send request to ash-chrome.
   chromeos::LacrosChromeServiceImpl::Get()->select_file_remote()->Select(