Ensure app install is aborted when the install dialog is closed

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

 // Copyright 2014 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.
 
 #include "base/run_loop.h"
 #include "base/strings/utf_string_conversions.h"
 #include "chrome/browser/extensions/extension_browsertest.h"
 #include "chrome/browser/extensions/extension_icon_manager.h"
 #include "chrome/browser/extensions/extension_install_prompt.h"
 #include "chrome/browser/extensions/extension_install_prompt_experiment.h"
@@ skipping 51 matching lines @@
     prompt_ = prompt;
   }
   ExtensionInstallPrompt::Prompt* get_prompt() {
     return prompt_;
   }
 
  private:
   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"));
 
   web_contents_ = browser()->tab_strip_model()->GetWebContentsAt(0);
 
   install_prompt_ = new MockExtensionInstallPrompt(web_contents_);
   install_prompt_->set_prompt(prompt_.get());
   prompt_->set_experiment(ExtensionInstallPromptExperiment::ControlGroup());
   prompt_->set_extension(extension_);
 
   scoped_ptr<ExtensionIconManager> icon_manager(new ExtensionIconManager());
   const SkBitmap icon_bitmap = icon_manager->GetIcon(extension_->id());
   gfx::Image icon = gfx::Image::CreateFrom1xBitmap(icon_bitmap);
   prompt_->set_icon(icon);
 
   this->SetPromptPermissions(std::vector<base::string16>());
   this->SetPromptDetails(std::vector<base::string16>());
   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 =
       CreateBrowserModalDialogViews(dialog, show_params.parent_window);
   modal->Show();
   content::BrowserThread::GetBlockingPool()->FlushForTesting();
   base::RunLoop().RunUntilIdle();
 
   // Check if the vertical scrollbar is visible.
   return dialog->scroll_view()->vertical_scroll_bar()->visible();
@@ skipping 20 matching lines @@
   base::string16 permission_string(base::ASCIIToUTF16(
       "Read and modify your data on *.facebook.com"));
   std::vector<base::string16> permissions;
   permissions.push_back(permission_string);
   this->SetPromptPermissions(permissions);
   std::vector<base::string16> details;
   details.push_back(base::string16());
   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());
+  }
+}