Update app banner state machine to use more states.

chrome/browser/banners/app_banner_manager.cc

 // Copyright 2015 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 "chrome/browser/banners/app_banner_manager.h"
 
 #include <algorithm>
 
 #include "base/bind.h"
 #include "base/command_line.h"
@@ skipping 45 matching lines @@
 void AppBannerManager::SetTotalEngagementToTrigger(double engagement) {
   AppBannerSettingsHelper::SetTotalEngagementToTrigger(engagement);
 }
 
 void AppBannerManager::RequestAppBanner(const GURL& validated_url,
                                         bool is_debug_mode) {
   content::WebContents* contents = web_contents();
 
   // The only time we should start the pipeline while it is already running is
   // if it's been triggered from devtools.
-  if (is_active_or_pending()) {
+  if (state_ != State::INACTIVE) {
     DCHECK(is_debug_mode);
     weak_factory_.InvalidateWeakPtrs();
   }
 
   UpdateState(State::ACTIVE);
   triggered_by_devtools_ = is_debug_mode;
-  page_requested_prompt_ = false;
 
   // We only need to call ReportStatus if we aren't in debug mode (this avoids
   // skew from testing).
   DCHECK(!need_to_log_status_);
   need_to_log_status_ = !IsDebugMode();
 
   // Exit if this is an incognito window, non-main frame, or insecure context.
   InstallableStatusCode code = NO_ERROR_DETECTED;
   if (Profile::FromBrowserContext(contents->GetBrowserContext())
           ->IsOffTheRecord()) {
@@ skipping 10 matching lines @@
     return;
   }
 
   if (validated_url_.is_empty())
     validated_url_ = validated_url;
 
   // Any existing binding is invalid when we request a new banner.
   if (binding_.is_bound())
     binding_.Close();
 
-  UpdateState(State::PENDING_MANIFEST);
+  UpdateState(State::FETCHING_MANIFEST);
   manager_->GetData(
       ParamsToGetManifest(),
       base::Bind(&AppBannerManager::OnDidGetManifest, GetWeakPtr()));
 }
 
 void AppBannerManager::OnInstall() {
   blink::mojom::InstallationServicePtr installation_service;
   web_contents()->GetMainFrame()->GetRemoteInterfaces()->GetInterface(
       mojo::MakeRequest(&installation_service));
   DCHECK(installation_service);
@@ skipping 23 matching lines @@
 AppBannerManager::AppBannerManager(content::WebContents* web_contents)
     : content::WebContentsObserver(web_contents),
       SiteEngagementObserver(SiteEngagementService::Get(
           Profile::FromBrowserContext(web_contents->GetBrowserContext()))),
       state_(State::INACTIVE),
       manager_(nullptr),
       event_request_id_(-1),
       binding_(this),
       has_sufficient_engagement_(false),
       load_finished_(false),
-      page_requested_prompt_(false),
       triggered_by_devtools_(false),
       need_to_log_status_(false),
       weak_factory_(this) {
   // Ensure the InstallableManager exists since we have a hard dependency on it.
   InstallableManager::CreateForWebContents(web_contents);
   manager_ = InstallableManager::FromWebContents(web_contents);
   DCHECK(manager_);
 
   AppBannerSettingsHelper::UpdateFromFieldTrial();
 }
@@ skipping 150 matching lines @@
   manifest_ = content::Manifest();
   manifest_url_ = GURL();
   validated_url_ = GURL();
   referrer_.erase();
 }
 
 void AppBannerManager::Stop() {
   // Record the status if we are currently waiting for data.
   InstallableStatusCode code = NO_ERROR_DETECTED;
   switch (state_) {
-    case State::PENDING_EVENT:
-      if (!page_requested_prompt_) {
-        TrackBeforeInstallEvent(
-            BEFORE_INSTALL_EVENT_PROMPT_NOT_CALLED_AFTER_PREVENT_DEFAULT);
-        code = RENDERER_CANCELLED;
-      }
+    case State::PENDING_PROMPT:
+      TrackBeforeInstallEvent(
+          BEFORE_INSTALL_EVENT_PROMPT_NOT_CALLED_AFTER_PREVENT_DEFAULT);
+      code = RENDERER_CANCELLED;
       break;
     case State::PENDING_ENGAGEMENT:
       if (!has_sufficient_engagement_) {
         TrackDisplayEvent(DISPLAY_EVENT_NOT_VISITED_ENOUGH);
         code = INSUFFICIENT_ENGAGEMENT;
       }
       break;
-    case State::PENDING_MANIFEST:
+    case State::FETCHING_MANIFEST:
       code = WAITING_FOR_MANIFEST;
       break;
     case State::PENDING_INSTALLABLE_CHECK:
       code = WAITING_FOR_INSTALLABLE_CHECK;
       break;
     case State::ACTIVE:
+    case State::SENDING_EVENT:
+    case State::SENDING_EVENT_GOT_EARLY_PROMPT:
     case State::INACTIVE:
     case State::COMPLETE:
       break;
   }
 
   if (code != NO_ERROR_DETECTED)
     ReportStatus(web_contents(), code);
 
   // In every non-debug run through the banner pipeline, we should have called
   // ReportStatus() and set need_to_log_status_ to false. The only case where
   // we don't is if we're still active and waiting for native app data, which is
   // explicitly not logged.
   DCHECK(!need_to_log_status_ || is_active());
 
   weak_factory_.InvalidateWeakPtrs();
   binding_.Close();
   controller_.reset();
   event_.reset();
 
   UpdateState(State::COMPLETE);
   need_to_log_status_ = false;
   has_sufficient_engagement_ = false;
 }
 
 void AppBannerManager::SendBannerPromptRequest() {
   RecordCouldShowBanner();
 
+  UpdateState(State::SENDING_EVENT);
   TrackBeforeInstallEvent(BEFORE_INSTALL_EVENT_CREATED);
   event_request_id_ = ++gCurrentRequestID;
 
   web_contents()->GetMainFrame()->GetRemoteInterfaces()->GetInterface(
       mojo::MakeRequest(&controller_));
 
   blink::mojom::AppBannerServicePtr banner_proxy;
   binding_.Bind(mojo::MakeRequest(&banner_proxy));
   controller_->BannerPromptRequest(
       std::move(banner_proxy), mojo::MakeRequest(&event_), {GetBannerType()},
       base::Bind(&AppBannerManager::OnBannerPromptReply, GetWeakPtr()));
 }
 
 void AppBannerManager::UpdateState(State state) {
   state_ = state;
-
-  // If we are PENDING_EVENT, we must have sufficient engagement.
-  DCHECK(!is_pending_event() || has_sufficient_engagement_);

[benwells, 2017/06/27 22:55:58]

We could put this back in, but it seems like there are lots of invariants we
could check here. Not sure if this one is particularly fragile.

 }
 
 void AppBannerManager::DidStartNavigation(content::NavigationHandle* handle) {
   if (!handle->IsInMainFrame() || handle->IsSameDocument())
     return;
 
-  if (is_active_or_pending())
+  if (state_ != State::COMPLETE && state_ != State::INACTIVE)
     Stop();
   UpdateState(State::INACTIVE);
   load_finished_ = false;
   has_sufficient_engagement_ = false;
-  page_requested_prompt_ = false;
 }
 
 void AppBannerManager::DidFinishNavigation(content::NavigationHandle* handle) {
   if (handle->IsInMainFrame() && handle->HasCommitted() &&
       !handle->IsSameDocument()) {
     ResetCurrentPageData();
   }
 }
 
 void AppBannerManager::DidFinishLoad(
@@ skipping 12 matching lines @@
   // Additionally, if the page already has enough engagement, trigger the
   // pipeline immediately.
   if (AppBannerSettingsHelper::HasSufficientEngagement(0) ||
       AppBannerSettingsHelper::HasSufficientEngagement(
           GetSiteEngagementService()->GetScore(validated_url))) {
     has_sufficient_engagement_ = true;
   }
 
   // Start the pipeline immediately if we pass (or bypass) the engagement check,
   // or if the feature to run the installability check on page load is enabled.
-  if (!is_active_or_pending() &&
+  if (state_ == State::INACTIVE &&
       (has_sufficient_engagement_ ||
        base::FeatureList::IsEnabled(
            features::kCheckInstallabilityForBannerOnLoad))) {
     RequestAppBanner(validated_url, false /* is_debug_mode */);
   }
 }
 
 void AppBannerManager::MediaStartedPlaying(const MediaPlayerInfo& media_info,
                                            const MediaPlayerId& id) {
   active_media_players_.push_back(id);
 }
 
 void AppBannerManager::MediaStoppedPlaying(const MediaPlayerInfo& media_info,
                                            const MediaPlayerId& id) {
   active_media_players_.erase(std::remove(active_media_players_.begin(),
                                           active_media_players_.end(), id),
                               active_media_players_.end());
 }
 
 void AppBannerManager::WebContentsDestroyed() {
   Stop();
 }
 
 void AppBannerManager::OnEngagementIncreased(content::WebContents* contents,
                                              const GURL& url,
                                              double score) {
-  // In the ACTIVE state, we may have triggered the installability check, but
-  // not checked engagement yet. In the INACTIVE or PENDING_ENGAGEMENT states,
-  // we are waiting for an engagement signal to trigger the pipeline.
-  if (is_complete() || is_pending_event())
-    return;
-

[benwells, 2017/06/27 22:55:58]

I think this is unnecessary due to the logic in the rest of the function.

   // Only trigger a banner using site engagement if:
   //  1. engagement increased for the web contents which we are attached to; and
   //  2. there are no currently active media players; and
   //  3. we have accumulated sufficient engagement.
   if (web_contents() == contents && active_media_players_.empty() &&
       AppBannerSettingsHelper::HasSufficientEngagement(score)) {
     has_sufficient_engagement_ = true;
 
     if (is_pending_engagement()) {
       // We have already finished the installability eligibility checks. Proceed
       // directly to sending the banner prompt request.
       UpdateState(State::ACTIVE);
       SendBannerPromptRequest();
-    } else if (load_finished_ && !is_active_or_pending()) {
+    } else if (load_finished_ && state_ == State::INACTIVE) {
       // This performs some simple tests and starts async checks to test
       // installability. It should be safe to start in response to user input.
       // Don't call if we're already working on processing a banner request.
       RequestAppBanner(url, false /* is_debug_mode */);
     }
   }
 }
 
 void AppBannerManager::RecordCouldShowBanner() {
   content::WebContents* contents = web_contents();
@@ skipping 50 matching lines @@
   // We don't need the controller any more, so reset it so the Blink-side object
   // is destroyed.
   controller_.reset();
   content::WebContents* contents = web_contents();
 
   // The renderer might have requested the prompt to be canceled. They may
   // request that it is redisplayed later, so don't Stop() here. However, log
   // that the cancelation was requested, so Stop() can be called if a redisplay
   // isn't asked for.
   //
-  // We use the additional page_requested_prompt_ variable because the redisplay
-  // request may be received *before* the Cancel prompt reply (e.g. if redisplay
-  // is requested in the beforeinstallprompt event handler).
+  // If the redisplay request has not been received already, we stop here and
+  // wait for the prompt function to be called. If the redisplay request has
+  // already been received before cancel was sent (e.g. if redisplay was
+  // requested in the beforeinstallprompt event handle), we keep going and show
+  // the banner immediately.
   referrer_ = referrer;
   if (reply == blink::mojom::AppBannerPromptReply::CANCEL) {
-    UpdateState(State::PENDING_EVENT);
     TrackBeforeInstallEvent(BEFORE_INSTALL_EVENT_PREVENT_DEFAULT_CALLED);
-    if (!page_requested_prompt_)
+    if (state_ == State::SENDING_EVENT) {
+      UpdateState(State::PENDING_PROMPT);
       return;
+    }
+    DCHECK(state_ == State::SENDING_EVENT_GOT_EARLY_PROMPT);
   }
 
-  // If we haven't yet returned, but we're in the PENDING_EVENT state or
-  // |page_requested_prompt_| is true, the page has requested a delayed showing
-  // of the prompt. Otherwise, the prompt was never canceled by the page.
-  if (is_pending_event()) {
+  // If we are still in the SENDING_EVENT state, the prompt was never canceled
+  // by the page. Otherwise the page requested a delayed showing of the prompt.
+  if (state_ == State::SENDING_EVENT) {
+    TrackBeforeInstallEvent(BEFORE_INSTALL_EVENT_NO_ACTION);
+  } else {
     TrackBeforeInstallEvent(
         BEFORE_INSTALL_EVENT_PROMPT_CALLED_AFTER_PREVENT_DEFAULT);
-    UpdateState(State::ACTIVE);

[benwells, 2017/06/27 22:55:58]

I think going to ACTIVE here isn't needed, as we go straight to COMPLETE in a
few lines.

-  } else {
-    TrackBeforeInstallEvent(BEFORE_INSTALL_EVENT_NO_ACTION);
   }
 
   AppBannerSettingsHelper::RecordMinutesFromFirstVisitToShow(
       contents, validated_url_, GetAppIdentifier(), GetCurrentTime());
 
   DCHECK(!manifest_url_.is_empty());
   DCHECK(!manifest_.IsEmpty());
   DCHECK(!primary_icon_url_.is_empty());
   DCHECK(!primary_icon_.drawsNothing());
 
   TrackBeforeInstallEvent(BEFORE_INSTALL_EVENT_COMPLETE);
   ShowBanner();
   UpdateState(State::COMPLETE);
 }
 
 void AppBannerManager::DisplayAppBanner(bool user_gesture) {
-  if (is_pending_event()) {
+  if (state_ == State::PENDING_PROMPT) {
     // Simulate a non-canceled OnBannerPromptReply to show the delayed banner.
     OnBannerPromptReply(blink::mojom::AppBannerPromptReply::NONE, referrer_);
-  } else {
-    // Log that the prompt request was made for when we get the prompt reply.
-    page_requested_prompt_ = true;
+  } else if (state_ == State::SENDING_EVENT) {
+    UpdateState(State::SENDING_EVENT_GOT_EARLY_PROMPT);
   }
 }
 
 }  // namespace banners