Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(200)

Issue 1129103003: Log messages regarding why app banners aren't displayed to the console (Closed)

Created:
5 years, 7 months ago by dominickn (DO NOT USE)
Modified:
5 years, 7 months ago
CC:
chromium-reviews
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Log messages regarding why app banners aren't displayed to the console App banners require several engagement, service worker, and manifest checks to pass before being displayed. This allows developers to see in the console the reasons why banners are not displayed, given the switch --bypass-app-banner-engagement-checks on chrome startup BUG=458866 Committed: https://crrev.com/0b93e0d31157dac42756c416e3dcac3a7669d6eb Cr-Commit-Position: refs/heads/master@{#329751}

Patch Set 1 #

Total comments: 22

Patch Set 2 : Rebase #

Patch Set 3 : Improving code formatting and debug logging detail #

Patch Set 4 : Factoring out debug log message function #

Total comments: 48

Patch Set 5 : Factoring out log messages into constants #

Total comments: 10

Patch Set 6 : Addressing code health and style issues #

Total comments: 4

Patch Set 7 : Correcting non-alphabetical file listing and lack of || wrappers #

Total comments: 4

Patch Set 8 : Adding a full stop to the end of a comment #

Unified diffs Side-by-side diffs Delta from patch set Stats (+218 lines, -17 lines) Patch
M chrome/browser/android/banners/app_banner_manager_android.h View 1 2 3 2 chunks +5 lines, -0 lines 0 comments Download
M chrome/browser/android/banners/app_banner_manager_android.cc View 1 2 3 4 5 3 chunks +29 lines, -4 lines 0 comments Download
M chrome/browser/banners/app_banner_data_fetcher.h View 1 2 3 4 5 6 7 1 chunk +6 lines, -1 line 0 comments Download
M chrome/browser/banners/app_banner_data_fetcher.cc View 1 2 3 4 5 6 8 chunks +51 lines, -10 lines 0 comments Download
M chrome/browser/banners/app_banner_data_fetcher_unittest.cc View 1 2 3 4 1 chunk +4 lines, -1 line 0 comments Download
A chrome/browser/banners/app_banner_debug_log.h View 1 2 3 4 1 chunk +43 lines, -0 lines 0 comments Download
A chrome/browser/banners/app_banner_debug_log.cc View 1 2 3 4 5 1 chunk +59 lines, -0 lines 0 comments Download
M chrome/browser/banners/app_banner_manager.cc View 1 2 3 4 2 chunks +5 lines, -1 line 0 comments Download
M chrome/chrome_browser.gypi View 1 2 3 4 5 6 1 chunk +2 lines, -0 lines 0 comments Download
M chrome/common/render_messages.h View 1 2 3 4 1 chunk +5 lines, -0 lines 0 comments Download
M chrome/renderer/chrome_render_frame_observer.h View 1 chunk +1 line, -0 lines 0 comments Download
M chrome/renderer/chrome_render_frame_observer.cc View 1 2 chunks +8 lines, -0 lines 0 comments Download

Messages

