app_list: Set AppList as parent of uninstall dialog

Currently, when the uninstall dialog is opened from the Launcher, the Launcher
is dismissed, which is confusing to the user. By parenting the dialog to the
Launcher, the UX is much better.

Because we cannot directly access Ash windows anymore, this fix requires us to
explicitly set the dialog's parent window via MUS properties.

TEST=manually verified + new browser tests
BUG=814619

Change-Id: Ic25cb376191aaba8048f95fa5d0032b0328b8bc8
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1496447Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
Commit-Queue: Tim Song <tengs@chromium.org>
Cr-Commit-Position: refs/heads/master@{#638016}

ash/public/interfaces/shell_test_api.test-mojom

@@ -33,4 +33,8 @@ interface ShellTestApi {
 
   // Set the minimum velocity to cause fling gesture.
   SetMinFlingVelocity(float velocity);
+
+  // Returns the number of child windows for the given container in the primary
+  // root window.
+  GetChildWindowCountInContainer(int32 container_id) => (int32 count);
 };

ash/shell_test_api.cc

@@ -126,4 +126,15 @@ void ShellTestApi::SetMinFlingVelocity(float velocity) {
   ui::GestureConfiguration::GetInstance()->set_min_fling_velocity(velocity);
 }
 
+void ShellTestApi::GetChildWindowCountInContainer(
+    int container_id,
+    GetChildWindowCountInContainerCallback cb) {
+  auto* container =
+      ash::Shell::GetPrimaryRootWindow()->GetChildById(container_id);
+  // Return an negative count to indicate that the container is invalid.
+  if (!container)
+    std::move(cb).Run(-1);
+  std::move(cb).Run(container->children().size());
+}
+
 }  // namespace ash

ash/shell_test_api.h

