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..92cfa18f254ce2e7df97a5a9a754d6b7310a96fe 100644
--- a/chrome/browser/banners/app_banner_manager.cc
+++ b/chrome/browser/banners/app_banner_manager.cc
@@ -7,6 +7,8 @@
 #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 "chrome/browser/engagement/site_engagement_service.h"
+#include "chrome/browser/profiles/profile.h"
 #include "content/public/browser/navigation_details.h"
 #include "content/public/browser/render_frame_host.h"
 #include "content/public/browser/web_contents.h"
@@ -36,17 +38,17 @@ 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),
+      is_visible_(false),
       data_fetcher_(nullptr),
       weak_factory_(this) {
   AppBannerSettingsHelper::UpdateFromFieldTrial();
+  if (AppBannerSettingsHelper::ShouldUseSiteEngagementScore()) {
+    SiteEngagementObserver::Observe(SiteEngagementService::Get(
+        Profile::FromBrowserContext(web_contents->GetBrowserContext())));
+  }
 }
 
 AppBannerManager::~AppBannerManager() {
@@ -54,7 +56,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 +65,11 @@ bool AppBannerManager::IsFetcherActive() {
   return data_fetcher_ && data_fetcher_->is_active();
 }
 
-void AppBannerManager::DidNavigateMainFrame(
-    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 +84,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 +95,30 @@ void AppBannerManager::RequestAppBanner(
   data_fetcher_->Start(validated_url, last_transition_type_);
 }
 
+void AppBannerManager::DidNavigateMainFrame(
+    const content::LoadCommittedDetails& details,
+    const content::FrameNavigateParams& params) {
+  last_transition_type_ = params.transition;
+}
+
 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 */);
+  if (AppBannerSettingsHelper::ShouldUseSiteEngagementScore()) {
+    validated_url_ = validated_url;
+  } else {
+    // 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::WasShown() {
+  is_visible_ = true;
+}
+
+void AppBannerManager::WasHidden() {
+  is_visible_ = false;

[benwells, 2016/06/02 04:44:36]

What's this for? Is this something you found in testing?

In particular I'm wondering whether this is something that should be checked at
the end of the pipeline for testing validity.

[dominickn, 2016/06/02 04:54:37]

If a user has multiple tabs open (multiple WebContents), there's an
AppBannerManager on each one, and each one is listening to the
SiteEngagementService. Previously, since we triggered on main frame navigation,
only one app banner flow would be instigated at any time. With the change to the
callback interface, now it's possible that there are two tabs open to the same
URL. We only want the banner flow to start on the tab that's in the foreground.

 }
 
 bool AppBannerManager::HandleNonWebApp(const std::string& platform,
@@ -115,6 +128,19 @@ bool AppBannerManager::HandleNonWebApp(const std::string& platform,
   return false;
 }
 
+void AppBannerManager::OnEngagementIncreased(
+    const SiteEngagementService* service,
+    const GURL& url,
+    double score) {
+  if (AppBannerSettingsHelper::HasSufficientEngagement(score) &&
+      url == validated_url_ && is_visible_) {

[benwells, 2016/06/02 04:44:36]

I'm slightly worried there will be cases where we've navigated to a different
page on the same PWA when we get this call. This may not be a problem (e.g. it
might not be racy), and at the moment it will probably be fine as we'll get more
engagement calls later down the track with the right URL. But if we do any fancy
stuff to limit how often we get here (see next comment) it might become a
problem.

[dominickn, 2016/06/02 04:54:37]

Yes, this is definitely a concern. We can certainly implement a check that |url|
is a subpath of the PWA's path + start_url, but that will require fetching and
parsing the manifest at the very least. A simpler check might be that |url| is a
subpath of |validated_url| that we got from DidFinishLoad. WDYT?

+    // This call will perform some simple tests, create a data fetcher, and then
+    // start a series of asynchronous checks to test installability. It should
+    // be safe to start this in response to user input.
+    RequestAppBanner(url, false /* is_debug_mode */);

[benwells, 2016/06/02 04:44:36]

IIUC this will be called a lot more now. Have you considered the impact of that?

[dominickn, 2016/06/02 04:54:38]

I did think about this, and I believe this shouldn't be called that much more
often. User input is the noisiest source of engagement increases, but that's
gated to 10 seconds after navigation, and every 10 seconds after that. Media
playing is the main wildcard, but again that's on a periodic timer.

Once we remove the navigation based backend, I'm planning on moving the
have-we-shown-the-banner-before content settings check to here, so that the rest
of the flow doesn't start if we've already shown the banner upfront.

+  }
+}
+
 void AppBannerManager::CancelActiveFetcher() {
   if (data_fetcher_) {
     data_fetcher_->Cancel();