Refactored ExtensionUninstallDialog to take a NativeWindow instead of a Browser

chrome/browser/extensions/extension_uninstall_dialog.h

 // 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.
 
 #ifndef CHROME_BROWSER_EXTENSIONS_EXTENSION_UNINSTALL_DIALOG_H_
 #define CHROME_BROWSER_EXTENSIONS_EXTENSION_UNINSTALL_DIALOG_H_
 
 #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 {
 class MessageLoop;
 }
 
 namespace gfx {
 class Image;
@@ skipping 11 matching lines @@
     // We call this method to signal that the uninstallation should continue.
     virtual void ExtensionUninstallAccepted() = 0;
 
     // We call this method to signal that the uninstallation should stop.
     virtual void ExtensionUninstallCanceled() = 0;
 
    protected:
     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. No
+  // parameters can be NULL.
   static ExtensionUninstallDialog* Create(Profile* profile,
                                           Browser* browser,
                                           Delegate* delegate);
 
+  // Creates a platform specific implementation of ExtensionUninstallDialog as
+  // a modal to the given parent window. If |parent| is NULL, creates a

[not at google - send to devlin, 2014/07/21 20:07:48]

I find this comment a bit confusing, since you say that it's modal to the given
parent window, but parent can be NULL. if parent is NULL does that mean that
it's modal to the entire desktop? does it mean it's not modal at all?

[sashab, 2014/07/22 00:30:43]

Yeah... It's a tradeoff between making another method, and keeping this one with
the nullable parameter. I've changed the comment for now, but I'm happy to make
CreateStandaloneDialog again, which just has the second parameter as NULL.

+  // standalone dialog.
+  static ExtensionUninstallDialog* CreateModal(Profile* profile,
+                                               gfx::NativeWindow parent,
+                                               Delegate* delegate);
+
   virtual ~ExtensionUninstallDialog();
 
   // This is called to verify whether the uninstallation should proceed.
   // Starts the process of showing a confirmation UI, which is split into two.
   // 1) Set off a 'load icon' task.
   // 2) Handle the load icon response and show the UI (OnImageLoaded).
   void ConfirmUninstall(const Extension* extension);
 
   // This shows the same dialog as above, except it also shows which extension
   // triggered the dialog by calling chrome.management.uninstall API.
   void ConfirmProgrammaticUninstall(const Extension* extension,
                                     const Extension* triggering_extension);
 
   std::string GetHeadingText();
 
  protected:
   // Constructor used by the derived classes.
   ExtensionUninstallDialog(Profile* profile,
-                           Browser* browser,
+                           gfx::NativeWindow parent,
                            Delegate* delegate);
 
+  // Registers this class as an observer for when the browser closes.
+  virtual void RegisterObserver(Browser* browser);

[not at google - send to devlin, 2014/07/21 20:07:48]

I prefer the Browser-in-constructor approach to needing this. The inheritance
and construction behaviour is pretty convoluted already.

[sashab, 2014/07/22 00:30:43]

Agreed. In that case, I also moved the GetParent() logic into the
ExtensionUninstallDialog constructor, but now the constructor is more
complicated. It's a tough call...

+
   Profile* const profile_;
 
-  Browser* browser_;
+  gfx::NativeWindow parent_;
 
   // The delegate we will call Accepted/Canceled on after confirmation dialog.
   Delegate* delegate_;
 
   // The extension we are showing the dialog for.
   const Extension* extension_;
 
   // The extension triggering the dialog if the dialog was shown by
   // chrome.management.uninstall.
   const Extension* triggering_extension_;
@@ skipping 29 matching lines @@
   base::MessageLoop* ui_loop_;
 
   content::NotificationRegistrar registrar_;
 
   DISALLOW_COPY_AND_ASSIGN(ExtensionUninstallDialog);
 };
 
 }  // namespace extensions
 
 #endif  // CHROME_BROWSER_EXTENSIONS_EXTENSION_UNINSTALL_DIALOG_H_