Add new app banner metrics using InstallableStatusCode.

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 "base/bind.h"
 #include "base/callback.h"
 #include "base/command_line.h"
 #include "base/strings/string_number_conversions.h"
@@ skipping 75 matching lines @@
 
 // static
 bool AppBannerManager::URLsAreForTheSamePage(const GURL& first,
                                              const GURL& second) {
   return first.GetWithEmptyPath() == second.GetWithEmptyPath() &&
          first.path() == second.path() && first.query() == second.query();
 }
 
 void AppBannerManager::RequestAppBanner(const GURL& validated_url,
                                         bool is_debug_mode) {
+  // Don't start a redundant banner request. Otherwise, if one is running,
+  // invalidate our weak pointers so it terminates.
   content::WebContents* contents = web_contents();
+  if (is_active_) {
+    if (URLsAreForTheSamePage(validated_url, contents->GetLastCommittedURL()))
+      return;
+    else
+      weak_factory_.InvalidateWeakPtrs();
+  }
+
+  is_active_ = true;
+  is_debug_mode_ = is_debug_mode;
+
+  DCHECK(!need_to_log_status_);
+  need_to_log_status_ = !IsDebugMode();
+
   if (contents->GetMainFrame()->GetParent()) {
-    ReportError(contents, NOT_IN_MAIN_FRAME);
+    ReportStatus(contents, NOT_IN_MAIN_FRAME);
+    Stop();
     return;
   }
 
-  // Don't start a redundant banner request.
-  if (is_active_ &&
-      URLsAreForTheSamePage(validated_url, contents->GetLastCommittedURL())) {
-    return;
-  }
-
   // A secure origin is required to show banners, so exit early if we see the
   // URL is invalid.
   if (!content::IsOriginSecure(validated_url) &&
       !gDisableSecureCheckForTesting) {
-    ReportError(contents, NOT_FROM_SECURE_ORIGIN);
+    ReportStatus(contents, NOT_FROM_SECURE_ORIGIN);
+    Stop();
     return;
   }
 
-  is_debug_mode_ = is_debug_mode;
-  is_active_ = true;
-
-  // We start by requesting the manifest from the InstallableManager. The
-  // default-constructed params will have all other fields as false.
   manager_->GetData(
       ParamsToGetManifest(),
       base::Bind(&AppBannerManager::OnDidGetManifest, GetWeakPtr()));
 }
 
 base::Closure AppBannerManager::FetchWebappSplashScreenImageCallback(
     const std::string& webapp_id) {
   return base::Closure();
 }
 
 AppBannerManager::AppBannerManager(content::WebContents* web_contents)
     : content::WebContentsObserver(web_contents),
       SiteEngagementObserver(nullptr),
       manager_(nullptr),
       event_request_id_(-1),
       is_active_(false),
       banner_request_queued_(false),
       load_finished_(false),
       was_canceled_by_page_(false),
       page_requested_prompt_(false),
       is_debug_mode_(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();
 }
 
 AppBannerManager::~AppBannerManager() { }
 
 std::string AppBannerManager::GetAppIdentifier() {
   DCHECK(!manifest_.IsEmpty());
   return manifest_.start_url.spec();
 }
 
 std::string AppBannerManager::GetBannerType() {
   return "web";
 }
 
-std::string AppBannerManager::GetErrorParam(InstallableErrorCode code) {
+std::string AppBannerManager::GetStatusParam(InstallableStatusCode code) {
   if (code == NO_ACCEPTABLE_ICON || code == MANIFEST_MISSING_SUITABLE_ICON) {
     return base::IntToString(InstallableManager::GetMinimumIconSizeInPx());
   }
 
   return std::string();
 }
 
 int AppBannerManager::GetIdealIconSizeInDp() {
   return ConvertIconSizeFromPxToDp(
       InstallableManager::GetMinimumIconSizeInPx());
@@ skipping 15 matching lines @@
 }
 
 bool AppBannerManager::IsWebAppInstalled(
     content::BrowserContext* browser_context,
     const GURL& start_url) {
   return false;
 }
 
 void AppBannerManager::OnDidGetManifest(const InstallableData& data) {
   if (data.error_code != NO_ERROR_DETECTED) {
-    ReportError(web_contents(), data.error_code);
+    ReportStatus(web_contents(), data.error_code);
     Stop();
   }
 
   if (!is_active_)
     return;
 
   DCHECK(!data.manifest_url.is_empty());
   DCHECK(!data.manifest.IsEmpty());
 
   manifest_url_ = data.manifest_url;
   manifest_ = data.manifest;
   app_title_ = (manifest_.name.is_null()) ? manifest_.short_name.string()
                                           : manifest_.name.string();
 
   PerformInstallableCheck();
 }
 
 void AppBannerManager::PerformInstallableCheck() {
   if (IsWebAppInstalled(web_contents()->GetBrowserContext(),
                         manifest_.start_url) &&
       !IsDebugMode()) {
+    ReportStatus(web_contents(), ALREADY_INSTALLED);
     Stop();
   }
 
   if (!is_active_)
     return;
 
   // Fetch and verify the other required information.
   manager_->GetData(ParamsToPerformInstallableCheck(GetIdealIconSizeInDp(),
                                                     GetMinimumIconSizeInDp()),
                     base::Bind(&AppBannerManager::OnDidPerformInstallableCheck,
                                GetWeakPtr()));
 }
 
 void AppBannerManager::OnDidPerformInstallableCheck(
     const InstallableData& data) {
   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);
 
-    ReportError(web_contents(), data.error_code);
+    ReportStatus(web_contents(), data.error_code);
     Stop();
   }
 
   if (!is_active_)
     return;
 
   DCHECK(data.is_installable);
   DCHECK(!data.icon_url.is_empty());
   DCHECK(data.icon);
 
   icon_url_ = data.icon_url;
   icon_.reset(new SkBitmap(*data.icon));
 
   SendBannerPromptRequest();
 }
 
 void AppBannerManager::RecordDidShowBanner(const std::string& event_name) {
   content::WebContents* contents = web_contents();
   DCHECK(contents);
 
   AppBannerSettingsHelper::RecordBannerEvent(
       contents, validated_url_, GetAppIdentifier(),
       AppBannerSettingsHelper::APP_BANNER_EVENT_DID_SHOW,
       GetCurrentTime());
   rappor::SampleDomainAndRegistryFromGURL(g_browser_process->rappor_service(),
                                           event_name,
                                           contents->GetLastCommittedURL());
 }
 
