Chromium Code Reviews
|
|
Chromium Code Reviews
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. |
DescriptionLog 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 #Messages
Total messages: 53 (22 generated)
The CQ bit was checked by dominickn@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1129103003/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: No LGTM from a valid reviewer yet. Only full committers are accepted. Even if an LGTM may have been provided, it was from a non-committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
dominickn@google.com changed reviewers: + benwells@chromium.org
The CQ bit was checked by dominickn@google.com
The CQ bit was unchecked by dominickn@google.com
The CQ bit was checked by benwells@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1129103003/1
The CQ bit was unchecked by commit-bot@chromium.org
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_...) ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...) mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
Looking pretty good! Most of my comments are trivial. I think for native banners there are additional checks which happen in c/b/android/banners/app_banner_*. Could we put in some logging from there too? I ran the cq dry run for you, seems you need to be a committer or have lg's. Note the red - looks like you need to rebase. https://codereview.chromium.org/1129103003/diff/1/chrome/browser/banners/app_... File chrome/browser/banners/app_banner_data_fetcher.cc (right): https://codereview.chromium.org/1129103003/diff/1/chrome/browser/banners/app_... chrome/browser/banners/app_banner_data_fetcher.cc:88: switches::kBypassAppBannerEngagementChecks); 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. https://codereview.chromium.org/1129103003/diff/1/chrome/browser/banners/app_... chrome/browser/banners/app_banner_data_fetcher.cc:148: if (!is_active_) Nit: you need braces around all these statements as at least one is multiline. See the coding style for details. https://codereview.chromium.org/1129103003/diff/1/chrome/browser/banners/app_... chrome/browser/banners/app_banner_data_fetcher.cc:150: "Banner not shown: display pipeline halted before " 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. https://codereview.chromium.org/1129103003/diff/1/chrome/browser/banners/app_... chrome/browser/banners/app_banner_data_fetcher.cc:151: "prompt reply checking."); For this error I think something like 'user navigated.' would be more understandable. https://codereview.chromium.org/1129103003/diff/1/chrome/browser/banners/app_... chrome/browser/banners/app_banner_data_fetcher.cc:154: "Banner not shown: no web content available for banner display."); Something like 'web contents closed.' would be good here. https://codereview.chromium.org/1129103003/diff/1/chrome/browser/banners/app_... chrome/browser/banners/app_banner_data_fetcher.cc:156: SendDebugMessage("Banner not shown: incorrect request id."); We shouldn't show the error in this case. This means that there is another fetcher active which should handle the message. https://codereview.chromium.org/1129103003/diff/1/chrome/browser/banners/app_... chrome/browser/banners/app_banner_data_fetcher.cc:166: "prompt be cancelled."); Nit: indenting here is also off. https://codereview.chromium.org/1129103003/diff/1/chrome/browser/banners/app_... chrome/browser/banners/app_banner_data_fetcher.cc:275: SendDebugMessage("Banner not shown: invalid manifest."); 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. https://codereview.chromium.org/1129103003/diff/1/chrome/browser/banners/app_... chrome/browser/banners/app_banner_data_fetcher.cc:306: "manifest checking."); 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. https://codereview.chromium.org/1129103003/diff/1/chrome/browser/banners/app_... chrome/browser/banners/app_banner_data_fetcher.cc:331: SendDebugMessage("Banner not shown: no service worker detected."); 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. https://codereview.chromium.org/1129103003/diff/1/chrome/common/render_messag... File chrome/common/render_messages.h (right): https://codereview.chromium.org/1129103003/diff/1/chrome/common/render_messag... chrome/common/render_messages.h:457: // non-displayed app banner Nit: techincally this is incorrect as we also log a message for banners that are shown. Maybe just '... regarding app banners.'
The CQ bit was checked by dominickn@google.com to run a CQ dry run
The CQ bit was unchecked by dominickn@google.com
https://codereview.chromium.org/1129103003/diff/1/chrome/browser/banners/app_... File chrome/browser/banners/app_banner_data_fetcher.cc (right): https://codereview.chromium.org/1129103003/diff/1/chrome/browser/banners/app_... chrome/browser/banners/app_banner_data_fetcher.cc:88: switches::kBypassAppBannerEngagementChecks); On 2015/05/07 01:57:20, benwells wrote: > 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. Done. https://codereview.chromium.org/1129103003/diff/1/chrome/browser/banners/app_... chrome/browser/banners/app_banner_data_fetcher.cc:148: if (!is_active_) On 2015/05/07 01:57:20, benwells wrote: > Nit: you need braces around all these statements as at least one is multiline. > See the coding style for details. Done. https://codereview.chromium.org/1129103003/diff/1/chrome/browser/banners/app_... chrome/browser/banners/app_banner_data_fetcher.cc:150: "Banner not shown: display pipeline halted before " On 2015/05/07 01:57:20, benwells wrote: > 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. Done. https://codereview.chromium.org/1129103003/diff/1/chrome/browser/banners/app_... chrome/browser/banners/app_banner_data_fetcher.cc:151: "prompt reply checking."); On 2015/05/07 01:57:20, benwells wrote: > For this error I think something like 'user navigated.' would be more > understandable. Done. https://codereview.chromium.org/1129103003/diff/1/chrome/browser/banners/app_... chrome/browser/banners/app_banner_data_fetcher.cc:154: "Banner not shown: no web content available for banner display."); On 2015/05/07 01:57:19, benwells wrote: > Something like 'web contents closed.' would be good here. Done. https://codereview.chromium.org/1129103003/diff/1/chrome/browser/banners/app_... chrome/browser/banners/app_banner_data_fetcher.cc:156: SendDebugMessage("Banner not shown: incorrect request id."); On 2015/05/07 01:57:19, benwells wrote: > We shouldn't show the error in this case. This means that there is another > fetcher active which should handle the message. Done. https://codereview.chromium.org/1129103003/diff/1/chrome/browser/banners/app_... chrome/browser/banners/app_banner_data_fetcher.cc:166: "prompt be cancelled."); On 2015/05/07 01:57:20, benwells wrote: > Nit: indenting here is also off. Done. https://codereview.chromium.org/1129103003/diff/1/chrome/browser/banners/app_... chrome/browser/banners/app_banner_data_fetcher.cc:275: SendDebugMessage("Banner not shown: invalid manifest."); On 2015/05/07 01:57:20, benwells wrote: > 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. Done. https://codereview.chromium.org/1129103003/diff/1/chrome/browser/banners/app_... chrome/browser/banners/app_banner_data_fetcher.cc:306: "manifest checking."); On 2015/05/07 01:57:20, benwells wrote: > 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. Done. https://codereview.chromium.org/1129103003/diff/1/chrome/browser/banners/app_... chrome/browser/banners/app_banner_data_fetcher.cc:331: SendDebugMessage("Banner not shown: no service worker detected."); On 2015/05/07 01:57:20, benwells wrote: > 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. Done. https://codereview.chromium.org/1129103003/diff/1/chrome/common/render_messag... File chrome/common/render_messages.h (right): https://codereview.chromium.org/1129103003/diff/1/chrome/common/render_messag... chrome/common/render_messages.h:457: // non-displayed app banner On 2015/05/07 01:57:20, benwells wrote: > Nit: techincally this is incorrect as we also log a message for banners that are > shown. Maybe just '... regarding app banners.' Done.
The CQ bit was checked by dominickn@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1129103003/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: No LGTM from a valid reviewer yet. Only full committers are accepted. Even if an LGTM may have been provided, it was from a non-committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
The CQ bit was checked by benwells@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1129103003/40001
The CQ bit was unchecked by commit-bot@chromium.org
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_r...)
> I think for native banners there are additional checks which happen in > c/b/android/banners/app_banner_*. Could we put in some logging from there too? The refactoring in patch set 3 required making IsManifestValidForWebApp non-static (to allow it to access SendDebugMessage). This seemed to work fine, but it actually breaks the unit tests for AppBannerDataFetcher, which call the method without instantiating an object. I think the correct solution here is to factor out SendDebugMessage and sit it outside of the classes (like banners::TrackDisplayEvent). It will take a bool (whether to send the message), and the web_contents pointer so that it can send the message to the console. This way, the debug messages can be triggered from any of these classes, including the native banners in c/b/android/banners/app_banner_*
The CQ bit was checked by dominickn@google.com
The CQ bit was unchecked by dominickn@google.com
The CQ bit was checked by dominickn@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1129103003/60001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: No LGTM from a valid reviewer yet. Only full committers are accepted. Even if an LGTM may have been provided, it was from a non-committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
Refactored console logging to allow access from non-webapp banners.
https://codereview.chromium.org/1129103003/diff/60001/chrome/browser/android/... File chrome/browser/android/banners/app_banner_manager_android.cc (right): https://codereview.chromium.org/1129103003/diff/60001/chrome/browser/android/... chrome/browser/android/banners/app_banner_manager_android.cc:74: bool AppBannerManagerAndroid::CheckPlatformAndId(const std::string& platform, Actually ... this probably shouldn't generate a 'not shown' message, as it might be shown for the web platform (or not, if there are other problems). Maybe it should just generate a message saying 'XXX platform application ignored as not supported on Android' or 'play application ignored as there is no id'. https://codereview.chromium.org/1129103003/diff/60001/chrome/browser/android/... chrome/browser/android/banners/app_banner_manager_android.cc:80: } else if (id.empty()) { Nit: no else after return (and ditto later). https://codereview.chromium.org/1129103003/diff/60001/chrome/browser/android/... chrome/browser/android/banners/app_banner_manager_android.cc:91: "not shown: data fetcher is not active"); This error should be the same as the 'the user navigated' one. https://codereview.chromium.org/1129103003/diff/60001/chrome/browser/android/... chrome/browser/android/banners/app_banner_manager_android.cc:97: "not shown: page URL does not match fetcher URL"); Same, here, I think this only happens when the user navigates. https://codereview.chromium.org/1129103003/diff/60001/chrome/browser/banners/... File chrome/browser/banners/app_banner_data_fetcher.cc (right): https://codereview.chromium.org/1129103003/diff/60001/chrome/browser/banners/... chrome/browser/banners/app_banner_data_fetcher.cc:8: #include "base/command_line.h" I don't think this include is needed now. https://codereview.chromium.org/1129103003/diff/60001/chrome/browser/banners/... chrome/browser/banners/app_banner_data_fetcher.cc:19: #include "chrome/common/chrome_switches.h" This too. https://codereview.chromium.org/1129103003/diff/60001/chrome/browser/banners/... chrome/browser/banners/app_banner_data_fetcher.cc:155: // TODO(mlamouri,benwells): we should probably record that to behave This TODO has been removed, it must have come back in your rebase. https://codereview.chromium.org/1129103003/diff/60001/chrome/browser/banners/... chrome/browser/banners/app_banner_data_fetcher.cc:245: if (manifest.IsEmpty()) This is a little convoluted. It would probably be nicer to pull the manifest emptiness check out into a new if clause. https://codereview.chromium.org/1129103003/diff/60001/chrome/browser/banners/... chrome/browser/banners/app_banner_data_fetcher.cc:306: "not shown: could not find icon specified in manifest"); I don't really understand how this could happen as I haven't looked at the FindBestMatchingIcon code, but I'm guessing it shouldn't happen based on our manifest checks. Despite that, in its current form it sounds more like we couldn't download the icon, so it should be rephrased as 'could not determine the best icon to use.' or something. https://codereview.chromium.org/1129103003/diff/60001/chrome/browser/banners/... chrome/browser/banners/app_banner_data_fetcher.cc:312: "the page URL"); This error is great but could still be improved. The exact rules for how the matching needs to work are kind of confusing. The matching is coded here: https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/se... The rule is that the service worker that is registered for the current page needs to also control the start URL from the manifest. I think a message that captures this would be something like: 'no matching service worker detected. You may need to reload the page, or check that the service worker for the current page also controls the start URL from the manifest.' https://codereview.chromium.org/1129103003/diff/60001/chrome/browser/banners/... chrome/browser/banners/app_banner_data_fetcher.cc:330: SendDebugMessage(web_contents, "not shown: no icon available to display"); Ditto about separating into two ifs. https://codereview.chromium.org/1129103003/diff/60001/chrome/browser/banners/... chrome/browser/banners/app_banner_data_fetcher.cc:337: SendDebugMessage(web_contents, "not shown: engagement checks failed"); This one is tricky. The only way a message will get printed here is if the banner has been added to the homescreen. As that might be confusing for developers it would be good to be more specific about it, but I'd also add a comment as it might not be obvious that this is the only case that can cause debug output. https://codereview.chromium.org/1129103003/diff/60001/chrome/browser/banners/... chrome/browser/banners/app_banner_data_fetcher.cc:342: SendDebugMessage(web_contents, "shown"); I don't think it is necessary to output this. This means the 'not shown' bit can move into SendDebugMessage. https://codereview.chromium.org/1129103003/diff/60001/chrome/browser/banners/... chrome/browser/banners/app_banner_data_fetcher.cc:375: "not shown: display pipeline halted before " + section); This message is too detailed. Please update to something like 'the user navigated before the banner could be shown.'. Also, the section is not needed. https://codereview.chromium.org/1129103003/diff/60001/chrome/browser/banners/... chrome/browser/banners/app_banner_data_fetcher.cc:379: return false; For completeness a message here would be good, like 'web contents closed.' https://codereview.chromium.org/1129103003/diff/60001/chrome/browser/banners/... chrome/browser/banners/app_banner_data_fetcher.cc:398: "not shown: manifest name and short name are missing"); It would be good to capture in the error that only one of them needs to be present. Something like 'one of manifest name or short name need to be specified.' https://codereview.chromium.org/1129103003/diff/60001/chrome/browser/banners/... chrome/browser/banners/app_banner_data_fetcher.cc:403: "not shown: manifest does not contain a suitable icon"); It would be good to capture more details about what a suitable icon is. https://codereview.chromium.org/1129103003/diff/60001/chrome/browser/banners/... File chrome/browser/banners/app_banner_data_fetcher.h (right): https://codereview.chromium.org/1129103003/diff/60001/chrome/browser/banners/... chrome/browser/banners/app_banner_data_fetcher.h:155: bool log_err_; Is this needed? https://codereview.chromium.org/1129103003/diff/60001/chrome/browser/banners/... File chrome/browser/banners/app_banner_data_fetcher_unittest.cc (right): https://codereview.chromium.org/1129103003/diff/60001/chrome/browser/banners/... chrome/browser/banners/app_banner_data_fetcher_unittest.cc:37: // to the console. logging doesn't take place if the pointer is null Please use proper case, class names and grammar in your comments. E.g.: 'The second argument is the WebContents pointer .... Logging doesn't take place if the ...'. https://codereview.chromium.org/1129103003/diff/60001/chrome/browser/banners/... File chrome/browser/banners/app_banner_debug_log.cc (right): https://codereview.chromium.org/1129103003/diff/60001/chrome/browser/banners/... chrome/browser/banners/app_banner_debug_log.cc:16: void SendDebugMessage(content::WebContents* web_contents, This name is very vague. Debug usually refers to chrome developers debug. Maybe 'OutputDeveloperMessage' would be better. Or something. https://codereview.chromium.org/1129103003/diff/60001/chrome/browser/banners/... chrome/browser/banners/app_banner_debug_log.cc:18: std::string log_message = "App banner " + message; Should this be "App banner: " with a colon? Edit: Ah, I see now. We should prevent the 'not shown' being repeated everywhere. Either just add it unconditionally here, or if it is not always used have two versions of the function. https://codereview.chromium.org/1129103003/diff/60001/chrome/browser/banners/... chrome/browser/banners/app_banner_debug_log.cc:27: }; // namespace banners Nit: no ";" https://codereview.chromium.org/1129103003/diff/60001/chrome/browser/banners/... File chrome/browser/banners/app_banner_debug_log.h (right): https://codereview.chromium.org/1129103003/diff/60001/chrome/browser/banners/... chrome/browser/banners/app_banner_debug_log.h:14: namespace banners { Since we have a shiny new file (yay!) it would be good to get all the constants for logging defined in here too. https://codereview.chromium.org/1129103003/diff/60001/chrome/browser/banners/... File chrome/browser/banners/app_banner_manager.cc (right): https://codereview.chromium.org/1129103003/diff/60001/chrome/browser/banners/... chrome/browser/banners/app_banner_manager.cc:55: "not shown: page not served over secure scheme"); FYI this is about to go out of date. To match the next version say something like 'from a secure origin.'
I'm trying to test my latest refactor locally before committing again, but the app banners don't seem to be working with the most up to date tree. Just checking out the clean tree and building that gives a binary which completely freezes when I load a site where an app banner can be displayed. I'll try rebasing and rebuilding tomorrow. https://codereview.chromium.org/1129103003/diff/60001/chrome/browser/android/... File chrome/browser/android/banners/app_banner_manager_android.cc (right): https://codereview.chromium.org/1129103003/diff/60001/chrome/browser/android/... chrome/browser/android/banners/app_banner_manager_android.cc:74: bool AppBannerManagerAndroid::CheckPlatformAndId(const std::string& platform, On 2015/05/11 08:08:39, benwells wrote: > Actually ... this probably shouldn't generate a 'not shown' message, as it might > be shown for the web platform (or not, if there are other problems). > > Maybe it should just generate a message saying 'XXX platform application ignored > as not supported on Android' or 'play application ignored as there is no id'. Done. https://codereview.chromium.org/1129103003/diff/60001/chrome/browser/android/... chrome/browser/android/banners/app_banner_manager_android.cc:91: "not shown: data fetcher is not active"); On 2015/05/11 08:08:39, benwells wrote: > This error should be the same as the 'the user navigated' one. Done. https://codereview.chromium.org/1129103003/diff/60001/chrome/browser/android/... chrome/browser/android/banners/app_banner_manager_android.cc:97: "not shown: page URL does not match fetcher URL"); On 2015/05/11 08:08:39, benwells wrote: > Same, here, I think this only happens when the user navigates. Done. https://codereview.chromium.org/1129103003/diff/60001/chrome/browser/banners/... File chrome/browser/banners/app_banner_data_fetcher.cc (right): https://codereview.chromium.org/1129103003/diff/60001/chrome/browser/banners/... chrome/browser/banners/app_banner_data_fetcher.cc:8: #include "base/command_line.h" On 2015/05/11 08:08:40, benwells wrote: > I don't think this include is needed now. Done. https://codereview.chromium.org/1129103003/diff/60001/chrome/browser/banners/... chrome/browser/banners/app_banner_data_fetcher.cc:19: #include "chrome/common/chrome_switches.h" On 2015/05/11 08:08:40, benwells wrote: > This too. Done. https://codereview.chromium.org/1129103003/diff/60001/chrome/browser/banners/... chrome/browser/banners/app_banner_data_fetcher.cc:155: // TODO(mlamouri,benwells): we should probably record that to behave On 2015/05/11 08:08:40, benwells wrote: > This TODO has been removed, it must have come back in your rebase. Done. https://codereview.chromium.org/1129103003/diff/60001/chrome/browser/banners/... chrome/browser/banners/app_banner_data_fetcher.cc:245: if (manifest.IsEmpty()) On 2015/05/11 08:08:40, benwells wrote: > This is a little convoluted. It would probably be nicer to pull the manifest > emptiness check out into a new if clause. Done. https://codereview.chromium.org/1129103003/diff/60001/chrome/browser/banners/... chrome/browser/banners/app_banner_data_fetcher.cc:306: "not shown: could not find icon specified in manifest"); On 2015/05/11 08:08:40, benwells wrote: > I don't really understand how this could happen as I haven't looked at the > FindBestMatchingIcon code, but I'm guessing it shouldn't happen based on our > manifest checks. > > Despite that, in its current form it sounds more like we couldn't download the > icon, so it should be rephrased as 'could not determine the best icon to use.' > or something. Done. https://codereview.chromium.org/1129103003/diff/60001/chrome/browser/banners/... chrome/browser/banners/app_banner_data_fetcher.cc:312: "the page URL"); On 2015/05/11 08:08:40, benwells wrote: > This error is great but could still be improved. The exact rules for how the > matching needs to work are kind of confusing. The matching is coded here: > https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/se... > > The rule is that the service worker that is registered for the current page > needs to also control the start URL from the manifest. > > I think a message that captures this would be something like: 'no matching > service worker detected. You may need to reload the page, or check that the > service worker for the current page also controls the start URL from the > manifest.' Done. https://codereview.chromium.org/1129103003/diff/60001/chrome/browser/banners/... chrome/browser/banners/app_banner_data_fetcher.cc:312: "the page URL"); On 2015/05/11 08:08:40, benwells wrote: > This error is great but could still be improved. The exact rules for how the > matching needs to work are kind of confusing. The matching is coded here: > https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/se... > > The rule is that the service worker that is registered for the current page > needs to also control the start URL from the manifest. > > I think a message that captures this would be something like: 'no matching > service worker detected. You may need to reload the page, or check that the > service worker for the current page also controls the start URL from the > manifest.' Done. https://codereview.chromium.org/1129103003/diff/60001/chrome/browser/banners/... chrome/browser/banners/app_banner_data_fetcher.cc:330: SendDebugMessage(web_contents, "not shown: no icon available to display"); On 2015/05/11 08:08:40, benwells wrote: > Ditto about separating into two ifs. Done. https://codereview.chromium.org/1129103003/diff/60001/chrome/browser/banners/... chrome/browser/banners/app_banner_data_fetcher.cc:337: SendDebugMessage(web_contents, "not shown: engagement checks failed"); On 2015/05/11 08:08:40, benwells wrote: > This one is tricky. The only way a message will get printed here is if the > banner has been added to the homescreen. As that might be confusing for > developers it would be good to be more specific about it, but I'd also add a > comment as it might not be obvious that this is the only case that can cause > debug output. Done. https://codereview.chromium.org/1129103003/diff/60001/chrome/browser/banners/... chrome/browser/banners/app_banner_data_fetcher.cc:342: SendDebugMessage(web_contents, "shown"); On 2015/05/11 08:08:40, benwells wrote: > I don't think it is necessary to output this. This means the 'not shown' bit can > move into SendDebugMessage. Done. https://codereview.chromium.org/1129103003/diff/60001/chrome/browser/banners/... chrome/browser/banners/app_banner_data_fetcher.cc:375: "not shown: display pipeline halted before " + section); On 2015/05/11 08:08:40, benwells wrote: > This message is too detailed. Please update to something like 'the user > navigated before the banner could be shown.'. Also, the section is not needed. Done. https://codereview.chromium.org/1129103003/diff/60001/chrome/browser/banners/... chrome/browser/banners/app_banner_data_fetcher.cc:379: return false; On 2015/05/11 08:08:40, benwells wrote: > For completeness a message here would be good, like 'web contents closed.' But if the web_contents pointer doesn't exist, we can't actually log a message to the console, because that requires a valid web_contents pointer. https://codereview.chromium.org/1129103003/diff/60001/chrome/browser/banners/... chrome/browser/banners/app_banner_data_fetcher.cc:398: "not shown: manifest name and short name are missing"); On 2015/05/11 08:08:40, benwells wrote: > It would be good to capture in the error that only one of them needs to be > present. Something like 'one of manifest name or short name need to be > specified.' Done. https://codereview.chromium.org/1129103003/diff/60001/chrome/browser/banners/... chrome/browser/banners/app_banner_data_fetcher.cc:403: "not shown: manifest does not contain a suitable icon"); On 2015/05/11 08:08:40, benwells wrote: > It would be good to capture more details about what a suitable icon is. Done. https://codereview.chromium.org/1129103003/diff/60001/chrome/browser/banners/... File chrome/browser/banners/app_banner_data_fetcher.h (right): https://codereview.chromium.org/1129103003/diff/60001/chrome/browser/banners/... chrome/browser/banners/app_banner_data_fetcher.h:155: bool log_err_; On 2015/05/11 08:08:40, benwells wrote: > Is this needed? Done. https://codereview.chromium.org/1129103003/diff/60001/chrome/browser/banners/... File chrome/browser/banners/app_banner_data_fetcher_unittest.cc (right): https://codereview.chromium.org/1129103003/diff/60001/chrome/browser/banners/... chrome/browser/banners/app_banner_data_fetcher_unittest.cc:37: // to the console. logging doesn't take place if the pointer is null On 2015/05/11 08:08:40, benwells wrote: > Please use proper case, class names and grammar in your comments. E.g.: > 'The second argument is the WebContents pointer .... Logging doesn't take place > if the ...'. Done. https://codereview.chromium.org/1129103003/diff/60001/chrome/browser/banners/... File chrome/browser/banners/app_banner_debug_log.cc (right): https://codereview.chromium.org/1129103003/diff/60001/chrome/browser/banners/... chrome/browser/banners/app_banner_debug_log.cc:16: void SendDebugMessage(content::WebContents* web_contents, On 2015/05/11 08:08:40, benwells wrote: > This name is very vague. Debug usually refers to chrome developers debug. Maybe > 'OutputDeveloperMessage' would be better. Or something. Done. https://codereview.chromium.org/1129103003/diff/60001/chrome/browser/banners/... chrome/browser/banners/app_banner_debug_log.cc:18: std::string log_message = "App banner " + message; On 2015/05/11 08:08:40, benwells wrote: > Should this be "App banner: " with a colon? > > Edit: Ah, I see now. We should prevent the 'not shown' being repeated > everywhere. Either just add it unconditionally here, or if it is not always used > have two versions of the function. Done. https://codereview.chromium.org/1129103003/diff/60001/chrome/browser/banners/... chrome/browser/banners/app_banner_debug_log.cc:27: }; // namespace banners On 2015/05/11 08:08:40, benwells wrote: > Nit: no ";" Done. https://codereview.chromium.org/1129103003/diff/60001/chrome/browser/banners/... File chrome/browser/banners/app_banner_debug_log.h (right): https://codereview.chromium.org/1129103003/diff/60001/chrome/browser/banners/... chrome/browser/banners/app_banner_debug_log.h:14: namespace banners { On 2015/05/11 08:08:40, benwells wrote: > Since we have a shiny new file (yay!) it would be good to get all the constants > for logging defined in here too. Done. https://codereview.chromium.org/1129103003/diff/60001/chrome/browser/banners/... File chrome/browser/banners/app_banner_manager.cc (right): https://codereview.chromium.org/1129103003/diff/60001/chrome/browser/banners/... chrome/browser/banners/app_banner_manager.cc:55: "not shown: page not served over secure scheme"); On 2015/05/11 08:08:40, benwells wrote: > FYI this is about to go out of date. To match the next version say something > like 'from a secure origin.' Done.
The CQ bit was checked by dominickn@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1129103003/80001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: No LGTM from a valid reviewer yet. Only full committers are accepted. Even if an LGTM may have been provided, it was from a non-committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
Just a bunch of nits now. https://codereview.chromium.org/1129103003/diff/80001/chrome/browser/android/... File chrome/browser/android/banners/app_banner_manager_android.cc (right): https://codereview.chromium.org/1129103003/diff/80001/chrome/browser/android/... chrome/browser/android/banners/app_banner_manager_android.cc:80: } else if (id.empty()) { Nit (here and elsewhere): no else after return. https://codereview.chromium.org/1129103003/diff/80001/chrome/browser/banners/... File chrome/browser/banners/app_banner_data_fetcher.cc (right): https://codereview.chromium.org/1129103003/diff/80001/chrome/browser/banners/... chrome/browser/banners/app_banner_data_fetcher.cc:240: } else if (manifest.IsEmpty()) { Nit: no else after return. https://codereview.chromium.org/1129103003/diff/80001/chrome/browser/banners/... chrome/browser/banners/app_banner_data_fetcher.cc:322: } else if (!icon) { Nit: no else after return. https://codereview.chromium.org/1129103003/diff/80001/chrome/browser/banners/... File chrome/browser/banners/app_banner_debug_log.cc (right): https://codereview.chromium.org/1129103003/diff/80001/chrome/browser/banners/... chrome/browser/banners/app_banner_debug_log.cc:41: const char kIgnoredNoId[] = "Play application ignored as there is no id"; I think this one will end up coming out as "App banner Play application ignored as there is no id". It would be nice if it has a colon somewhere, maybe "App banner play application ignored: no id provided." https://codereview.chromium.org/1129103003/diff/80001/chrome/browser/banners/... chrome/browser/banners/app_banner_debug_log.cc:42: Nit: only one blank line here please.
https://codereview.chromium.org/1129103003/diff/80001/chrome/browser/android/... File chrome/browser/android/banners/app_banner_manager_android.cc (right): https://codereview.chromium.org/1129103003/diff/80001/chrome/browser/android/... chrome/browser/android/banners/app_banner_manager_android.cc:80: } else if (id.empty()) { On 2015/05/13 02:55:20, benwells wrote: > Nit (here and elsewhere): no else after return. Done. https://codereview.chromium.org/1129103003/diff/80001/chrome/browser/banners/... File chrome/browser/banners/app_banner_data_fetcher.cc (right): https://codereview.chromium.org/1129103003/diff/80001/chrome/browser/banners/... chrome/browser/banners/app_banner_data_fetcher.cc:240: } else if (manifest.IsEmpty()) { On 2015/05/13 02:55:20, benwells wrote: > Nit: no else after return. Done. https://codereview.chromium.org/1129103003/diff/80001/chrome/browser/banners/... chrome/browser/banners/app_banner_data_fetcher.cc:322: } else if (!icon) { On 2015/05/13 02:55:20, benwells wrote: > Nit: no else after return. Done. https://codereview.chromium.org/1129103003/diff/80001/chrome/browser/banners/... File chrome/browser/banners/app_banner_debug_log.cc (right): https://codereview.chromium.org/1129103003/diff/80001/chrome/browser/banners/... chrome/browser/banners/app_banner_debug_log.cc:41: const char kIgnoredNoId[] = "Play application ignored as there is no id"; On 2015/05/13 02:55:20, benwells wrote: > I think this one will end up coming out as "App banner Play application ignored > as there is no id". > > It would be nice if it has a colon somewhere, maybe "App banner play application > ignored: no id provided." Done. https://codereview.chromium.org/1129103003/diff/80001/chrome/browser/banners/... chrome/browser/banners/app_banner_debug_log.cc:42: On 2015/05/13 02:55:20, benwells wrote: > Nit: only one blank line here please. Done.
Nice! lgtm
dominickn@google.com changed reviewers: + dfalcantara@chromium.org, thestig@chromium.org, tsepez@chromium.org
+dfalcantara: chrome/browser/android +tsepez: chrome/common +thestig: chrome/renderer
chrome/renderer lgtm https://codereview.chromium.org/1129103003/diff/100001/chrome/browser/banners... File chrome/browser/banners/app_banner_data_fetcher.cc (right): https://codereview.chromium.org/1129103003/diff/100001/chrome/browser/banners... chrome/browser/banners/app_banner_data_fetcher.cc:374: return false; // We cannot show a message if web_contents is null nit: |web_contents| https://codereview.chromium.org/1129103003/diff/100001/chrome/chrome_browser.... File chrome/chrome_browser.gypi (right): https://codereview.chromium.org/1129103003/diff/100001/chrome/chrome_browser.... chrome/chrome_browser.gypi:219: 'browser/banners/app_banner_debug_log.cc', nit: alphabetical order please.
https://codereview.chromium.org/1129103003/diff/100001/chrome/browser/banners... File chrome/browser/banners/app_banner_data_fetcher.cc (right): https://codereview.chromium.org/1129103003/diff/100001/chrome/browser/banners... chrome/browser/banners/app_banner_data_fetcher.cc:374: return false; // We cannot show a message if web_contents is null On 2015/05/13 06:06:30, Lei Zhang wrote: > nit: |web_contents| Done. https://codereview.chromium.org/1129103003/diff/100001/chrome/chrome_browser.... File chrome/chrome_browser.gypi (right): https://codereview.chromium.org/1129103003/diff/100001/chrome/chrome_browser.... chrome/chrome_browser.gypi:219: 'browser/banners/app_banner_debug_log.cc', On 2015/05/13 06:06:30, Lei Zhang wrote: > nit: alphabetical order please. Done.
Messages LGTM.
lgtm https://codereview.chromium.org/1129103003/diff/120001/chrome/browser/banners... File chrome/browser/banners/app_banner_data_fetcher.h (right): https://codereview.chromium.org/1129103003/diff/120001/chrome/browser/banners... chrome/browser/banners/app_banner_data_fetcher.h:142: // closed nit: add a period https://codereview.chromium.org/1129103003/diff/120001/chrome/browser/banners... File chrome/browser/banners/app_banner_debug_log.cc (right): https://codereview.chromium.org/1129103003/diff/120001/chrome/browser/banners... chrome/browser/banners/app_banner_debug_log.cc:17: "renderer has requested the banner prompt be cancelled"; Are these strings we need to worry about translating?
https://codereview.chromium.org/1129103003/diff/120001/chrome/browser/banners... File chrome/browser/banners/app_banner_data_fetcher.h (right): https://codereview.chromium.org/1129103003/diff/120001/chrome/browser/banners... chrome/browser/banners/app_banner_data_fetcher.h:142: // closed On 2015/05/13 17:54:30, dfalcantara wrote: > nit: add a period Done. https://codereview.chromium.org/1129103003/diff/120001/chrome/browser/banners... File chrome/browser/banners/app_banner_debug_log.cc (right): https://codereview.chromium.org/1129103003/diff/120001/chrome/browser/banners... chrome/browser/banners/app_banner_debug_log.cc:17: "renderer has requested the banner prompt be cancelled"; On 2015/05/13 17:54:30, dfalcantara wrote: > Are these strings we need to worry about translating? I think that for developer-facing strings, localisation isn't needed (e.g. https://cs.corp.google.com/#clankium/src/extensions/common/manifest_constants...)
The CQ bit was checked by dominickn@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from thestig@chromium.org, benwells@chromium.org, tsepez@chromium.org, dfalcantara@chromium.org Link to the patchset: https://codereview.chromium.org/1129103003/#ps140001 (title: "Adding a full stop to the end of a comment")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1129103003/140001
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/0b93e0d31157dac42756c416e3dcac3a7669d6eb Cr-Commit-Position: refs/heads/master@{#329751} |
