Log messages regarding why app banners aren't displayed to the console

chrome/browser/banners/app_banner_data_fetcher.cc

Index: chrome/browser/banners/app_banner_data_fetcher.cc
diff --git a/chrome/browser/banners/app_banner_data_fetcher.cc b/chrome/browser/banners/app_banner_data_fetcher.cc
index e46110efd2e89e78b040607442028309c351dae9..c0fcb795e283e176450431c9f84ce36f230a5fb9 100644
--- a/chrome/browser/banners/app_banner_data_fetcher.cc
+++ b/chrome/browser/banners/app_banner_data_fetcher.cc
@@ -5,6 +5,7 @@
 #include "chrome/browser/banners/app_banner_data_fetcher.h"
 
 #include "base/bind.h"
+#include "base/command_line.h"
 #include "base/strings/string_util.h"
 #include "base/strings/utf_string_conversions.h"
 #include "chrome/browser/banners/app_banner_metrics.h"
@@ -14,6 +15,7 @@
 #include "chrome/browser/infobars/infobar_service.h"
 #include "chrome/browser/manifest/manifest_icon_selector.h"
 #include "chrome/browser/profiles/profile.h"
+#include "chrome/common/chrome_switches.h"
 #include "chrome/common/render_messages.h"
 #include "components/infobars/core/infobar.h"
 #include "components/rappor/rappor_utils.h"