Total messages: 53 (22 generated)
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1129103003/1
5 years, 7 months ago (2015-05-07 01:09:09 UTC) #2
commit-bot: I haz the power
Dry run: No LGTM from a valid reviewer yet. Only full committers are accepted. Even ...
5 years, 7 months ago (2015-05-07 01:09:11 UTC) #4
dominickn (DO NOT USE)
5 years, 7 months ago (2015-05-07 01:09:12 UTC) #6
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1129103003/1
5 years, 7 months ago (2015-05-07 01:41:03 UTC) #10
commit-bot: I haz the power
Dry run: Try jobs failed on following builders: linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_compile_dbg_32_ng/builds/50312) ios_dbg_simulator_ninja on ...
5 years, 7 months ago (2015-05-07 01:46:15 UTC) #12
benwells
Looking pretty good! Most of my comments are trivial. I think for native banners there ...
5 years, 7 months ago (2015-05-07 01:57:20 UTC) #13
dominickn (DO NOT USE)
https://codereview.chromium.org/1129103003/diff/1/chrome/browser/banners/app_banner_data_fetcher.cc File chrome/browser/banners/app_banner_data_fetcher.cc (right): https://codereview.chromium.org/1129103003/diff/1/chrome/browser/banners/app_banner_data_fetcher.cc#newcode88 chrome/browser/banners/app_banner_data_fetcher.cc:88: switches::kBypassAppBannerEngagementChecks); On 2015/05/07 01:57:20, benwells wrote: > Nit: I ...
5 years, 7 months ago (2015-05-07 05:37:24 UTC) #16
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1129103003/40001
5 years, 7 months ago (2015-05-07 05:39:03 UTC) #18
commit-bot: I haz the power
Dry run: No LGTM from a valid reviewer yet. Only full committers are accepted. Even ...
5 years, 7 months ago (2015-05-07 05:39:09 UTC) #20
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1129103003/40001
5 years, 7 months ago (2015-05-07 06:26:04 UTC) #22
commit-bot: I haz the power
Dry run: Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_rel_ng/builds/20266)
5 years, 7 months ago (2015-05-07 06:52:49 UTC) #24
dominickn (DO NOT USE)
> I think for native banners there are additional checks which happen in > c/b/android/banners/app_banner_*. ...
5 years, 7 months ago (2015-05-07 06:56:52 UTC) #25
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1129103003/60001
5 years, 7 months ago (2015-05-10 22:58:19 UTC) #29
commit-bot: I haz the power
Dry run: No LGTM from a valid reviewer yet. Only full committers are accepted. Even ...
5 years, 7 months ago (2015-05-10 22:58:26 UTC) #31
dominickn (DO NOT USE)
Refactored console logging to allow access from non-webapp banners.
5 years, 7 months ago (2015-05-10 23:00:17 UTC) #32
benwells
https://codereview.chromium.org/1129103003/diff/60001/chrome/browser/android/banners/app_banner_manager_android.cc File chrome/browser/android/banners/app_banner_manager_android.cc (right): https://codereview.chromium.org/1129103003/diff/60001/chrome/browser/android/banners/app_banner_manager_android.cc#newcode74 chrome/browser/android/banners/app_banner_manager_android.cc:74: bool AppBannerManagerAndroid::CheckPlatformAndId(const std::string& platform, Actually ... this probably shouldn't ...
5 years, 7 months ago (2015-05-11 08:08:41 UTC) #33
dominickn (DO NOT USE)
I'm trying to test my latest refactor locally before committing again, but the app banners ...
5 years, 7 months ago (2015-05-12 07:41:32 UTC) #34
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1129103003/80001
5 years, 7 months ago (2015-05-12 09:05:30 UTC) #36
commit-bot: I haz the power
Dry run: No LGTM from a valid reviewer yet. Only full committers are accepted. Even ...
5 years, 7 months ago (2015-05-12 09:05:37 UTC) #38
benwells
Just a bunch of nits now. https://codereview.chromium.org/1129103003/diff/80001/chrome/browser/android/banners/app_banner_manager_android.cc File chrome/browser/android/banners/app_banner_manager_android.cc (right): https://codereview.chromium.org/1129103003/diff/80001/chrome/browser/android/banners/app_banner_manager_android.cc#newcode80 chrome/browser/android/banners/app_banner_manager_android.cc:80: } else if ...
5 years, 7 months ago (2015-05-13 02:55:20 UTC) #39
dominickn (DO NOT USE)
https://codereview.chromium.org/1129103003/diff/80001/chrome/browser/android/banners/app_banner_manager_android.cc File chrome/browser/android/banners/app_banner_manager_android.cc (right): https://codereview.chromium.org/1129103003/diff/80001/chrome/browser/android/banners/app_banner_manager_android.cc#newcode80 chrome/browser/android/banners/app_banner_manager_android.cc:80: } else if (id.empty()) { On 2015/05/13 02:55:20, benwells ...
5 years, 7 months ago (2015-05-13 03:08:06 UTC) #40
benwells
Nice! lgtm
5 years, 7 months ago (2015-05-13 03:09:28 UTC) #41
dominickn (DO NOT USE)
+dfalcantara: chrome/browser/android +tsepez: chrome/common +thestig: chrome/renderer
5 years, 7 months ago (2015-05-13 03:14:23 UTC) #43
Lei Zhang
chrome/renderer lgtm https://codereview.chromium.org/1129103003/diff/100001/chrome/browser/banners/app_banner_data_fetcher.cc File chrome/browser/banners/app_banner_data_fetcher.cc (right): https://codereview.chromium.org/1129103003/diff/100001/chrome/browser/banners/app_banner_data_fetcher.cc#newcode374 chrome/browser/banners/app_banner_data_fetcher.cc:374: return false; // We cannot show a ...
5 years, 7 months ago (2015-05-13 06:06:31 UTC) #44
dominickn (DO NOT USE)
https://codereview.chromium.org/1129103003/diff/100001/chrome/browser/banners/app_banner_data_fetcher.cc File chrome/browser/banners/app_banner_data_fetcher.cc (right): https://codereview.chromium.org/1129103003/diff/100001/chrome/browser/banners/app_banner_data_fetcher.cc#newcode374 chrome/browser/banners/app_banner_data_fetcher.cc:374: return false; // We cannot show a message if ...
5 years, 7 months ago (2015-05-13 06:13:47 UTC) #45
Tom Sepez
Messages LGTM.
5 years, 7 months ago (2015-05-13 17:03:37 UTC) #46
gone
lgtm https://codereview.chromium.org/1129103003/diff/120001/chrome/browser/banners/app_banner_data_fetcher.h File chrome/browser/banners/app_banner_data_fetcher.h (right): https://codereview.chromium.org/1129103003/diff/120001/chrome/browser/banners/app_banner_data_fetcher.h#newcode142 chrome/browser/banners/app_banner_data_fetcher.h:142: // closed nit: add a period https://codereview.chromium.org/1129103003/diff/120001/chrome/browser/banners/app_banner_debug_log.cc File ...
5 years, 7 months ago (2015-05-13 17:54:30 UTC) #47
dominickn (DO NOT USE)
https://codereview.chromium.org/1129103003/diff/120001/chrome/browser/banners/app_banner_data_fetcher.h File chrome/browser/banners/app_banner_data_fetcher.h (right): https://codereview.chromium.org/1129103003/diff/120001/chrome/browser/banners/app_banner_data_fetcher.h#newcode142 chrome/browser/banners/app_banner_data_fetcher.h:142: // closed On 2015/05/13 17:54:30, dfalcantara wrote: > nit: ...
5 years, 7 months ago (2015-05-13 23:23:40 UTC) #48
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1129103003/140001
5 years, 7 months ago (2015-05-13 23:27:58 UTC) #51
commit-bot: I haz the power
Committed patchset #8 (id:140001)
5 years, 7 months ago (2015-05-14 00:09:07 UTC) #52
commit-bot: I haz the power
5 years, 7 months ago (2015-05-14 00:09:54 UTC) #53
Message was sent while issue was closed.
Patchset 8 (id:??) landed as
https://crrev.com/0b93e0d31157dac42756c416e3dcac3a7669d6eb
Cr-Commit-Position: refs/heads/master@{#329751}

Powered by Google App Engine
This is Rietveld 408576698