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..761e7f0f4884d095a4fff17a0638f443d4c4e16a 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,15 @@ gfx::NativeWindow GetParent(Browser* browser) {
 class ExtensionUninstallDialogViews
     : public extensions::ExtensionUninstallDialog {
  public:
+  // If |browser| is set, it will be used as the browser window that this dialog
+  // is launched from. If |parent| is set, it it used as the parent of the
+  // dialog instead. If both |browser| and |parent| are NULL, this is launched
+  // as a standalone window. Only at most one of |browser| and |parent| should
+  // be set.
   ExtensionUninstallDialogViews(
       Profile* profile,
       Browser* browser,
+      gfx::NativeWindow parent,
       extensions::ExtensionUninstallDialog::Delegate* delegate);
   virtual ~ExtensionUninstallDialogViews();
 
@@ -58,7 +64,7 @@ class ExtensionUninstallDialogViews
   virtual void Show() OVERRIDE;
 
   ExtensionUninstallDialogDelegateView* view_;
-  bool show_in_app_list_;
+  gfx::NativeWindow parent_;
 
   DISALLOW_COPY_AND_ASSIGN(ExtensionUninstallDialogViews);
 };
@@ -112,10 +118,11 @@ class ExtensionUninstallDialogDelegateView : public views::DialogDelegateView {
 ExtensionUninstallDialogViews::ExtensionUninstallDialogViews(
     Profile* profile,
     Browser* browser,

[msw, 2014/07/14 17:38:31]

The |browser| param is only ever used to get it's NativeWindow to use as
|parent_|. Why not remove the |browser| param from the [Views] ctors and just
pass in GetParent(browser)?

[sashab, 2014/07/17 23:24:41]

Not quite (I also thought this as I was refactoring). If you look closely in
ExtensionUninstallDialogViews::Show() (the existing or current implementation),
there's a check to see if browser_ is set but browser_->window() is not (ie
CreateBrowserWindow has not finished), and if so, cancel the dialog.

I guess we could move this check upwards, ie callers check browser()->window()
and pass GetNativeWindow(), but I think there could be a code path where
browser()->window() is not set when this object is constructed, but it will be
set by the time Show() is called.

[msw, 2014/07/18 04:03:37]

Yeah, I asked: "nit: why not just return a "standalone" dialog in that case?"
 
I think that's unlikely, and not worth supporting without evidence.

[sashab, 2014/07/21 02:50:30]

I'm of mixed opinions on this one. Technically, if the uninstall dialog *is*
launched without a browser window, we probably want to display it as a
standalone dialog anyway. But the code is there to close the dialog immediately
if there is no browser window. I guess if an API creates an uninstall dialog
while Chrome is closing, we don't want to open the dialog at all (we want to
cancel it immediately. I've left that case in there for now, with comments, for
this reason.

+    gfx::NativeWindow parent,
     extensions::ExtensionUninstallDialog::Delegate* delegate)
     : extensions::ExtensionUninstallDialog(profile, browser, delegate),
       view_(NULL),
-      show_in_app_list_(!browser) {
+      parent_(parent) {
 }
 
 ExtensionUninstallDialogViews::~ExtensionUninstallDialogViews() {
@@ -127,18 +134,18 @@ 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) {
+  // Callers of ExtensionUninstallDialog::Create() expect a non-NULL result, but

[msw, 2014/07/14 17:38:31]

nit: why not just return a "standalone" dialog in that case?

+  // it's possible that a browser window couldn't be found. In that case,
+  // immediately cancel the dialog when shown.
+  if (browser_ && !GetParent(browser_)) {
     delegate_->ExtensionUninstallCanceled();
     return;
   }
 
   view_ = new ExtensionUninstallDialogDelegateView(
       this, extension_, triggering_extension_, &icon_);
+
+  gfx::NativeWindow parent = parent_ ? parent_ : GetParent(browser_);
   CreateBrowserModalDialogViews(view_, parent)->Show();
 }
 
@@ -247,5 +254,21 @@ 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::CreateForAppList(Profile* profile,
+                                                       gfx::NativeWindow parent,
+                                                       Delegate* delegate) {
+  return new ExtensionUninstallDialogViews(profile, NULL, parent, delegate);
+}
+
+// static
+extensions::ExtensionUninstallDialog*
+extensions::ExtensionUninstallDialog::CreateAsStandaloneDialog(
+    Profile* profile,
+    Delegate* delegate) {
+  return new ExtensionUninstallDialogViews(profile, NULL, NULL, delegate);
 }