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 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.
   // 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.
+  // 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_;
 
   // 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_