Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1092873002:
[Icons NTP] Refactor large_icon_source to extract the logic shared between desktop and Android to f… (Closed)
Description[Icons NTP] Refactor large_icon_source to extract the logic shared between desktop and Android to a new large_icon_service.
This is required since the Android implementation of the icon-based NTP will rely on custom Java code to render the fallback. As a result we want to share the logic needed to retrieve large icons and to compute the fallback style without needing them to go through the full-featured chrome://large-icon that performs the rendering of fallback icons. The Java code will hook directly into the large_icon_service.
This CL also fixes a bug, making sure only non-square icons can be returned as large icons.
Besides this, this CL doesn't change the behavior but will make it possible to add the large icon selection logic to large_icon_service where it can be shared with the Android code.
BUG=467712
Committed: https://crrev.com/3e75e59228ae3c4388379ed90ebea2e30ca0c646
Cr-Commit-Position: refs/heads/master@{#326325}
Patch Set 1 #Patch Set 2 : #
Total comments: 26
Patch Set 3 : #Patch Set 4 : #
Total comments: 16
Patch Set 5 : #Patch Set 6 : #
Total comments: 18
Patch Set 7 : #
Total comments: 20
Patch Set 8 : #
Total comments: 3
Patch Set 9 : Added back changes to instant_service.cc #
Total comments: 4
Patch Set 10 : #
Total comments: 16
Patch Set 11 : #Patch Set 12 : #
Total comments: 10
Patch Set 13 : #Patch Set 14 : #Messages
Total messages: 54 (18 generated)
beaudoin@chromium.org changed reviewers: + huangs@chromium.org, jhawkins@chromium.org, pkotwicz@chromium.org, sky@chromium.org
REVIEWERS: - pkotwicz@, huangs@ : Global. - sky@ OWNERS: components/favicon_base/ (Should pkotwicz@ own this?) - jhawkins@ OWNERS: chrome/browser/ui/webui/ - newt@ FYI
Thanks for taking this on! Please consider the proposal to centralized some of the logic to FallbackIconStyle. The Clank code should also benefit from this. https://chromiumcodereview.appspot.com/1092873002/diff/20001/chrome/browser/u... File chrome/browser/ui/webui/large_icon_source.cc (right): https://chromiumcodereview.appspot.com/1092873002/diff/20001/chrome/browser/u... chrome/browser/ui/webui/large_icon_source.cc:82: favicon_service_->getLargeIcon( NIT: GetLargeIcon() https://chromiumcodereview.appspot.com/1092873002/diff/20001/chrome/browser/u... chrome/browser/ui/webui/large_icon_source.cc:140: style.text_color = text_color; These values can be moved to FallbackIconStyle: - Assign kDefaultBackgroundColor = kDarkGray; - Add & assign kDefaultTextColorDark = SK_ColorWHITE; - Assign kDefaultFontSizeRatio = 0.44; - Assign kDefaultRoundness = 0; Then we can dispense with result.is_color_valid() and remove SendDefaultFallbackIcon(), since the style is always valid, and dominant color extraction is an embellishment. https://chromiumcodereview.appspot.com/1092873002/diff/20001/chrome/browser/u... File chrome/browser/ui/webui/large_icon_source.h (right): https://chromiumcodereview.appspot.com/1092873002/diff/20001/chrome/browser/u... chrome/browser/ui/webui/large_icon_source.h:9: #include <vector> Can move #include <vector> to the .cc file. https://chromiumcodereview.appspot.com/1092873002/diff/20001/components/favic... File components/favicon/core/favicon_service.cc (right): https://chromiumcodereview.appspot.com/1092873002/diff/20001/components/favic... components/favicon/core/favicon_service.cc:185: const GURL& pageURL, NIT: pageURL --> page_url https://chromiumcodereview.appspot.com/1092873002/diff/20001/components/favic... components/favicon/core/favicon_service.cc:403: NIT: remove space https://chromiumcodereview.appspot.com/1092873002/diff/20001/components/favic... components/favicon/core/favicon_service.cc:406: if (!bitmap_result.is_valid()) { Semantically one would expect result.bitmap = bitmap_result; to appear here. The fact that this assignment doesn't occur yet is an optimization, so maybe add a comment blurb? https://chromiumcodereview.appspot.com/1092873002/diff/20001/components/favic... components/favicon/core/favicon_service.cc:418: color_utils::HSL color_hsl; I think this logic should be moved to FallbackIconStyle (which already has dependence on ui/gfx/color_utils.h). Perhaps as a helper routine to void SetColorFromSmallIcon( scoped_refptr<base::RefCountedMemory> bitmap_data FallbackIconStyle* style); This works with comments in large_icon_source.cc to move more logic to FallbackIconStyle. https://chromiumcodereview.appspot.com/1092873002/diff/20001/components/favic... File components/favicon/core/favicon_service.h (right): https://chromiumcodereview.appspot.com/1092873002/diff/20001/components/favic... components/favicon/core/favicon_service.h:135: // |desired_size_in_pixel|. If no good large icon can be found, return the NIT: return --> returns https://chromiumcodereview.appspot.com/1092873002/diff/20001/components/favic... components/favicon/core/favicon_service.h:137: base::CancelableTaskTracker::TaskId getLargeIcon( NIT: Should capitalize => GetLargeIcon(); also fix in comments. Perhaps rename to GetLargeIconOrDominantColor()? https://chromiumcodereview.appspot.com/1092873002/diff/20001/components/favic... components/favicon/core/favicon_service.h:138: const GURL& pageURL, NIT: pageURL --> page_url https://chromiumcodereview.appspot.com/1092873002/diff/20001/components/favic... components/favicon/core/favicon_service.h:252: // Intermediate callback for getLargeIcon(). Ensures the large icon is the NIT: GetLargeIcon() https://chromiumcodereview.appspot.com/1092873002/diff/20001/components/favic... File components/favicon_base/favicon_types.h (right): https://chromiumcodereview.appspot.com/1092873002/diff/20001/components/favic... components/favicon_base/favicon_types.h:84: bool is_valid() const { return bitmap.is_valid() || is_color_valid(); } It seems this is never used? https://chromiumcodereview.appspot.com/1092873002/diff/20001/components/favic... components/favicon_base/favicon_types.h:93: SkColor dominant_color; This can be FallbackIconStyle, which is always properly initialized. Then is_color_valid() is always true, and we remove it along with is_valid() (which is unused anyway).
beaudoin@ can you please explain why you are doing this refactor? (I am confused mostly because this CL is not associated with any bug) It seems that GetLargestRawFaviconForPageURL() is always called with the same |icon_types| parameter. It would therefore be useful to remove the |icon_types| parameter. What's the thinking behind introducing LargeIconResult?
On 2015/04/17 13:56:15, pkotwicz wrote: > beaudoin@ can you please explain why you are doing this refactor? (I am confused > mostly because this CL is not associated with any bug) > > It seems that GetLargestRawFaviconForPageURL() is always called with the same > |icon_types| parameter. It would therefore be useful to remove the |icon_types| > parameter. > > What's the thinking behind introducing LargeIconResult? Sam: All done, simplified code quite a bit in the process. \o/ Peter: - Added more detailed explanation + bug to the CL. - I tracked GetLargestRawFaviconForPageURL up to this Java code [1] and stopped because I felt figuring out if |icon_types| was really constant wasn't worth scaling up this CL to touch that many files. - LargeIconResult allow us to pass either the bitmap or the fallback style. This is the output of the key piece of logic we'd like to share between Android and desktop. [1] https://code.google.com/p/chromium/codesearch#chromium/src/chrome/android/jav...
https://codereview.chromium.org/1092873002/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/large_icon_source.cc (right): https://codereview.chromium.org/1092873002/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/large_icon_source.cc:82: favicon_service_->getLargeIcon( On 2015/04/17 03:55:17, huangs wrote: > NIT: GetLargeIcon() Done. https://codereview.chromium.org/1092873002/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/large_icon_source.cc:140: style.text_color = text_color; On 2015/04/17 03:55:17, huangs wrote: > These values can be moved to FallbackIconStyle: > - Assign kDefaultBackgroundColor = kDarkGray; > - Add & assign kDefaultTextColorDark = SK_ColorWHITE; > - Assign kDefaultFontSizeRatio = 0.44; > - Assign kDefaultRoundness = 0; > > Then we can dispense with result.is_color_valid() and remove > SendDefaultFallbackIcon(), since the style is always valid, and dominant color > extraction is an embellishment. Done. https://codereview.chromium.org/1092873002/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/large_icon_source.h (right): https://codereview.chromium.org/1092873002/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/large_icon_source.h:9: #include <vector> On 2015/04/17 03:55:17, huangs wrote: > Can move #include <vector> to the .cc file. Done. https://codereview.chromium.org/1092873002/diff/20001/components/favicon/core... File components/favicon/core/favicon_service.cc (right): https://codereview.chromium.org/1092873002/diff/20001/components/favicon/core... components/favicon/core/favicon_service.cc:185: const GURL& pageURL, On 2015/04/17 03:55:17, huangs wrote: > NIT: pageURL --> page_url Done. https://codereview.chromium.org/1092873002/diff/20001/components/favicon/core... components/favicon/core/favicon_service.cc:403: On 2015/04/17 03:55:17, huangs wrote: > NIT: remove space Done. https://codereview.chromium.org/1092873002/diff/20001/components/favicon/core... components/favicon/core/favicon_service.cc:406: if (!bitmap_result.is_valid()) { On 2015/04/17 03:55:17, huangs wrote: > Semantically one would expect > result.bitmap = bitmap_result; > to appear here. The fact that this assignment doesn't occur yet is an > optimization, so maybe add a comment blurb? Done. https://codereview.chromium.org/1092873002/diff/20001/components/favicon/core... components/favicon/core/favicon_service.cc:418: color_utils::HSL color_hsl; On 2015/04/17 03:55:17, huangs wrote: > I think this logic should be moved to FallbackIconStyle (which already has > dependence on ui/gfx/color_utils.h). Perhaps as a helper routine to > > void SetColorFromSmallIcon( > scoped_refptr<base::RefCountedMemory> bitmap_data > FallbackIconStyle* style); > > This works with comments in large_icon_source.cc to move more logic to > FallbackIconStyle. Done. https://codereview.chromium.org/1092873002/diff/20001/components/favicon/core... File components/favicon/core/favicon_service.h (right): https://codereview.chromium.org/1092873002/diff/20001/components/favicon/core... components/favicon/core/favicon_service.h:135: // |desired_size_in_pixel|. If no good large icon can be found, return the On 2015/04/17 03:55:17, huangs wrote: > NIT: return --> returns Done. https://codereview.chromium.org/1092873002/diff/20001/components/favicon/core... components/favicon/core/favicon_service.h:137: base::CancelableTaskTracker::TaskId getLargeIcon( On 2015/04/17 03:55:17, huangs wrote: > NIT: Should capitalize => GetLargeIcon(); also fix in comments. > > Perhaps rename to GetLargeIconOrDominantColor()? Done. https://codereview.chromium.org/1092873002/diff/20001/components/favicon/core... components/favicon/core/favicon_service.h:138: const GURL& pageURL, On 2015/04/17 03:55:17, huangs wrote: > NIT: pageURL --> page_url Done. https://codereview.chromium.org/1092873002/diff/20001/components/favicon/core... components/favicon/core/favicon_service.h:252: // Intermediate callback for getLargeIcon(). Ensures the large icon is the On 2015/04/17 03:55:17, huangs wrote: > NIT: GetLargeIcon() Done. https://codereview.chromium.org/1092873002/diff/20001/components/favicon_base... File components/favicon_base/favicon_types.h (right): https://codereview.chromium.org/1092873002/diff/20001/components/favicon_base... components/favicon_base/favicon_types.h:84: bool is_valid() const { return bitmap.is_valid() || is_color_valid(); } On 2015/04/17 03:55:17, huangs wrote: > It seems this is never used? Done. https://codereview.chromium.org/1092873002/diff/20001/components/favicon_base... components/favicon_base/favicon_types.h:93: SkColor dominant_color; On 2015/04/17 03:55:17, huangs wrote: > This can be FallbackIconStyle, which is always properly initialized. Then > is_color_valid() is always true, and we remove it along with is_valid() (which > is unused anyway). Done.
Much cleaner! LGTM with nits. https://chromiumcodereview.appspot.com/1092873002/diff/60001/chrome/browser/u... File chrome/browser/ui/webui/large_icon_source.cc (right): https://chromiumcodereview.appspot.com/1092873002/diff/60001/chrome/browser/u... chrome/browser/ui/webui/large_icon_source.cc:100: // Bitmap is invalid, use the fallback. NIT: "...use the fallback if service is available." https://chromiumcodereview.appspot.com/1092873002/diff/60001/components/favic... File components/favicon/core/favicon_service.cc (right): https://chromiumcodereview.appspot.com/1092873002/diff/60001/components/favic... components/favicon/core/favicon_service.cc:418: result.bitmap = bitmap_result; Can move this into "else" of previous "if" to use common callback.Run(result). https://chromiumcodereview.appspot.com/1092873002/diff/60001/components/favic... File components/favicon/core/favicon_service.h (right): https://chromiumcodereview.appspot.com/1092873002/diff/60001/components/favic... components/favicon/core/favicon_service.h:254: // icon is the desired size, if compute the icon fallback style and use it to TYPO: "if compute" https://chromiumcodereview.appspot.com/1092873002/diff/60001/components/favic... File components/favicon_base/fallback_icon_style.cc (right): https://chromiumcodereview.appspot.com/1092873002/diff/60001/components/favic... components/favicon_base/fallback_icon_style.cc:59: color_utils::CalculateKMeanColorOfPNG(bitmap_data); // Assumes |style.text_color| is light, and clamps luminance down to a reasonable maximum value so text is readable. https://chromiumcodereview.appspot.com/1092873002/diff/60001/components/favic... File components/favicon_base/fallback_icon_style.h (right): https://chromiumcodereview.appspot.com/1092873002/diff/60001/components/favic... components/favicon_base/fallback_icon_style.h:41: // Set |style|'s background color to the dominant color of |bitmap_data|, NIT: The fact that default text is light seems to be implementation detail. Perhaps: // Reassign |style|'s |background_color| to the dominant color of |bitmap_data|, but adjust its luminance to better match existing |text_color|. https://chromiumcodereview.appspot.com/1092873002/diff/60001/components/favic... components/favicon_base/fallback_icon_style.h:47: } // namespace favicon_base NIT: Add space?
Much cleaner now! LGTM with nits.
newt@chromium.org changed reviewers: + newt@chromium.org
this looks good from the Android perspective
Per discussion with beaudoin@, this also allows Clank to get background, text color, and font size ratio (if need). However, "roundness" is set to 0 because we decided to implement round corners in JS (for fallbacks AND icons). This means Android will have to do its own thing for round corners, and the setting is duplicated. Current spec is 4dp corner radius for 48dp x 48dp icon. FYI it is implemented via -webkit-clip-path: inset(0 0 0 0 round 4px); https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res...
I think that it would be useful to mention the name (or proposed name) of the large icon request endpoint on Android. I was able to track Android's request for thumbnails (which this is replacing) up to MostVisitedSites::GetURLThumbnail() but I am unsure where the request originates If you figure out whether |icon_types| is the same for all callers to FaviconService::GetLargestRawFaviconForPageURL() and michaelbai@ is ok with removing the parameter, I do not mind doing the work to remove the parameter in both Chromium code and the private Android code. https://codereview.chromium.org/1092873002/diff/60001/components/favicon/core... File components/favicon/core/favicon_service.cc (right): https://codereview.chromium.org/1092873002/diff/60001/components/favicon/core... components/favicon/core/favicon_service.cc:188: // a large icon is known but its bitmap is not available. I recommend creating a new service LargeIconService and adding this method there Currently FaviconService is a thin wrapper on top of HistoryService. I kind of want to keep it that way - At least initially I would like the logic for fetching on demand not to live in the FaviconService (Maybe we will end up merging LargeIconService and FaviconService) - I am undecided whether the logic to create a fallback icon with the dominant color of a smaller icon if a large icon is not available should live in FaviconService - The logic to return a fallback icon using the first letter of the domain and the default color when a favicon is not found should be in FaviconService The current behavior of the function would place it most logically in FallbackIconService but given that LargeIconService will likely exist, it is probably worth creating the service and putting it there
Correction: "I think that it would be useful to mention the name (or proposed name) of the large icon request endpoint on Android in the CL description."
Patchset #5 (id:80001) has been deleted
Sam: Did most of your nit except one. Peter: - On Android the service's method will likely be called directly (by Java code?) so there's no chrome:// endpoint for it. I've updated the CL description to reflect that. - I haven't looked deeper into whether |icon_types| has many different list. In general the logic of GetLargestRawFaviconForPageURL is very counterintuitive though and it definitely doesn't return the "largest" favicon available. The order of the elements in |icon_types| is critical to the way it works. Be wary of that if you undertake a refactoring. - I'm in the process of extracting a new service. https://codereview.chromium.org/1092873002/diff/60001/chrome/browser/ui/webui... File chrome/browser/ui/webui/large_icon_source.cc (right): https://codereview.chromium.org/1092873002/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/large_icon_source.cc:100: // Bitmap is invalid, use the fallback. On 2015/04/17 15:30:47, huangs wrote: > NIT: "...use the fallback if service is available." Done. https://codereview.chromium.org/1092873002/diff/60001/components/favicon/core... File components/favicon/core/favicon_service.cc (right): https://codereview.chromium.org/1092873002/diff/60001/components/favicon/core... components/favicon/core/favicon_service.cc:188: // a large icon is known but its bitmap is not available. On 2015/04/17 18:39:14, pkotwicz wrote: > I recommend creating a new service LargeIconService and adding this method there > > Currently FaviconService is a thin wrapper on top of HistoryService. I kind of > want to keep it that way > > - At least initially I would like the logic for fetching on demand not to live > in the FaviconService (Maybe we will end up merging LargeIconService and > FaviconService) > - I am undecided whether the logic to create a fallback icon with the dominant > color of a smaller icon if a large icon is not available should live in > FaviconService > - The logic to return a fallback icon using the first letter of the domain and > the default color when a favicon is not found should be in FaviconService > > The current behavior of the function would place it most logically in > FallbackIconService but given that LargeIconService will likely exist, it is > probably worth creating the service and putting it there Ok, I'll get that out in another service. Another reason to not put it in FallbackIconService is that Android shouldn't need to instantiate a FallbackIconService given it wants to render its fallback elsewhere. https://codereview.chromium.org/1092873002/diff/60001/components/favicon/core... File components/favicon/core/favicon_service.h (right): https://codereview.chromium.org/1092873002/diff/60001/components/favicon/core... components/favicon/core/favicon_service.h:254: // icon is the desired size, if compute the icon fallback style and use it to On 2015/04/17 15:30:47, huangs wrote: > TYPO: "if compute" Done. https://codereview.chromium.org/1092873002/diff/60001/components/favicon_base... File components/favicon_base/fallback_icon_style.cc (right): https://codereview.chromium.org/1092873002/diff/60001/components/favicon_base... components/favicon_base/fallback_icon_style.cc:59: color_utils::CalculateKMeanColorOfPNG(bitmap_data); On 2015/04/17 15:30:47, huangs wrote: > // Assumes |style.text_color| is light, and clamps luminance down to a > reasonable maximum value so text is readable. Done. https://codereview.chromium.org/1092873002/diff/60001/components/favicon_base... File components/favicon_base/fallback_icon_style.h (right): https://codereview.chromium.org/1092873002/diff/60001/components/favicon_base... components/favicon_base/fallback_icon_style.h:41: // Set |style|'s background color to the dominant color of |bitmap_data|, On 2015/04/17 15:30:47, huangs wrote: > NIT: The fact that default text is light seems to be implementation detail. > Perhaps: > > // Reassign |style|'s |background_color| to the dominant color of |bitmap_data|, > but adjust its luminance to better match existing |text_color|. I agree with you, but if the text is dark we should force a minimum luminance, if it's gray we should have an invalid luminance zone in the middle, etc. I think it's OK to talk about light text here because the operation only makes sense for light text. (And generalizing it would be overkill.) https://codereview.chromium.org/1092873002/diff/60001/components/favicon_base... components/favicon_base/fallback_icon_style.h:47: } // namespace favicon_base On 2015/04/17 15:30:47, huangs wrote: > NIT: Add space? Done.
Ping on 1 comment. https://codereview.chromium.org/1092873002/diff/60001/components/favicon/core... File components/favicon/core/favicon_service.cc (right): https://codereview.chromium.org/1092873002/diff/60001/components/favicon/core... components/favicon/core/favicon_service.cc:418: result.bitmap = bitmap_result; Ping on this comment (feel to reject, but just want to be sure that you saw this :) https://codereview.chromium.org/1092873002/diff/60001/components/favicon_base... File components/favicon_base/fallback_icon_style.h (right): https://codereview.chromium.org/1092873002/diff/60001/components/favicon_base... components/favicon_base/fallback_icon_style.h:41: // Set |style|'s background color to the dominant color of |bitmap_data|, Acknowledged.
Patchset #6 (id:120001) has been deleted
Patchset #6 (id:140001) has been deleted
beaudoin@chromium.org changed reviewers: + kmadhusu@chromium.org - newt@chromium.org
PTAL: Extracted to a new large_icon_service, as per Peter's request. REVIEWERS: - pkotwicz@, huangs@ : Global. OWNERS REVIEW: - sky@: components/favicon_base/ - jhawkins@: chrome/browser/ui/webui/ - kmadhusu@: chrome/browser/search/ FYI: - newt@ https://codereview.chromium.org/1092873002/diff/60001/components/favicon/core... File components/favicon/core/favicon_service.cc (right): https://codereview.chromium.org/1092873002/diff/60001/components/favicon/core... components/favicon/core/favicon_service.cc:418: result.bitmap = bitmap_result; On 2015/04/17 20:44:04, huangs wrote: > Ping on this comment (feel to reject, but just want to be sure that you saw this > :) Done.
Some quick comments. https://chromiumcodereview.appspot.com/1092873002/diff/160001/chrome/browser/... File chrome/browser/favicon/large_icon_service_factory.cc (right): https://chromiumcodereview.appspot.com/1092873002/diff/160001/chrome/browser/... chrome/browser/favicon/large_icon_service_factory.cc:39: FaviconServiceFactory::GetForProfile(Profile::FromBrowserContext(context), Perhaps in favicon_service_factory.cc, add // static favicon::FaviconService* FaviconServiceFactory::GetForBrowserContext( BrowserContext* context) { return static_cast<favicon::FaviconService*>( GetInstance()->GetServiceForBrowserContext(context, true)); } Then just call that guy. Then you can get rid of Profile usage and those pesky presubmits. https://chromiumcodereview.appspot.com/1092873002/diff/160001/chrome/browser/... File chrome/browser/favicon/large_icon_service_factory.h (right): https://chromiumcodereview.appspot.com/1092873002/diff/160001/chrome/browser/... chrome/browser/favicon/large_icon_service_factory.h:1: // Copyright 2015 The Chromium Authors. All rights reserved. I presume this file was branched from a sibling file. On your next upload, please set git cl upload --similarity=20 (or some thing) so we better see diffs. https://chromiumcodereview.appspot.com/1092873002/diff/160001/chrome/browser/... File chrome/browser/search/instant_service.cc (right): https://chromiumcodereview.appspot.com/1092873002/diff/160001/chrome/browser/... chrome/browser/search/instant_service.cc:131: content::URLDataSource::Add(profile_, new LargeIconSource( NIT: inconsistent ordering; move this after FaviconSource below? https://chromiumcodereview.appspot.com/1092873002/diff/160001/chrome/browser/... File chrome/browser/ui/webui/large_icon_source.h (right): https://chromiumcodereview.appspot.com/1092873002/diff/160001/chrome/browser/... chrome/browser/ui/webui/large_icon_source.h:70: favicon::FaviconService* favicon_service_; This is no longer used? https://chromiumcodereview.appspot.com/1092873002/diff/160001/chrome/browser/... File chrome/browser/ui/webui/ntp/most_visited_handler.cc (right): https://chromiumcodereview.appspot.com/1092873002/diff/160001/chrome/browser/... chrome/browser/ui/webui/ntp/most_visited_handler.cc:99: // Register chrome://large-icon as a data source for large icons. NIT: Maybe move this guy after FaviconSource few lines after, for nice ordering? https://chromiumcodereview.appspot.com/1092873002/diff/160001/chrome/browser/... chrome/browser/ui/webui/ntp/most_visited_handler.cc:102: content::URLDataSource::Add(profile, Hmm weird, I missed adding // Register chrome://fallback-icon as a data source for fallback icons. https://chromiumcodereview.appspot.com/1092873002/diff/160001/components/favi... File components/favicon.gypi (right): https://chromiumcodereview.appspot.com/1092873002/diff/160001/components/favi... components/favicon.gypi:39: 'favicon/core/large_icon_service.cc', You also need to update components\favicon\core\BUILD.gn. https://chromiumcodereview.appspot.com/1092873002/diff/160001/components/favi... File components/favicon/core/favicon_service.cc (right): https://chromiumcodereview.appspot.com/1092873002/diff/160001/components/favi... components/favicon/core/favicon_service.cc:158: int minimum_size_in_pixel, Note that changes in favicon_service.h/cc now reduced to trivial renaming. I'm fine with this, but some reviewers might not be.
sky@chromium.org changed reviewers: - pkotwicz@chromium.org
-sky as pkotwicz@chromium.org will be an owner of favicon_base as soon as https://codereview.chromium.org/1096133002/ lands
sky@chromium.org changed reviewers: - sky@chromium.org
Answered Sam's comment. I believe this is ready for final review. Don't hesitate! :) https://codereview.chromium.org/1092873002/diff/160001/chrome/browser/favicon... File chrome/browser/favicon/large_icon_service_factory.cc (right): https://codereview.chromium.org/1092873002/diff/160001/chrome/browser/favicon... chrome/browser/favicon/large_icon_service_factory.cc:39: FaviconServiceFactory::GetForProfile(Profile::FromBrowserContext(context), On 2015/04/20 19:50:25, huangs wrote: > Perhaps in favicon_service_factory.cc, add > > // static > favicon::FaviconService* FaviconServiceFactory::GetForBrowserContext( > BrowserContext* context) { > return static_cast<favicon::FaviconService*>( > GetInstance()->GetServiceForBrowserContext(context, true)); > } > > Then just call that guy. Then you can get rid of Profile usage and those pesky > presubmits. Done. https://codereview.chromium.org/1092873002/diff/160001/chrome/browser/favicon... File chrome/browser/favicon/large_icon_service_factory.h (right): https://codereview.chromium.org/1092873002/diff/160001/chrome/browser/favicon... chrome/browser/favicon/large_icon_service_factory.h:1: // Copyright 2015 The Chromium Authors. All rights reserved. On 2015/04/20 19:50:25, huangs wrote: > I presume this file was branched from a sibling file. On your next upload, > please set > > git cl upload --similarity=20 > > (or some thing) > so we better see diffs. Done. https://codereview.chromium.org/1092873002/diff/160001/chrome/browser/search/... File chrome/browser/search/instant_service.cc (right): https://codereview.chromium.org/1092873002/diff/160001/chrome/browser/search/... chrome/browser/search/instant_service.cc:131: content::URLDataSource::Add(profile_, new LargeIconSource( On 2015/04/20 19:50:25, huangs wrote: > NIT: inconsistent ordering; move this after FaviconSource below? Done. https://codereview.chromium.org/1092873002/diff/160001/chrome/browser/ui/webu... File chrome/browser/ui/webui/large_icon_source.h (right): https://codereview.chromium.org/1092873002/diff/160001/chrome/browser/ui/webu... chrome/browser/ui/webui/large_icon_source.h:70: favicon::FaviconService* favicon_service_; On 2015/04/20 19:50:25, huangs wrote: > This is no longer used? Done. https://codereview.chromium.org/1092873002/diff/160001/chrome/browser/ui/webu... File chrome/browser/ui/webui/ntp/most_visited_handler.cc (right): https://codereview.chromium.org/1092873002/diff/160001/chrome/browser/ui/webu... chrome/browser/ui/webui/ntp/most_visited_handler.cc:99: // Register chrome://large-icon as a data source for large icons. On 2015/04/20 19:50:25, huangs wrote: > NIT: Maybe move this guy after FaviconSource few lines after, for nice ordering? Done. https://codereview.chromium.org/1092873002/diff/160001/chrome/browser/ui/webu... chrome/browser/ui/webui/ntp/most_visited_handler.cc:102: content::URLDataSource::Add(profile, On 2015/04/20 19:50:25, huangs wrote: > Hmm weird, I missed adding > > // Register chrome://fallback-icon as a data source for fallback icons. Done. https://codereview.chromium.org/1092873002/diff/160001/components/favicon.gypi File components/favicon.gypi (right): https://codereview.chromium.org/1092873002/diff/160001/components/favicon.gyp... components/favicon.gypi:39: 'favicon/core/large_icon_service.cc', On 2015/04/20 19:50:25, huangs wrote: > You also need to update components\favicon\core\BUILD.gn. Done. https://codereview.chromium.org/1092873002/diff/160001/components/favicon/cor... File components/favicon/core/favicon_service.cc (right): https://codereview.chromium.org/1092873002/diff/160001/components/favicon/cor... components/favicon/core/favicon_service.cc:158: int minimum_size_in_pixel, On 2015/04/20 19:50:25, huangs wrote: > Note that changes in favicon_service.h/cc now reduced to trivial renaming. I'm > fine with this, but some reviewers might not be. Good point. I'll leave that for the next person to edit this. It should probably be in a CL of its own. Done.
LGTM++ with NITS. Please also run "git cl lint". https://chromiumcodereview.appspot.com/1092873002/diff/180001/chrome/browser/... File chrome/browser/favicon/large_icon_service_factory.cc (right): https://chromiumcodereview.appspot.com/1092873002/diff/180001/chrome/browser/... chrome/browser/favicon/large_icon_service_factory.cc:1: // Copyright 2015 The Chromium Authors. All rights reserved. NOTE: The left file was unfortunately matched against chrome_fallback_icon_client_factory.cc but fallback_icon_service_factory.cc would have been a better fit... https://chromiumcodereview.appspot.com/1092873002/diff/180001/chrome/browser/... File chrome/browser/search/instant_service.cc (right): https://chromiumcodereview.appspot.com/1092873002/diff/180001/chrome/browser/... chrome/browser/search/instant_service.cc:19: #include "chrome/browser/themes/theme_properties.h" Artifact from syncing? https://chromiumcodereview.appspot.com/1092873002/diff/180001/chrome/browser/... chrome/browser/search/instant_service.cc:50: namespace { Artifact from syncing? Etc. https://chromiumcodereview.appspot.com/1092873002/diff/180001/chrome/browser/... File chrome/browser/ui/webui/large_icon_source.cc (right): https://chromiumcodereview.appspot.com/1092873002/diff/180001/chrome/browser/... chrome/browser/ui/webui/large_icon_source.cc:66: base::Bind(&LargeIconSource::OnIconDataAvailable, base::Unretained(this), OnLargeIconDataAvailable() https://chromiumcodereview.appspot.com/1092873002/diff/180001/chrome/browser/... chrome/browser/ui/webui/large_icon_source.cc:90: void LargeIconSource::OnIconDataAvailable( NIT: OnLargeIconDataAvailable(). https://chromiumcodereview.appspot.com/1092873002/diff/180001/chrome/browser/... File chrome/browser/ui/webui/large_icon_source.h (right): https://chromiumcodereview.appspot.com/1092873002/diff/180001/chrome/browser/... chrome/browser/ui/webui/large_icon_source.h:56: void OnIconDataAvailable( NIT: OnLargeIconDataAvailable(), also change comments? https://chromiumcodereview.appspot.com/1092873002/diff/180001/components/favi... File components/favicon/core/large_icon_service.cc (right): https://chromiumcodereview.appspot.com/1092873002/diff/180001/components/favi... components/favicon/core/large_icon_service.cc:63: result.bitmap = bitmap_result; // FIXME: Resize the bitmap to be |desired_size_in_pixel|. FYI the resizing code that can be used was abandoned a while back, but still visible from https://chromiumcodereview.appspot.com/1016833003/diff/1/chrome/browser/ui/we... (lines 134). https://chromiumcodereview.appspot.com/1092873002/diff/180001/components/favi... File components/favicon/core/large_icon_service.h (right): https://chromiumcodereview.appspot.com/1092873002/diff/180001/components/favi... components/favicon/core/large_icon_service.h:45: // to invoke |callback|. I noticed that the "ensure the large icon is the desired size", i.e., resizing large icon to match |desired_size_in_pixel|, is broken. No need to fix this in CL, but please update comments by removing the "Eusure..." and add add // FIXME: Ensure large icon is the desired size. Alternatively we can abandon the attempt to do resizing here. but then we'll have to worry about: - Ensuring chrome-search://large-icon is not usable by NTP (only allowed in iframe). - Verifying Android code will do the proper resizing at some point (I think it does already). https://chromiumcodereview.appspot.com/1092873002/diff/180001/components/favi... File components/favicon_base/fallback_icon_style.cc (right): https://chromiumcodereview.appspot.com/1092873002/diff/180001/components/favi... components/favicon_base/fallback_icon_style.cc:6: #include <algorithm> since you used std::min(). https://chromiumcodereview.appspot.com/1092873002/diff/180001/components/favi... File components/favicon_base/favicon_types.h (right): https://chromiumcodereview.appspot.com/1092873002/diff/180001/components/favi... components/favicon_base/favicon_types.h:7: #include <base/memory/scoped_ptr.h>
Patchset #8 (id:200001) has been deleted
https://codereview.chromium.org/1092873002/diff/180001/chrome/browser/favicon... File chrome/browser/favicon/large_icon_service_factory.cc (right): https://codereview.chromium.org/1092873002/diff/180001/chrome/browser/favicon... chrome/browser/favicon/large_icon_service_factory.cc:1: // Copyright 2015 The Chromium Authors. All rights reserved. On 2015/04/21 05:00:27, huangs wrote: > NOTE: The left file was unfortunately matched against > chrome_fallback_icon_client_factory.cc > but > fallback_icon_service_factory.cc > would have been a better fit... :( No idea what to do about this though... https://codereview.chromium.org/1092873002/diff/180001/chrome/browser/search/... File chrome/browser/search/instant_service.cc (right): https://codereview.chromium.org/1092873002/diff/180001/chrome/browser/search/... chrome/browser/search/instant_service.cc:19: #include "chrome/browser/themes/theme_properties.h" On 2015/04/21 05:00:27, huangs wrote: > Artifact from syncing? Artifact from a revert before syncing. Good catch. https://codereview.chromium.org/1092873002/diff/180001/chrome/browser/search/... chrome/browser/search/instant_service.cc:50: namespace { On 2015/04/21 05:00:27, huangs wrote: > Artifact from syncing? Etc. Done. https://codereview.chromium.org/1092873002/diff/180001/chrome/browser/ui/webu... File chrome/browser/ui/webui/large_icon_source.cc (right): https://codereview.chromium.org/1092873002/diff/180001/chrome/browser/ui/webu... chrome/browser/ui/webui/large_icon_source.cc:66: base::Bind(&LargeIconSource::OnIconDataAvailable, base::Unretained(this), On 2015/04/21 05:00:28, huangs wrote: > OnLargeIconDataAvailable() Done. https://codereview.chromium.org/1092873002/diff/180001/chrome/browser/ui/webu... chrome/browser/ui/webui/large_icon_source.cc:90: void LargeIconSource::OnIconDataAvailable( On 2015/04/21 05:00:28, huangs wrote: > NIT: OnLargeIconDataAvailable(). Done. https://codereview.chromium.org/1092873002/diff/180001/chrome/browser/ui/webu... File chrome/browser/ui/webui/large_icon_source.h (right): https://codereview.chromium.org/1092873002/diff/180001/chrome/browser/ui/webu... chrome/browser/ui/webui/large_icon_source.h:56: void OnIconDataAvailable( On 2015/04/21 05:00:28, huangs wrote: > NIT: OnLargeIconDataAvailable(), also change comments? Done. https://codereview.chromium.org/1092873002/diff/180001/components/favicon/cor... File components/favicon/core/large_icon_service.cc (right): https://codereview.chromium.org/1092873002/diff/180001/components/favicon/cor... components/favicon/core/large_icon_service.cc:63: result.bitmap = bitmap_result; On 2015/04/21 05:00:28, huangs wrote: > // FIXME: Resize the bitmap to be |desired_size_in_pixel|. > > FYI the resizing code that can be used was abandoned a while back, but still > visible from > https://chromiumcodereview.appspot.com/1016833003/diff/1/chrome/browser/ui/we... > (lines 134). Done. https://codereview.chromium.org/1092873002/diff/180001/components/favicon/cor... File components/favicon/core/large_icon_service.h (right): https://codereview.chromium.org/1092873002/diff/180001/components/favicon/cor... components/favicon/core/large_icon_service.h:45: // to invoke |callback|. On 2015/04/21 05:00:28, huangs wrote: > I noticed that the "ensure the large icon is the desired size", i.e., resizing > large icon to match |desired_size_in_pixel|, is broken. No need to fix this in > CL, but please update comments by removing the "Eusure..." and add add > > // FIXME: Ensure large icon is the desired size. > > Alternatively we can abandon the attempt to do resizing here. but then we'll > have to worry about: > - Ensuring chrome-search://large-icon is not usable by NTP (only allowed in > iframe). > - Verifying Android code will do the proper resizing at some point (I think it > does already). Added TODO in large_icon_service.cc. Let's discuss if we want to resize it here or in HTML later. https://codereview.chromium.org/1092873002/diff/180001/components/favicon_bas... File components/favicon_base/fallback_icon_style.cc (right): https://codereview.chromium.org/1092873002/diff/180001/components/favicon_bas... components/favicon_base/fallback_icon_style.cc:6: On 2015/04/21 05:00:28, huangs wrote: > #include <algorithm> > > since you used std::min(). Done. https://codereview.chromium.org/1092873002/diff/180001/components/favicon_bas... File components/favicon_base/favicon_types.h (right): https://codereview.chromium.org/1092873002/diff/180001/components/favicon_bas... components/favicon_base/favicon_types.h:7: On 2015/04/21 05:00:28, huangs wrote: > #include <base/memory/scoped_ptr.h> Done.
beaudoin@chromium.org changed reviewers: + pkotwicz@chromium.org
LGTM for chrome/browser/search changes. https://codereview.chromium.org/1092873002/diff/240001/chrome/browser/search/... File chrome/browser/search/instant_service.cc (right): https://codereview.chromium.org/1092873002/diff/240001/chrome/browser/search/... chrome/browser/search/instant_service.cc:129: content::URLDataSource::Add(profile_, style nit: For function declarations and definitions, put each argument on a separate line unless the whole declaration fits on one line. Ref: https://www.chromium.org/developers/coding-style https://codereview.chromium.org/1092873002/diff/240001/components/favicon_bas... File components/favicon_base/fallback_icon_style.cc (right): https://codereview.chromium.org/1092873002/diff/240001/components/favicon_bas... components/favicon_base/fallback_icon_style.cc:70: nit: Please remove this blank line.
LGTM with optional comment. https://codereview.chromium.org/1092873002/diff/260001/chrome/browser/ui/webu... File chrome/browser/ui/webui/large_icon_source.h (right): https://codereview.chromium.org/1092873002/diff/260001/chrome/browser/ui/webu... chrome/browser/ui/webui/large_icon_source.h:68: favicon::FallbackIconService* fallback_icon_service_; I know we document that an object owns a given point (e.g., // Owner ptr), but I don't recall if we document when an object does not own a given pointer. Optional, but it may make the ownership less ambiguous to document that fact (e.g., // Owned by client)
Answered James and Kausalya's comments. https://codereview.chromium.org/1092873002/diff/240001/chrome/browser/search/... File chrome/browser/search/instant_service.cc (right): https://codereview.chromium.org/1092873002/diff/240001/chrome/browser/search/... chrome/browser/search/instant_service.cc:129: content::URLDataSource::Add(profile_, On 2015/04/21 17:00:27, kmadhusu wrote: > style nit: For function declarations and definitions, put each argument on a > separate line unless the whole declaration fits on one line. > > Ref: https://www.chromium.org/developers/coding-style Done. https://codereview.chromium.org/1092873002/diff/240001/components/favicon_bas... File components/favicon_base/fallback_icon_style.cc (right): https://codereview.chromium.org/1092873002/diff/240001/components/favicon_bas... components/favicon_base/fallback_icon_style.cc:70: On 2015/04/21 17:00:27, kmadhusu wrote: > nit: Please remove this blank line. Done. https://codereview.chromium.org/1092873002/diff/260001/chrome/browser/ui/webu... File chrome/browser/ui/webui/large_icon_source.h (right): https://codereview.chromium.org/1092873002/diff/260001/chrome/browser/ui/webu... chrome/browser/ui/webui/large_icon_source.h:68: favicon::FallbackIconService* fallback_icon_service_; On 2015/04/21 17:08:25, James Hawkins wrote: > I know we document that an object owns a given point (e.g., // Owner ptr), but I > don't recall if we document when an object does not own a given pointer. > Optional, but it may make the ownership less ambiguous to document that fact > (e.g., // Owned by client) Done.
The CQ bit was checked by beaudoin@chromium.org to run a CQ dry run
The patchset sent to the CQ was uploaded after l-g-t-m from huangs@chromium.org, jhawkins@chromium.org, kmadhusu@chromium.org Link to the patchset: https://codereview.chromium.org/1092873002/#ps280001 (title: " ")
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1092873002/280001
Mostly looks good. Thank you for addding LargeIconService! https://codereview.chromium.org/1092873002/diff/220001/components/favicon_bas... File components/favicon_base/favicon_types.h (right): https://codereview.chromium.org/1092873002/diff/220001/components/favicon_bas... components/favicon_base/favicon_types.h:90: scoped_ptr<FallbackIconStyle> fallback_icon_style; I wonder whether the API would be easier to understand if this class had a single member: "scoped_refptr<base::RefCountedMemory> bitmap_data" |bitmap_data| would have the PNG bytes for the favicon if it is large enough and the PNG bytes of the rendered fallback icon otherwise. https://codereview.chromium.org/1092873002/diff/260001/chrome/browser/favicon... File chrome/browser/favicon/large_icon_service_factory.cc (right): https://codereview.chromium.org/1092873002/diff/260001/chrome/browser/favicon... chrome/browser/favicon/large_icon_service_factory.cc:38: FaviconServiceFactory::GetForBrowserContext(context); Can't you pass in the profile (You can get the profile with Profile::FromBrowserContext()) and EXPLICIT_ACCESS? I am unsure why you added a new accessor to FaviconServiceFactory. I have not checked but it looks like this might produce interesting behavior in incognito mode https://codereview.chromium.org/1092873002/diff/260001/chrome/browser/ui/webu... File chrome/browser/ui/webui/large_icon_source.h (right): https://codereview.chromium.org/1092873002/diff/260001/chrome/browser/ui/webu... chrome/browser/ui/webui/large_icon_source.h:13: #include "components/favicon_base/favicon_types.h" You can remove the above include? https://codereview.chromium.org/1092873002/diff/260001/chrome/browser/ui/webu... chrome/browser/ui/webui/large_icon_source.h:55: // Callback to retrieve data for large icon. How about: "Called with results of large icon retrieval request." https://codereview.chromium.org/1092873002/diff/260001/components/favicon/cor... File components/favicon/core/large_icon_service.cc (right): https://codereview.chromium.org/1092873002/diff/260001/components/favicon/cor... components/favicon/core/large_icon_service.cc:59: bitmap_result.pixel_size.width() != bitmap_result.pixel_size.height()) { It is probably worth mentioning in the CL description that you are now using the fallback icon if you get a non-square result from the favicon database. (Otherwise, someone may wrongly assume that this is a bug in the refactor). Maybe even worth putting up as a trivial follow up CL https://codereview.chromium.org/1092873002/diff/260001/components/favicon/cor... File components/favicon/core/large_icon_service.h (right): https://codereview.chromium.org/1092873002/diff/260001/components/favicon/cor... components/favicon/core/large_icon_service.h:54: // for large icons. Note: this is simply an optimization over populating an Nit: "Note: this is simply an optimization" -> "This is an optimization" https://codereview.chromium.org/1092873002/diff/260001/components/favicon_bas... File components/favicon_base/fallback_icon_style.cc (right): https://codereview.chromium.org/1092873002/diff/260001/components/favicon_bas... components/favicon_base/fallback_icon_style.cc:26: const SkColor kDefaultBackgroundColor = kDarkGray; Nit: Remove separate |kDarkGray| variable. It only has one user. https://codereview.chromium.org/1092873002/diff/260001/components/favicon_bas... components/favicon_base/fallback_icon_style.cc:59: FallbackIconStyle* style) { Aside & for another CL: I wonder whether it would make more sense to adjust the text color for the dominant background color rather than the background color.
Answered Peter's comments. https://codereview.chromium.org/1092873002/diff/260001/chrome/browser/favicon... File chrome/browser/favicon/large_icon_service_factory.cc (right): https://codereview.chromium.org/1092873002/diff/260001/chrome/browser/favicon... chrome/browser/favicon/large_icon_service_factory.cc:38: FaviconServiceFactory::GetForBrowserContext(context); On 2015/04/21 18:19:48, pkotwicz wrote: > Can't you pass in the profile (You can get the profile with > Profile::FromBrowserContext()) and EXPLICIT_ACCESS? > I am unsure why you added a new accessor to FaviconServiceFactory. > > I have not checked but it looks like this might produce interesting behavior in > incognito mode Reverted back to the approach I was using in: https://codereview.chromium.org/1092873002/diff/160001/chrome/browser/favicon... Discussed with huangs@ and he's OK with the revert. https://codereview.chromium.org/1092873002/diff/260001/chrome/browser/ui/webu... File chrome/browser/ui/webui/large_icon_source.h (right): https://codereview.chromium.org/1092873002/diff/260001/chrome/browser/ui/webu... chrome/browser/ui/webui/large_icon_source.h:13: #include "components/favicon_base/favicon_types.h" On 2015/04/21 18:19:48, pkotwicz wrote: > You can remove the above include? Done. https://codereview.chromium.org/1092873002/diff/260001/chrome/browser/ui/webu... chrome/browser/ui/webui/large_icon_source.h:55: // Callback to retrieve data for large icon. On 2015/04/21 18:19:48, pkotwicz wrote: > How about: "Called with results of large icon retrieval request." Done. https://codereview.chromium.org/1092873002/diff/260001/components/favicon/cor... File components/favicon/core/large_icon_service.cc (right): https://codereview.chromium.org/1092873002/diff/260001/components/favicon/cor... components/favicon/core/large_icon_service.cc:59: bitmap_result.pixel_size.width() != bitmap_result.pixel_size.height()) { On 2015/04/21 18:19:48, pkotwicz wrote: > It is probably worth mentioning in the CL description that you are now using the > fallback icon if you get a non-square result from the favicon database. > (Otherwise, someone may wrongly assume that this is a bug in the refactor). > Maybe even worth putting up as a trivial follow up CL Good point. I think it was a blatant bug in the original so will just update the CL desc. https://codereview.chromium.org/1092873002/diff/260001/components/favicon/cor... File components/favicon/core/large_icon_service.h (right): https://codereview.chromium.org/1092873002/diff/260001/components/favicon/cor... components/favicon/core/large_icon_service.h:54: // for large icons. Note: this is simply an optimization over populating an On 2015/04/21 18:19:48, pkotwicz wrote: > Nit: "Note: this is simply an optimization" -> "This is an optimization" Done. https://codereview.chromium.org/1092873002/diff/260001/components/favicon_bas... File components/favicon_base/fallback_icon_style.cc (right): https://codereview.chromium.org/1092873002/diff/260001/components/favicon_bas... components/favicon_base/fallback_icon_style.cc:26: const SkColor kDefaultBackgroundColor = kDarkGray; On 2015/04/21 18:19:48, pkotwicz wrote: > Nit: Remove separate |kDarkGray| variable. It only has one user. Done. https://codereview.chromium.org/1092873002/diff/260001/components/favicon_bas... components/favicon_base/fallback_icon_style.cc:59: FallbackIconStyle* style) { On 2015/04/21 18:19:48, pkotwicz wrote: > Aside & for another CL: I wonder whether it would make more sense to adjust the > text color for the dominant background color rather than the background color. This was in the UI spec. I personally agree that uniform text color is more important than precisely matching the dominant color. (Which is just an approximation to begin with.)
https://codereview.chromium.org/1092873002/diff/160001/chrome/browser/favicon... File chrome/browser/favicon/large_icon_service_factory.cc (right): https://codereview.chromium.org/1092873002/diff/160001/chrome/browser/favicon... chrome/browser/favicon/large_icon_service_factory.cc:39: FaviconServiceFactory::GetForProfile(Profile::FromBrowserContext(context), Ping me if you want to discuss some more. I noticed that you pinged me offline. (Sorry that I missed it!) https://codereview.chromium.org/1092873002/diff/220001/components/favicon_bas... File components/favicon_base/favicon_types.h (right): https://codereview.chromium.org/1092873002/diff/220001/components/favicon_bas... components/favicon_base/favicon_types.h:90: scoped_ptr<FallbackIconStyle> fallback_icon_style; Ping on this comment
https://codereview.chromium.org/1092873002/diff/160001/chrome/browser/favicon... File chrome/browser/favicon/large_icon_service_factory.cc (right): https://codereview.chromium.org/1092873002/diff/160001/chrome/browser/favicon... chrome/browser/favicon/large_icon_service_factory.cc:39: FaviconServiceFactory::GetForProfile(Profile::FromBrowserContext(context), On 2015/04/21 20:13:28, pkotwicz wrote: > Ping me if you want to discuss some more. I noticed that you pinged me offline. > (Sorry that I missed it!) > NP. I think this is better. https://codereview.chromium.org/1092873002/diff/220001/components/favicon_bas... File components/favicon_base/favicon_types.h (right): https://codereview.chromium.org/1092873002/diff/220001/components/favicon_bas... components/favicon_base/favicon_types.h:90: scoped_ptr<FallbackIconStyle> fallback_icon_style; On 2015/04/21 20:13:28, pkotwicz wrote: > Ping on this comment > Sorry missed that comment. Even though I only ever use bitmap_data I'm tempted to push back on your proposal. I find it would be bad practice to discard the pixel_size or the icon_type or anything else in FaviconRawBitmapResult. The only thing that makes this struct hard to understand, IMHO, is the naming convention where we use *Result for classes that contain information about a favicon or a large icon. If it was called FaviconRawBitmap and LargeIcon the composition pattern used here would make perfect sense.
LGTM Please mention why you chose to make LargeIconService return FallbackIconStyle instead of the rendered fallback icon in the CL description. The reason is not obvious https://codereview.chromium.org/1092873002/diff/300001/components/favicon/cor... File components/favicon/core/large_icon_service.cc (right): https://codereview.chromium.org/1092873002/diff/300001/components/favicon/cor... components/favicon/core/large_icon_service.cc:64: bitmap_result.bitmap_data, result.fallback_icon_style.get()); Nit: For the sake of consistency, run the callback here and return (and remove the else). That would be more consistent with what you are doing in lines 49 - 52 https://codereview.chromium.org/1092873002/diff/300001/components/favicon_bas... File components/favicon_base/favicon_callback.h (right): https://codereview.chromium.org/1092873002/diff/300001/components/favicon_bas... components/favicon_base/favicon_callback.h:35: // fallback to use if a large icon of the desired size could not be found. "if a large icon of the desired size" -> "a sufficiently large icon" https://codereview.chromium.org/1092873002/diff/300001/components/favicon_bas... File components/favicon_base/favicon_types.h (right): https://codereview.chromium.org/1092873002/diff/300001/components/favicon_bas... components/favicon_base/favicon_types.h:8: #include <base/memory/scoped_ptr.h> Is there a reason for the weird include syntax for scoped_ptr.h https://codereview.chromium.org/1092873002/diff/300001/components/favicon_bas... components/favicon_base/favicon_types.h:80: // one was available; or the style of the fallback icon otherwise. How about: "Result returned by LargeIconService::GetLargeIconOrFallbackStyle(). Contains either the bitmap data if the favicon database has a sufficiently large favicon bitmap and the style of the fallback icon otherwise." https://codereview.chromium.org/1092873002/diff/300001/components/favicon_bas... components/favicon_base/favicon_types.h:85: // The bitmap of the large icon if available. How about: "The bitmap from the favicon database if the database has a sufficiently large one."
Patchset #13 (id:320001) has been deleted
The CQ bit was checked by beaudoin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jhawkins@chromium.org, huangs@chromium.org, kmadhusu@chromium.org, pkotwicz@chromium.org Link to the patchset: https://codereview.chromium.org/1092873002/#ps340001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1092873002/340001
https://codereview.chromium.org/1092873002/diff/300001/components/favicon/cor... File components/favicon/core/large_icon_service.cc (right): https://codereview.chromium.org/1092873002/diff/300001/components/favicon/cor... components/favicon/core/large_icon_service.cc:64: bitmap_result.bitmap_data, result.fallback_icon_style.get()); On 2015/04/21 21:41:05, pkotwicz wrote: > Nit: For the sake of consistency, run the callback here and return (and remove > the else). That would be more consistent with what you are doing in lines 49 - > 52 (Funny, this is how I did it before and huangs@ argued for that else. :)) Done. https://codereview.chromium.org/1092873002/diff/300001/components/favicon_bas... File components/favicon_base/favicon_callback.h (right): https://codereview.chromium.org/1092873002/diff/300001/components/favicon_bas... components/favicon_base/favicon_callback.h:35: // fallback to use if a large icon of the desired size could not be found. On 2015/04/21 21:41:05, pkotwicz wrote: > "if a large icon of the desired size" -> "a sufficiently large icon" Done. https://codereview.chromium.org/1092873002/diff/300001/components/favicon_bas... File components/favicon_base/favicon_types.h (right): https://codereview.chromium.org/1092873002/diff/300001/components/favicon_bas... components/favicon_base/favicon_types.h:8: #include <base/memory/scoped_ptr.h> On 2015/04/21 21:41:05, pkotwicz wrote: > Is there a reason for the weird include syntax for scoped_ptr.h Nope. Changed. Done. https://codereview.chromium.org/1092873002/diff/300001/components/favicon_bas... components/favicon_base/favicon_types.h:80: // one was available; or the style of the fallback icon otherwise. On 2015/04/21 21:41:05, pkotwicz wrote: > How about: "Result returned by LargeIconService::GetLargeIconOrFallbackStyle(). > Contains either the bitmap data if the favicon database has a sufficiently large > favicon bitmap and the style of the fallback icon otherwise." Done. https://codereview.chromium.org/1092873002/diff/300001/components/favicon_bas... components/favicon_base/favicon_types.h:85: // The bitmap of the large icon if available. On 2015/04/21 21:41:05, pkotwicz wrote: > How about: "The bitmap from the favicon database if the database has a > sufficiently large one." Done.
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by beaudoin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pkotwicz@chromium.org, jhawkins@chromium.org, huangs@chromium.org, kmadhusu@chromium.org Link to the patchset: https://codereview.chromium.org/1092873002/#ps360001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1092873002/360001
Message was sent while issue was closed.
Committed patchset #14 (id:360001)
Message was sent while issue was closed.
Patchset 14 (id:??) landed as https://crrev.com/3e75e59228ae3c4388379ed90ebea2e30ca0c646 Cr-Commit-Position: refs/heads/master@{#326325} |
