SelectFileDialogExtension should be non-modal when the caller specifies owning_window=NULL

USE CASE: Launching a SelectFileDialog from an ARC Android app on ChromeOS (go/arc-file-picker)

PROBLEM: SelectFileDialogExtension is shown as a modal to the
default browser window [1] when the user specifies owning_window=NULL
in SelectFileDialog::SelectFile, even though the method comment says
"|owning_window| is the window the dialog is modal to, or NULL for
a modeless dialog." [2]

SOLUTION: Extend ExtensionDialog::Show with is_modal param and make
it launch the dialog with CreateDialogWidget() instead of
CreateBrowserModalDialogViews(). SelectFileDialogExtension still
passes a non-null |parent_window| to ExtensionDialog::Show for
obtaining screen_size, etc.

[1] https://cs.chromium.org/chromium/src/chrome/browser/ui/views/select_file_dialog_extension.cc?q=base_window.%5C?&g=0&l=440
[2] https://cs.chromium.org/chromium/src/ui/shell_dialogs/select_file_dialog.h?q=owning_window&g=0&l=174

BUG=b:117912148
TEST=manually tested if non-modal dialog is shown

Change-Id: Ie5ef18d7d613cefad544ee9e837f71b22a94bd4c
Reviewed-on: https://chromium-review.googlesource.com/c/1304190
Commit-Queue: Satoshi Niwa <niwa@chromium.org>
Reviewed-by: Peter Kasting <pkasting@chromium.org>
Reviewed-by: Sam McNally <sammc@chromium.org>
Cr-Commit-Position: refs/heads/master@{#606349}

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

@@ -56,17 +56,17 @@ ExtensionDialog::~ExtensionDialog() {
 }
 
 // static
-ExtensionDialog* ExtensionDialog::Show(
-    const GURL& url,
-    gfx::NativeWindow parent_window,
-    Profile* profile,
-    WebContents* web_contents,
-    int width,
-    int height,
-    int min_width,
-    int min_height,
-    const base::string16& title,
-    ExtensionDialogObserver* observer) {
+ExtensionDialog* ExtensionDialog::Show(const GURL& url,
+                                       gfx::NativeWindow parent_window,
+                                       Profile* profile,
+                                       WebContents* web_contents,
+                                       bool is_modal,
+                                       int width,
+                                       int height,
+                                       int min_width,
+                                       int min_height,
+                                       const base::string16& title,
+                                       ExtensionDialogObserver* observer) {
   extensions::ExtensionViewHost* host =
       extensions::ExtensionViewHostFactory::CreateDialogHost(url, profile);
   if (!host)
@@ -81,7 +81,7 @@ ExtensionDialog* ExtensionDialog::Show(
   DCHECK(parent_window);
   ExtensionDialog* dialog = new ExtensionDialog(host, observer);
   dialog->set_title(title);
-  dialog->InitWindow(parent_window, width, height);
+  dialog->InitWindow(parent_window, is_modal, width, height);
 
   // Show a white background while the extension loads.  This is prettier than
   // flashing a black unfilled window frame.
@@ -94,23 +94,27 @@ ExtensionDialog* ExtensionDialog::Show(
 }
 
 void ExtensionDialog::InitWindow(gfx::NativeWindow parent,
+                                 bool is_modal,
                                  int width,
                                  int height) {
   views::Widget* window =
-      constrained_window::CreateBrowserModalDialogViews(this, parent);
+      is_modal ? constrained_window::CreateBrowserModalDialogViews(this, parent)
+               : views::DialogDelegate::CreateDialogWidget(
+                     this, nullptr /* context */, nullptr /* parent */);
 
-  // Center the window over the browser.
-  views::Widget* parent_widget =
-      views::Widget::GetWidgetForNativeWindow(parent);
-  gfx::Rect bounds_rect = parent_widget->GetWindowBoundsInScreen();
+  // Center the window over the parent browser window or the screen.
+  gfx::Rect screen_rect =
+      display::Screen::GetScreen()->GetDisplayNearestWindow(parent).work_area();
+  gfx::Rect bounds_rect = parent
+                              ? views::Widget::GetWidgetForNativeWindow(parent)
+                                    ->GetWindowBoundsInScreen()
+                              : screen_rect;
   bounds_rect.ClampToCenteredSize({width, height});
 
   // Ensure the top left and top right of the window are on screen, with
   // priority given to the top left. Use the display's work_area() rather than
   // bounds(), since the work_area() may be smaller e.g. when the docked
   // magnifier is enabled.
-  gfx::Rect screen_rect =
-      display::Screen::GetScreen()->GetDisplayNearestWindow(parent).work_area();
   bounds_rect.AdjustToFit(screen_rect);
   window->SetBounds(bounds_rect);
 

chrome/browser/ui/views/extensions/extension_dialog.h

@@ -37,11 +37,13 @@ class ExtensionDialog : public views::DialogDelegate,
   // |parent_window| is the parent window to which the pop-up will be attached.
   // |profile| is the profile that the extension is registered with.
   // |web_contents| is the tab that spawned the dialog.
+  // |is_modal| determines whether the dialog is modal to |parent_window|.
   // |width| and |height| are the size of the dialog in pixels.
   static ExtensionDialog* Show(const GURL& url,
                                gfx::NativeWindow parent_window,
                                Profile* profile,
                                content::WebContents* web_contents,
+                               bool is_modal,
                                int width,
                                int height,
                                int min_width,
@@ -92,7 +94,10 @@ class ExtensionDialog : public views::DialogDelegate,
   ExtensionDialog(extensions::ExtensionViewHost* host,
                   ExtensionDialogObserver* observer);
 
-  void InitWindow(gfx::NativeWindow parent_window, int width, int height);
+  void InitWindow(gfx::NativeWindow parent_window,
+                  bool is_modal,
+                  int width,
+                  int height);
 
   ExtensionViewViews* GetExtensionView() const;
   static ExtensionViewViews* GetExtensionView(

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

@@ -61,9 +61,9 @@ class ExtensionDialogBoundsTest
     auto* dialog = ExtensionDialog::Show(
         extension->url().Resolve("main.html"),
         browser()->window()->GetNativeWindow(), browser()->profile(),
-        nullptr /* web_contents */, kDialogWidth, kDialogHeight,
-        kDialogMinimumWidth, kDialogMinimumHeight, base::string16() /* title */,
-        nullptr /* observer */);
+        nullptr /* web_contents */, true /* is_modal */, kDialogWidth,
+        kDialogHeight, kDialogMinimumWidth, kDialogMinimumHeight,
+        base::string16() /* title */, nullptr /* observer */);
     ASSERT_TRUE(dialog);
     ASSERT_TRUE(init_listener.WaitUntilSatisfied());
   }

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

@@ -46,8 +46,8 @@ IN_PROC_BROWSER_TEST_F(ExtensionDialogUiTest, MAYBE_TabFocusLoop) {
   // The main.html contains three buttons.
   ExtensionDialog* dialog = ExtensionDialog::Show(
       extension->url().Resolve("main.html"),
-      browser()->window()->GetNativeWindow(), browser()->profile(),
-      NULL, 300, 300, 300, 300, base::string16(), NULL);
+      browser()->window()->GetNativeWindow(), browser()->profile(), nullptr,
+      true, 300, 300, 300, 300, base::string16(), nullptr);
   ASSERT_TRUE(dialog);
   ASSERT_TRUE(init_listener.WaitUntilSatisfied());
 

chrome/browser/ui/views/select_file_dialog_extension.cc

@@ -437,13 +437,9 @@ void SelectFileDialogExtension::SelectFileImpl(
 
   ExtensionDialog* dialog = ExtensionDialog::Show(
       file_manager_url,
-      base_window ? base_window->GetNativeWindow() : owner_window,
-      profile_,
-      web_contents,
-      kFileManagerWidth,
-      kFileManagerHeight,
-      kFileManagerMinimumWidth,
-      kFileManagerMinimumHeight,
+      base_window ? base_window->GetNativeWindow() : owner_window, profile_,
+      web_contents, (owner_window != nullptr) /* is_modal */, kFileManagerWidth,
+      kFileManagerHeight, kFileManagerMinimumWidth, kFileManagerMinimumHeight,
       file_manager::util::GetSelectFileDialogTitle(type),
       this /* ExtensionDialog::Observer */);
   if (!dialog) {