@@ -63,6 +63,9 @@ class ShellTestApi : public mojom::ShellTestApi {
   void ToggleOverviewMode(ToggleOverviewModeCallback cb) override;
   void AddRemoveDisplay() override;
   void SetMinFlingVelocity(float velocity) override;
+  void GetChildWindowCountInContainer(
+      int container_id,
+      GetChildWindowCountInContainerCallback cb) override;
 
  private:
   Shell* shell_;  // not owned

chrome/browser/extensions/extension_uninstall_dialog.cc

@@ -98,6 +98,7 @@ void ExtensionUninstallDialog::ConfirmUninstall(
 
   extension_ = extension;
   uninstall_reason_ = reason;
+  uninstall_source_ = source;
 
   if (parent() && parent_window_tracker_->WasNativeWindowClosed()) {
     OnDialogClosed(CLOSE_ACTION_CANCELED);

chrome/browser/extensions/extension_uninstall_dialog.h

@@ -119,6 +119,7 @@ class ExtensionUninstallDialog
       return triggering_extension_.get(); }
   const gfx::ImageSkia& icon() const { return icon_->image_skia(); }
   gfx::NativeWindow parent() { return parent_; }
+  UninstallSource uninstall_source() { return uninstall_source_; }
 
  private:
   // Uninstalls the extension. Returns true on success, and populates |error| on
@@ -172,6 +173,8 @@ class ExtensionUninstallDialog
 
   UninstallReason uninstall_reason_ = UNINSTALL_REASON_FOR_TESTING;
 
+  UninstallSource uninstall_source_ = UNINSTALL_SOURCE_FOR_TESTING;
+
   ScopedObserver<ExtensionRegistry, ExtensionRegistryObserver> observer_;
 
   base::ThreadChecker thread_checker_;

chrome/browser/ui/views/extensions/extension_uninstall_dialog_view.cc

@@ -36,8 +36,30 @@
 #include "ui/views/widget/widget.h"
 #include "ui/views/window/dialog_delegate.h"
 
+#if defined(OS_CHROMEOS)
+#include "ash/public/cpp/shell_window_ids.h"
+#include "chrome/browser/ui/ash/ash_util.h"
+#endif
+
 namespace {
 
+#if defined(OS_CHROMEOS)
+views::Widget* CreateChromeOSAppListParentedDialog(
+    views::DialogDelegateView* delegate_view) {
+  views::Widget* widget = new views::Widget;
+  views::Widget::InitParams params =
+      views::DialogDelegate::GetDialogWidgetInitParams(
+          delegate_view, nullptr /* context */, nullptr /* parent */,
+          gfx::Rect() /* bounds */);
+
+  ash_util::SetupWidgetInitParamsForContainer(
+      &params, ash::kShellWindowId_AppListContainer);
+
+  widget->Init(params);
+  return widget;
+}
+#endif
+
 ToolbarActionView* GetExtensionAnchorView(const std::string& extension_id,
                                           gfx::NativeWindow window) {
   BrowserView* browser_view =
@@ -147,6 +169,14 @@ void ExtensionUninstallDialogViews::Show() {
   if (anchor_view) {
     views::BubbleDialogDelegateView::CreateBubble(view_)->Show();
   } else {
+#if defined(OS_CHROMEOS)
+    // On ChromeOS, the uninstall dialog can be created from the App List, so we
+    // need to attach the AppList window as the parent window.
+    if (uninstall_source() == extensions::UNINSTALL_SOURCE_APP_LIST) {
+      CreateChromeOSAppListParentedDialog(view_)->Show();
+      return;
+    }
+#endif
     constrained_window::CreateBrowserModalDialogViews(view_, parent())->Show();
   }
 }

chrome/browser/ui/views/extensions/extension_uninstall_dialog_view_browsertest.cc

@@ -27,6 +27,15 @@
 #include "extensions/common/extension_urls.h"
 #include "extensions/common/value_builder.h"
 
+#if defined(OS_CHROMEOS)
+#include "ash/public/cpp/shell_window_ids.h"
+#include "ash/public/interfaces/constants.mojom.h"
+#include "ash/public/interfaces/shell_test_api.test-mojom-test-utils.h"
+#include "content/public/common/service_manager_connection.h"
+#include "services/service_manager/public/cpp/connector.h"
+#include "ui/aura/test/mus/change_completion_waiter.h"
+#endif
+
 namespace {
 
 const char kUninstallUrl[] = "https://www.google.com/";
@@ -53,6 +62,25 @@ void SetUninstallURL(extensions::ExtensionPrefs* prefs,
                              std::make_unique<base::Value>(kUninstallUrl));
 }
 
+#if defined(OS_CHROMEOS)
+// Returns the number of child windows in the AppList container on ChromeOS.
+// Blocks until the ash service responds.
+int GetWindowCountForAppListContainer() {
+  // Wait for window visibility to stabilize.
+  aura::test::WaitForAllChangesToComplete();
+
+  ash::mojom::ShellTestApiPtr shell_test_api;
+  content::ServiceManagerConnection::GetForProcess()
+      ->GetConnector()
+      ->BindInterface(ash::mojom::kServiceName, &shell_test_api);
+  ash::mojom::ShellTestApiAsyncWaiter waiter(shell_test_api.get());
+  int child_window_count = 0;
+  waiter.GetChildWindowCountInContainer(ash::kShellWindowId_AppListContainer,
+                                        &child_window_count);
+  return child_window_count;
+}
+#endif
+
 class TestExtensionUninstallDialogDelegate
     : public extensions::ExtensionUninstallDialog::Delegate {
  public:
@@ -136,6 +164,54 @@ IN_PROC_BROWSER_TEST_F(ExtensionUninstallDialogViewBrowserTest,
 }
 
 #if defined(OS_CHROMEOS)
+IN_PROC_BROWSER_TEST_F(ExtensionUninstallDialogViewBrowserTest,
+                       ParentWindowIsAppList) {
+  scoped_refptr<const extensions::Extension> extension(BuildTestExtension());
+  extensions::ExtensionSystem::Get(browser()->profile())
+      ->extension_service()
+      ->AddExtension(extension.get());
+
+  // Initially the AppList window should have no children.
+  EXPECT_EQ(0, GetWindowCountForAppListContainer());
+
+  // Open the extension uninstall dialog without a browser parent window. This
+  // occurs when the dialog is opened from the Launcher.
+  std::unique_ptr<extensions::ExtensionUninstallDialog> dialog;
+  dialog = extensions::ExtensionUninstallDialog::Create(browser()->profile(),
+                                                        nullptr, nullptr);
+  dialog->ConfirmUninstall(extension.get(),
+                           extensions::UNINSTALL_REASON_FOR_TESTING,
+                           extensions::UNINSTALL_SOURCE_APP_LIST);
+
+  // Verify that a child dialog window has been added to the AppList as a child.
+  EXPECT_EQ(1, GetWindowCountForAppListContainer());
+}
+
+IN_PROC_BROWSER_TEST_F(ExtensionUninstallDialogViewBrowserTest,
+                       ParentWindowIsBrowser) {
+  scoped_refptr<const extensions::Extension> extension(BuildTestExtension());
+  extensions::ExtensionSystem::Get(browser()->profile())
+      ->extension_service()
+      ->AddExtension(extension.get());
+
+  // When the uninstall dialog is launched from a browser, the AppList should
+  // never be the parent.
+  EXPECT_EQ(0, GetWindowCountForAppListContainer());
+
+  // Open the extension uninstall dialog with a browser parent window. This is
+  // occurs when the dialog is opened from chrome://extensions rather than the
+  // launcher.
+  std::unique_ptr<extensions::ExtensionUninstallDialog> dialog;
+  dialog = extensions::ExtensionUninstallDialog::Create(
+      browser()->profile(), browser()->window()->GetNativeWindow(), nullptr);
+  dialog->ConfirmUninstall(extension.get(),
+                           extensions::UNINSTALL_REASON_FOR_TESTING,
+                           extensions::UNINSTALL_SOURCE_FOR_TESTING);
+
+  // Verify that the uninstall dialog is not parented to the AppList.
+  EXPECT_EQ(0, GetWindowCountForAppListContainer());
+}
+
 // Test that we don't crash when uninstalling an extension from a bookmark app
 // window in Ash. Context: crbug.com/825554
 IN_PROC_BROWSER_TEST_F(ExtensionUninstallDialogViewBrowserTest,