Refactored ExtensionUninstallDialog to take a NativeWindow instead of a Browser

chrome/browser/extensions/extension_uninstall_dialog.h

Index: chrome/browser/extensions/extension_uninstall_dialog.h
diff --git a/chrome/browser/extensions/extension_uninstall_dialog.h b/chrome/browser/extensions/extension_uninstall_dialog.h
index 0d2f112a1ad781feb8fb145e165efd98af28205a..10b75950b949b07748b4d6da912c07a177613d3e 100644
--- a/chrome/browser/extensions/extension_uninstall_dialog.h
+++ b/chrome/browser/extensions/extension_uninstall_dialog.h
@@ -12,6 +12,7 @@
 #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;
@@ -43,14 +44,19 @@ 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. No
+  // parameters can be NULL.
   static ExtensionUninstallDialog* Create(Profile* profile,
                                           Browser* browser,
                                           Delegate* delegate);
 
+  // Creates a platform specific implementation of ExtensionUninstallDialog as
+  // a standalone dialog. If |parent| is not NULL, makes the dialog modal to
+  // this parent window.

[not at google - send to devlin, 2014/07/23 01:49:21]

comment still not clear. this method is called "CreateModal" but the comment
indicates that it's only *sometimes* modal. is the parent-NULL situation not the
same as Create()?

should I be thinking of "Create" as "CreateStandalone" because I'm struggling a
bit to keep the inheritance structure of this paged in. I'm leaning on comments.

+  static ExtensionUninstallDialog* CreateModal(Profile* profile,
+                                               gfx::NativeWindow parent,
+                                               Delegate* delegate);
+
   virtual ~ExtensionUninstallDialog();
 
   // This is called to verify whether the uninstallation should proceed.
@@ -67,14 +73,17 @@ class ExtensionUninstallDialog
   std::string GetHeadingText();
 
  protected:
-  // Constructor used by the derived classes.
+  // Constructor used by the derived classes. If |browser| is set, |parent| is
+  // not needed; the native window of |browser| is used instead.

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

If this is too complicated, we can move the GetParent() logic back out of this
constructor - that way, only parent is used to determine the parent window, but
Browser is used for registering the observer.

[not at google - send to devlin, 2014/07/23 01:49:21]

I think it's too complicated, but only because you've phrased it that way.
Perhaps say just "Constructor used by derived classes. If |browser| is non-NULL
then this dialogue will automatically close when |browser| is destroyed". that
decouples it from |parent| and it's less for me to process.

   ExtensionUninstallDialog(Profile* profile,
                            Browser* browser,
+                           gfx::NativeWindow parent,
                            Delegate* delegate);
 
   Profile* const profile_;
 
   Browser* browser_;

[not at google - send to devlin, 2014/07/23 01:49:21]

I don't actually see |browser_| being used anywhere anymore.

+  gfx::NativeWindow parent_;
 
   // The delegate we will call Accepted/Canceled on after confirmation dialog.
   Delegate* delegate_;