Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2452603004:
Update comments in InstallableManager. (Closed)
DescriptionUpdate comments in InstallableManager.
Update comments on kIconMinimumSizeInPx to reveal the origin of the
144px minimum icon size requirement.
BUG=649771
Committed: https://crrev.com/76b843bdcc0284eedd4c66c381dd85a4300b77b0
Cr-Commit-Position: refs/heads/master@{#428379}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Addressing comments #Messages
Total messages: 16 (7 generated)
Description was changed from ========== Update comments in InstallableManager. BUG= ========== to ========== Update comments in InstallableManager. Update comments on kIconMinimumSizeInPx to reveal the origin of the 144px minimum icon size requirement. Update comments on CheckInstallable(), CheckAndFetchBestIcon(), etc. to better reflect the designed work flow, which is well tested in unit tests and browser tests. BUG=649771 ==========
zpeng@chromium.org changed reviewers: + dominickn@chromium.org, pkotwicz@chromium.org
Hi Dominick & Peter, PTAL. Thanks!
Looks mostly good to me. I am passing this one to Dominick
Please wrap the CL description to a width of 80 characters. https://codereview.chromium.org/2452603004/diff/1/chrome/browser/installable/... File chrome/browser/installable/installable_manager.cc (right): https://codereview.chromium.org/2452603004/diff/1/chrome/browser/installable/... chrome/browser/installable/installable_manager.cc:28: // https://developers.google.com/web/fundamentals/engage-and-retain/app-install-..., The links here are unnecessary (and overflow the 80 character width limit). Please remove them. The only addition that's really necessary is the sentence. "It is the currently advertised minimum icon size for triggering banners." https://codereview.chromium.org/2452603004/diff/1/chrome/browser/installable/... File chrome/browser/installable/installable_manager.h (right): https://codereview.chromium.org/2452603004/diff/1/chrome/browser/installable/... chrome/browser/installable/installable_manager.h:160: // Methods checking for InstallableParams-independent basic requirements. This comment isn't accurate. CheckServiceWorker() isn't a basic requirement, it's only run if InstallableParams.check_installable is true. I don't think either of the new comments in this header file are really necessary. Are the method names not descriptive enough?
Shameless self promotion: The extension at https://goto.google.com/reflow_chromium_cl_comments makes wrapping CL descriptions to 72 characters easy
Description was changed from ========== Update comments in InstallableManager. Update comments on kIconMinimumSizeInPx to reveal the origin of the 144px minimum icon size requirement. Update comments on CheckInstallable(), CheckAndFetchBestIcon(), etc. to better reflect the designed work flow, which is well tested in unit tests and browser tests. BUG=649771 ========== to ========== Update comments in InstallableManager. Update comments on kIconMinimumSizeInPx to reveal the origin of the 144px minimum icon size requirement. Update comments on CheckInstallable(), CheckAndFetchBestIcon(), etc. to better reflect the designed work flow, which is well tested in unit tests and browser tests. BUG=649771 ==========
Description was changed from ========== Update comments in InstallableManager. Update comments on kIconMinimumSizeInPx to reveal the origin of the 144px minimum icon size requirement. Update comments on CheckInstallable(), CheckAndFetchBestIcon(), etc. to better reflect the designed work flow, which is well tested in unit tests and browser tests. BUG=649771 ========== to ========== Update comments in InstallableManager. Update comments on kIconMinimumSizeInPx to reveal the origin of the 144px minimum icon size requirement. BUG=649771 ==========
Thanks Dominick & Peter, PTAL! Also thanks for the extension, Peter :) https://codereview.chromium.org/2452603004/diff/1/chrome/browser/installable/... File chrome/browser/installable/installable_manager.cc (right): https://codereview.chromium.org/2452603004/diff/1/chrome/browser/installable/... chrome/browser/installable/installable_manager.cc:28: // https://developers.google.com/web/fundamentals/engage-and-retain/app-install-..., On 2016/10/27 00:26:36, dominickn wrote: > The links here are unnecessary (and overflow the 80 character width limit). > Please remove them. The only addition that's really necessary is the sentence. > > "It is the currently advertised minimum icon size for triggering banners." Done. https://codereview.chromium.org/2452603004/diff/1/chrome/browser/installable/... File chrome/browser/installable/installable_manager.h (right): https://codereview.chromium.org/2452603004/diff/1/chrome/browser/installable/... chrome/browser/installable/installable_manager.h:160: // Methods checking for InstallableParams-independent basic requirements. On 2016/10/27 00:26:36, dominickn wrote: > This comment isn't accurate. CheckServiceWorker() isn't a basic requirement, > it's only run if InstallableParams.check_installable is true. > > I don't think either of the new comments in this header file are really > necessary. Are the method names not descriptive enough? Thanks for pointing out the check_installable bit! I was trying to further distinguish these two groups of functions because CheckAndFetchBestIcon() actually uses InstallableParams inside function body like a function parameter while CheckInstallable() does not.
lgtm, thanks
The CQ bit was checked by zpeng@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Update comments in InstallableManager. Update comments on kIconMinimumSizeInPx to reveal the origin of the 144px minimum icon size requirement. BUG=649771 ========== to ========== Update comments in InstallableManager. Update comments on kIconMinimumSizeInPx to reveal the origin of the 144px minimum icon size requirement. BUG=649771 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Update comments in InstallableManager. Update comments on kIconMinimumSizeInPx to reveal the origin of the 144px minimum icon size requirement. BUG=649771 ========== to ========== Update comments in InstallableManager. Update comments on kIconMinimumSizeInPx to reveal the origin of the 144px minimum icon size requirement. BUG=649771 Committed: https://crrev.com/76b843bdcc0284eedd4c66c381dd85a4300b77b0 Cr-Commit-Position: refs/heads/master@{#428379} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/76b843bdcc0284eedd4c66c381dd85a4300b77b0 Cr-Commit-Position: refs/heads/master@{#428379} |
