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

chrome/browser/banners/app_banner_manager_browsertest.cc

Index: chrome/browser/banners/app_banner_manager_browsertest.cc
diff --git a/chrome/browser/banners/app_banner_manager_browsertest.cc b/chrome/browser/banners/app_banner_manager_browsertest.cc
index ee8fd3113aaf103163d83b7750684b115f843488..9a597206ab2ab3c67e361e664a0af8bbc78c8a62 100644
--- a/chrome/browser/banners/app_banner_manager_browsertest.cc
+++ b/chrome/browser/banners/app_banner_manager_browsertest.cc
@@ -19,6 +19,7 @@
 #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 {
@@ -26,7 +27,9 @@ 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) {}
@@ -35,12 +38,13 @@ class AppBannerManagerTest : public AppBannerManager {
 
   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;
   }
 
@@ -66,13 +70,28 @@ class AppBannerManagerTest : public AppBannerManager {
     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 {
@@ -88,12 +107,19 @@ class AppBannerManagerBrowserTest : public InProcessBrowserTest {
   }
 
  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,
@@ -105,8 +131,12 @@ class AppBannerManagerBrowserTest : public InProcessBrowserTest {
     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
@@ -133,6 +163,7 @@ class AppBannerManagerBrowserTest : public InProcessBrowserTest {
     // 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();
@@ -250,6 +281,18 @@ IN_PROC_BROWSER_TEST_F(AppBannerManagerBrowserTest, PromptBannerInHandler) {
                 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,