[Webapps]: Clear AppBannerManager::page_requested_prompt_ at start of banner flow

chrome/browser/banners/app_banner_manager_browsertest.cc

 // Copyright 2016 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 <vector>
 
 #include "base/command_line.h"
 #include "base/run_loop.h"
 #include "base/test/histogram_tester.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "chrome/browser/banners/app_banner_manager.h"
 #include "chrome/browser/banners/app_banner_metrics.h"
 #include "chrome/browser/banners/app_banner_settings_helper.h"
 #include "chrome/browser/engagement/site_engagement_service.h"
 #include "chrome/browser/installable/installable_logging.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/tabs/tab_strip_model.h"
 #include "chrome/common/chrome_switches.h"
 #include "chrome/test/base/in_process_browser_test.h"
 #include "chrome/test/base/ui_test_utils.h"
+#include "content/public/browser/web_contents_user_data.h"
 #include "net/test/embedded_test_server/embedded_test_server.h"
 
 namespace banners {
 
 // Browser tests for web app banners.
 // NOTE: this test relies on service workers; failures and flakiness may be due
 // to changes in SW code.
-class AppBannerManagerTest : public AppBannerManager {
+class AppBannerManagerTest
+    : public AppBannerManager,
+      public content::WebContentsUserData<AppBannerManagerTest> {

[dominickn, 2017/02/06 05:22:01]

Not sure if it's necessary to have this full WebContentsUserData in this test (I
can't find another test file that declares a WebContentsUserData subclass). The
right way to do this is to probably move the std::unique_ptr<AppBannerManager>
up a level and pass it in the raw AppBannerManagerTest as a pointer argument.

[pkotwicz, 2017/02/09 00:57:37]

Passing in AppBannerManagerTest to RunBannerTest() is another way of writing my
test. I like this way better because it is closer to how the production code
works.

[dominickn, 2017/02/09 04:49:04]

I'm not really convinced of the closer to how production code works argument (if
we want to do that we'd also simulate user input and so forth).

  public:
   explicit AppBannerManagerTest(content::WebContents* web_contents)
       : AppBannerManager(web_contents) {}
 
   ~AppBannerManagerTest() override {}
 
   bool will_show() { return will_show_.get() && *will_show_; }
 
+  void clear_will_show() { will_show_.reset(); }
+
   bool is_active() { return AppBannerManager::is_active(); }
 
   bool need_to_log_status() { return need_to_log_status_; }
 
   void Prepare(base::Closure quit_closure) {
-    will_show_.reset(nullptr);
     quit_closure_ = quit_closure;
   }
 
  protected:
   // All calls to RequestAppBanner should terminate in one of Stop() (not
   // showing banner) or ShowBanner(). Override those two methods to capture test
   // status.
   void Stop() override {
     AppBannerManager::Stop();
     ASSERT_FALSE(will_show_.get());
     will_show_.reset(new bool(false));
     base::ThreadTaskRunnerHandle::Get()->PostTask(FROM_HERE, quit_closure_);
   }
 
   void ShowBanner() override {
     // Fake the call to ReportStatus here - this is usually called in
     // platform-specific code which is not exposed here.
     ReportStatus(nullptr, SHOWING_WEB_APP_BANNER);
     RecordDidShowBanner("AppBanner.WebApp.Shown");
 
     ASSERT_FALSE(will_show_.get());
     will_show_.reset(new bool(true));
     base::ThreadTaskRunnerHandle::Get()->PostTask(FROM_HERE, quit_closure_);
   }
 
+  void WebContentsDestroyed() override {
+    will_show_.reset(nullptr);
+    AppBannerManager::WebContentsDestroyed();

[dominickn, 2017/02/06 05:22:00]

WebContentsDestroyed() calls Stop(), which sets will_show_ and calls the quit
closure. Why is this needed? (At the very least, why is will_show_ reset?)

[pkotwicz, 2017/02/09 00:57:37]

AppBannerManager::WebContentsDestroyed() always calls Stop() regardless of
whether the banner was shown or not.

I am resetting |will_show_| to avoid failing on the assertion on line 57.

Previously this was not an issue because AppBannerManagerTest's lifetime was
super short so AppBannerManagerTest::WebContentsDestroyed() was not called in
tests

[dominickn, 2017/02/09 04:49:04]

I'm not a huge fan of this (this is partially why I had such a short lifetime
for the test manager previously), but I don't want to block this CL.

+  }
+
  private:
+  friend class content::WebContentsUserData<AppBannerManagerTest>;
+
   bool IsDebugMode() const override { return false; }
 
   base::Closure quit_closure_;
   std::unique_ptr<bool> will_show_;
+
+  DISALLOW_COPY_AND_ASSIGN(AppBannerManagerTest);
 };
 
+}  // namespace banners
+
+DEFINE_WEB_CONTENTS_USER_DATA_KEY(banners::AppBannerManagerTest);
+
+namespace banners {
+
 class AppBannerManagerBrowserTest : public InProcessBrowserTest {
  public:
   void SetUpOnMainThread() override {
     AppBannerSettingsHelper::SetTotalEngagementToTrigger(10);
     ASSERT_TRUE(embedded_test_server()->Start());
     InProcessBrowserTest::SetUpOnMainThread();
   }
 
   void SetUpCommandLine(base::CommandLine* command_line) override {
     // Make sure app banners are disabled in the browser, otherwise they will
     // interfere with the test.
     command_line->AppendSwitch(switches::kDisableAddToShelf);
   }
 
  protected:
+  // Returns a test server URL to page |page_url| with |manifest_url| injected
+  // as the manifest tag.
+  std::string GetURLOfPageWithManifest(const std::string& page_url,
+                                       const std::string& manifest_url) {
+    return page_url + embedded_test_server()->GetURL(manifest_url).spec();
+  }
+
   // Returns a test server URL to a page controlled by a service worker with
   // |manifest_url| injected as the manifest tag.
   std::string GetURLOfPageWithServiceWorkerAndManifest(
       const std::string& manifest_url) {
-    return "/banners/manifest_test_page.html?manifest=" +
-           embedded_test_server()->GetURL(manifest_url).spec();
+    return GetURLOfPageWithManifest(
+        "/banners/manifest_test_page.html?manifest=", manifest_url);
   }
 
   void RunBannerTest(Browser* browser,
                      const std::string& url,
                      const std::vector<double>& engagement_scores,
                      InstallableStatusCode expected_code_for_histogram,
                      bool expected_to_show) {
     base::HistogramTester histograms;
     GURL test_url = embedded_test_server()->GetURL(url);
     content::WebContents* web_contents =
         browser->tab_strip_model()->GetActiveWebContents();
-    std::unique_ptr<AppBannerManagerTest> manager(
-        new AppBannerManagerTest(web_contents));
+
+    // Create manager if one does not already exist.
+    AppBannerManagerTest::CreateForWebContents(web_contents);
+    AppBannerManagerTest* manager =
+        AppBannerManagerTest::FromWebContents(web_contents);
+    manager->clear_will_show();
 
     // Loop through the vector of engagement scores. We only expect the banner
     // pipeline to trigger on the last one; otherwise, nothing is expected to
     // happen.
     int iterations = 0;
     SiteEngagementService* service =
         SiteEngagementService::Get(browser->profile());
     for (double engagement : engagement_scores) {
       if (iterations > 0) {
         ui_test_utils::NavigateToURL(browser, test_url);
 
         EXPECT_EQ(false, manager->will_show());
         EXPECT_FALSE(manager->is_active());
 
         histograms.ExpectTotalCount(banners::kMinutesHistogram, 0);
         histograms.ExpectTotalCount(banners::kInstallableStatusCodeHistogram,
                                     0);
       }
       service->ResetScoreForURL(test_url, engagement);
       ++iterations;
     }
 
     // On the final loop, we expect the banner pipeline to trigger - the
     // navigation should generate the final engagement to show the banner. Spin
     // the run loop, which should be quit by either Stop() or ShowBanner().
     base::RunLoop run_loop;
+    manager->clear_will_show();
     manager->Prepare(run_loop.QuitClosure());
     ui_test_utils::NavigateToURL(browser, test_url);
     run_loop.Run();
 
     EXPECT_EQ(expected_to_show, manager->will_show());
     EXPECT_FALSE(manager->is_active());
 
     // Navigate to ensure the InstallableStatusCodeHistogram is logged.
     ui_test_utils::NavigateToURL(browser, GURL("about:blank"));
 
@@ skipping 97 matching lines @@
   RunBannerTest(browser(), "/banners/prompt_test_page.html", engagement_scores,
                 SHOWING_WEB_APP_BANNER, true);
 }
 
 IN_PROC_BROWSER_TEST_F(AppBannerManagerBrowserTest, PromptBannerInHandler) {
   std::vector<double> engagement_scores{0, 2, 5, 10};
   RunBannerTest(browser(), "/banners/prompt_in_handler_test_page.html",
                 engagement_scores, SHOWING_WEB_APP_BANNER, true);
 }
 
+IN_PROC_BROWSER_TEST_F(AppBannerManagerBrowserTest,
+    CancelBannerAfterPromptInHandler) {
+  std::vector<double> engagement_scores{10};
+  RunBannerTest(browser(), "/banners/prompt_in_handler_test_page.html",
+                engagement_scores, SHOWING_WEB_APP_BANNER, true);
+  std::string cancel_test_page_url = GetURLOfPageWithManifest(
+      "/banners/cancel_test_page.html?manifest=",
+      "/banners/manifest_different_start_url.json");
+  RunBannerTest(browser(), cancel_test_page_url, engagement_scores,
+                RENDERER_CANCELLED, false);
+}
+
 IN_PROC_BROWSER_TEST_F(AppBannerManagerBrowserTest, WebAppBannerInIFrame) {
   std::vector<double> engagement_scores{10};
   RunBannerTest(browser(), "/banners/iframe_test_page.html", engagement_scores,
                 NO_MANIFEST, false);
 }
 
 IN_PROC_BROWSER_TEST_F(AppBannerManagerBrowserTest, DoesNotShowInIncognito) {
   std::vector<double> engagement_scores{10};
   Browser* incognito_browser =
       OpenURLOffTheRecord(browser()->profile(), GURL("about:blank"));
   RunBannerTest(incognito_browser, "/banners/manifest_test_page.html",
                 engagement_scores, IN_INCOGNITO, false);
 }
 
 }  // namespace banners