Refactored ExtensionUninstallDialog to take a NativeWindow instead of a Browser

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

Index: chrome/browser/ui/views/extensions/extension_uninstall_dialog_view.cc
diff --git a/chrome/browser/ui/views/extensions/extension_uninstall_dialog_view.cc b/chrome/browser/ui/views/extensions/extension_uninstall_dialog_view.cc
index d6d3a8c5964cfc01de4fcaa12927df3a1940c441..a0ac052de0b71b81f744c4ea4eaaa2d77873c4c6 100644
--- a/chrome/browser/ui/views/extensions/extension_uninstall_dialog_view.cc
+++ b/chrome/browser/ui/views/extensions/extension_uninstall_dialog_view.cc
@@ -42,9 +42,13 @@ gfx::NativeWindow GetParent(Browser* browser) {
 class ExtensionUninstallDialogViews
     : public extensions::ExtensionUninstallDialog {
  public:
+  // Exactly one of |browser| and |parent| should be NULL. If |browser| is set,
+  // this is for launching from a browser window. If |parent| is set, this is
+  // launching from a non-browser window.
   ExtensionUninstallDialogViews(
       Profile* profile,
       Browser* browser,
+      gfx::NativeWindow parent,
       extensions::ExtensionUninstallDialog::Delegate* delegate);
   virtual ~ExtensionUninstallDialogViews();
 
@@ -58,7 +62,7 @@ class ExtensionUninstallDialogViews
   virtual void Show() OVERRIDE;
 
   ExtensionUninstallDialogDelegateView* view_;
-  bool show_in_app_list_;
+  gfx::NativeWindow parent_;
 
   DISALLOW_COPY_AND_ASSIGN(ExtensionUninstallDialogViews);
 };
@@ -112,10 +116,11 @@ class ExtensionUninstallDialogDelegateView : public views::DialogDelegateView {
 ExtensionUninstallDialogViews::ExtensionUninstallDialogViews(
     Profile* profile,
     Browser* browser,
+    gfx::NativeWindow parent,
     extensions::ExtensionUninstallDialog::Delegate* delegate)
     : extensions::ExtensionUninstallDialog(profile, browser, delegate),
       view_(NULL),
-      show_in_app_list_(!browser) {
+      parent_(parent) {
 }
 
 ExtensionUninstallDialogViews::~ExtensionUninstallDialogViews() {
@@ -127,19 +132,15 @@ ExtensionUninstallDialogViews::~ExtensionUninstallDialogViews() {
 }
 
 void ExtensionUninstallDialogViews::Show() {
-  // TODO(tapted): A true |desktop_type| needs to be passed in at creation time
-  // to remove reliance on GetActiveDesktop(). http://crbug.com/308360
-  gfx::NativeWindow parent = show_in_app_list_ ?
-      AppListService::Get(chrome::GetActiveDesktop())->GetAppListWindow() :
-      GetParent(browser_);
-  if (browser_ && !parent) {
+  if (browser_ && !GetParent(browser_)) {

[tapted, 2014/07/11 04:39:15]

From the earlier CL, this needed a comment - If I understand the comment
correctly, something like

// Callers of ExtensionUninstallDialog::Create() expect a non-NULL result, but
it's possible that a browser window couldn't be found. In that case, immediately
cancel the dialog when shown.

[sashab, 2014/07/11 05:58:57]

Yup, I understand. Done.

     delegate_->ExtensionUninstallCanceled();
     return;
   }
 
   view_ = new ExtensionUninstallDialogDelegateView(
       this, extension_, triggering_extension_, &icon_);
-  CreateBrowserModalDialogViews(view_, parent)->Show();
+  CreateBrowserModalDialogViews(view_, parent_ ? parent_ : GetParent(browser_))

[tapted, 2014/07/11 04:39:15]

What happens if both browser_ and parent_ are NULL? I think this will segfault.
Currently I *think* CreateBrowserModalDialogViews has a fallback to center the
dialog in the screen, which you might need to keep.

[sashab, 2014/07/11 05:58:57]

If you follow the logic carefully, this is actually the exact same behaviour as
before (show_in_app_list_ is !browser_). The comment on the constructor says
that exactly one of |browser_| and |parent_| must be NULL, so they can't both be
NULL. We know GetParent(browser_) is not-NULL because the previous if-statement
returns.

CreateBrowserModalDialogViews is actually safe to call with NULL anyway - it
just creates a non-parented window (so an independent window). So even if they
could both be NULL, this wouldn't segfault.

I originally had a DCHECK(parent_ ^ browser_) somewhere, but thought it was too
obscure/reflected badly in the class's design. So I left it open here, and as
you can see, it's probably safe if they're both NULL anyway.

[tapted, 2014/07/11 06:10:40]

ah - I think I read it as

... : GetParent(browser)
    ->Show());

instead of

... : GetParent(browser))
    ->Show();


I think a separate variable will help here - we're spread across 2 lines anyway.
i.e.

  gfx::NativeWindow parent = parent ? parent : GetParent(browser_);
  CreateBrowserModalDialogViews(view_, parent)->Show();

[sashab, 2014/07/13 22:57:25]

Done.

+      ->Show();
 }
 
 void ExtensionUninstallDialogViews::ExtensionUninstallAccepted() {
@@ -247,5 +248,14 @@ extensions::ExtensionUninstallDialog*
 extensions::ExtensionUninstallDialog::Create(Profile* profile,
                                              Browser* browser,
                                              Delegate* delegate) {
-  return new ExtensionUninstallDialogViews(profile, browser, delegate);
+  return new ExtensionUninstallDialogViews(profile, browser, NULL, delegate);
+}
+
+// static
+extensions::ExtensionUninstallDialog*
+extensions::ExtensionUninstallDialog::CreateForNonBrowserWindow(

[tapted, 2014/07/11 04:39:15]

You'll need a mac version of this too

[sashab, 2014/07/11 05:58:57]

Done. Please test that the Uninstall dialog still works from the Mac app list
for me :)

+    Profile* profile,
+    gfx::NativeWindow parent,
+    Delegate* delegate) {
+  return new ExtensionUninstallDialogViews(profile, NULL, parent, delegate);
 }