Refactored UninstallDialog to take a NativeWindow instead of a Browser

Originally, ExtensionUninstallDialog took a Browser argument (which
determined which browser window it should be modal to), and a special
case was added so the dialog would be modal to the app list if the
Browser argument was NULL. Updated this constructor to take the parent
NativeWindow directly instead of a Browser, so the dialog can be modal
to any parent window. For example, this allows it to be parent to the
App Info dialog, and be a proper standalone dialog in
ExtensionStorageMonitor. Further refactors can be made as a result of
this CL since some of the callsites could be getting a Browser just to
be able to launch the dialog, whereas that is not needed anymore if the
intention is to launch it as a non-modal dialog.

Updated uninstall dialogs launched from the following callsites:
- The management Chrome API
- The extensions context menu
- The 'app disabled' message for when it has requested new permissions
- The chrome://extensions page
- The context menu of apps in the app list
- The 'remove' button in the App Info dialog
- The context menu on the New Tab page

BUG=388746,362308

Review URL: https://codereview.chromium.org/382133003

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@285541 0039d316-1c4b-4281-b951-d872f2087c98

chrome/browser/extensions/api/management/management_api.cc

@@ -23,6 +23,7 @@
 #include "chrome/browser/extensions/extension_ui_util.h"
 #include "chrome/browser/extensions/extension_uninstall_dialog.h"
 #include "chrome/browser/extensions/launch_util.h"
+#include "chrome/browser/extensions/window_controller.h"
 #include "chrome/browser/favicon/favicon_service_factory.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/ui/browser_dialogs.h"
@@ -610,8 +611,11 @@ bool ManagementUninstallFunctionBase::Uninstall(
   if (auto_confirm_for_test == DO_NOT_SKIP) {
     if (show_confirm_dialog) {
       AddRef();  // Balanced in ExtensionUninstallAccepted/Canceled
+      extensions::WindowController* controller = GetExtensionWindowController();
       extension_uninstall_dialog_.reset(ExtensionUninstallDialog::Create(
-          GetProfile(), GetCurrentBrowser(), this));
+          GetProfile(),
+          controller ? controller->window()->GetNativeWindow() : NULL,
+          this));
       if (extension_id() != target_extension_id) {
         extension_uninstall_dialog_->ConfirmProgrammaticUninstall(
             target_extension, GetExtension());

chrome/browser/extensions/extension_context_menu_model.cc

@@ -13,6 +13,7 @@
 #include "chrome/browser/extensions/extension_tab_util.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/ui/browser.h"
+#include "chrome/browser/ui/browser_window.h"
 #include "chrome/browser/ui/chrome_pages.h"
 #include "chrome/browser/ui/tabs/tab_strip_model.h"
 #include "chrome/common/extensions/extension_constants.h"
@@ -125,7 +126,7 @@ void ExtensionContextMenuModel::ExecuteCommand(int command_id,
       AddRef();  // Balanced in Accepted() and Canceled()
       extension_uninstall_dialog_.reset(
           extensions::ExtensionUninstallDialog::Create(
-              profile_, browser_, this));
+              profile_, browser_->window()->GetNativeWindow(), this));
       extension_uninstall_dialog_->ConfirmUninstall(extension);
       break;
     }

chrome/browser/extensions/extension_disabled_ui.cc

@@ -23,6 +23,7 @@
 #include "chrome/browser/extensions/extension_util.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/ui/browser.h"
+#include "chrome/browser/ui/browser_window.h"
 #include "chrome/browser/ui/global_error/global_error.h"
 #include "chrome/browser/ui/global_error/global_error_service.h"
 #include "chrome/browser/ui/global_error/global_error_service_factory.h"
