Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1261143004:
Implement manifest icon downloader (Closed)
|
Created:
5 years, 4 months ago by Lalit Maganti Modified:
5 years, 4 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. |
DescriptionImplement manifest icon downloader
This CL unifies code used by app banners and shortcut
helper into one class. It further implements icon scaling
which fixes the issue of Intent overflow on Android when
there is a bad manifest/only a big icon is provided.
It also refactors code for a future CL which will implement
downloading of another icon for installed webapps
splashscreen on Android.
BUG=513666
Committed: https://crrev.com/e109fb09878be947da59c39b5876ae36c187d534
Cr-Commit-Position: refs/heads/master@{#344787}
Patch Set 1 #Patch Set 2 : Implement downloader algorithm #Patch Set 3 : Add scaling to non correct icons #
Total comments: 36
Patch Set 4 : Fix issues raised in comments #
Total comments: 30
Patch Set 5 : Fix issues raised in review #Patch Set 6 : Refactor and add unit tests #Patch Set 7 : Make ShortcutHelper use icon download class too #
Total comments: 2
Patch Set 8 : Address comments on previous review #
Total comments: 24
Patch Set 9 : Address comments on review #
Total comments: 16
Patch Set 10 : Fix Mounir's comments #Patch Set 11 : Fix compile #Patch Set 12 : Fix test failures #Patch Set 13 : Remove unecessary code #Patch Set 14 : Fix test failures on Mac #Patch Set 15 : Fix remaining test failures #Patch Set 16 : Fix test compile #Patch Set 17 : Implement lower bound for icons and ignore scaling for smaller icons #Patch Set 18 : Implement discussed algorithm #Patch Set 19 : Fix compile #
Total comments: 6
Patch Set 20 : Rework icon downloader code #Patch Set 21 : Set upstream #Patch Set 22 : Fix bad file #Patch Set 23 : Fix compile and test failures #Patch Set 24 : Move app banner test changes to icon selector cl #Patch Set 25 : Remove bad file (again) #Patch Set 26 : Fix algorithm again #Patch Set 27 : Address silly regressions due to switching between Android <-> Linux #
Total comments: 10
Patch Set 28 : Fix comments on last review #Patch Set 29 : Rebase #
Total comments: 22
Patch Set 30 : Address review comments #Patch Set 31 : Rebase #Patch Set 32 : Fix favicon fallback in add to homescreen helper #
Total comments: 12
Patch Set 33 : Fix review comments #
Total comments: 6
Patch Set 34 : Address comments on review #Patch Set 35 : Rebase #Messages
Total messages: 79 (17 generated)
lalitm@google.com changed reviewers: + dfalcantara@chromium.org
Dan: as we discussed yesterday, I've factored out all the icon downloading/choosing code into its own class. Would appreciate if you could take a quick look and let me know if I'm on the right track. Once Mounir gets back I'll also discuss the IconSelector code with him and see if any improvements can be made there too. https://codereview.chromium.org/1261143004/diff/30008/chrome/browser/android/... File chrome/browser/android/banners/app_banner_manager_android.cc (right): https://codereview.chromium.org/1261143004/diff/30008/chrome/browser/android/... chrome/browser/android/banners/app_banner_manager_android.cc:161: return true /*base::FieldTrialList::FindFullName("AppBanners") == "Enabled"*/; Temporary hack because app banners are not enabled for me :(
https://codereview.chromium.org/1261143004/diff/30008/chrome/browser/android/... File chrome/browser/android/banners/app_banner_manager_android.cc (right): https://codereview.chromium.org/1261143004/diff/30008/chrome/browser/android/... chrome/browser/android/banners/app_banner_manager_android.cc:161: return true /*base::FieldTrialList::FindFullName("AppBanners") == "Enabled"*/; On 2015/07/31 13:49:22, Lalit Maganti wrote: > Temporary hack because app banners are not enabled for me :( Finch flags for bannera can be manually set via the command line flag: --force-fieldtrials="AppBanners/Enabled/"
Can you attach a bug number to this?
The CQ bit was checked by dfalcantara@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/1261143004/30008 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1261143004/30008
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
https://codereview.chromium.org/1261143004/diff/30008/chrome/browser/banners/... File chrome/browser/banners/app_banner_data_fetcher.cc (right): https://codereview.chromium.org/1261143004/diff/30008/chrome/browser/banners/... chrome/browser/banners/app_banner_data_fetcher.cc:299: OnHasServiceWorker(web_contents); Not sure why you pulled this out. The function was already short, you now have to put Cancel() in both places, and I've generally seen OnBlah function names used for bound callbacks (like OnDidCheckHasServiceWorker). https://codereview.chromium.org/1261143004/diff/30008/chrome/browser/banners/... chrome/browser/banners/app_banner_data_fetcher.cc:321: bool AppBannerDataFetcher::FetchAppIcon(content::WebContents* web_contents, Was there anything else that was going to call this function? If not, inline it. https://codereview.chromium.org/1261143004/diff/30008/chrome/browser/banners/... chrome/browser/banners/app_banner_data_fetcher.cc:354: const SkBitmap& icon) { * Why can't you call GetWebContents() here? That'll have the most up to date pointer (e.g. if it's been destroyed, you might be holding onto a bad pointer) * You may as well inline this function, too. The only reason it wasn't inlined before was because we had to ensure proper refcounting. https://codereview.chromium.org/1261143004/diff/30008/chrome/browser/banners/... File chrome/browser/banners/app_banner_data_fetcher.h (right): https://codereview.chromium.org/1261143004/diff/30008/chrome/browser/banners/... chrome/browser/banners/app_banner_data_fetcher.h:123: void OnHasServiceWorker(content::WebContents* web_contents); This should be private. Remove the comment; it makes it sound like a callback when it's not. https://codereview.chromium.org/1261143004/diff/30008/chrome/browser/banners/... chrome/browser/banners/app_banner_data_fetcher.h:130: const scoped_ptr<ManifestIconDownloader>& icon_downloader() { return icon_downloader_; } Why is this accessor necessary? Does anything call it? https://codereview.chromium.org/1261143004/diff/30008/chrome/browser/manifest... File chrome/browser/manifest/manifest_icon_downloader.cc (right): https://codereview.chromium.org/1261143004/diff/30008/chrome/browser/manifest... chrome/browser/manifest/manifest_icon_downloader.cc:38: int ideal_icon_size, rename this ideal_icon_size_in_units (dp?) https://codereview.chromium.org/1261143004/diff/30008/chrome/browser/manifest... chrome/browser/manifest/manifest_icon_downloader.cc:54: const float scaled_ideal_icon_size = ideal_icon_size * device_scale_factor; rename this so that the unit is obvious https://codereview.chromium.org/1261143004/diff/30008/chrome/browser/manifest... chrome/browser/manifest/manifest_icon_downloader.cc:56: const SkBitmap* best_upper = nullptr; best upper what? Document everything about the selection algorithm you're using, especially since it seems to be heuristic and different from the one in ManifestIconSelector::FindBestMatchingIcon (which documents all the things). https://codereview.chromium.org/1261143004/diff/30008/chrome/browser/manifest... chrome/browser/manifest/manifest_icon_downloader.cc:78: float soft_upper_bound = 1.5 * scaled_ideal_icon_size; How'd you come up with these bounds? 2/1 and 1/2 I can I reason about, but 3/2 and 9/10? https://codereview.chromium.org/1261143004/diff/30008/chrome/browser/manifest... chrome/browser/manifest/manifest_icon_downloader.cc:84: if (best_upper && best_upper->height() < soft_upper_bound) { I don't think it ever makes sense to scale up if you have a higher quality icon that you can scale down. https://codereview.chromium.org/1261143004/diff/30008/chrome/browser/manifest... File chrome/browser/manifest/manifest_icon_downloader.h (right): https://codereview.chromium.org/1261143004/diff/30008/chrome/browser/manifest... chrome/browser/manifest/manifest_icon_downloader.h:26: typedef base::Callback<void(const SkBitmap&)> Callback; nit: Move the typedef above the constructor. Seems cleaner and is the same way the CancelableTaskTracker and WaitableEventWatcher do it. https://codereview.chromium.org/1261143004/diff/30008/chrome/browser/manifest... chrome/browser/manifest/manifest_icon_downloader.h:27: bool Download(const GURL& icon_url, Add documentation. https://codereview.chromium.org/1261143004/diff/30008/chrome/browser/manifest... chrome/browser/manifest/manifest_icon_downloader.h:32: // Callback run after an attempt to download manifest icon has been made. nit: to download the manifest icon
Thanks for reviewing! Just a point which you've mentioned multiple times which I want to address here: I'm not really sure what the convention is for preparing code for future CL in a parent one but much of the code I extracted out into functions in app_banner_fetcher was because I would call that code from a child class in my CL which downloads the splashscreen icon too. https://codereview.chromium.org/1261143004/diff/30008/chrome/browser/banners/... File chrome/browser/banners/app_banner_data_fetcher.cc (right): https://codereview.chromium.org/1261143004/diff/30008/chrome/browser/banners/... chrome/browser/banners/app_banner_data_fetcher.cc:299: OnHasServiceWorker(web_contents); On 2015/07/31 at 18:09:19, dfalcantara wrote: > Not sure why you pulled this out. The function was already short, you now have to put Cancel() in both places, and I've generally seen OnBlah function names used for bound callbacks (like OnDidCheckHasServiceWorker). For future use. https://codereview.chromium.org/1261143004/diff/30008/chrome/browser/banners/... chrome/browser/banners/app_banner_data_fetcher.cc:321: bool AppBannerDataFetcher::FetchAppIcon(content::WebContents* web_contents, On 2015/07/31 at 18:09:20, dfalcantara wrote: > Was there anything else that was going to call this function? If not, inline it. For future use. https://codereview.chromium.org/1261143004/diff/30008/chrome/browser/banners/... chrome/browser/banners/app_banner_data_fetcher.cc:354: const SkBitmap& icon) { On 2015/07/31 at 18:09:20, dfalcantara wrote: > * Why can't you call GetWebContents() here? That'll have the most up to date pointer (e.g. if it's been destroyed, you might be holding onto a bad pointer) > > * You may as well inline this function, too. The only reason it wasn't inlined before was because we had to ensure proper refcounting. 1. I can I guess. 2. Yeah I don't need this so I can inline this I think. https://codereview.chromium.org/1261143004/diff/30008/chrome/browser/banners/... File chrome/browser/banners/app_banner_data_fetcher.h (right): https://codereview.chromium.org/1261143004/diff/30008/chrome/browser/banners/... chrome/browser/banners/app_banner_data_fetcher.h:123: void OnHasServiceWorker(content::WebContents* web_contents); On 2015/07/31 at 18:09:20, dfalcantara wrote: > This should be private. Remove the comment; it makes it sound like a callback when it's not. protected for future use. Will remove comment. https://codereview.chromium.org/1261143004/diff/30008/chrome/browser/banners/... chrome/browser/banners/app_banner_data_fetcher.h:130: const scoped_ptr<ManifestIconDownloader>& icon_downloader() { return icon_downloader_; } On 2015/07/31 at 18:09:20, dfalcantara wrote: > Why is this accessor necessary? Does anything call it? For future use. https://codereview.chromium.org/1261143004/diff/30008/chrome/browser/manifest... File chrome/browser/manifest/manifest_icon_downloader.cc (right): https://codereview.chromium.org/1261143004/diff/30008/chrome/browser/manifest... chrome/browser/manifest/manifest_icon_downloader.cc:38: int ideal_icon_size, On 2015/07/31 at 18:09:20, dfalcantara wrote: > rename this ideal_icon_size_in_units (dp?) Will do. https://codereview.chromium.org/1261143004/diff/30008/chrome/browser/manifest... chrome/browser/manifest/manifest_icon_downloader.cc:54: const float scaled_ideal_icon_size = ideal_icon_size * device_scale_factor; On 2015/07/31 at 18:09:20, dfalcantara wrote: > rename this so that the unit is obvious Will do. https://codereview.chromium.org/1261143004/diff/30008/chrome/browser/manifest... chrome/browser/manifest/manifest_icon_downloader.cc:56: const SkBitmap* best_upper = nullptr; On 2015/07/31 at 18:09:20, dfalcantara wrote: > best upper what? Document everything about the selection algorithm you're using, especially since it seems to be heuristic and different from the one in ManifestIconSelector::FindBestMatchingIcon (which documents all the things). Will do. https://codereview.chromium.org/1261143004/diff/30008/chrome/browser/manifest... chrome/browser/manifest/manifest_icon_downloader.cc:78: float soft_upper_bound = 1.5 * scaled_ideal_icon_size; On 2015/07/31 at 18:09:20, dfalcantara wrote: > How'd you come up with these bounds? 2/1 and 1/2 I can I reason about, but 3/2 and 9/10? They are completely arbitary - I just put them in as placeholders for now. https://codereview.chromium.org/1261143004/diff/30008/chrome/browser/manifest... chrome/browser/manifest/manifest_icon_downloader.cc:84: if (best_upper && best_upper->height() < soft_upper_bound) { On 2015/07/31 at 18:09:20, dfalcantara wrote: > I don't think it ever makes sense to scale up if you have a higher quality icon that you can scale down. Hmmm this is an interesting one. I was thinking along the same lines but then I also think that if you have an icon which is just a bit smaller than required (say 95%), it may be better to scale it up rather than scaling down a 200% icon. I'm not really sure if this is correct thinking though - I've never really looked into image scaling too deeply. https://codereview.chromium.org/1261143004/diff/30008/chrome/browser/manifest... File chrome/browser/manifest/manifest_icon_downloader.h (right): https://codereview.chromium.org/1261143004/diff/30008/chrome/browser/manifest... chrome/browser/manifest/manifest_icon_downloader.h:26: typedef base::Callback<void(const SkBitmap&)> Callback; On 2015/07/31 at 18:09:20, dfalcantara wrote: > nit: Move the typedef above the constructor. Seems cleaner and is the same way the CancelableTaskTracker and WaitableEventWatcher do it. Will do. https://codereview.chromium.org/1261143004/diff/30008/chrome/browser/manifest... chrome/browser/manifest/manifest_icon_downloader.h:27: bool Download(const GURL& icon_url, On 2015/07/31 at 18:09:20, dfalcantara wrote: > Add documentation. Will do. https://codereview.chromium.org/1261143004/diff/30008/chrome/browser/manifest... chrome/browser/manifest/manifest_icon_downloader.h:32: // Callback run after an attempt to download manifest icon has been made. On 2015/07/31 at 18:09:20, dfalcantara wrote: > nit: to download the manifest icon Will do.
Thanks for reviewing! Just a point which you've mentioned multiple times which I want to address here: I'm not really sure what the convention is for preparing code for future CL in a parent one but much of the code I extracted out into functions in app_banner_fetcher was because I would call that code from a child class in my CL which downloads the splashscreen icon too. https://codereview.chromium.org/1261143004/diff/30008/chrome/browser/banners/... File chrome/browser/banners/app_banner_data_fetcher.cc (right): https://codereview.chromium.org/1261143004/diff/30008/chrome/browser/banners/... chrome/browser/banners/app_banner_data_fetcher.cc:299: OnHasServiceWorker(web_contents); On 2015/07/31 at 18:09:19, dfalcantara wrote: > Not sure why you pulled this out. The function was already short, you now have to put Cancel() in both places, and I've generally seen OnBlah function names used for bound callbacks (like OnDidCheckHasServiceWorker). For future use. https://codereview.chromium.org/1261143004/diff/30008/chrome/browser/banners/... chrome/browser/banners/app_banner_data_fetcher.cc:321: bool AppBannerDataFetcher::FetchAppIcon(content::WebContents* web_contents, On 2015/07/31 at 18:09:20, dfalcantara wrote: > Was there anything else that was going to call this function? If not, inline it. For future use. https://codereview.chromium.org/1261143004/diff/30008/chrome/browser/banners/... chrome/browser/banners/app_banner_data_fetcher.cc:354: const SkBitmap& icon) { On 2015/07/31 at 18:09:20, dfalcantara wrote: > * Why can't you call GetWebContents() here? That'll have the most up to date pointer (e.g. if it's been destroyed, you might be holding onto a bad pointer) > > * You may as well inline this function, too. The only reason it wasn't inlined before was because we had to ensure proper refcounting. 1. I can I guess. 2. Yeah I don't need this so I can inline this I think. https://codereview.chromium.org/1261143004/diff/30008/chrome/browser/banners/... File chrome/browser/banners/app_banner_data_fetcher.h (right): https://codereview.chromium.org/1261143004/diff/30008/chrome/browser/banners/... chrome/browser/banners/app_banner_data_fetcher.h:123: void OnHasServiceWorker(content::WebContents* web_contents); On 2015/07/31 at 18:09:20, dfalcantara wrote: > This should be private. Remove the comment; it makes it sound like a callback when it's not. protected for future use. Will remove comment. https://codereview.chromium.org/1261143004/diff/30008/chrome/browser/banners/... chrome/browser/banners/app_banner_data_fetcher.h:130: const scoped_ptr<ManifestIconDownloader>& icon_downloader() { return icon_downloader_; } On 2015/07/31 at 18:09:20, dfalcantara wrote: > Why is this accessor necessary? Does anything call it? For future use. https://codereview.chromium.org/1261143004/diff/30008/chrome/browser/manifest... File chrome/browser/manifest/manifest_icon_downloader.cc (right): https://codereview.chromium.org/1261143004/diff/30008/chrome/browser/manifest... chrome/browser/manifest/manifest_icon_downloader.cc:38: int ideal_icon_size, On 2015/07/31 at 18:09:20, dfalcantara wrote: > rename this ideal_icon_size_in_units (dp?) Will do. https://codereview.chromium.org/1261143004/diff/30008/chrome/browser/manifest... chrome/browser/manifest/manifest_icon_downloader.cc:54: const float scaled_ideal_icon_size = ideal_icon_size * device_scale_factor; On 2015/07/31 at 18:09:20, dfalcantara wrote: > rename this so that the unit is obvious Will do. https://codereview.chromium.org/1261143004/diff/30008/chrome/browser/manifest... chrome/browser/manifest/manifest_icon_downloader.cc:56: const SkBitmap* best_upper = nullptr; On 2015/07/31 at 18:09:20, dfalcantara wrote: > best upper what? Document everything about the selection algorithm you're using, especially since it seems to be heuristic and different from the one in ManifestIconSelector::FindBestMatchingIcon (which documents all the things). Will do. https://codereview.chromium.org/1261143004/diff/30008/chrome/browser/manifest... chrome/browser/manifest/manifest_icon_downloader.cc:78: float soft_upper_bound = 1.5 * scaled_ideal_icon_size; On 2015/07/31 at 18:09:20, dfalcantara wrote: > How'd you come up with these bounds? 2/1 and 1/2 I can I reason about, but 3/2 and 9/10? They are completely arbitary - I just put them in as placeholders for now. https://codereview.chromium.org/1261143004/diff/30008/chrome/browser/manifest... chrome/browser/manifest/manifest_icon_downloader.cc:84: if (best_upper && best_upper->height() < soft_upper_bound) { On 2015/07/31 at 18:09:20, dfalcantara wrote: > I don't think it ever makes sense to scale up if you have a higher quality icon that you can scale down. Hmmm this is an interesting one. I was thinking along the same lines but then I also think that if you have an icon which is just a bit smaller than required (say 95%), it may be better to scale it up rather than scaling down a 200% icon. I'm not really sure if this is correct thinking though - I've never really looked into image scaling too deeply. https://codereview.chromium.org/1261143004/diff/30008/chrome/browser/manifest... File chrome/browser/manifest/manifest_icon_downloader.h (right): https://codereview.chromium.org/1261143004/diff/30008/chrome/browser/manifest... chrome/browser/manifest/manifest_icon_downloader.h:26: typedef base::Callback<void(const SkBitmap&)> Callback; On 2015/07/31 at 18:09:20, dfalcantara wrote: > nit: Move the typedef above the constructor. Seems cleaner and is the same way the CancelableTaskTracker and WaitableEventWatcher do it. Will do. https://codereview.chromium.org/1261143004/diff/30008/chrome/browser/manifest... chrome/browser/manifest/manifest_icon_downloader.h:27: bool Download(const GURL& icon_url, On 2015/07/31 at 18:09:20, dfalcantara wrote: > Add documentation. Will do. https://codereview.chromium.org/1261143004/diff/30008/chrome/browser/manifest... chrome/browser/manifest/manifest_icon_downloader.h:32: // Callback run after an attempt to download manifest icon has been made. On 2015/07/31 at 18:09:20, dfalcantara wrote: > nit: to download the manifest icon Will do.
In cases of refactoring for future CLs, it helps if you link to a follow up CL where you're using the new functions you're exposing. This lets the reviewers see if it makes sense to expose them, or if there's a better way to do it. Barring that, you generally want to at least say something about it in the commit message. Having neither results in a lot of head scratching for the reviewers :/ https://codereview.chromium.org/1261143004/diff/30008/chrome/browser/banners/... File chrome/browser/banners/app_banner_data_fetcher.cc (right): https://codereview.chromium.org/1261143004/diff/30008/chrome/browser/banners/... chrome/browser/banners/app_banner_data_fetcher.cc:299: OnHasServiceWorker(web_contents); On 2015/08/03 08:58:29, Lalit Maganti wrote: > On 2015/07/31 at 18:09:19, dfalcantara wrote: > > Not sure why you pulled this out. The function was already short, you now > have to put Cancel() in both places, and I've generally seen OnBlah function > names used for bound callbacks (like OnDidCheckHasServiceWorker). > > For future use. Is the future use going to be a bound callback? https://codereview.chromium.org/1261143004/diff/30008/chrome/browser/manifest... File chrome/browser/manifest/manifest_icon_downloader.cc (right): https://codereview.chromium.org/1261143004/diff/30008/chrome/browser/manifest... chrome/browser/manifest/manifest_icon_downloader.cc:78: float soft_upper_bound = 1.5 * scaled_ideal_icon_size; On 2015/08/03 08:58:29, Lalit Maganti wrote: > On 2015/07/31 at 18:09:20, dfalcantara wrote: > > How'd you come up with these bounds? 2/1 and 1/2 I can I reason about, but > 3/2 and 9/10? > > They are completely arbitary - I just put them in as placeholders for now. I'd argue that having anything larger that you can scale down is better than capping it at 2x. https://codereview.chromium.org/1261143004/diff/30008/chrome/browser/manifest... chrome/browser/manifest/manifest_icon_downloader.cc:84: if (best_upper && best_upper->height() < soft_upper_bound) { On 2015/08/03 08:58:29, Lalit Maganti wrote: > On 2015/07/31 at 18:09:20, dfalcantara wrote: > > I don't think it ever makes sense to scale up if you have a higher quality > icon that you can scale down. > > Hmmm this is an interesting one. I was thinking along the same lines but then I > also think that if you have an icon which is just a bit smaller than required > (say 95%), it may be better to scale it up rather than scaling down a 200% icon. > I'm not really sure if this is correct thinking though - I've never really > looked into image scaling too deeply. It is actually always preferable to scale down a 200% over scaling up a 95% icon because the pixels line up. They don't when you scale up a 95% icon, resulting in extra fuzziness. But yeah, that's the reason the original code ignored anything too small. I've asked multiple folks here in the office and we're all in agreement about never scaling up. The problem is that scaling down means you have an extra amount of information you can use to determine what a downscaled image's pixel would look like -- with upscaling you have to guess about what should have been in that spot. The wikipedia article on image scaling is actually worth a read: https://en.wikipedia.org/wiki/Image_scaling We'd considered using complicated algorithms at some point to scale tiny favicons up to look like app icons, but the quality was terrible.
Apologies for not linking the followup/mentioning in the commit message. I'll remember that for the future :) https://codereview.chromium.org/1261143004/diff/30008/chrome/browser/banners/... File chrome/browser/banners/app_banner_data_fetcher.cc (right): https://codereview.chromium.org/1261143004/diff/30008/chrome/browser/banners/... chrome/browser/banners/app_banner_data_fetcher.cc:299: OnHasServiceWorker(web_contents); On 2015/08/03 23:27:58, dfalcantara wrote: > On 2015/08/03 08:58:29, Lalit Maganti wrote: > > On 2015/07/31 at 18:09:19, dfalcantara wrote: > > > Not sure why you pulled this out. The function was already short, you now > > have to put Cancel() in both places, and I've generally seen OnBlah function > > names used for bound callbacks (like OnDidCheckHasServiceWorker). > > > > For future use. > > Is the future use going to be a bound callback? No. I plan to override the method and use it to download the splashscreeen icon before calling the super class method to fetch the app icon. https://codereview.chromium.org/1261143004/diff/30008/chrome/browser/manifest... File chrome/browser/manifest/manifest_icon_downloader.cc (right): https://codereview.chromium.org/1261143004/diff/30008/chrome/browser/manifest... chrome/browser/manifest/manifest_icon_downloader.cc:78: float soft_upper_bound = 1.5 * scaled_ideal_icon_size; On 2015/08/03 23:27:58, dfalcantara wrote: > On 2015/08/03 08:58:29, Lalit Maganti wrote: > > On 2015/07/31 at 18:09:20, dfalcantara wrote: > > > How'd you come up with these bounds? 2/1 and 1/2 I can I reason about, but > > 3/2 and 9/10? > > > > They are completely arbitary - I just put them in as placeholders for now. > > I'd argue that having anything larger that you can scale down is better than > capping it at 2x. Right OK. https://codereview.chromium.org/1261143004/diff/30008/chrome/browser/manifest... chrome/browser/manifest/manifest_icon_downloader.cc:84: if (best_upper && best_upper->height() < soft_upper_bound) { On 2015/08/03 23:27:58, dfalcantara wrote: > On 2015/08/03 08:58:29, Lalit Maganti wrote: > > On 2015/07/31 at 18:09:20, dfalcantara wrote: > > > I don't think it ever makes sense to scale up if you have a higher quality > > icon that you can scale down. > > > > Hmmm this is an interesting one. I was thinking along the same lines but then > I > > also think that if you have an icon which is just a bit smaller than required > > (say 95%), it may be better to scale it up rather than scaling down a 200% > icon. > > I'm not really sure if this is correct thinking though - I've never really > > looked into image scaling too deeply. > > It is actually always preferable to scale down a 200% over scaling up a 95% icon > because the pixels line up. They don't when you scale up a 95% icon, resulting > in extra fuzziness. > > But yeah, that's the reason the original code ignored anything too small. I've > asked multiple folks here in the office and we're all in agreement about never > scaling up. The problem is that scaling down means you have an extra amount of > information you can use to determine what a downscaled image's pixel would look > like -- with upscaling you have to guess about what should have been in that > spot. > > The wikipedia article on image scaling is actually worth a read: > https://en.wikipedia.org/wiki/Image_scaling > > We'd considered using complicated algorithms at some point to scale tiny > favicons up to look like app icons, but the quality was terrible. OK that sounds good to me - I can vastly simplify this code and just pick the exact or closest one higher then and get rid of all the scale up code.
Apologies for not linking the followup/mentioning in the commit message. I'll remember that for the future :)
Hopefully this should solve most of the issues you raised Dan :) https://codereview.chromium.org/1261143004/diff/30008/chrome/browser/banners/... File chrome/browser/banners/app_banner_data_fetcher.cc (right): https://codereview.chromium.org/1261143004/diff/30008/chrome/browser/banners/... chrome/browser/banners/app_banner_data_fetcher.cc:354: const SkBitmap& icon) { On 2015/07/31 18:09:20, dfalcantara wrote: > * Why can't you call GetWebContents() here? That'll have the most up to date > pointer (e.g. if it's been destroyed, you might be holding onto a bad pointer) > > * You may as well inline this function, too. The only reason it wasn't inlined > before was because we had to ensure proper refcounting. Done. https://codereview.chromium.org/1261143004/diff/30008/chrome/browser/banners/... File chrome/browser/banners/app_banner_data_fetcher.h (right): https://codereview.chromium.org/1261143004/diff/30008/chrome/browser/banners/... chrome/browser/banners/app_banner_data_fetcher.h:123: void OnHasServiceWorker(content::WebContents* web_contents); On 2015/07/31 18:09:20, dfalcantara wrote: > This should be private. Remove the comment; it makes it sound like a callback > when it's not. Done.
Would probably have given you an l-g-t-m if you had a real commit message by this point, but you're pretty close. https://codereview.chromium.org/1261143004/diff/50001/chrome/browser/manifest... File chrome/browser/manifest/manifest_icon_downloader.cc (right): https://codereview.chromium.org/1261143004/diff/50001/chrome/browser/manifest... chrome/browser/manifest/manifest_icon_downloader.cc:59: // failing that, an icon which is as close to our ideal size which we can Other reviewers are very pretty nit-picky about comments having "we" in them. The comment also seems wrong -- you do an early return instead of scaling when you find an icon that's the ideal size. How about: This algorithm searches for an icon equal to the ideal size. Failing that, it picks the smallest icon larger than the ideal size to downscale. Icons are assumed to be square-shaped -- only the height is checked.
mlamouri@chromium.org changed reviewers: + mlamouri@chromium.org
I have a few comments but mostly nits. The only real blockers is the lack of tests for the algorithm you've added. https://codereview.chromium.org/1261143004/diff/50001/chrome/browser/android/... File chrome/browser/android/banners/app_banner_data_fetcher_android.cc (right): https://codereview.chromium.org/1261143004/diff/50001/chrome/browser/android/... chrome/browser/android/banners/app_banner_data_fetcher_android.cc:40: DCHECK(web_contents); nit: I would move the DCHECK() inside FetchAppIcon(). IOW, I would do: return FetchAppIcon(GetWebContents(), image_url); https://codereview.chromium.org/1261143004/diff/50001/chrome/browser/android/... File chrome/browser/android/banners/app_banner_manager_android.cc (right): https://codereview.chromium.org/1261143004/diff/50001/chrome/browser/android/... chrome/browser/android/banners/app_banner_manager_android.cc:161: return true /*base::FieldTrialList::FindFullName("AppBanners") == "Enabled"*/; I assume you should remove that change, right? https://codereview.chromium.org/1261143004/diff/50001/chrome/browser/banners/... File chrome/browser/banners/app_banner_data_fetcher.cc (right): https://codereview.chromium.org/1261143004/diff/50001/chrome/browser/banners/... chrome/browser/banners/app_banner_data_fetcher.cc:324: return icon_downloader_->Download( nit: add DCHECK(web_contents); https://codereview.chromium.org/1261143004/diff/50001/chrome/browser/manifest... File chrome/browser/manifest/manifest_icon_downloader.cc (right): https://codereview.chromium.org/1261143004/diff/50001/chrome/browser/manifest... chrome/browser/manifest/manifest_icon_downloader.cc:22: content::WebContents* contents = web_contents(); nit: no need for that, web_contents() is an accessor from the object. https://codereview.chromium.org/1261143004/diff/50001/chrome/browser/manifest... chrome/browser/manifest/manifest_icon_downloader.cc:37: void ManifestIconDownloader::OnIconFetched( It would be great to have unit tests for this method. https://codereview.chromium.org/1261143004/diff/50001/chrome/browser/manifest... chrome/browser/manifest/manifest_icon_downloader.cc:45: content::WebContents* contents = web_contents(); ditto https://codereview.chromium.org/1261143004/diff/50001/chrome/browser/manifest... chrome/browser/manifest/manifest_icon_downloader.cc:69: if (height == ideal_icon_size_in_px) { This is assuming height === width, right? Shouldn't you first ignore bitmaps with height != width? https://codereview.chromium.org/1261143004/diff/50001/chrome/browser/manifest... chrome/browser/manifest/manifest_icon_downloader.cc:80: if (best_upper) { That's me or you don't actually close that { ? Though, I would handle the error case first: if (!best_upper) { callback.Run(SkBitMap()); return; } [...] https://codereview.chromium.org/1261143004/diff/50001/chrome/browser/manifest... File chrome/browser/manifest/manifest_icon_downloader.h (right): https://codereview.chromium.org/1261143004/diff/50001/chrome/browser/manifest... chrome/browser/manifest/manifest_icon_downloader.h:12: #include "content/public/common/manifest.h" nit: do you need that include? https://codereview.chromium.org/1261143004/diff/50001/chrome/browser/manifest... chrome/browser/manifest/manifest_icon_downloader.h:14: #include "url/gurl.h" I think you can forward declare GURL. https://codereview.chromium.org/1261143004/diff/50001/chrome/browser/manifest... chrome/browser/manifest/manifest_icon_downloader.h:23: typedef base::Callback<void(const SkBitmap&)> Callback; nit: Callback is too generic of a name, maybe IconFetchCallback or DownloadCallback? also, let's be modern and do: using YourCallbackName = base::Callback<void(const SkBitmap&)>; https://codereview.chromium.org/1261143004/diff/50001/chrome/browser/manifest... chrome/browser/manifest/manifest_icon_downloader.h:26: virtual ~ManifestIconDownloader() {} s/{}/= default;/ https://codereview.chromium.org/1261143004/diff/50001/chrome/browser/manifest... chrome/browser/manifest/manifest_icon_downloader.h:29: // at the URL then it attempts to pick the one closest in size bigger than or nit: I would say "If the file contains multiple icons" instead of "If more than one icon is present at the URL" https://codereview.chromium.org/1261143004/diff/50001/chrome/browser/manifest... chrome/browser/manifest/manifest_icon_downloader.h:32: // to ideal_icon_size_in_dp. nit: could you document what the return value means?
Have addressed both your comments as well as written tests and made shortcuthelper use the new class. https://codereview.chromium.org/1261143004/diff/50001/chrome/browser/android/... File chrome/browser/android/banners/app_banner_data_fetcher_android.cc (right): https://codereview.chromium.org/1261143004/diff/50001/chrome/browser/android/... chrome/browser/android/banners/app_banner_data_fetcher_android.cc:40: DCHECK(web_contents); On 2015/08/05 at 08:20:16, Mounir Lamouri wrote: > nit: I would move the DCHECK() inside FetchAppIcon(). > > IOW, I would do: > return FetchAppIcon(GetWebContents(), image_url); Got rid of web contents all together as I don't even need it. https://codereview.chromium.org/1261143004/diff/50001/chrome/browser/android/... File chrome/browser/android/banners/app_banner_manager_android.cc (right): https://codereview.chromium.org/1261143004/diff/50001/chrome/browser/android/... chrome/browser/android/banners/app_banner_manager_android.cc:161: return true /*base::FieldTrialList::FindFullName("AppBanners") == "Enabled"*/; On 2015/08/05 at 08:20:16, Mounir Lamouri wrote: > I assume you should remove that change, right? Done. https://codereview.chromium.org/1261143004/diff/50001/chrome/browser/banners/... File chrome/browser/banners/app_banner_data_fetcher.cc (right): https://codereview.chromium.org/1261143004/diff/50001/chrome/browser/banners/... chrome/browser/banners/app_banner_data_fetcher.cc:324: return icon_downloader_->Download( On 2015/08/05 at 08:20:16, Mounir Lamouri wrote: > nit: add DCHECK(web_contents); Removed web contents altogether. https://codereview.chromium.org/1261143004/diff/50001/chrome/browser/manifest... File chrome/browser/manifest/manifest_icon_downloader.cc (right): https://codereview.chromium.org/1261143004/diff/50001/chrome/browser/manifest... chrome/browser/manifest/manifest_icon_downloader.cc:22: content::WebContents* contents = web_contents(); On 2015/08/05 at 08:20:16, Mounir Lamouri wrote: > nit: no need for that, web_contents() is an accessor from the object. Done. https://codereview.chromium.org/1261143004/diff/50001/chrome/browser/manifest... chrome/browser/manifest/manifest_icon_downloader.cc:37: void ManifestIconDownloader::OnIconFetched( On 2015/08/05 at 08:20:16, Mounir Lamouri wrote: > It would be great to have unit tests for this method. Added. https://codereview.chromium.org/1261143004/diff/50001/chrome/browser/manifest... chrome/browser/manifest/manifest_icon_downloader.cc:45: content::WebContents* contents = web_contents(); On 2015/08/05 at 08:20:16, Mounir Lamouri wrote: > ditto Done. https://codereview.chromium.org/1261143004/diff/50001/chrome/browser/manifest... chrome/browser/manifest/manifest_icon_downloader.cc:59: // failing that, an icon which is as close to our ideal size which we can On 2015/08/04 at 21:41:45, dfalcantara wrote: > Other reviewers are very pretty nit-picky about comments having "we" in them. The comment also seems wrong -- you do an early return instead of scaling when you find an icon that's the ideal size. > > How about: > This algorithm searches for an icon equal to the ideal size. Failing that, it picks the smallest icon larger than the ideal size to downscale. Icons are assumed to be square-shaped -- only the height is checked. Done. https://codereview.chromium.org/1261143004/diff/50001/chrome/browser/manifest... chrome/browser/manifest/manifest_icon_downloader.cc:69: if (height == ideal_icon_size_in_px) { On 2015/08/05 at 08:20:16, Mounir Lamouri wrote: > This is assuming height === width, right? Shouldn't you first ignore bitmaps with height != width? Done. https://codereview.chromium.org/1261143004/diff/50001/chrome/browser/manifest... chrome/browser/manifest/manifest_icon_downloader.cc:80: if (best_upper) { On 2015/08/05 at 08:20:16, Mounir Lamouri wrote: > That's me or you don't actually close that { ? > > Though, I would handle the error case first: > > if (!best_upper) { > callback.Run(SkBitMap()); > return; > } > > [...] Done. https://codereview.chromium.org/1261143004/diff/50001/chrome/browser/manifest... File chrome/browser/manifest/manifest_icon_downloader.h (right): https://codereview.chromium.org/1261143004/diff/50001/chrome/browser/manifest... chrome/browser/manifest/manifest_icon_downloader.h:12: #include "content/public/common/manifest.h" On 2015/08/05 at 08:20:17, Mounir Lamouri wrote: > nit: do you need that include? Removed. https://codereview.chromium.org/1261143004/diff/50001/chrome/browser/manifest... chrome/browser/manifest/manifest_icon_downloader.h:14: #include "url/gurl.h" On 2015/08/05 at 08:20:17, Mounir Lamouri wrote: > I think you can forward declare GURL. Done. https://codereview.chromium.org/1261143004/diff/50001/chrome/browser/manifest... chrome/browser/manifest/manifest_icon_downloader.h:23: typedef base::Callback<void(const SkBitmap&)> Callback; On 2015/08/05 at 08:20:17, Mounir Lamouri wrote: > nit: Callback is too generic of a name, maybe IconFetchCallback or DownloadCallback? > > also, let's be modern and do: > using YourCallbackName = base::Callback<void(const SkBitmap&)>; Done. https://codereview.chromium.org/1261143004/diff/50001/chrome/browser/manifest... chrome/browser/manifest/manifest_icon_downloader.h:26: virtual ~ManifestIconDownloader() {} On 2015/08/05 at 08:20:17, Mounir Lamouri wrote: > s/{}/= default;/ Done. https://codereview.chromium.org/1261143004/diff/50001/chrome/browser/manifest... chrome/browser/manifest/manifest_icon_downloader.h:29: // at the URL then it attempts to pick the one closest in size bigger than or On 2015/08/05 at 08:20:17, Mounir Lamouri wrote: > nit: I would say "If the file contains multiple icons" instead of "If more than one icon is present at the URL" Done. https://codereview.chromium.org/1261143004/diff/50001/chrome/browser/manifest... chrome/browser/manifest/manifest_icon_downloader.h:32: // to ideal_icon_size_in_dp. On 2015/08/05 at 08:20:17, Mounir Lamouri wrote: > nit: could you document what the return value means? Done.
lgtm @ mlamouri's comments. Good catch on removing the debugging flag enabling, Mounir. One of the hazards of checking only the differences between patch sets. https://codereview.chromium.org/1261143004/diff/110001/chrome/browser/banners... File chrome/browser/banners/app_banner_data_fetcher.cc (right): https://codereview.chromium.org/1261143004/diff/110001/chrome/browser/banners... chrome/browser/banners/app_banner_data_fetcher.cc:342: } nit: put this newline back. the RecordCouldShowBanner() call gets lost when it's sandwiched between two larger conditional blocks. https://codereview.chromium.org/1261143004/diff/110001/chrome/browser/banners... File chrome/browser/banners/app_banner_data_fetcher.h (right): https://codereview.chromium.org/1261143004/diff/110001/chrome/browser/banners... chrome/browser/banners/app_banner_data_fetcher.h:122: void OnHasServiceWorker(content::WebContents* web_contents); nit: Add a function comment here.
Er, that was %, not @. I'm also not sure why you're prefixing the CL with chrome: description. It's obviously chrome code.
Dan: on why I prefix my commits with the folder: 1. Old habit of mine from my personal projects. 2. It helps me tell apart muti-CL changes especially as I often put the same commit message for Blink and Chromium. If it's an issue I can stop doing it.
On 2015/08/06 13:14:48, Lalit Maganti wrote:
> Dan: on why I prefix my commits with the folder:
> 1. Old habit of mine from my personal projects.
> 2. It helps me tell apart muti-CL changes especially as I often put the same
> commit message for Blink and Chromium.
> If it's an issue I can stop doing it.
Don't know what convention is, but when I've had multi-repo CLs like that, I've
used more or less the same commit message, then linked to the other CL from each
CL's commit message ("This is the Blink half; Chromium CL here: <link>") to make
it obvious what's supposed to go with what. In this case, is there a Blink
half?
A general rule is to keep in mind that the commit log is practically never for
you: it's for the people who will be looking up the CL 3 years down the line and
trying to understand what you were thinking when you wrote the CL.
On 2015/08/06 at 16:42:04, dfalcantara wrote:
> On 2015/08/06 13:14:48, Lalit Maganti wrote:
> > Dan: on why I prefix my commits with the folder:
> > 1. Old habit of mine from my personal projects.
> > 2. It helps me tell apart muti-CL changes especially as I often put the same
> > commit message for Blink and Chromium.
> > If it's an issue I can stop doing it.
>
> Don't know what convention is, but when I've had multi-repo CLs like that,
I've used more or less the same commit message, then linked to the other CL from
each CL's commit message ("This is the Blink half; Chromium CL here: <link>") to
make it obvious what's supposed to go with what. In this case, is there a Blink
half?
>
> A general rule is to keep in mind that the commit log is practically never for
you: it's for the people who will be looking up the CL 3 years down the line and
trying to understand what you were thinking when you wrote the CL.
No there isn't a Blink half. I've removed the prefix for this commit too.
Also I've fixed your nits on the previous patch.
https://codereview.chromium.org/1261143004/diff/130001/chrome/browser/android... File chrome/browser/android/shortcut_data_fetcher.cc (right): https://codereview.chromium.org/1261143004/diff/130001/chrome/browser/android... chrome/browser/android/shortcut_data_fetcher.cc:114: bool success = icon_downloader_->Download( nit: no need for the |success| temporary variable. I would also change the "// Grab the best favicon [...]" to "// If fetching the Manifest icon fails, fallback to the best favicon [...]". https://codereview.chromium.org/1261143004/diff/130001/chrome/browser/banners... File chrome/browser/banners/app_banner_data_fetcher.cc (right): https://codereview.chromium.org/1261143004/diff/130001/chrome/browser/banners... chrome/browser/banners/app_banner_data_fetcher.cc:305: Cancel(); optional nit: I would prefer this the other way around: early return for the error case, ie: if (!has_service_worker) { TrackDisplayEvent(DISPLAY_EVENT_LACKS_SERVICE_WORKER); OutputDeveloperNotShownMessage(web_contents, kNoMatchingServiceWorker); Cancel(); return; } OnHasServiceWorker(web_contents); https://codereview.chromium.org/1261143004/diff/130001/chrome/browser/banners... chrome/browser/banners/app_banner_data_fetcher.cc:316: if (FetchAppIcon(icon_url)) return; optional nit: again, it's odd to early return for a success case, I would do: if (!FetchAppIcon(icon_url)) { OutputDeveloperNotShownMessage(web_contents, kCannotDetermineBestIcon); Cancel(); } https://codereview.chromium.org/1261143004/diff/130001/chrome/browser/manifes... File chrome/browser/manifest/manifest_icon_downloader.cc (right): https://codereview.chromium.org/1261143004/diff/130001/chrome/browser/manifes... chrome/browser/manifest/manifest_icon_downloader.cc:34: false, Could you add comments after the three arguments above to say what they mean? |icon_url| is clear but I have no idea what |false| or 0 mean here. https://codereview.chromium.org/1261143004/diff/130001/chrome/browser/manifes... chrome/browser/manifest/manifest_icon_downloader.cc:57: const SkBitmap& scaled = skia::ImageOperations::Resize( We need to make sure this happens in the IO thread. Could you add: DCHECK_CURRENTLY_ON(content::BrowserThread::IO); at the beginning of that method? Also, shouldn't we only call that method if there is a difference between ideal and what we got? https://codereview.chromium.org/1261143004/diff/130001/chrome/browser/manifes... chrome/browser/manifest/manifest_icon_downloader.cc:77: for (size_t i = 0; i < bitmaps.size(); ++i) { Let's use some C++11 syntax sugar: for (const auto& bitmap : bitmaps) { [...] } https://codereview.chromium.org/1261143004/diff/130001/chrome/browser/manifes... chrome/browser/manifest/manifest_icon_downloader.cc:79: int height = current_bitmap.height(); no need to have a temporary variable here. https://codereview.chromium.org/1261143004/diff/130001/chrome/browser/manifes... File chrome/browser/manifest/manifest_icon_downloader.h (right): https://codereview.chromium.org/1261143004/diff/130001/chrome/browser/manifes... chrome/browser/manifest/manifest_icon_downloader.h:38: // started. If this is false then the callback will NOT be called. Could you change the last two lines to: Returns whether the download has started. It will return false if the current context or information do not allow to download the image. https://codereview.chromium.org/1261143004/diff/130001/chrome/browser/manifes... chrome/browser/manifest/manifest_icon_downloader.h:44: // Callback run after an attempt to download the manifest icon has been made. nit: this comment is confusing because it might be interpreted as "callback is run after the download starts" which isn't actually correct, right? https://codereview.chromium.org/1261143004/diff/130001/chrome/browser/manifes... File chrome/browser/manifest/manifest_icon_downloader_unittest.cc (right): https://codereview.chromium.org/1261143004/diff/130001/chrome/browser/manifes... chrome/browser/manifest/manifest_icon_downloader_unittest.cc:14: ManifestIconDownloaderTest() {} nit: = default; https://codereview.chromium.org/1261143004/diff/130001/chrome/browser/manifes... chrome/browser/manifest/manifest_icon_downloader_unittest.cc:23: SkBitmap CreateBitmap(int width, int height) { nit: CreateDummyBitmap() https://codereview.chromium.org/1261143004/diff/130001/chrome/browser/manifes... chrome/browser/manifest/manifest_icon_downloader_unittest.cc:70: } Could you test: - bitmap is 10x10, 8x8, 6x6 and ideal is 9 - bitmap is 10x10, 8x8, 6x6 and ideal is 7 - bitmap isn't square
https://codereview.chromium.org/1261143004/diff/130001/chrome/browser/android... File chrome/browser/android/shortcut_data_fetcher.cc (right): https://codereview.chromium.org/1261143004/diff/130001/chrome/browser/android... chrome/browser/android/shortcut_data_fetcher.cc:114: bool success = icon_downloader_->Download( On 2015/08/07 at 10:11:11, Mounir Lamouri wrote: > nit: no need for the |success| temporary variable. > > I would also change the "// Grab the best favicon [...]" to "// If fetching the Manifest icon fails, fallback to the best favicon [...]". Done. https://codereview.chromium.org/1261143004/diff/130001/chrome/browser/banners... File chrome/browser/banners/app_banner_data_fetcher.cc (right): https://codereview.chromium.org/1261143004/diff/130001/chrome/browser/banners... chrome/browser/banners/app_banner_data_fetcher.cc:305: Cancel(); On 2015/08/07 at 10:11:11, Mounir Lamouri wrote: > optional nit: I would prefer this the other way around: early return for the error case, ie: > if (!has_service_worker) { > TrackDisplayEvent(DISPLAY_EVENT_LACKS_SERVICE_WORKER); > OutputDeveloperNotShownMessage(web_contents, kNoMatchingServiceWorker); > Cancel(); > return; > } > > OnHasServiceWorker(web_contents); Done. https://codereview.chromium.org/1261143004/diff/130001/chrome/browser/banners... chrome/browser/banners/app_banner_data_fetcher.cc:316: if (FetchAppIcon(icon_url)) return; On 2015/08/07 at 10:11:11, Mounir Lamouri wrote: > optional nit: again, it's odd to early return for a success case, I would do: > if (!FetchAppIcon(icon_url)) { > OutputDeveloperNotShownMessage(web_contents, kCannotDetermineBestIcon); > Cancel(); > } Done. https://codereview.chromium.org/1261143004/diff/130001/chrome/browser/manifes... File chrome/browser/manifest/manifest_icon_downloader.cc (right): https://codereview.chromium.org/1261143004/diff/130001/chrome/browser/manifes... chrome/browser/manifest/manifest_icon_downloader.cc:34: false, On 2015/08/07 at 10:11:11, Mounir Lamouri wrote: > Could you add comments after the three arguments above to say what they mean? |icon_url| is clear but I have no idea what |false| or 0 mean here. Done. https://codereview.chromium.org/1261143004/diff/130001/chrome/browser/manifes... chrome/browser/manifest/manifest_icon_downloader.cc:57: const SkBitmap& scaled = skia::ImageOperations::Resize( On 2015/08/07 at 10:11:11, Mounir Lamouri wrote: > We need to make sure this happens in the IO thread. Could you add: > DCHECK_CURRENTLY_ON(content::BrowserThread::IO); > at the beginning of that method? > > Also, shouldn't we only call that method if there is a difference between ideal and what we got? Refactored to make resizing happen in background. https://codereview.chromium.org/1261143004/diff/130001/chrome/browser/manifes... chrome/browser/manifest/manifest_icon_downloader.cc:77: for (size_t i = 0; i < bitmaps.size(); ++i) { On 2015/08/07 at 10:11:11, Mounir Lamouri wrote: > Let's use some C++11 syntax sugar: > for (const auto& bitmap : bitmaps) { > [...] > } I would if I could but I need the index to return :( https://codereview.chromium.org/1261143004/diff/130001/chrome/browser/manifes... chrome/browser/manifest/manifest_icon_downloader.cc:79: int height = current_bitmap.height(); On 2015/08/07 at 10:11:11, Mounir Lamouri wrote: > no need to have a temporary variable here. Done. https://codereview.chromium.org/1261143004/diff/130001/chrome/browser/manifes... File chrome/browser/manifest/manifest_icon_downloader.h (right): https://codereview.chromium.org/1261143004/diff/130001/chrome/browser/manifes... chrome/browser/manifest/manifest_icon_downloader.h:38: // started. If this is false then the callback will NOT be called. On 2015/08/07 at 10:11:12, Mounir Lamouri wrote: > Could you change the last two lines to: > Returns whether the download has started. It will return false if the current context or information do not allow to download the image. Done. https://codereview.chromium.org/1261143004/diff/130001/chrome/browser/manifes... chrome/browser/manifest/manifest_icon_downloader.h:44: // Callback run after an attempt to download the manifest icon has been made. On 2015/08/07 at 10:11:11, Mounir Lamouri wrote: > nit: this comment is confusing because it might be interpreted as "callback is run after the download starts" which isn't actually correct, right? Fixed. https://codereview.chromium.org/1261143004/diff/130001/chrome/browser/manifes... File chrome/browser/manifest/manifest_icon_downloader_unittest.cc (right): https://codereview.chromium.org/1261143004/diff/130001/chrome/browser/manifes... chrome/browser/manifest/manifest_icon_downloader_unittest.cc:14: ManifestIconDownloaderTest() {} On 2015/08/07 at 10:11:12, Mounir Lamouri wrote: > nit: = default; Done. https://codereview.chromium.org/1261143004/diff/130001/chrome/browser/manifes... chrome/browser/manifest/manifest_icon_downloader_unittest.cc:23: SkBitmap CreateBitmap(int width, int height) { On 2015/08/07 at 10:11:12, Mounir Lamouri wrote: > nit: CreateDummyBitmap() Done. https://codereview.chromium.org/1261143004/diff/130001/chrome/browser/manifes... chrome/browser/manifest/manifest_icon_downloader_unittest.cc:70: } On 2015/08/07 at 10:11:12, Mounir Lamouri wrote: > Could you test: > - bitmap is 10x10, 8x8, 6x6 and ideal is 9 > - bitmap is 10x10, 8x8, 6x6 and ideal is 7 > - bitmap isn't square Done.
lgtm with comments applied. https://codereview.chromium.org/1261143004/diff/150001/chrome/browser/android... File chrome/browser/android/shortcut_data_fetcher.cc (right): https://codereview.chromium.org/1261143004/diff/150001/chrome/browser/android... chrome/browser/android/shortcut_data_fetcher.cc:184: if (!web_contents() || !weak_observer_ || is_icon_saved_) return; nit: in C++, |return;| should be in a new line, contrary to Java coding style. https://codereview.chromium.org/1261143004/diff/150001/chrome/browser/banners... File chrome/browser/banners/app_banner_data_fetcher.h (right): https://codereview.chromium.org/1261143004/diff/150001/chrome/browser/banners... chrome/browser/banners/app_banner_data_fetcher.h:123: // criterion of having a manifest and a service worker. First time I read "criterion" :) https://codereview.chromium.org/1261143004/diff/150001/chrome/browser/manifes... File chrome/browser/manifest/manifest_icon_downloader.cc (right): https://codereview.chromium.org/1261143004/diff/150001/chrome/browser/manifes... chrome/browser/manifest/manifest_icon_downloader.cc:50: const int closest_index = FindClosestBitmapIndex( nit: add |DCHECK_CURRENTLY_ON(content::BrowserThread::UI);| given that we are actually making that assumption. https://codereview.chromium.org/1261143004/diff/150001/chrome/browser/manifes... chrome/browser/manifest/manifest_icon_downloader.cc:90: void ManifestIconDownloader::RunCallbackOnUIThread( You don't actually use that, maybe you could remove it? https://codereview.chromium.org/1261143004/diff/150001/chrome/browser/manifes... File chrome/browser/manifest/manifest_icon_downloader.h (right): https://codereview.chromium.org/1261143004/diff/150001/chrome/browser/manifes... chrome/browser/manifest/manifest_icon_downloader.h:20: } // namespace gfx nit: two spaces between "}" and "//" https://codereview.chromium.org/1261143004/diff/150001/chrome/browser/manifes... chrome/browser/manifest/manifest_icon_downloader.h:25: : public content::WebContentsObserver { Sorry to point that so late but it seems that using WebContentsObserver isn't really needed except to have a WebContents for ::Download. Given that the callers have a WebContents, I think it's better to simply pass it when calling ::Download(). https://codereview.chromium.org/1261143004/diff/150001/chrome/browser/manifes... chrome/browser/manifest/manifest_icon_downloader.h:53: static void ScaleIcon(const int ideal_icon_size_in_px, nit: to be consistent, leave an empty line before this method declaration. https://codereview.chromium.org/1261143004/diff/150001/chrome/browser/manifes... chrome/browser/manifest/manifest_icon_downloader.h:56: static void RunCallbackOnUIThread( ditto
https://codereview.chromium.org/1261143004/diff/150001/chrome/browser/android... File chrome/browser/android/shortcut_data_fetcher.cc (right): https://codereview.chromium.org/1261143004/diff/150001/chrome/browser/android... chrome/browser/android/shortcut_data_fetcher.cc:184: if (!web_contents() || !weak_observer_ || is_icon_saved_) return; On 2015/08/10 at 15:08:08, Mounir Lamouri wrote: > nit: in C++, |return;| should be in a new line, contrary to Java coding style. Done - although there are a lot of cases in this file where the return is on the same line. https://codereview.chromium.org/1261143004/diff/150001/chrome/browser/banners... File chrome/browser/banners/app_banner_data_fetcher.h (right): https://codereview.chromium.org/1261143004/diff/150001/chrome/browser/banners... chrome/browser/banners/app_banner_data_fetcher.h:123: // criterion of having a manifest and a service worker. On 2015/08/10 at 15:08:08, Mounir Lamouri wrote: > First time I read "criterion" :) In this case "criteria" is actually correct because it's a plural - initially I only had the service worker part there https://codereview.chromium.org/1261143004/diff/150001/chrome/browser/manifes... File chrome/browser/manifest/manifest_icon_downloader.cc (right): https://codereview.chromium.org/1261143004/diff/150001/chrome/browser/manifes... chrome/browser/manifest/manifest_icon_downloader.cc:50: const int closest_index = FindClosestBitmapIndex( On 2015/08/10 at 15:08:08, Mounir Lamouri wrote: > nit: add |DCHECK_CURRENTLY_ON(content::BrowserThread::UI);| given that we are actually making that assumption. Done. https://codereview.chromium.org/1261143004/diff/150001/chrome/browser/manifes... chrome/browser/manifest/manifest_icon_downloader.cc:90: void ManifestIconDownloader::RunCallbackOnUIThread( On 2015/08/10 at 15:08:08, Mounir Lamouri wrote: > You don't actually use that, maybe you could remove it? Done. https://codereview.chromium.org/1261143004/diff/150001/chrome/browser/manifes... File chrome/browser/manifest/manifest_icon_downloader.h (right): https://codereview.chromium.org/1261143004/diff/150001/chrome/browser/manifes... chrome/browser/manifest/manifest_icon_downloader.h:20: } // namespace gfx On 2015/08/10 at 15:08:08, Mounir Lamouri wrote: > nit: two spaces between "}" and "//" Done. https://codereview.chromium.org/1261143004/diff/150001/chrome/browser/manifes... chrome/browser/manifest/manifest_icon_downloader.h:25: : public content::WebContentsObserver { On 2015/08/10 at 15:08:08, Mounir Lamouri wrote: > Sorry to point that so late but it seems that using WebContentsObserver isn't really needed except to have a WebContents for ::Download. Given that the callers have a WebContents, I think it's better to simply pass it when calling ::Download(). Done. https://codereview.chromium.org/1261143004/diff/150001/chrome/browser/manifes... chrome/browser/manifest/manifest_icon_downloader.h:53: static void ScaleIcon(const int ideal_icon_size_in_px, On 2015/08/10 at 15:08:08, Mounir Lamouri wrote: > nit: to be consistent, leave an empty line before this method declaration. Done. https://codereview.chromium.org/1261143004/diff/150001/chrome/browser/manifes... chrome/browser/manifest/manifest_icon_downloader.h:56: static void RunCallbackOnUIThread( On 2015/08/10 at 15:08:08, Mounir Lamouri wrote: > ditto Removed altogether.
lalitm@google.com changed reviewers: + newt@chromium.org
newt@: could you please take a look at the shortcut_data_fetcher changes?
rubberstamp lgtm for shortcut_data_fetcher.cc/h based on dfalcantara's review.
The CQ bit was checked by lalitm@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from dfalcantara@chromium.org, mlamouri@chromium.org, newt@chromium.org Link to the patchset: https://codereview.chromium.org/1261143004/#ps190001 (title: "Fix compile")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1261143004/190001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1261143004/190001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
So the test failures are due to the icons being used in tests drawing no data onto the screen - they're empty/transparent drawables basically. I'm not actually sure how to handle this case :/
On 2015/08/11 13:23:42, Lalit Maganti wrote: > So the test failures are due to the icons being used in tests drawing no data > onto the screen - they're empty/transparent drawables basically. I'm not > actually sure how to handle this case :/ Never mind. Ignore my comment - I should have looked at the icons before I sent that.
Dan and Mounir: To fix the test failures, on Mounir's suggestion, I've removed the density parameter from the manifest as it's irrelevant to the tests themselves. The density parameter was causing a too small icon to be chosen. If this is OK with you both then I will commit.
The CQ bit was checked by lalitm@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/1261143004/230001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1261143004/230001
still lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
I'm fine with removing the density, but it seems that wasn't the cause of your failures. Could have sworn there were some tests that used density at some point, but maybe that's wishful thinking.
On 2015/08/11 17:46:58, dfalcantara wrote: > I'm fine with removing the density, but it seems that wasn't the cause of your > failures. Could have sworn there were some tests that used density at some > point, but maybe that's wishful thinking. The density change fixed many of the tests but a few more are still broken.
On 2015/08/11 18:24:15, Lalit Maganti wrote: > On 2015/08/11 17:46:58, dfalcantara wrote: > > I'm fine with removing the density, but it seems that wasn't the cause of your > > failures. Could have sworn there were some tests that used density at some > > point, but maybe that's wishful thinking. > > The density change fixed many of the tests but a few more are still broken. Mounir and Dan: OK hopefully that should be tests fixed. The reason was again due to the test cases requiring too big an icon on Android which have scaling (i.e. on desktop scale factor is 1 so we had big enough icons as dp = px but on Android, the Nexus 6 has a factor of 3.5 which requires an icon of 168 px). I've added a 4x icon as well as making the native app tests use it and adding it to the manifest. On a related point, Mounir and I discussed maybe bringing back scaling up of smaller icons if a big enough icon is not present. Maybe have a floor of 75% or so just so websites aren't stuck with a terrible looking generated icon.
On 2015/08/12 at 13:37:05, lalitm wrote: > On 2015/08/11 18:24:15, Lalit Maganti wrote: > > On 2015/08/11 17:46:58, dfalcantara wrote: > > > I'm fine with removing the density, but it seems that wasn't the cause of your > > > failures. Could have sworn there were some tests that used density at some > > > point, but maybe that's wishful thinking. > > > > The density change fixed many of the tests but a few more are still broken. > > Mounir and Dan: OK hopefully that should be tests fixed. The reason was again due to the test cases requiring too big an icon on Android which have scaling (i.e. on desktop scale factor is 1 so we had big enough icons as dp = px but on Android, the Nexus 6 has a factor of 3.5 which requires an icon of 168 px). I've added a 4x icon as well as making the native app tests use it and adding it to the manifest. > > On a related point, Mounir and I discussed maybe bringing back scaling up of smaller icons if a big enough icon is not present. Maybe have a floor of 75% or so just so websites aren't stuck with a terrible looking generated icon. The reason why all these tests are failing is because we are currently picking a smaller icon if we have no other choice, allowing a degraded but still okay experience. The new algorithm rejects icons that are not big enough. I would be in favour of keeping the current behaviour because it would be sad to have a default icon because the icons in the Manifest where a bit too small. We could obviously have a treshold after which we wouldn't accept the icon but I would be willing to do that in another CL given that it might lead to regressions.
On 2015/08/12 13:43:44, Mounir Lamouri wrote: > On 2015/08/12 at 13:37:05, lalitm wrote: > > On 2015/08/11 18:24:15, Lalit Maganti wrote: > > > On 2015/08/11 17:46:58, dfalcantara wrote: > > > > I'm fine with removing the density, but it seems that wasn't the cause of > your > > > > failures. Could have sworn there were some tests that used density at > some > > > > point, but maybe that's wishful thinking. > > > > > > The density change fixed many of the tests but a few more are still broken. > > > > Mounir and Dan: OK hopefully that should be tests fixed. The reason was again > due to the test cases requiring too big an icon on Android which have scaling > (i.e. on desktop scale factor is 1 so we had big enough icons as dp = px but on > Android, the Nexus 6 has a factor of 3.5 which requires an icon of 168 px). I've > added a 4x icon as well as making the native app tests use it and adding it to > the manifest. > > > > On a related point, Mounir and I discussed maybe bringing back scaling up of > smaller icons if a big enough icon is not present. Maybe have a floor of 75% or > so just so websites aren't stuck with a terrible looking generated icon. > > The reason why all these tests are failing is because we are currently picking a > smaller icon if we have no other choice, allowing a degraded but still okay > experience. The new algorithm rejects icons that are not big enough. I would be > in favour of keeping the current behaviour because it would be sad to have a > default icon because the icons in the Manifest where a bit too small. We could > obviously have a treshold after which we wouldn't accept the icon but I would be > willing to do that in another CL given that it might lead to regressions. I believe the requirement for a minimum icon size is a UX-decreed thing because these icons would look terribly fuzzy on Android's Recents and the home screen when next to properly defined icons. Did we intentionally decide to fudge it with the previous algorithm? I'm asking rolfe@ for input; will report back.
Er, I guess discussion is continuing about upscaling is happening on the email thread for Splash Screen mocks.
On 2015/08/12 17:57:52, dfalcantara wrote: > Er, I guess discussion is continuing about upscaling is happening on the email > thread for Splash Screen mocks. Good at forming coherent sentences I am this morning.
My understanding of the discussion we had is that we might want to pick a smaller icon for the homescreen icon because it might be better than the default lettered icon, right?
On 2015/08/14 09:40:22, Mounir Lamouri wrote: > My understanding of the discussion we had is that we might want to pick a > smaller icon for the homescreen icon because it might be better than the default > lettered icon, right? Yes this is correct but there is a cut off below which we do not pick icons. This is similar to Android which picks one density bucket below the actual density but (from what I understood from Dan's link) does not pick icons 2 or more density buckets below.
Mounir and Dan: this should hopefully be the algorithm which we agreed over email implemented.
https://codereview.chromium.org/1261143004/diff/350001/chrome/browser/manifes... File chrome/browser/manifest/manifest_icon_downloader.cc (right): https://codereview.chromium.org/1261143004/diff/350001/chrome/browser/manifes... chrome/browser/manifest/manifest_icon_downloader.cc:29: const float lowest_scale_factor = device_scale_factor - 1; If the device_scale_factor is something like 3.5, the lowest would then be 2.5. Should we use floor() and allow 2? https://codereview.chromium.org/1261143004/diff/350001/chrome/browser/manifes... chrome/browser/manifest/manifest_icon_downloader.cc:121: if (current_bitmap.height() > ideal_icon_size_in_px && what about something like: if (bigger than ideal size || bigger or equal than smaller size) { if (better than current one) { select it; } } https://codereview.chromium.org/1261143004/diff/350001/chrome/browser/manifes... chrome/browser/manifest/manifest_icon_downloader.cc:133: return best_upper_index == -1 ? best_lower_index : best_upper_index; Why do you need two indexes?
https://codereview.chromium.org/1261143004/diff/350001/chrome/browser/manifes... File chrome/browser/manifest/manifest_icon_downloader.cc (right): https://codereview.chromium.org/1261143004/diff/350001/chrome/browser/manifes... chrome/browser/manifest/manifest_icon_downloader.cc:29: const float lowest_scale_factor = device_scale_factor - 1; On 2015/08/18 14:48:17, Mounir Lamouri wrote: > If the device_scale_factor is something like 3.5, the lowest would then be 2.5. > Should we use floor() and allow 2? Oh yeah I forgot to comment on this. I want to figure out which way it should be - ceil or floor? Depends on whether we want to be more restrictive or not. https://codereview.chromium.org/1261143004/diff/350001/chrome/browser/manifes... chrome/browser/manifest/manifest_icon_downloader.cc:121: if (current_bitmap.height() > ideal_icon_size_in_px && On 2015/08/18 14:48:18, Mounir Lamouri wrote: > what about something like: > if (bigger than ideal size || bigger or equal than smaller size) { > if (better than current one) { > select it; > } > } The complexity in this one would be in the "better than current one" logic. Better would mean that the icon is not smaller than ideal and this is bigger than ideal or both are less than ideal but the new one is bigger or both are bigger than ideal and the new one is smaller. https://codereview.chromium.org/1261143004/diff/350001/chrome/browser/manifes... chrome/browser/manifest/manifest_icon_downloader.cc:133: return best_upper_index == -1 ? best_lower_index : best_upper_index; On 2015/08/18 14:48:18, Mounir Lamouri wrote: > Why do you need two indexes? Because I always want to pick an icon bigger than ideal over one which is smaller than ideal.
I've completely reworked the algorithm in the downloader to be much simpler. As I said in the other CL, the code belonged in the selector class as this is only to do with multiple icons in the same file. I've still kept scaling down intact for bigger icons. But scaling up is no longer done as it wastes space and the system might well do a better job when actually displaying the icon.
https://chromiumcodereview.appspot.com/1261143004/diff/510001/chrome/browser/... File chrome/browser/banners/app_banner_data_fetcher.h (right): https://chromiumcodereview.appspot.com/1261143004/diff/510001/chrome/browser/... chrome/browser/banners/app_banner_data_fetcher.h:122: // Called when it is determined that the webapp has fulfilled the intial nit: initial https://chromiumcodereview.appspot.com/1261143004/diff/510001/chrome/browser/... File chrome/browser/manifest/manifest_icon_downloader.cc (right): https://chromiumcodereview.appspot.com/1261143004/diff/510001/chrome/browser/... chrome/browser/manifest/manifest_icon_downloader.cc:57: // Only scale if we need to scale down. For scaling up we will let the system Given that FindClosestBitmapIndex() only returns bitmaps >= the ideal size, in what situation will we ever have to scale up? https://chromiumcodereview.appspot.com/1261143004/diff/510001/chrome/browser/... chrome/browser/manifest/manifest_icon_downloader.cc:95: // equal to the preferred size. |bitmaps| is ordered from bigger to smaller. the explicit restriction on |bitmaps| should be put in header as a function comment https://chromiumcodereview.appspot.com/1261143004/diff/510001/chrome/browser/... File chrome/browser/manifest/manifest_icon_downloader.h (right): https://chromiumcodereview.appspot.com/1261143004/diff/510001/chrome/browser/... chrome/browser/manifest/manifest_icon_downloader.h:31: // Downloads the icon located at icon_url. If the file contains multiple icons nit: This sounds more like a class comment. Consider moving it there? https://chromiumcodereview.appspot.com/1261143004/diff/510001/chrome/browser/... File chrome/browser/manifest/manifest_icon_downloader_unittest.cc (right): https://chromiumcodereview.appspot.com/1261143004/diff/510001/chrome/browser/... chrome/browser/manifest/manifest_icon_downloader_unittest.cc:90: TEST_F(ManifestIconDownloaderTest, ClosestToExactIsChosenUnordered) { how is this test different from ClosestToExactIsChosen?
https://codereview.chromium.org/1261143004/diff/510001/chrome/browser/banners... File chrome/browser/banners/app_banner_data_fetcher.h (right): https://codereview.chromium.org/1261143004/diff/510001/chrome/browser/banners... chrome/browser/banners/app_banner_data_fetcher.h:122: // Called when it is determined that the webapp has fulfilled the intial On 2015/08/19 22:21:07, dfalcantara wrote: > nit: initial Done. https://codereview.chromium.org/1261143004/diff/510001/chrome/browser/manifes... File chrome/browser/manifest/manifest_icon_downloader.cc (right): https://codereview.chromium.org/1261143004/diff/510001/chrome/browser/manifes... chrome/browser/manifest/manifest_icon_downloader.cc:57: // Only scale if we need to scale down. For scaling up we will let the system On 2015/08/19 22:21:07, dfalcantara wrote: > Given that FindClosestBitmapIndex() only returns bitmaps >= the ideal size, in > what situation will we ever have to scale up? Ah the algortihm doesn't take into account minimum size restrictions. :/ ideal_icon_size_in_px should probably be minimum_icon_size_in_px in the algorithm. https://codereview.chromium.org/1261143004/diff/510001/chrome/browser/manifes... chrome/browser/manifest/manifest_icon_downloader.cc:95: // equal to the preferred size. |bitmaps| is ordered from bigger to smaller. On 2015/08/19 22:21:07, dfalcantara wrote: > the explicit restriction on |bitmaps| should be put in header as a function > comment Done. https://codereview.chromium.org/1261143004/diff/510001/chrome/browser/manifes... File chrome/browser/manifest/manifest_icon_downloader.h (right): https://codereview.chromium.org/1261143004/diff/510001/chrome/browser/manifes... chrome/browser/manifest/manifest_icon_downloader.h:31: // Downloads the icon located at icon_url. If the file contains multiple icons On 2015/08/19 22:21:07, dfalcantara wrote: > nit: This sounds more like a class comment. Consider moving it there? Done. https://codereview.chromium.org/1261143004/diff/510001/chrome/browser/manifes... File chrome/browser/manifest/manifest_icon_downloader_unittest.cc (right): https://codereview.chromium.org/1261143004/diff/510001/chrome/browser/manifes... chrome/browser/manifest/manifest_icon_downloader_unittest.cc:90: TEST_F(ManifestIconDownloaderTest, ClosestToExactIsChosenUnordered) { On 2015/08/19 22:21:08, dfalcantara wrote: > how is this test different from ClosestToExactIsChosen? It's not anymore because of the sorting restriction. Removed.
overall re-lgtm https://codereview.chromium.org/1261143004/diff/550001/chrome/browser/manifes... File chrome/browser/manifest/manifest_icon_downloader.cc (right): https://codereview.chromium.org/1261143004/diff/550001/chrome/browser/manifes... chrome/browser/manifest/manifest_icon_downloader.cc:112: // best_index == -1 means the first good icon which has been found if you have to explicitly explain what -1 means, define it as a constant. it's used in a few places already, IIRC, so it'd be a good thing to do anyway.
https://codereview.chromium.org/1261143004/diff/550001/chrome/browser/android... File chrome/browser/android/banners/app_banner_data_fetcher_android.cc (right): https://codereview.chromium.org/1261143004/diff/550001/chrome/browser/android... chrome/browser/android/banners/app_banner_data_fetcher_android.cc:39: return FetchAppIcon(web_contents, image_url); nit: instead do: return FetchAppIcon(GetWebContents(), image_url); https://codereview.chromium.org/1261143004/diff/550001/chrome/browser/manifes... File chrome/browser/manifest/manifest_icon_downloader.cc (right): https://codereview.chromium.org/1261143004/diff/550001/chrome/browser/manifes... chrome/browser/manifest/manifest_icon_downloader.cc:18: if (!web_contents || !icon_url.is_valid()) return false; style: contrary to java, you must have |return false;| on a new line https://codereview.chromium.org/1261143004/diff/550001/chrome/browser/manifes... chrome/browser/manifest/manifest_icon_downloader.cc:55: const int closest_index = FindClosestBitmapIndex( nit: leave a blank line between the DCHECK() and the const int https://codereview.chromium.org/1261143004/diff/550001/chrome/browser/manifes... chrome/browser/manifest/manifest_icon_downloader.cc:93: content::BrowserThread::PostTask( nit: I would add a blank line, up to you. https://codereview.chromium.org/1261143004/diff/550001/chrome/browser/manifes... chrome/browser/manifest/manifest_icon_downloader.cc:114: // bigger than the minimum. Hmm, if you found an icon bigger than minimum and smaller than the ideal, given than the next one is going to be smaller, shouldn't you stop there and return it? Then, maybe you don't need best_index and you should just return -1 after the loop? https://codereview.chromium.org/1261143004/diff/550001/chrome/browser/manifes... File chrome/browser/manifest/manifest_icon_downloader.h (right): https://codereview.chromium.org/1261143004/diff/550001/chrome/browser/manifes... chrome/browser/manifest/manifest_icon_downloader.h:9: #include "base/bind.h" Do you need bind.h or callback_fwd.h? https://codereview.chromium.org/1261143004/diff/550001/chrome/browser/manifes... chrome/browser/manifest/manifest_icon_downloader.h:10: #include "base/memory/ref_counted.h" Do you need this include? https://codereview.chromium.org/1261143004/diff/550001/chrome/browser/manifes... chrome/browser/manifest/manifest_icon_downloader.h:11: #include "content/public/browser/web_contents_observer.h" I think you can remove this #include. https://codereview.chromium.org/1261143004/diff/550001/chrome/browser/manifes... chrome/browser/manifest/manifest_icon_downloader.h:29: class ManifestIconDownloader { make it final? https://codereview.chromium.org/1261143004/diff/550001/chrome/browser/manifes... chrome/browser/manifest/manifest_icon_downloader.h:37: // current context or information do not allow to download the image. nit: line break after the first sentence.
https://codereview.chromium.org/1261143004/diff/550001/chrome/browser/manifes... File chrome/browser/manifest/manifest_icon_downloader.cc (right): https://codereview.chromium.org/1261143004/diff/550001/chrome/browser/manifes... chrome/browser/manifest/manifest_icon_downloader.cc:18: if (!web_contents || !icon_url.is_valid()) return false; On 2015/08/21 at 09:27:11, Mounir Lamouri wrote: > style: contrary to java, you must have |return false;| on a new line Done. https://codereview.chromium.org/1261143004/diff/550001/chrome/browser/manifes... chrome/browser/manifest/manifest_icon_downloader.cc:55: const int closest_index = FindClosestBitmapIndex( On 2015/08/21 at 09:27:11, Mounir Lamouri wrote: > nit: leave a blank line between the DCHECK() and the const int Done. https://codereview.chromium.org/1261143004/diff/550001/chrome/browser/manifes... chrome/browser/manifest/manifest_icon_downloader.cc:93: content::BrowserThread::PostTask( On 2015/08/21 at 09:27:11, Mounir Lamouri wrote: > nit: I would add a blank line, up to you. Done. https://codereview.chromium.org/1261143004/diff/550001/chrome/browser/manifes... chrome/browser/manifest/manifest_icon_downloader.cc:112: // best_index == -1 means the first good icon which has been found On 2015/08/20 18:48:42, dfalcantara wrote: > if you have to explicitly explain what -1 means, define it as a constant. it's > used in a few places already, IIRC, so it'd be a good thing to do anyway. Code has changed enough such that use of -1 is much clearer without the constant. https://codereview.chromium.org/1261143004/diff/550001/chrome/browser/manifes... chrome/browser/manifest/manifest_icon_downloader.cc:114: // bigger than the minimum. On 2015/08/21 09:27:11, Mounir Lamouri wrote: > Hmm, if you found an icon bigger than minimum and smaller than the ideal, given > than the next one is going to be smaller, shouldn't you stop there and return > it? > > Then, maybe you don't need best_index and you should just return -1 after the > loop? Changed as discussed offline. https://codereview.chromium.org/1261143004/diff/550001/chrome/browser/manifes... File chrome/browser/manifest/manifest_icon_downloader.h (right): https://codereview.chromium.org/1261143004/diff/550001/chrome/browser/manifes... chrome/browser/manifest/manifest_icon_downloader.h:9: #include "base/bind.h" On 2015/08/21 at 09:27:11, Mounir Lamouri wrote: > Do you need bind.h or callback_fwd.h? Done. https://codereview.chromium.org/1261143004/diff/550001/chrome/browser/manifes... chrome/browser/manifest/manifest_icon_downloader.h:10: #include "base/memory/ref_counted.h" On 2015/08/21 at 09:27:12, Mounir Lamouri wrote: > Do you need this include? Done. https://codereview.chromium.org/1261143004/diff/550001/chrome/browser/manifes... chrome/browser/manifest/manifest_icon_downloader.h:11: #include "content/public/browser/web_contents_observer.h" On 2015/08/21 at 09:27:12, Mounir Lamouri wrote: > I think you can remove this #include. Done. https://codereview.chromium.org/1261143004/diff/550001/chrome/browser/manifes... chrome/browser/manifest/manifest_icon_downloader.h:11: #include "content/public/browser/web_contents_observer.h" On 2015/08/21 at 09:27:12, Mounir Lamouri wrote: > I think you can remove this #include. Done. https://codereview.chromium.org/1261143004/diff/550001/chrome/browser/manifes... chrome/browser/manifest/manifest_icon_downloader.h:29: class ManifestIconDownloader { On 2015/08/21 at 09:27:11, Mounir Lamouri wrote: > make it final? Done. https://codereview.chromium.org/1261143004/diff/550001/chrome/browser/manifes... chrome/browser/manifest/manifest_icon_downloader.h:37: // current context or information do not allow to download the image. On 2015/08/21 at 09:27:11, Mounir Lamouri wrote: > nit: line break after the first sentence. Done.
still lgtm with comments applied There were so many iterations that I missed a few things. https://codereview.chromium.org/1261143004/diff/610001/chrome/browser/banners... File chrome/browser/banners/app_banner_data_fetcher.h (right): https://codereview.chromium.org/1261143004/diff/610001/chrome/browser/banners... chrome/browser/banners/app_banner_data_fetcher.h:131: const scoped_ptr<ManifestIconDownloader>& icon_downloader() { Is that actually used somewhere? https://codereview.chromium.org/1261143004/diff/610001/chrome/browser/manifes... File chrome/browser/manifest/manifest_icon_downloader.cc (right): https://codereview.chromium.org/1261143004/diff/610001/chrome/browser/manifes... chrome/browser/manifest/manifest_icon_downloader.cc:110: int max_negative_delta = minimum_icon_size_in_px - ideal_icon_size_in_px; nit: const int https://codereview.chromium.org/1261143004/diff/610001/chrome/browser/manifes... chrome/browser/manifest/manifest_icon_downloader.cc:124: } The loop sgtm but it doesn't have to be one big blob. Could you add some blank lines for readability? https://codereview.chromium.org/1261143004/diff/610001/chrome/browser/manifes... File chrome/browser/manifest/manifest_icon_downloader.h (right): https://codereview.chromium.org/1261143004/diff/610001/chrome/browser/manifes... chrome/browser/manifest/manifest_icon_downloader.h:37: bool Download(content::WebContents* web_contents, Hmm, again sorry for seeing that so late but it seems that you have a class with a method and no members, right? Maybe you can make that method static and do = delete on the ctor and dtor. https://codereview.chromium.org/1261143004/diff/610001/chrome/browser/manifes... File chrome/browser/manifest/manifest_icon_downloader_unittest.cc (right): https://codereview.chromium.org/1261143004/diff/610001/chrome/browser/manifes... chrome/browser/manifest/manifest_icon_downloader_unittest.cc:39: std::vector<SkBitmap> vector; nit: I should have seen that before but using |vector| here isn't a great idea. Could you rename to bitmaps? https://codereview.chromium.org/1261143004/diff/610001/chrome/browser/manifes... chrome/browser/manifest/manifest_icon_downloader_unittest.cc:123: TEST_F(ManifestIconDownloaderTest, NonSquareWidthIsIgnored) { nit: NonSquareHeightIsIgnored/NonSquareWidthIsIgnored is confusing. Just have NonSquareIsIgnored
https://codereview.chromium.org/1261143004/diff/610001/chrome/browser/banners... File chrome/browser/banners/app_banner_data_fetcher.h (right): https://codereview.chromium.org/1261143004/diff/610001/chrome/browser/banners... chrome/browser/banners/app_banner_data_fetcher.h:131: const scoped_ptr<ManifestIconDownloader>& icon_downloader() { On 2015/08/21 12:59:08, Mounir Lamouri wrote: > Is that actually used somewhere? It was intended for use in the future when splascreeh icon is downloaded but if I make the class static, I guess I can get rid of it. https://codereview.chromium.org/1261143004/diff/610001/chrome/browser/manifes... File chrome/browser/manifest/manifest_icon_downloader.cc (right): https://codereview.chromium.org/1261143004/diff/610001/chrome/browser/manifes... chrome/browser/manifest/manifest_icon_downloader.cc:110: int max_negative_delta = minimum_icon_size_in_px - ideal_icon_size_in_px; On 2015/08/21 12:59:08, Mounir Lamouri wrote: > nit: const int Done. https://codereview.chromium.org/1261143004/diff/610001/chrome/browser/manifes... chrome/browser/manifest/manifest_icon_downloader.cc:124: } On 2015/08/21 12:59:08, Mounir Lamouri wrote: > The loop sgtm but it doesn't have to be one big blob. Could you add some blank > lines for readability? Done. https://codereview.chromium.org/1261143004/diff/610001/chrome/browser/manifes... File chrome/browser/manifest/manifest_icon_downloader.h (right): https://codereview.chromium.org/1261143004/diff/610001/chrome/browser/manifes... chrome/browser/manifest/manifest_icon_downloader.h:37: bool Download(content::WebContents* web_contents, On 2015/08/21 12:59:08, Mounir Lamouri wrote: > Hmm, again sorry for seeing that so late but it seems that you have a class with > a method and no members, right? Maybe you can make that method static and do = > delete on the ctor and dtor. Done. https://codereview.chromium.org/1261143004/diff/610001/chrome/browser/manifes... File chrome/browser/manifest/manifest_icon_downloader_unittest.cc (right): https://codereview.chromium.org/1261143004/diff/610001/chrome/browser/manifes... chrome/browser/manifest/manifest_icon_downloader_unittest.cc:39: std::vector<SkBitmap> vector; On 2015/08/21 12:59:08, Mounir Lamouri wrote: > nit: I should have seen that before but using |vector| here isn't a great idea. > Could you rename to bitmaps? Done. https://codereview.chromium.org/1261143004/diff/610001/chrome/browser/manifes... chrome/browser/manifest/manifest_icon_downloader_unittest.cc:123: TEST_F(ManifestIconDownloaderTest, NonSquareWidthIsIgnored) { On 2015/08/21 12:59:08, Mounir Lamouri wrote: > nit: NonSquareHeightIsIgnored/NonSquareWidthIsIgnored is confusing. Just have > NonSquareIsIgnored Done.
The CQ bit was checked by lalitm@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from newt@chromium.org, dfalcantara@chromium.org, mlamouri@chromium.org Link to the patchset: https://codereview.chromium.org/1261143004/#ps630001 (title: "Fix review comments")
The CQ bit was unchecked by commit-bot@chromium.org
This CL has an open dependency (Issue 1285063003 Patch 120001). Please resolve the dependency and try again.
https://codereview.chromium.org/1261143004/diff/630001/chrome/browser/android... File chrome/browser/android/webapps/add_to_homescreen_data_fetcher.h (right): https://codereview.chromium.org/1261143004/diff/630001/chrome/browser/android... chrome/browser/android/webapps/add_to_homescreen_data_fetcher.h:26: class ManifestIconDownloader; nit: you no longer need this https://codereview.chromium.org/1261143004/diff/630001/chrome/browser/banners... File chrome/browser/banners/app_banner_data_fetcher.h (right): https://codereview.chromium.org/1261143004/diff/630001/chrome/browser/banners... chrome/browser/banners/app_banner_data_fetcher.h:13: #include "chrome/browser/manifest/manifest_icon_downloader.h" You probably don't need the include then. https://codereview.chromium.org/1261143004/diff/630001/chrome/browser/manifes... File chrome/browser/manifest/manifest_icon_downloader.h (right): https://codereview.chromium.org/1261143004/diff/630001/chrome/browser/manifes... chrome/browser/manifest/manifest_icon_downloader.h:24: // Downloads the icon located at icon_url. If the file contains multiple icons nit: say it's a helper.
https://codereview.chromium.org/1261143004/diff/630001/chrome/browser/android... File chrome/browser/android/webapps/add_to_homescreen_data_fetcher.h (right): https://codereview.chromium.org/1261143004/diff/630001/chrome/browser/android... chrome/browser/android/webapps/add_to_homescreen_data_fetcher.h:26: class ManifestIconDownloader; On 2015/08/21 13:34:18, Mounir Lamouri wrote: > nit: you no longer need this Done. https://codereview.chromium.org/1261143004/diff/630001/chrome/browser/banners... File chrome/browser/banners/app_banner_data_fetcher.h (right): https://codereview.chromium.org/1261143004/diff/630001/chrome/browser/banners... chrome/browser/banners/app_banner_data_fetcher.h:13: #include "chrome/browser/manifest/manifest_icon_downloader.h" On 2015/08/21 13:34:18, Mounir Lamouri wrote: > You probably don't need the include then. Done. https://codereview.chromium.org/1261143004/diff/630001/chrome/browser/manifes... File chrome/browser/manifest/manifest_icon_downloader.h (right): https://codereview.chromium.org/1261143004/diff/630001/chrome/browser/manifes... chrome/browser/manifest/manifest_icon_downloader.h:24: // Downloads the icon located at icon_url. If the file contains multiple icons On 2015/08/21 13:34:18, Mounir Lamouri wrote: > nit: say it's a helper. Done.
The CQ bit was checked by lalitm@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from mlamouri@chromium.org, dfalcantara@chromium.org, newt@chromium.org Link to the patchset: https://codereview.chromium.org/1261143004/#ps670001 (title: "Rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1261143004/670001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1261143004/670001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by lalitm@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1261143004/670001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1261143004/670001
Message was sent while issue was closed.
Committed patchset #35 (id:670001)
Message was sent while issue was closed.
Patchset 35 (id:??) landed as https://crrev.com/e109fb09878be947da59c39b5876ae36c187d534 Cr-Commit-Position: refs/heads/master@{#344787} |
