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

chrome/browser/banners/app_banner_data_fetcher.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_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"
 #include "chrome/browser/banners/app_banner_settings_helper.h"
 #include "chrome/browser/bitmap_fetcher/bitmap_fetcher.h"
 #include "chrome/browser/browser_process.h"
 #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"
 #include "content/public/browser/browser_context.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/navigation_details.h"
 #include "content/public/browser/render_frame_host.h"
 #include "content/public/browser/service_worker_context.h"
 #include "content/public/browser/storage_partition.h"
 #include "net/base/load_flags.h"
@@ skipping 37 matching lines @@
   gTimeDeltaForTesting = base::TimeDelta::FromDays(days);
 }
 
 AppBannerDataFetcher::AppBannerDataFetcher(
     content::WebContents* web_contents,
     base::WeakPtr<Delegate> delegate,
     int ideal_icon_size)
     : 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) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
 
   content::WebContents* web_contents = GetWebContents();
   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));
 }
 
 void AppBannerDataFetcher::Cancel() {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
   if (is_active_) {
     FOR_EACH_OBSERVER(Observer, observer_list_,
                       OnDecidedWhetherToShow(this, false));
@@ skipping 39 matching lines @@
 
   return handled;
 }
 
 void AppBannerDataFetcher::OnBannerPromptReply(
     content::RenderFrameHost* render_frame_host,
     int request_id,
     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;
   }
 
   // The renderer might have requested the prompt to be canceled.
   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;
   }
 
   // Definitely going to show the banner now.
   FOR_EACH_OBSERVER(Observer, observer_list_,
                     OnDecidedWhetherToShow(this, true));
 
   infobars::InfoBar* infobar = CreateBanner(app_icon_.get(), app_title_);
   if (infobar) {
@@ skipping 65 matching lines @@
   rappor::SampleDomainAndRegistryFromGURL(g_browser_process->rappor_service(),
                                           event_name,
                                           web_contents->GetURL());
   banners::TrackDisplayEvent(DISPLAY_EVENT_CREATED);
 }
 
 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;
   }
 
   if (manifest.prefer_related_applications &&
       manifest.related_applications.size()) {
     for (const auto& application : manifest.related_applications) {
       std::string platform = base::UTF16ToUTF8(application.platform.string());
       std::string id = base::UTF16ToUTF8(application.id.string());
       if (weak_delegate_->HandleNonWebApp(platform, application.url, id))
         return;
     }
   }
 
   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;
   }
 
   banners::TrackDisplayEvent(DISPLAY_EVENT_BANNER_REQUESTED);
 
   web_app_data_ = manifest;
   app_title_ = web_app_data_.name.string();
 
   // Check to see if there is a single service worker controlling this page
   // and the manifest's start url.
   Profile* profile =
       Profile::FromBrowserContext(web_contents->GetBrowserContext());
   content::StoragePartition* storage_partition =
       content::BrowserContext::GetStoragePartition(
           profile, web_contents->GetSiteInstance());
   DCHECK(storage_partition);
 
   storage_partition->GetServiceWorkerContext()->CheckHasServiceWorker(
       validated_url_, manifest.start_url,
       base::Bind(&AppBannerDataFetcher::OnDidCheckHasServiceWorker,
                  this));
 }
 
 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;
   }
 
   if (has_service_worker) {
     // Create an infobar to promote the manifest's app.
     GURL icon_url =
         ManifestIconSelector::FindBestMatchingIcon(
             web_app_data_.icons,
             ideal_icon_size_,
             gfx::Screen::GetScreenFor(web_contents->GetNativeView()));
     if (!icon_url.is_empty()) {
       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();
 }
 
 void AppBannerDataFetcher::OnFetchComplete(const GURL& url,
                                            const SkBitmap* icon) {
   if (is_active_)
     ShowBanner(icon);
 
   Release();
 }
 
 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(
           web_contents->GetMainFrame()->GetRoutingID(),
           ++gCurrentRequestID,
           GetBannerType()));
 }
 
 void AppBannerDataFetcher::RecordCouldShowBanner() {
   content::WebContents* web_contents = GetWebContents();
   DCHECK(web_contents);
 
   AppBannerSettingsHelper::RecordBannerEvent(
       web_contents, validated_url_, GetAppIdentifier(),
       AppBannerSettingsHelper::APP_BANNER_EVENT_COULD_SHOW,
       GetCurrentTime());
 }
 
 bool AppBannerDataFetcher::CheckIfShouldShowBanner() {
   content::WebContents* web_contents = GetWebContents();
   DCHECK(web_contents);
 
   return AppBannerSettingsHelper::ShouldShowBanner(
       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) {
   if (manifest.IsEmpty())
     return false;
   if (!manifest.start_url.is_valid())
     return false;
   if (manifest.name.is_null() && manifest.short_name.is_null())
     return false;
   if (!DoesManifestContainRequiredIcon(manifest))
     return false;
   return true;
 }
 
 }  // namespace banners