@@ -347,7 +348,7 @@ void ExtensionDisabledGlobalError::BubbleViewCancelButtonPressed(
     Browser* browser) {
 #if !defined(OS_ANDROID)
   uninstall_dialog_.reset(extensions::ExtensionUninstallDialog::Create(
-      service_->profile(), browser, this));
+      service_->profile(), browser->window()->GetNativeWindow(), this));
   // Delay showing the uninstall dialog, so that this function returns
   // immediately, to close the bubble properly. See crbug.com/121544.
   base::MessageLoop::current()->PostTask(

chrome/browser/extensions/extension_uninstall_dialog.cc

@@ -8,12 +8,8 @@
 #include "base/logging.h"
 #include "base/message_loop/message_loop.h"
 #include "base/strings/utf_string_conversions.h"
-#include "chrome/browser/chrome_notification_types.h"
 #include "chrome/browser/extensions/extension_util.h"
 #include "chrome/browser/profiles/profile.h"
-#include "chrome/browser/ui/browser.h"
-#include "content/public/browser/notification_service.h"
-#include "content/public/browser/notification_source.h"
 #include "extensions/browser/extension_registry.h"
 #include "extensions/browser/image_loader.h"
 #include "extensions/common/constants.h"
@@ -45,20 +41,14 @@ SkBitmap GetDefaultIconBitmapForMaxScaleFactor(bool is_app) {
 
 ExtensionUninstallDialog::ExtensionUninstallDialog(
     Profile* profile,
-    Browser* browser,
+    gfx::NativeWindow parent,
     ExtensionUninstallDialog::Delegate* delegate)
     : profile_(profile),
-      browser_(browser),
+      parent_(parent),
       delegate_(delegate),
       extension_(NULL),
       triggering_extension_(NULL),
-      state_(kImageIsLoading),
       ui_loop_(base::MessageLoop::current()) {
-  if (browser) {
-    registrar_.Add(this,
-                   chrome::NOTIFICATION_BROWSER_CLOSED,
-                   content::Source<Browser>(browser));
-  }
 }
 
 ExtensionUninstallDialog::~ExtensionUninstallDialog() {
@@ -82,9 +72,9 @@ void ExtensionUninstallDialog::ConfirmUninstall(const Extension* extension) {
       extension_, icon_size, ExtensionIconSet::MATCH_BIGGER);
 
   // Load the image asynchronously. The response will be sent to OnImageLoaded.
-  state_ = kImageIsLoading;
   ImageLoader* loader = ImageLoader::Get(profile_);
 
+  SetIcon(gfx::Image());
   std::vector<ImageLoader::ImageRepresentation> images_list;
   images_list.push_back(ImageLoader::ImageRepresentation(
       image,
@@ -123,30 +113,7 @@ void ExtensionUninstallDialog::OnImageLoaded(const std::string& extension_id,
   }
 
   SetIcon(image);
-
-  // Show the dialog unless the browser has been closed while we were waiting
-  // for the image.
-  DCHECK(state_ == kImageIsLoading || state_ == kBrowserIsClosing);
-  if (state_ == kImageIsLoading) {
-    state_ = kDialogIsShowing;
-    Show();
-  }
-}
-
-void ExtensionUninstallDialog::Observe(
-    int type,
-    const content::NotificationSource& source,
-    const content::NotificationDetails& details) {
-  DCHECK(type == chrome::NOTIFICATION_BROWSER_CLOSED);
-
-  browser_ = NULL;
-  // If the browser is closed while waiting for the image, we need to send a
-  // "cancel" event here, because there will not be another opportunity to
-  // notify the delegate of the cancellation as we won't open the dialog.
-  if (state_ == kImageIsLoading) {
-    state_ = kBrowserIsClosing;
-    delegate_->ExtensionUninstallCanceled();
-  }
+  Show();
 }
 
 std::string ExtensionUninstallDialog::GetHeadingText() {

chrome/browser/extensions/extension_uninstall_dialog.h

@@ -7,13 +7,10 @@
 
 #include "base/basictypes.h"
 #include "base/compiler_specific.h"
-#include "base/memory/scoped_ptr.h"
 #include "base/memory/weak_ptr.h"
-#include "content/public/browser/notification_observer.h"
-#include "content/public/browser/notification_registrar.h"
 #include "ui/gfx/image/image_skia.h"
+#include "ui/gfx/native_widget_types.h"
 
-class Browser;
 class Profile;
 
 namespace base {
@@ -28,8 +25,7 @@ namespace extensions {
 class Extension;
 
 class ExtensionUninstallDialog
-    : public content::NotificationObserver,
-      public base::SupportsWeakPtr<ExtensionUninstallDialog> {
+    : public base::SupportsWeakPtr<ExtensionUninstallDialog> {
  public:
   class Delegate {
    public:
@@ -43,12 +39,11 @@ class ExtensionUninstallDialog
     virtual ~Delegate() {}
   };
 
-  // Creates a platform specific implementation of ExtensionUninstallDialog.
-  // |profile| and |delegate| can never be NULL.
-  // |browser| can be NULL only for Ash when this is used with the applist
-  // window.
+  // Creates a platform specific implementation of ExtensionUninstallDialog. The
+  // dialog will be modal to |parent|, or a non-modal dialog if |parent| is
+  // NULL.
   static ExtensionUninstallDialog* Create(Profile* profile,
-                                          Browser* browser,
+                                          gfx::NativeWindow parent,
                                           Delegate* delegate);
 
   virtual ~ExtensionUninstallDialog();
@@ -69,12 +64,15 @@ class ExtensionUninstallDialog
  protected:
   // Constructor used by the derived classes.
   ExtensionUninstallDialog(Profile* profile,
-                           Browser* browser,
+                           gfx::NativeWindow parent,
                            Delegate* delegate);
 
+  // TODO(sashab): Remove protected members: crbug.com/397395
   Profile* const profile_;
 
-  Browser* browser_;
+  // TODO(sashab): Investigate lifetime issue of this window variable:
+  // crbug.com/397396
+  gfx::NativeWindow parent_;
 
   // The delegate we will call Accepted/Canceled on after confirmation dialog.
   Delegate* delegate_;
@@ -96,28 +94,12 @@ class ExtensionUninstallDialog
 
   void OnImageLoaded(const std::string& extension_id, const gfx::Image& image);
 
-  // content::NotificationObserver implementation.
-  virtual void Observe(int type,
-                       const content::NotificationSource& source,
-                       const content::NotificationDetails& details) OVERRIDE;
-
   // Displays the prompt. This should only be called after loading the icon.
   // The implementations of this method are platform-specific.
   virtual void Show() = 0;
 
-  // Keeps track of whether we're still waiting for an image to load before
-  // we show the dialog.
-  enum State {
-    kImageIsLoading,  // Image is loading asynchronously.
-    kDialogIsShowing, // Dialog is shown after image is loaded.
-    kBrowserIsClosing // Browser is closed while image is still loading.
-  };
-  State state_;
-
   base::MessageLoop* ui_loop_;
 
-  content::NotificationRegistrar registrar_;
-
   DISALLOW_COPY_AND_ASSIGN(ExtensionUninstallDialog);
 };
 

chrome/browser/ui/app_list/extension_uninstaller.cc

@@ -32,8 +32,8 @@ void ExtensionUninstaller::Run() {
     return;
   }
   controller_->OnShowChildDialog();
-  dialog_.reset(
-      extensions::ExtensionUninstallDialog::Create(profile_, NULL, this));
+  dialog_.reset(extensions::ExtensionUninstallDialog::Create(
+      profile_, controller_->GetAppListWindow(), this));
   dialog_->ConfirmUninstall(extension);
 }
 

chrome/browser/ui/cocoa/extensions/extension_action_context_menu_controller.mm

@@ -12,6 +12,7 @@
 #include "chrome/browser/extensions/extension_uninstall_dialog.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/ui/browser.h"
+#include "chrome/browser/ui/browser_window.h"
 #include "chrome/browser/ui/chrome_pages.h"
 #include "chrome/browser/ui/cocoa/browser_window_cocoa.h"
 #include "chrome/browser/ui/cocoa/browser_window_controller.h"
@@ -45,7 +46,8 @@ class AsyncUninstaller : public extensions::ExtensionUninstallDialog::Delegate {
       : extension_(extension),
         profile_(browser->profile()) {
     extension_uninstall_dialog_.reset(
-        extensions::ExtensionUninstallDialog::Create(profile_, browser, this));
+        extensions::ExtensionUninstallDialog::Create(
+            profile_, browser->window()->GetNativeWindow(), this));
     extension_uninstall_dialog_->ConfirmUninstall(extension_);
   }
 

chrome/browser/ui/cocoa/extensions/extension_uninstall_dialog_cocoa.mm

@@ -27,10 +27,9 @@ namespace {
 class ExtensionUninstallDialogCocoa
     : public extensions::ExtensionUninstallDialog {
  public:
-  ExtensionUninstallDialogCocoa(
-      Profile* profile,
-      Browser* browser,
-      Delegate* delegate);
+  ExtensionUninstallDialogCocoa(Profile* profile,
+                                gfx::NativeWindow parent,
+                                Delegate* delegate);
   virtual ~ExtensionUninstallDialogCocoa() OVERRIDE;
 
  private:
@@ -39,9 +38,9 @@ class ExtensionUninstallDialogCocoa
 
 ExtensionUninstallDialogCocoa::ExtensionUninstallDialogCocoa(
     Profile* profile,
-    Browser* browser,
+    gfx::NativeWindow parent,
     extensions::ExtensionUninstallDialog::Delegate* delegate)
-    : extensions::ExtensionUninstallDialog(profile, browser, delegate) {
+    : extensions::ExtensionUninstallDialog(profile, parent, delegate) {
 }
 
 ExtensionUninstallDialogCocoa::~ExtensionUninstallDialogCocoa() {}
@@ -74,7 +73,7 @@ void ExtensionUninstallDialogCocoa::Show() {
 // static
 extensions::ExtensionUninstallDialog*
 extensions::ExtensionUninstallDialog::Create(Profile* profile,
-                                             Browser* browser,
+                                             gfx::NativeWindow parent,
                                              Delegate* delegate) {
-  return new ExtensionUninstallDialogCocoa(profile, browser, delegate);
+  return new ExtensionUninstallDialogCocoa(profile, parent, delegate);
 }

chrome/browser/ui/views/apps/app_info_dialog/app_info_footer_panel.cc

@@ -169,7 +169,8 @@ bool AppInfoFooterPanel::CanSetPinnedToShelf() const {
 void AppInfoFooterPanel::UninstallApp() {
   DCHECK(CanUninstallApp());
   extension_uninstall_dialog_.reset(
-      extensions::ExtensionUninstallDialog::Create(profile_, NULL, this));
+      extensions::ExtensionUninstallDialog::Create(
+          profile_, GetWidget()->GetNativeWindow(), this));
   extension_uninstall_dialog_->ConfirmUninstall(app_);
 }
 

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

@@ -9,8 +9,6 @@
 #include "base/strings/string_util.h"
 #include "base/strings/utf_string_conversions.h"
 #include "chrome/browser/ui/app_list/app_list_service.h"
-#include "chrome/browser/ui/browser.h"
-#include "chrome/browser/ui/browser_window.h"
 #include "chrome/browser/ui/views/constrained_window_views.h"
 #include "extensions/common/extension.h"
 #include "grit/generated_resources.h"
@@ -31,20 +29,13 @@ const int kIconSize = 64;
 
 class ExtensionUninstallDialogDelegateView;
 
-// Returns parent window for extension uninstall dialog.
-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:
   ExtensionUninstallDialogViews(
       Profile* profile,
-      Browser* browser,
+      gfx::NativeWindow parent,
       extensions::ExtensionUninstallDialog::Delegate* delegate);
   virtual ~ExtensionUninstallDialogViews();
 
@@ -52,13 +43,10 @@ class ExtensionUninstallDialogViews
   void ExtensionUninstallAccepted();
   void ExtensionUninstallCanceled();
 
-  ExtensionUninstallDialogDelegateView* view() { return view_; }
-
  private:
   virtual void Show() OVERRIDE;
 
   ExtensionUninstallDialogDelegateView* view_;
-  bool show_in_app_list_;
 
   DISALLOW_COPY_AND_ASSIGN(ExtensionUninstallDialogViews);
 };
@@ -70,7 +58,7 @@ class ExtensionUninstallDialogDelegateView : public views::DialogDelegateView {
       ExtensionUninstallDialogViews* dialog_view,
       const extensions::Extension* extension,
       const extensions::Extension* triggering_extension,
-      gfx::ImageSkia* icon);
+      gfx::ImageSkia* image);
   virtual ~ExtensionUninstallDialogDelegateView();
 
   // Called when the ExtensionUninstallDialog has been destroyed to make sure
@@ -111,11 +99,10 @@ 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) {
+    : extensions::ExtensionUninstallDialog(profile, parent, delegate),
+      view_(NULL) {
 }
 
 ExtensionUninstallDialogViews::~ExtensionUninstallDialogViews() {
@@ -127,19 +114,9 @@ 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) {
-    delegate_->ExtensionUninstallCanceled();
-    return;
-  }
-
   view_ = new ExtensionUninstallDialogDelegateView(
       this, extension_, triggering_extension_, &icon_);
-  CreateBrowserModalDialogViews(view_, parent)->Show();
+  CreateBrowserModalDialogViews(view_, parent_)->Show();
 }
 
 void ExtensionUninstallDialogViews::ExtensionUninstallAccepted() {
@@ -158,16 +135,16 @@ ExtensionUninstallDialogDelegateView::ExtensionUninstallDialogDelegateView(
     ExtensionUninstallDialogViews* dialog_view,
     const extensions::Extension* extension,
     const extensions::Extension* triggering_extension,
-    gfx::ImageSkia* icon)
+    gfx::ImageSkia* image)
     : dialog_(dialog_view),
       triggered_by_extension_(triggering_extension != NULL) {
   // Scale down to icon size, but allow smaller icons (don't scale up).
-  gfx::Size size(icon->width(), icon->height());
+  gfx::Size size(image->width(), image->height());
   if (size.width() > kIconSize || size.height() > kIconSize)
     size = gfx::Size(kIconSize, kIconSize);
   icon_ = new views::ImageView();
   icon_->SetImageSize(size);
-  icon_->SetImage(*icon);
+  icon_->SetImage(*image);
   AddChildView(icon_);
 
   heading_ = new views::Label(base::UTF8ToUTF16(dialog_->GetHeadingText()));
@@ -245,7 +222,7 @@ void ExtensionUninstallDialogDelegateView::Layout() {
 // static
 extensions::ExtensionUninstallDialog*
 extensions::ExtensionUninstallDialog::Create(Profile* profile,
-                                             Browser* browser,
+                                             gfx::NativeWindow parent,
                                              Delegate* delegate) {
-  return new ExtensionUninstallDialogViews(profile, browser, delegate);
+  return new ExtensionUninstallDialogViews(profile, parent, delegate);
 }

chrome/browser/ui/webui/extensions/extension_settings_handler.cc

@@ -47,7 +47,9 @@
 #include "chrome/browser/supervised_user/supervised_user_service.h"
 #include "chrome/browser/supervised_user/supervised_user_service_factory.h"
 #include "chrome/browser/tab_contents/background_contents.h"
+#include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/browser_finder.h"
+#include "chrome/browser/ui/browser_window.h"
 #include "chrome/browser/ui/extensions/application_launch.h"
 #include "chrome/browser/ui/webui/extensions/extension_basic_info.h"
 #include "chrome/browser/ui/webui/extensions/extension_icon_source.h"
@@ -1369,7 +1371,8 @@ ExtensionSettingsHandler::GetExtensionUninstallDialog() {
         web_ui()->GetWebContents());
     extension_uninstall_dialog_.reset(
         ExtensionUninstallDialog::Create(extension_service_->profile(),
-                                         browser, this));
+                                         browser->window()->GetNativeWindow(),
+                                         this));
   }
   return extension_uninstall_dialog_.get();
 #else

chrome/browser/ui/webui/ntp/app_launcher_handler.cc

@@ -839,7 +839,9 @@ AppLauncherHandler::GetExtensionUninstallDialog() {
         web_ui()->GetWebContents());
     extension_uninstall_dialog_.reset(
         extensions::ExtensionUninstallDialog::Create(
-            extension_service_->profile(), browser, this));
+            extension_service_->profile(),
+            browser->window()->GetNativeWindow(),
+            this));
   }
   return extension_uninstall_dialog_.get();
 }