Allow banners to trigger on sites which don't register a service worker onload.

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 92 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);
   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 63 matching lines @@
 
 int AppBannerManager::GetIdealPrimaryIconSizeInPx() {
   return InstallableManager::GetMinimumIconSizeInPx();
 }
 
 int AppBannerManager::GetMinimumPrimaryIconSizeInPx() {
   return InstallableManager::GetMinimumIconSizeInPx();
 }
 
 bool AppBannerManager::IsDebugMode() const {
   return is_debug_mode_ ||

[benwells, 2017/06/14 11:39:06]

Nit: This is a bit confusing - IsDebugMode and is_debug_mode_ refer to different
concepts. I think you should change is_debug_mode_ to something like
from_devtools_ or something to clarify, but I'd be cool with a TODO for now. 

[dominickn, 2017/06/15 04:16:07]

Done.

          base::CommandLine::ForCurrentProcess()->HasSwitch(
              switches::kBypassAppBannerEngagementChecks);
 }
 
 bool AppBannerManager::IsWebAppInstalled(
     content::BrowserContext* browser_context,
     const GURL& start_url,
     const GURL& manifest_url) {
   return false;
 }
 
 void AppBannerManager::OnDidGetManifest(const InstallableData& data) {
+  UpdateState(State::ACTIVE);
   if (data.error_code != NO_ERROR_DETECTED) {
     ReportStatus(web_contents(), data.error_code);
     Stop();
   }
 
   if (!is_active())
     return;
 
   DCHECK(!data.manifest_url.is_empty());
   DCHECK(!data.manifest.IsEmpty());
@@ skipping 11 matching lines @@
   PerformInstallableCheck();
 }
 
 InstallableParams AppBannerManager::ParamsToPerformInstallableCheck() {
   InstallableParams params;
   params.ideal_primary_icon_size_in_px = GetIdealPrimaryIconSizeInPx();
   params.minimum_primary_icon_size_in_px = GetMinimumPrimaryIconSizeInPx();
   params.check_installable = true;
   params.fetch_valid_primary_icon = true;
 
+  // Don't wait for the worker if we're in debug mode. However, we want to
+  // ignore whether the bypass flag is on for this purpose (otherwise we won't
+  // wait for the worker in that instance).

[benwells, 2017/06/14 11:39:06]

Maybe it's just late but this confuses me. 

Edit: After seeing the rest of the patch I understand now. I think just renaming
is_debug_mode_ to from_devools_ will make it pretty obvious why you just want to
check that and not the full IsDebugMode.

[dominickn, 2017/06/15 04:16:07]

Done.

+  params.wait_for_worker = !is_debug_mode_;
+
   return params;
 }
 
 void AppBannerManager::PerformInstallableCheck() {
   if (!CheckIfShouldShowBanner())
     return;
 
   // Fetch and verify the other required information.
+  UpdateState(State::PENDING_INSTALLABLE_CHECK);
   manager_->GetData(ParamsToPerformInstallableCheck(),
                     base::Bind(&AppBannerManager::OnDidPerformInstallableCheck,
                                GetWeakPtr()));
 }
 
 void AppBannerManager::OnDidPerformInstallableCheck(
     const InstallableData& data) {
+  UpdateState(State::ACTIVE);
   if (data.is_installable)
     TrackDisplayEvent(DISPLAY_EVENT_WEB_APP_BANNER_REQUESTED);
 
   if (data.error_code != NO_ERROR_DETECTED) {
     if (data.error_code == NO_MATCHING_SERVICE_WORKER)
       TrackDisplayEvent(DISPLAY_EVENT_LACKS_SERVICE_WORKER);
 
     ReportStatus(web_contents(), data.error_code);
     Stop();
   }
@@ skipping 55 matching lines @@
 
 void AppBannerManager::Stop() {
   if (state_ == State::PENDING_EVENT && !page_requested_prompt_) {
     TrackBeforeInstallEvent(
         BEFORE_INSTALL_EVENT_PROMPT_NOT_CALLED_AFTER_PREVENT_DEFAULT);
     ReportStatus(web_contents(), RENDERER_CANCELLED);
   } else if (state_ == State::PENDING_ENGAGEMENT &&
              !has_sufficient_engagement_) {
     TrackDisplayEvent(DISPLAY_EVENT_NOT_VISITED_ENOUGH);
     ReportStatus(web_contents(), INSUFFICIENT_ENGAGEMENT);
+  } else if (state_ == State::PENDING_MANIFEST) {
+    ReportStatus(web_contents(), WAITING_FOR_MANIFEST);
+  } else if (state_ == State::PENDING_INSTALLABLE_CHECK) {
+    ReportStatus(web_contents(), WAITING_FOR_INSTALLABLE_CHECK);
   }
 
   // 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 a callback from the
-  // InstallableManager (e.g. the renderer crashes or the browser is shutting
-  // down). These situations are explicitly not logged.
-  DCHECK(!need_to_log_status_ || is_active());
+  // ReportStatus() and set need_to_log_status_ to false.
+  DCHECK(!need_to_log_status_);
 
   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();
 
   TrackBeforeInstallEvent(BEFORE_INSTALL_EVENT_CREATED);
   event_request_id_ = ++gCurrentRequestID;
 
   web_contents()->GetMainFrame()->GetRemoteInterfaces()->GetInterface(
@@ skipping 204 matching lines @@
   if (is_pending_event()) {
     // 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;
   }
 }
 
 }  // namespace banners