Ensure app install is aborted when the install dialog is closed

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

Index: chrome/browser/ui/views/extensions/extension_install_dialog_view_browsertest.cc
diff --git a/chrome/browser/ui/views/extensions/extension_install_dialog_view_browsertest.cc b/chrome/browser/ui/views/extensions/extension_install_dialog_view_browsertest.cc
index 9cb4e9147e6d7bdbd28fac664e10577b729a6348..5bbed06695d6883db1b415754884ae0c51f3cfc9 100644
--- a/chrome/browser/ui/views/extensions/extension_install_dialog_view_browsertest.cc
+++ b/chrome/browser/ui/views/extensions/extension_install_dialog_view_browsertest.cc
@@ -69,35 +69,41 @@ class MockExtensionInstallPrompt : public ExtensionInstallPrompt {
   ExtensionInstallPrompt::Prompt* prompt_;
 };
 
-class ScrollbarTest : public ExtensionBrowserTest {
+class ExtensionInstallDialogViewTestBase : public ExtensionBrowserTest {
  protected:
-  ScrollbarTest();
-  virtual ~ScrollbarTest() {}
+  ExtensionInstallDialogViewTestBase(

[tapted, 2014/09/08 07:42:20]

nit: explicit

[tmdiep, 2014/09/08 08:26:08]

Done.

+      ExtensionInstallPrompt::PromptType prompt_type);
+  virtual ~ExtensionInstallDialogViewTestBase() {}
 
   virtual void SetUpOnMainThread() OVERRIDE;
 
+  ExtensionInstallPrompt::Prompt* prompt() { return prompt_.get(); }
+  content::WebContents* web_contents() { return web_contents_; }
+  MockExtensionInstallPromptDelegate* delegate() { return &delegate_; }
+
   void SetPromptPermissions(std::vector<base::string16> permissions);
   void SetPromptDetails(std::vector<base::string16> details);
   void SetPromptRetainedFiles(std::vector<base::FilePath> files);
 
-  bool IsScrollbarVisible();
-
  private:
   const extensions::Extension* extension_;
   MockExtensionInstallPrompt* install_prompt_;
   scoped_refptr<ExtensionInstallPrompt::Prompt> prompt_;
   content::WebContents* web_contents_;
+  MockExtensionInstallPromptDelegate delegate_;
 };

[tapted, 2014/09/08 07:42:21]

nit: DISALLOW_COPY_AND_ASSIGN(..). The two new classes below should have it too

[tmdiep, 2014/09/08 08:26:08]

Done.

 
-ScrollbarTest::ScrollbarTest() :
-  extension_(NULL),
-  install_prompt_(NULL),
-  prompt_(new ExtensionInstallPrompt::Prompt(
-      ExtensionInstallPrompt::PERMISSIONS_PROMPT)),
-  web_contents_(NULL) {}
+ExtensionInstallDialogViewTestBase::ExtensionInstallDialogViewTestBase(
+    ExtensionInstallPrompt::PromptType prompt_type)
+    : extension_(NULL),
+      install_prompt_(NULL),
+      prompt_(new ExtensionInstallPrompt::Prompt(prompt_type)),
+      web_contents_(NULL) {
+}
 
-void ScrollbarTest::SetUpOnMainThread() {
+void ExtensionInstallDialogViewTestBase::SetUpOnMainThread() {
   ExtensionBrowserTest::SetUpOnMainThread();
+
   extension_ = ExtensionBrowserTest::LoadExtension(test_data_dir_.AppendASCII(
       "install_prompt/permissions_scrollbar_regression"));
 
@@ -118,26 +124,38 @@ void ScrollbarTest::SetUpOnMainThread() {
   this->SetPromptRetainedFiles(std::vector<base::FilePath>());
 }
 
-void ScrollbarTest::SetPromptPermissions(
+void ExtensionInstallDialogViewTestBase::SetPromptPermissions(
     std::vector<base::string16> permissions) {
   prompt_->SetPermissions(permissions);
 }
 
-void ScrollbarTest::SetPromptDetails(
+void ExtensionInstallDialogViewTestBase::SetPromptDetails(
     std::vector<base::string16> details) {
   prompt_->SetPermissionsDetails(details);
 }
 
-void ScrollbarTest::SetPromptRetainedFiles(
+void ExtensionInstallDialogViewTestBase::SetPromptRetainedFiles(
     std::vector<base::FilePath> files) {
   prompt_->set_retained_files(files);
 }
 
+class ScrollbarTest : public ExtensionInstallDialogViewTestBase {
+ protected:
+  ScrollbarTest();
+  virtual ~ScrollbarTest() {}
+
+  bool IsScrollbarVisible();
+};
+
+ScrollbarTest::ScrollbarTest()
+    : ExtensionInstallDialogViewTestBase(
+          ExtensionInstallPrompt::PERMISSIONS_PROMPT) {
+}
+
 bool ScrollbarTest::IsScrollbarVisible() {
-  ExtensionInstallPrompt::ShowParams show_params(web_contents_);
-  MockExtensionInstallPromptDelegate delegate;
-  ExtensionInstallDialogView* dialog =
-      new ExtensionInstallDialogView(show_params.navigator, &delegate, prompt_);
+  ExtensionInstallPrompt::ShowParams show_params(web_contents());
+  ExtensionInstallDialogView* dialog = new ExtensionInstallDialogView(
+      show_params.navigator, delegate(), prompt());
 
   // Create the modal view around the install dialog view.
   views::Widget* modal =
@@ -178,3 +196,64 @@ IN_PROC_BROWSER_TEST_F(ScrollbarTest, ScrollbarRegression) {
   this->SetPromptDetails(details);
   ASSERT_FALSE(IsScrollbarVisible()) << "Scrollbar is visible";
 }
+
+class ExtensionInstallDialogViewTest
+    : public ExtensionInstallDialogViewTestBase {
+ protected:
+  ExtensionInstallDialogViewTest()
+      : ExtensionInstallDialogViewTestBase(
+            ExtensionInstallPrompt::INSTALL_PROMPT) {}
+  virtual ~ExtensionInstallDialogViewTest() {}
+};
+
+// Verifies that the delegate is notified when the user selects to accept or
+// cancel the install.
+IN_PROC_BROWSER_TEST_F(ExtensionInstallDialogViewTest, NotifyDelegate) {
+  {
+    // The user confirms the install.
+    MockExtensionInstallPromptDelegate delegate;
+    scoped_ptr<ExtensionInstallDialogView> dialog(
+        new ExtensionInstallDialogView(web_contents(), &delegate, prompt()));
+    views::DialogDelegateView* delegate_view =
+        static_cast<views::DialogDelegateView*>(dialog.get());

[tapted, 2014/09/08 07:42:20]

I think you can just assign it rather than static_cast to get at the
made-private overrides. There's also a thing in macros.h that would let you do

  implicit_cast<views::DialogDelegateView*>(dialog.get())->Accept();

[tmdiep, 2014/09/08 08:26:08]

Done.

+
+    delegate_view->Accept();
+    delegate_view->OnClosed();
+    dialog.reset();

[tapted, 2014/09/08 07:42:20]

Can you let this just go out of scope?

[tmdiep, 2014/09/08 08:26:08]

I wanted to make sure that the dialog is completely destroyed before checking
the proceed and abort counts (in case something gets inserted in the destructor
in the future).

+
+    ASSERT_EQ(0, delegate.abort_count());

[tapted, 2014/09/08 07:42:20]

nit: ASSERT->EXPECT. Typically I see ASSERT just for when later code is
guaranteed to crash, otherwise the extra logging in a failure can be handy.

[tmdiep, 2014/09/08 08:26:08]

Done.

+    ASSERT_EQ(1, delegate.proceed_count());
+  }
+
+  {
+    // The user cancels the install.
+    MockExtensionInstallPromptDelegate delegate;
+    scoped_ptr<ExtensionInstallDialogView> dialog(
+        new ExtensionInstallDialogView(web_contents(), &delegate, prompt()));
+    views::DialogDelegateView* delegate_view =
+        static_cast<views::DialogDelegateView*>(dialog.get());
+
+    delegate_view->Cancel();
+    delegate_view->OnClosed();
+    dialog.reset();
+
+    ASSERT_EQ(1, delegate.abort_count());
+    ASSERT_EQ(0, delegate.proceed_count());
+  }
+
+  {
+    // Corner case: Dialog is closed without the user explicitly choosing to
+    // proceed or cancel.
+    MockExtensionInstallPromptDelegate delegate;
+    scoped_ptr<ExtensionInstallDialogView> dialog(
+        new ExtensionInstallDialogView(web_contents(), &delegate, prompt()));
+    views::DialogDelegateView* delegate_view =
+        static_cast<views::DialogDelegateView*>(dialog.get());
+
+    delegate_view->OnClosed();

[tapted, 2014/09/08 07:42:20]

The test might be more compelling if you can do dialog->GetWidget()->CloseNow();
I .. think all the bits are in the test harness for that to work, but the
lifetime of these things is sometimes odd in tests.

[tmdiep, 2014/09/08 08:26:08]

Yeah, I agree that it would be a better test of the scenario. But to avoid the
risk of a flaky test, maybe I'll just leave as is.

[tapted, 2014/09/08 09:28:56]

CloseNow usually avoids being flaky (Widget::Close needs a runloop, so needs
care, but is otherwise safe from flakiness too).

I guess you're not really getting much extra coverage though.

+    dialog.reset();
+
+    ASSERT_EQ(1, delegate.abort_count());
+    ASSERT_EQ(0, delegate.proceed_count());
+  }
+}