Refactored ExtensionUninstallDialog to take a NativeWindow instead of a Browser

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

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/extensions/extension_uninstall_dialog.h"
 
 #include "base/basictypes.h"
 #include "base/compiler_specific.h"
 #include "base/strings/string_util.h"
 #include "base/strings/utf_string_conversions.h"
@@ skipping 24 matching lines @@
 gfx::NativeWindow GetParent(Browser* browser) {
   if (browser && browser->window())
     return browser->window()->GetNativeWindow();
   return NULL;
 }
 
 // Views implementation of the uninstall dialog.
 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();
 
   // Forwards the accept and cancels to the delegate.
   void ExtensionUninstallAccepted();
   void ExtensionUninstallCanceled();
 
   ExtensionUninstallDialogDelegateView* view() { return view_; }
 
  private:
   virtual void Show() OVERRIDE;
 
   ExtensionUninstallDialogDelegateView* view_;
-  bool show_in_app_list_;
+  gfx::NativeWindow parent_;
 
   DISALLOW_COPY_AND_ASSIGN(ExtensionUninstallDialogViews);
 };
 
 // The dialog's view, owned by the views framework.
 class ExtensionUninstallDialogDelegateView : public views::DialogDelegateView {
  public:
   ExtensionUninstallDialogDelegateView(
       ExtensionUninstallDialogViews* dialog_view,
       const extensions::Extension* extension,
@@ skipping 32 matching lines @@
 
   views::ImageView* icon_;
   views::Label* heading_;
   bool triggered_by_extension_;
 
   DISALLOW_COPY_AND_ASSIGN(ExtensionUninstallDialogDelegateView);
 };
 
 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() {
   // Close the widget (the views framework will delete view_).
   if (view_) {
     view_->DialogDestroyed();
     view_->GetWidget()->CloseNow();
   }
 }
 
 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();
 }
 
 void ExtensionUninstallDialogViews::ExtensionUninstallAccepted() {
   // The widget gets destroyed when the dialog is accepted.
   view_ = NULL;
   delegate_->ExtensionUninstallAccepted();
 }
 
 void ExtensionUninstallDialogViews::ExtensionUninstallCanceled() {
@@ skipping 88 matching lines @@
   }
 }
 
 }  // namespace
 
 // static
 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);
+}