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.
Th
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 a
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
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 naviga
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 goo
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 mea
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 IsManifestValidForWeb
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
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 no
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) { |