-void AppBannerManager::ReportError(content::WebContents* web_contents,
-                                   InstallableErrorCode code) {
-  if (IsDebugMode())
-    LogErrorToConsole(web_contents, code, GetErrorParam(code));
+void AppBannerManager::ReportStatus(content::WebContents* web_contents,
+                                    InstallableStatusCode code) {
+  if (IsDebugMode()) {
+    LogErrorToConsole(web_contents, code, GetStatusParam(code));
+  } else {
+    // Ensure that we haven't yet logged a status code for this page. Only
+    // log status for pages where we are not in debug mode to avoid skew.
+    DCHECK(need_to_log_status_);
+    TrackInstallableStatusCode(code);
+    need_to_log_status_ = false;
+  }
 }
 
 void AppBannerManager::Stop() {
   if (was_canceled_by_page_ && !page_requested_prompt_) {
     TrackBeforeInstallEvent(
         BEFORE_INSTALL_EVENT_PROMPT_NOT_CALLED_AFTER_PREVENT_DEFAULT);
+    ReportStatus(web_contents(), RENDERER_CANCELLED);
   }
 
+  // Ensure that we've either logged an InstallableStatusCode, or that we're
+  // still active and waiting for a callback, or that we're in debug mode. The
+  // final time a status is logged is within the platform-specific ShowBanner()
+  // implementations, and is_active_ is not reset until those have run and
+  // logged a status.
+  DCHECK(!need_to_log_status_ || is_active_ || IsDebugMode());
+
   is_active_ = false;
   was_canceled_by_page_ = false;
   page_requested_prompt_ = false;
+  need_to_log_status_ = false;

[benwells, 2016/08/11 07:14:23]

I don't understand how this works - won't the DCHECK in ReportStatus fire when
ShowBannder later calls into it?

[dominickn, 2016/08/11 08:13:38]

need_to_log_status_ is set to true in RequestAppBanner, which is the start of
the app banner pipeline. Otherwise, it should not be set to true until that
method is called.

[benwells, 2016/08/12 01:34:23]

OK, as discussed offline there might be some subtle race still if the callbacks
are fired after Stop is called.

   referrer_.erase();
 }
 
 void AppBannerManager::SendBannerPromptRequest() {
   RecordCouldShowBanner();
 
   // Given all of the other checks that have been made, the only possible reason
   // for stopping now is that the app has been added to the homescreen.
-  if (!IsDebugMode() && !CheckIfShouldShowBanner())
+  if (!IsDebugMode() && !CheckIfShouldShowBanner()) {

[benwells, 2016/08/11 07:14:23]

Why is this changing?

[dominickn, 2016/08/11 08:13:38]

The only way is_active_ will be false here is if Stop() is called, since this
method is called synchronously from a location which already checks is_active_
is true.

     Stop();
-
-  if (!is_active_)
     return;
+  }
 
   TrackBeforeInstallEvent(BEFORE_INSTALL_EVENT_CREATED);
   event_request_id_ = ++gCurrentRequestID;
   content::RenderFrameHost* frame = web_contents()->GetMainFrame();
   frame->Send(new ChromeViewMsg_AppBannerPromptRequest(
       frame->GetRoutingID(), event_request_id_, GetBannerType()));
 }
 
 void AppBannerManager::DidStartNavigation(content::NavigationHandle* handle) {
   if (!handle->IsInMainFrame())
@@ skipping 50 matching lines @@
   Stop();
 }
 
 void AppBannerManager::OnEngagementIncreased(content::WebContents* contents,
                                              const GURL& url,
                                              double score) {
   // 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)) {
-    // Stop observing so we don't double-trigger the banner.
-    SiteEngagementObserver::Observe(nullptr);
+  if (web_contents() == contents && active_media_players_.empty()) {
+    if (AppBannerSettingsHelper::HasSufficientEngagement(score)) {

[benwells, 2016/08/11 07:14:23]

Why is this changing?

[dominickn, 2016/08/11 08:13:38]

Ah, I had a ReportStatus branch here that was removed in the last patchset since
this is before a banner is actually requested. Putting it back the way it was.

+      // Stop observing so we don't double-trigger the banner.
+      SiteEngagementObserver::Observe(nullptr);
 
-    if (!load_finished_) {
-      // Wait until the main frame finishes loading before requesting a banner.
-      banner_request_queued_ = true;
-    } else {
-      // Requesting a banner performs some simple tests, creates a data fetcher,
-      // and starts some asynchronous checks to test installability. It should
-      // be safe to start this in response to user input.
-      RequestAppBanner(url, false /* is_debug_mode */);
+      if (!load_finished_) {
+        // Queue the banner request until the main frame finishes loading.
+        banner_request_queued_ = true;
+      } else {
+        // A banner request performs some simple tests, creates a data fetcher,
+        // and starts some asynchronous checks to test installability. It should
+        // be safe to start this in response to user input.
+        RequestAppBanner(url, false /* is_debug_mode */);
+      }
     }
   }
 }
 
 void AppBannerManager::RecordCouldShowBanner() {
   content::WebContents* contents = web_contents();
   DCHECK(contents);
 
   AppBannerSettingsHelper::RecordBannerCouldShowEvent(
       contents, validated_url_, GetAppIdentifier(),
       GetCurrentTime(), last_transition_type_);
 }
 
 bool AppBannerManager::CheckIfShouldShowBanner() {
   content::WebContents* contents = web_contents();
   DCHECK(contents);
 
-  return AppBannerSettingsHelper::ShouldShowBanner(
+  InstallableStatusCode code = AppBannerSettingsHelper::ShouldShowBanner(
       contents, validated_url_, GetAppIdentifier(), GetCurrentTime());
+  if (code == NO_ERROR_DETECTED)
+    return true;
+
+  DCHECK(!IsDebugMode());

[benwells, 2016/08/11 07:14:23]

Could you have a comment explaining why this DCHECK is here? It isn't
immediately obvious.

[dominickn, 2016/08/11 08:13:38]

Done.

+  ReportStatus(web_contents(), code);
+  return false;
 }
 
 bool AppBannerManager::OnMessageReceived(
     const IPC::Message& message,
     content::RenderFrameHost* render_frame_host) {
   bool handled = true;
 
   IPC_BEGIN_MESSAGE_MAP_WITH_PARAM(AppBannerManager, message, render_frame_host)
     IPC_MESSAGE_HANDLER(ChromeViewHostMsg_AppBannerPromptReply,
                         OnBannerPromptReply)
@@ skipping 20 matching lines @@
   // 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).
   referrer_ = referrer;
   if (reply == blink::WebAppBannerPromptReply::Cancel &&
       !page_requested_prompt_) {
     TrackBeforeInstallEvent(BEFORE_INSTALL_EVENT_PREVENT_DEFAULT_CALLED);
     was_canceled_by_page_ = true;
-    ReportError(contents, RENDERER_CANCELLED);
     return;
   }
 
   // If we haven't yet returned, but either of |was_canceled_by_page_| 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 (was_canceled_by_page_ || page_requested_prompt_) {
     TrackBeforeInstallEvent(
         BEFORE_INSTALL_EVENT_PROMPT_CALLED_AFTER_PREVENT_DEFAULT);
     was_canceled_by_page_ = false;
   } else {
     TrackBeforeInstallEvent(BEFORE_INSTALL_EVENT_NO_ACTION);
   }
 
   AppBannerSettingsHelper::RecordMinutesFromFirstVisitToShow(
       contents, validated_url_, GetAppIdentifier(), GetCurrentTime());
 
   DCHECK(!manifest_url_.is_empty());
   DCHECK(!manifest_.IsEmpty());
   DCHECK(!icon_url_.is_empty());
   DCHECK(icon_.get());
+
   TrackBeforeInstallEvent(BEFORE_INSTALL_EVENT_COMPLETE);
   ShowBanner();
   is_active_ = false;
 }
 
 void AppBannerManager::OnRequestShowAppBanner(
     content::RenderFrameHost* render_frame_host,
     int request_id) {
   if (was_canceled_by_page_) {
     // Simulate a non-canceled OnBannerPromptReply to show the delayed banner.
     // Don't reset |was_canceled_by_page_| yet for metrics purposes.
     OnBannerPromptReply(render_frame_host, request_id,
                         blink::WebAppBannerPromptReply::None, referrer_);
   } else {
     // Log that the prompt request was made for when we get the prompt reply.
     page_requested_prompt_ = true;
   }
 }
 
 }  // namespace banners