Allow app banners to be triggered by increases in site engagement.

chrome/browser/banners/app_banner_manager.cc

Index: chrome/browser/banners/app_banner_manager.cc
diff --git a/chrome/browser/banners/app_banner_manager.cc b/chrome/browser/banners/app_banner_manager.cc
index e20ec795e24a02780581ddb694caf39cd9ede993..bea941f7a6499135bfe49a250b5f71c4e52040cf 100644
--- a/chrome/browser/banners/app_banner_manager.cc
+++ b/chrome/browser/banners/app_banner_manager.cc
@@ -7,12 +7,13 @@
 #include "chrome/browser/banners/app_banner_data_fetcher.h"
 #include "chrome/browser/banners/app_banner_debug_log.h"
 #include "chrome/browser/banners/app_banner_settings_helper.h"
-#include "content/public/browser/navigation_details.h"
+#include "chrome/browser/engagement/site_engagement_service.h"
+#include "chrome/browser/profiles/profile.h"
+#include "content/public/browser/navigation_handle.h"
 #include "content/public/browser/render_frame_host.h"
 #include "content/public/browser/web_contents.h"
 #include "content/public/common/frame_navigate_params.h"
 #include "content/public/common/origin_util.h"
-#include "net/base/load_flags.h"
 
 namespace {
 bool gDisableSecureCheckForTesting = false;
@@ -36,15 +37,13 @@ bool AppBannerManager::URLsAreForTheSamePage(const GURL& first,
          first.path() == second.path() && first.query() == second.query();
 }
 
-AppBannerManager::AppBannerManager()
-    : data_fetcher_(nullptr),
-      weak_factory_(this) {
-  AppBannerSettingsHelper::UpdateFromFieldTrial();
-}
-
 AppBannerManager::AppBannerManager(content::WebContents* web_contents)
     : content::WebContentsObserver(web_contents),
+      SiteEngagementObserver(nullptr),
       data_fetcher_(nullptr),
+      is_hidden_(false),
+      load_finished_(false),
+      banner_requested_(false),
       weak_factory_(this) {
   AppBannerSettingsHelper::UpdateFromFieldTrial();
 }
@@ -54,7 +53,7 @@ AppBannerManager::~AppBannerManager() {
 }
 
 void AppBannerManager::ReplaceWebContents(content::WebContents* web_contents) {
-  Observe(web_contents);
+  content::WebContentsObserver::Observe(web_contents);
   if (data_fetcher_.get())
     data_fetcher_.get()->ReplaceWebContents(web_contents);
 }
@@ -63,18 +62,11 @@ bool AppBannerManager::IsFetcherActive() {
   return data_fetcher_ && data_fetcher_->is_active();
 }
 
-void AppBannerManager::DidNavigateMainFrame(

[benwells, 2016/06/28 00:57:50]

Why did you change this from DidNavigateMainFrame to DidFinishNavigation?

[dominickn, 2016/06/28 03:52:29]

DidNavigateMainFrame is deprecated and is being replaced by DidFinishNavigation
in the PlzNavigate project. The content folks have advised migrating these
methods.

-    const content::LoadCommittedDetails& details,
-    const content::FrameNavigateParams& params) {
-  last_transition_type_ = params.transition;
-}
-
-void AppBannerManager::RequestAppBanner(
-    content::RenderFrameHost* render_frame_host,
-    const GURL& validated_url,
-    bool is_debug_mode) {
-  if (render_frame_host->GetParent()) {
-    OutputDeveloperNotShownMessage(web_contents(), kNotLoadedInMainFrame,
+void AppBannerManager::RequestAppBanner(const GURL& validated_url,
+                                        bool is_debug_mode) {
+  content::WebContents* contents = web_contents();
+  if (contents->GetMainFrame()->GetParent()) {
+    OutputDeveloperNotShownMessage(contents, kNotLoadedInMainFrame,
                                    is_debug_mode);
     return;
   }
@@ -89,7 +81,7 @@ void AppBannerManager::RequestAppBanner(
   // URL is invalid.
   if (!content::IsOriginSecure(validated_url) &&
       !gDisableSecureCheckForTesting) {
-    OutputDeveloperNotShownMessage(web_contents(), kNotServedFromSecureOrigin,
+    OutputDeveloperNotShownMessage(contents, kNotServedFromSecureOrigin,
                                    is_debug_mode);
     return;
   }
@@ -100,12 +92,62 @@ void AppBannerManager::RequestAppBanner(
   data_fetcher_->Start(validated_url, last_transition_type_);
 }
 
+void AppBannerManager::DidStartNavigation(
+    content::NavigationHandle* navigation_handle) {
+  if (AppBannerSettingsHelper::ShouldUseSiteEngagementScore() &&
+      navigation_handle->IsInMainFrame()) {
+    load_finished_ = false;

[benwells, 2016/06/28 00:57:50]

Seems strange that this is set false only if we're using the site engagement
service, but set true regardless later on.

[dominickn, 2016/06/28 03:52:29]

The non site engagement triggered banners don't use this variable for
consistency with existing behaviour. I'll move the setting of the variable
inside the equivalent check.

+
+    // Ensure that we are observing the site engagement service on navigation
+    // start. This may be the first navigation, or we may have stopped observing
+    // as the banner flow was triggered on the previous page.
+    if (GetSiteEngagementService() == nullptr) {
+      SiteEngagementObserver::Observe(SiteEngagementService::Get(
+          Profile::FromBrowserContext(web_contents()->GetBrowserContext())));
+    }
+  }
+}
+
+void AppBannerManager::DidFinishNavigation(
+    content::NavigationHandle* navigation_handle) {
+  if (navigation_handle->HasCommitted())
+    last_transition_type_ = navigation_handle->GetPageTransition();
+}
+
 void AppBannerManager::DidFinishLoad(
     content::RenderFrameHost* render_frame_host,
     const GURL& validated_url) {
-  // The third argument is the is_debug_mode boolean value, which is true only
-  // when it is triggered by the developer's action in DevTools.
-  RequestAppBanner(render_frame_host, validated_url, false /* is_debug_mode */);
+  // Don't start the banner flow unless the main frame has finished loading.
+  if (render_frame_host->GetParent())
+    return;
+
+  load_finished_ = true;
+  if (!AppBannerSettingsHelper::ShouldUseSiteEngagementScore() ||
+      banner_requested_) {

[benwells, 2016/06/28 00:57:50]

This logic reads weirdly. If banner_requested is true, set banner_requested to
false and request the banner? Maybe this variable should be called something
else?

[dominickn, 2016/06/28 03:52:29]

Changed to banner eligible.

[benwells, 2016/06/29 03:38:24]

Hmmm thanks.

Nit: maybe banner_request_queued_ or something would be better?

[dominickn, 2016/06/29 04:18:58]

Done.

+    banner_requested_ = false;
+
+    // The third argument is the is_debug_mode boolean value, which is true only
+    // when it is triggered by the developer's action in DevTools.
+    RequestAppBanner(validated_url, false /* is_debug_mode */);
+  }
+}
+
+void AppBannerManager::MediaStartedPlaying(const MediaPlayerId& id) {
+  active_media_players_.push_back(id);
+}
+
+void AppBannerManager::MediaStoppedPlaying(const MediaPlayerId& id) {
+  active_media_players_.erase(std::remove(active_media_players_.begin(),
+                                          active_media_players_.end(), id),
+                              active_media_players_.end());
+}
+
+void AppBannerManager::WasShown() {
+  is_hidden_ = false;
+}
+
+void AppBannerManager::WasHidden() {
+  is_hidden_ = true;
 }
 
 bool AppBannerManager::HandleNonWebApp(const std::string& platform,
@@ -115,6 +157,31 @@ bool AppBannerManager::HandleNonWebApp(const std::string& platform,
   return false;
 }
 
+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 && !is_hidden_ &&
+      active_media_players_.empty() &&
+      AppBannerSettingsHelper::HasSufficientEngagement(score)) {
+    // Stop observing so we don't double-trigger the banner.

[benwells, 2016/06/28 00:57:50]

What will happen if you have two visible web contents showing the same origin,
and you are flipping between them? Could you trigger on both, or would the
content setting prevent that?

[dominickn, 2016/06/28 03:52:29]

The content setting should prevent that, since a banner being shown immediately
sets the APP_BANNER_EVENT_DID_SHOW. There will be a potential race if the two
banners are triggered before one of them can set the DID_SHOW event, but in
practice the banner flow happens so quickly that I'm not even sure it would be
possible to reliably reproduce this.

[benwells, 2016/06/29 03:38:24]

Acknowledged.

+    SiteEngagementObserver::Observe(nullptr);
+
+    if (!load_finished_ || banner_requested_) {
+      // Wait until the main frame finishes loading before requesting a banner.
+      banner_requested_ = 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 */);
+    }
+  }
+}
+
 void AppBannerManager::CancelActiveFetcher() {
   if (data_fetcher_) {
     data_fetcher_->Cancel();