@@ -71,7 +73,8 @@ AppBannerDataFetcher::AppBannerDataFetcher(
     : WebContentsObserver(web_contents),
       ideal_icon_size_(ideal_icon_size),
       weak_delegate_(delegate),
-      is_active_(false) {
+      is_active_(false),
+      log_err_(false) {
 }
 
 void AppBannerDataFetcher::Start(const GURL& validated_url) {
@@ -81,6 +84,8 @@ void AppBannerDataFetcher::Start(const GURL& validated_url) {
   DCHECK(web_contents);
 
   is_active_ = true;
+  log_err_ = base::CommandLine::ForCurrentProcess()->HasSwitch(
+                    switches::kBypassAppBannerEngagementChecks);

[benwells, 2015/05/07 01:57:20]

Nit: I think this indenting isn't quite right.

The coding style we use is mainly documented here:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml, with chromium
specific stuff here: https://cs.corp.google.com/#chrome.

Here I think the second line should just be indented 4 lines. But instead of
remembering all the rules you can also run 'git cl format' which will run clang
format over the bits of your patch you've updated. If you're setup with all the
right vim things you can also do it interactively.

[dominickn (DO NOT USE), 2015/05/07 05:37:24]

Done.

   validated_url_ = validated_url;
   web_contents->GetManifest(
       base::Bind(&AppBannerDataFetcher::OnDidGetManifest, this));
@@ -140,6 +145,15 @@ void AppBannerDataFetcher::OnBannerPromptReply(
     blink::WebAppBannerPromptReply reply) {
   content::WebContents* web_contents = GetWebContents();
   if (!is_active_ || !web_contents || request_id != gCurrentRequestID) {
+    if (!is_active_)

[benwells, 2015/05/07 01:57:20]

Nit: you need braces around all these statements as at least one is multiline.
See the coding style for details.

[dominickn (DO NOT USE), 2015/05/07 05:37:24]

Done.

+      SendDebugMessage(
+          "Banner not shown: display pipeline halted before "

[benwells, 2015/05/07 01:57:20]

I think 'Banner not shown' is probably a touch too general. 'App banner not
shown' would probably be better. As this prefix is copied everywhere, it can
also move into the SendDebugMessage function.

[dominickn (DO NOT USE), 2015/05/07 05:37:24]

Done.

+          "prompt reply checking.");

[benwells, 2015/05/07 01:57:20]

For this error I think something like 'user navigated.' would be more
understandable.

[dominickn (DO NOT USE), 2015/05/07 05:37:24]

Done.

+    else if (!web_contents)
+      SendDebugMessage(
+          "Banner not shown: no web content available for banner display.");

[benwells, 2015/05/07 01:57:19]

Something like 'web contents closed.' would be good here.

[dominickn (DO NOT USE), 2015/05/07 05:37:24]

Done.

+    else if (request_id != gCurrentRequestID)
+      SendDebugMessage("Banner not shown: incorrect request id.");

[benwells, 2015/05/07 01:57:19]

We shouldn't show the error in this case. This means that there is another
fetcher active which should handle the message.

[dominickn (DO NOT USE), 2015/05/07 05:37:24]

Done.

     Cancel();
     return;
   }
@@ -148,6 +162,8 @@ void AppBannerDataFetcher::OnBannerPromptReply(
   if (reply == blink::WebAppBannerPromptReply::Cancel) {
     // TODO(mlamouri,benwells): we should probably record that to behave
     // differently with regard to showing the banner.
+    SendDebugMessage("Banner not shown: renderer has requested the banner "
+                      "prompt be cancelled.");

[benwells, 2015/05/07 01:57:20]

Nit: indenting here is also off.

[dominickn (DO NOT USE), 2015/05/07 05:37:24]

Done.

     Cancel();
     return;
   }
@@ -233,6 +249,14 @@ void AppBannerDataFetcher::OnDidGetManifest(
     const content::Manifest& manifest) {
   content::WebContents* web_contents = GetWebContents();
   if (!is_active_ || !web_contents || manifest.IsEmpty()) {
+    if (!is_active_)
+      SendDebugMessage("Banner not shown: display pipeline halted before "
+                        "manifest checking.");
+    else if (!web_contents)
+      SendDebugMessage(
+          "Banner not shown: no web content available for banner display.");
+    else if (manifest.IsEmpty())
+      SendDebugMessage("Banner not shown: manifest is empty.");
     Cancel();
     return;
   }
@@ -248,6 +272,7 @@ void AppBannerDataFetcher::OnDidGetManifest(
   }
 
   if (!IsManifestValidForWebApp(manifest)) {
+    SendDebugMessage("Banner not shown: invalid manifest.");

[benwells, 2015/05/07 01:57:20]

Can we move this output into IsManifestValidForWebApp and give more details
about what is missing? Also avoid the use of the word 'invalid' here as it
implies something more like invalid json.

[dominickn (DO NOT USE), 2015/05/07 05:37:24]

Done.

     Cancel();
     return;
   }
@@ -276,6 +301,12 @@ void AppBannerDataFetcher::OnDidCheckHasServiceWorker(
     bool has_service_worker) {
   content::WebContents* web_contents = GetWebContents();
   if (!is_active_ || !web_contents) {
+    if (!is_active_)
+      SendDebugMessage("Banner not shown: display pipeline halted before "
+                        "manifest checking.");

[benwells, 2015/05/07 01:57:20]

This code block is repeated a few times. Would be good to centralize it into
something like if (!CheckFetcherIsStillAlive) which does the is_active and
web_contents checks and prints out appropriate debug messages.

[dominickn (DO NOT USE), 2015/05/07 05:37:24]

Done.

+    else if (!web_contents)
+      SendDebugMessage(
+          "Banner not shown: no web content available for banner display.");
     Cancel();
     return;
   }
@@ -291,10 +322,13 @@ void AppBannerDataFetcher::OnDidCheckHasServiceWorker(
       FetchIcon(icon_url);
       return;
     }
+    SendDebugMessage(
+        "Banner not shownL could not find icon specified in manifest");
   } else {
     TrackDisplayEvent(DISPLAY_EVENT_LACKS_SERVICE_WORKER);
   }
 
+  SendDebugMessage("Banner not shown: no service worker detected.");

[benwells, 2015/05/07 01:57:20]

A common cause of this will be that the site is not SSL, in which case service
workers are ignored. It would be great to communicate that explicitly.

Another cause could be that there is a service worker, but it doesn't match
correctly. "CheckHasServiceWorker" is a poorly named function as it does more
than just check that there is a service worker, it checks both urls passed in
have the *same* service worker.

[dominickn (DO NOT USE), 2015/05/07 05:37:24]

Done.

   Cancel();
 }
 
@@ -309,16 +343,26 @@ void AppBannerDataFetcher::OnFetchComplete(const GURL& url,
 void AppBannerDataFetcher::ShowBanner(const SkBitmap* icon) {
   content::WebContents* web_contents = GetWebContents();
   if (!is_active_ || !web_contents || !icon) {
+    if (!is_active_)
+      SendDebugMessage(
+          "Banner not shown: display pipeline halted before banner display.");
+    else if (!web_contents)
+      SendDebugMessage(
+          "Banner not shown: no web content available for banner display.");
+    else if (!icon)
+      SendDebugMessage("Banner not shown: no icon available to display.");
     Cancel();
     return;
   }
 
   RecordCouldShowBanner();
   if (!CheckIfShouldShowBanner()) {
+    SendDebugMessage("Banner not shown: engagement checks failed.");
     Cancel();
     return;
   }
 
+  SendDebugMessage("Showing banner.");
   app_icon_.reset(new SkBitmap(*icon));
   web_contents->GetMainFrame()->Send(
       new ChromeViewMsg_AppBannerPromptRequest(
@@ -345,6 +389,17 @@ bool AppBannerDataFetcher::CheckIfShouldShowBanner() {
       web_contents, validated_url_, GetAppIdentifier(), GetCurrentTime());
 }
 
+void AppBannerDataFetcher::SendDebugMessage(const std::string& message) {
+  if (log_err_) {
+    content::WebContents* web_contents = GetWebContents();
+    if (web_contents) {
+      web_contents->GetMainFrame()->Send(
+          new ChromeViewMsg_AppBannerDebugMessageRequest(
+              web_contents->GetMainFrame()->GetRoutingID(), message));
+    }
+  }
+}
+
 // static
 bool AppBannerDataFetcher::IsManifestValidForWebApp(
     const content::Manifest& manifest) {