Split AppBannerManager::Stop() into two methods.

This means that ReportStatus is only called in one place
(apart from AppBannerManagerDesktop::ShowBannerUi), making it easier
to verify that it is called the appropriate number of times.

It also removes the need for hacks such as pre-updating the state to COMPLETE
in AppBannerManager::DisplayAppBanner.


Bug: 770016
Change-Id: Iedf93b7a3c6c211b2188032259906bd49bcd4555
Reviewed-on: https://chromium-review.googlesource.com/691494
Commit-Queue: Michael McGreevy <mcgreevy@chromium.org>
Reviewed-by: Dominick Ng <dominickn@chromium.org>
Cr-Commit-Position: refs/heads/master@{#505334}

chrome/browser/android/banners/app_banner_manager_android.cc

@@ -174,6 +174,8 @@ void AppBannerManagerAndroid::PerformInstallableCheck() {
     for (const auto& application : manifest_.related_applications) {
       std::string platform = base::UTF16ToUTF8(application.platform.string());
       std::string id = base::UTF16ToUTF8(application.id.string());
+      // TODO(crbug/770050): convert CanHandleNonWebApp() to return an
+      // InstallableStatusCode and pass it to StopWithCode here.
       if (CanHandleNonWebApp(platform, application.url, id))
         return;
     }
@@ -182,8 +184,7 @@ void AppBannerManagerAndroid::PerformInstallableCheck() {
 
   if (can_install_webapk_) {
     if (!AreWebManifestUrlsWebApkCompatible(manifest_)) {
-      ReportStatus(web_contents(), URL_NOT_SUPPORTED_FOR_WEBAPK);
-      Stop();
+      StopWithCode(URL_NOT_SUPPORTED_FOR_WEBAPK);
       return;
     }
   }
@@ -206,8 +207,7 @@ void AppBannerManagerAndroid::OnDidPerformInstallableCheck(
 
 void AppBannerManagerAndroid::OnAppIconFetched(const SkBitmap& bitmap) {
   if (bitmap.drawsNothing()) {
-    ReportStatus(web_contents(), NO_ICON_AVAILABLE);
-    Stop();
+    StopWithCode(NO_ICON_AVAILABLE);
     return;
   }
 

chrome/browser/banners/app_banner_manager.cc

@@ -87,8 +87,7 @@ void AppBannerManager::RequestAppBanner(const GURL& validated_url,
   }
 
   if (code != NO_ERROR_DETECTED) {
-    ReportStatus(contents, code);
-    Stop();
+    StopWithCode(code);
     return;
   }
 
@@ -183,8 +182,7 @@ bool AppBannerManager::IsWebAppInstalled(
 void AppBannerManager::OnDidGetManifest(const InstallableData& data) {
   UpdateState(State::ACTIVE);
   if (data.error_code != NO_ERROR_DETECTED) {
-    ReportStatus(web_contents(), data.error_code);
-    Stop();
+    StopWithCode(data.error_code);
     return;
   }
 
@@ -229,8 +227,7 @@ void AppBannerManager::OnDidPerformInstallableCheck(
     if (data.error_code == NO_MATCHING_SERVICE_WORKER)
       TrackDisplayEvent(DISPLAY_EVENT_LACKS_SERVICE_WORKER);
 
-    ReportStatus(web_contents(), data.error_code);
-    Stop();
+    StopWithCode(data.error_code);
     return;
   }
 
@@ -287,7 +284,6 @@ void AppBannerManager::ResetCurrentPageData() {
 }
 
 void AppBannerManager::Stop() {
-  // Record the status if we are currently waiting for data.
   InstallableStatusCode code = NO_ERROR_DETECTED;
   switch (state_) {
     case State::PENDING_PROMPT:
@@ -314,7 +310,10 @@ void AppBannerManager::Stop() {
     case State::COMPLETE:
       break;
   }
+  StopWithCode(code);
+}
 
+void AppBannerManager::StopWithCode(InstallableStatusCode code) {
   if (code != NO_ERROR_DETECTED)
     ReportStatus(web_contents(), code);
 
@@ -516,8 +515,7 @@ bool AppBannerManager::CheckIfShouldShowBanner() {
       default:
         NOTREACHED();
     }
-    ReportStatus(contents, code);
-    Stop();
+    StopWithCode(code);
     return false;
   }
   return true;
@@ -585,12 +583,7 @@ void AppBannerManager::ShowBanner() {
 
 void AppBannerManager::DisplayAppBanner(bool user_gesture) {
   if (IsExperimentalAppBannersEnabled() && !user_gesture) {
-    ReportStatus(web_contents(), NO_GESTURE);
-
-    // The state is manually set to COMPLETE before calling Stop, because
-    // otherwise Stop will complain that the status has already been reported.
-    UpdateState(State::COMPLETE);
-    Stop();
+    StopWithCode(NO_GESTURE);
     return;
   }
 

chrome/browser/banners/app_banner_manager.h

@@ -192,10 +192,12 @@ class AppBannerManager : public content::WebContentsObserver,
   // Resets all fetched data for the current page.
   virtual void ResetCurrentPageData();
 
+  void Stop();
+
   // Stops the banner pipeline, preventing any outstanding callbacks from
   // running and resetting the manager state. This method is virtual to allow
   // tests to intercept it and verify correct behaviour.
-  virtual void Stop();
+  virtual void StopWithCode(InstallableStatusCode code);
 
   // Sends a message to the renderer that the page has met the requirements to
   // show a banner. The page can respond to cancel the banner (and possibly

chrome/browser/banners/app_banner_manager_browsertest.cc

@@ -82,12 +82,12 @@ class AppBannerManagerTest : public AppBannerManager {
   }
 
  protected:
-  // All calls to RequestAppBanner should terminate in one of Stop() (not
-  // showing banner), UpdateState(State::PENDING_ENGAGEMENT) (waiting for
+  // All calls to RequestAppBanner should terminate in one of StopWithCode()
+  // (not showing banner), UpdateState(State::PENDING_ENGAGEMENT) (waiting for
   // sufficient engagement), or ShowBannerUi(). Override these methods to
   // capture test status.
-  void Stop() override {
-    AppBannerManager::Stop();
+  void StopWithCode(InstallableStatusCode code) override {
+    AppBannerManager::StopWithCode(code);
     ASSERT_FALSE(will_show_.get());
     will_show_.reset(new bool(false));
     base::ThreadTaskRunnerHandle::Get()->PostTask(FROM_HERE, quit_closure_);