Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2784233003:
[LargeIconService] Allow decoding of images in the service (Closed)
Description[LargeIconService] Allow decoding of images in the service
Previously, LargeIconService only returned raw results. This is
advantagous for some clients that cache (or in other way need to use)
raw image data as encoding back is expensive. Still, many clients of the
service need to do the decoding on their side.
This CL adds another API function with the same behaviour that returns
a decoded gfx::Image if available. It leaves renaming the existing
function, callback, and result struct to a follow-up CL.
BUG=706787
Review-Url: https://codereview.chromium.org/2784233003
Cr-Commit-Position: refs/heads/master@{#462125}
Committed: https://chromium.googlesource.com/chromium/src/+/5fb02f69ac851beaeafbfcc84fb649744bc0944d
Patch Set 1 #Patch Set 2 : Minor changes #
Total comments: 10
Patch Set 3 : Peter's comments #Patch Set 4 : Minor changes #Patch Set 5 : Minor changes #2 #
Total comments: 2
Patch Set 6 : Peter's comments #
Total comments: 19
Patch Set 7 : Peter's comments #2 #
Total comments: 24
Patch Set 8 : Peter's comments #3 #
Total comments: 8
Patch Set 9 : Peter's comments #4 #
Messages
Total messages: 42 (28 generated)
The CQ bit was checked by jkrcal@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
jkrcal@chromium.org changed reviewers: + pkotwicz@chromium.org
Peter, could you PTAL? Apart from my own application of this new API function, I would like to move Android large_icon_bridge to this new function (as it is doing the decoding on its own and passing decoded images to java). https://codereview.chromium.org/2784233003/diff/20001/components/favicon/core... File components/favicon/core/large_icon_service_unittest.cc (right): https://codereview.chromium.org/2784233003/diff/20001/components/favicon/core... components/favicon/core/large_icon_service_unittest.cc:190: TEST_F(LargeIconServiceTest, RawBitmapSameSize) { I am duplicating most of the tests. I was thinking about parameterized tests for this but I think it would hurt readability which is not worth for k=2. WDYT? https://codereview.chromium.org/2784233003/diff/20001/components/favicon_base... File components/favicon_base/favicon_types.h (right): https://codereview.chromium.org/2784233003/diff/20001/components/favicon_base... components/favicon_base/favicon_types.h:109: explicit LargeIconImageResult(FallbackIconStyle* fallback_icon_style_in); Orthogonal to this CL: I find taking ownership by plain pointer a bit ugly but I want to keep it consistent with LargeIconResult. Wouldn't it be worth (in a separate CL) to: - change these FallbackIconStyle constructors to take unique_ptr, and - change the callbacks to pass unique_ptrs of LargeIcon(Image)Result? This would allow us to get rid of "new ..." and to use std::move out things if you need to. (see, e.g. LargeIconCache::CloneLargeIconResult) WDYT?
https://codereview.chromium.org/2784233003/diff/20001/components/favicon/core... File components/favicon/core/large_icon_service.cc (right): https://codereview.chromium.org/2784233003/diff/20001/components/favicon/core... components/favicon/core/large_icon_service.cc:329: base::Unretained(this), callback)); Can the decoding occur in the service worker? Also can we avoid: 1) decoding the PNG to SkBitmap in ResizeLargeIconOnBackgroundThreadIfValid() 2) encoding the SkBitmap to PNG in ResizeLargeIconOnBackgroundThreadIfValid() 3) decoding the PNG to SkBitmap in DecodingRawBitmapFinished() - You can pass to LargeIconWorker an enum to indicate which type of image you want and use gfx::Image to hold either a gfx::ImageSkia or PNG data. Please don't change LargeIconService::GetLargeIconOrFallbackStyle() to use a LargeIconImageCallback (instead of having both GetLargeIconOrFallbackStyle() and GetLargeIconImageOrFallbackStyle()). Large icons are large and take a while to convert from PNG data to SkBitmap (I don't want to make it easier to overlook the magical PNG image decoding which occurs in gfx::Image) I would want to rename your new method to LargeIconService::GetLargeNativeImageOrFallbackStyle() and have the callback return a gfx::NativeImage. However, this change would likely be super controversial so I won't ask you to make it https://codereview.chromium.org/2784233003/diff/20001/components/favicon/core... File components/favicon/core/large_icon_service_unittest.cc (right): https://codereview.chromium.org/2784233003/diff/20001/components/favicon/core... components/favicon/core/large_icon_service_unittest.cc:190: TEST_F(LargeIconServiceTest, RawBitmapSameSize) { I don't think that duplicating the tests is worth it. Since GetLargeIconImageOrFallbackStyle() uses GetLargeIconOrFallbackStyle(), in my opinion you just need to test GetLargeIconOrFallbackStyle() https://codereview.chromium.org/2784233003/diff/20001/components/favicon_base... File components/favicon_base/favicon_types.h (right): https://codereview.chromium.org/2784233003/diff/20001/components/favicon_base... components/favicon_base/favicon_types.h:109: explicit LargeIconImageResult(FallbackIconStyle* fallback_icon_style_in); That would be nice. However, it is not very pressing
Thanks! PTAL, again. https://codereview.chromium.org/2784233003/diff/20001/components/favicon/core... File components/favicon/core/large_icon_service.cc (right): https://codereview.chromium.org/2784233003/diff/20001/components/favicon/core... components/favicon/core/large_icon_service.cc:329: base::Unretained(this), callback)); On 2017/03/30 19:14:23, pkotwicz wrote: > Can the decoding occur in the service worker? > > Also can we avoid: > 1) decoding the PNG to SkBitmap in ResizeLargeIconOnBackgroundThreadIfValid() > 2) encoding the SkBitmap to PNG in ResizeLargeIconOnBackgroundThreadIfValid() > 3) decoding the PNG to SkBitmap in DecodingRawBitmapFinished() > > - You can pass to LargeIconWorker an enum to indicate which type of image you > want and use gfx::Image to hold either a gfx::ImageSkia or PNG data. > > Please don't change LargeIconService::GetLargeIconOrFallbackStyle() to use a > LargeIconImageCallback (instead of having both GetLargeIconOrFallbackStyle() and > GetLargeIconImageOrFallbackStyle()). Large icons are large and take a while to > convert from PNG data to SkBitmap (I don't want to make it easier to overlook > the magical PNG image decoding which occurs in gfx::Image) > > I would want to rename your new method to > LargeIconService::GetLargeNativeImageOrFallbackStyle() and have the callback > return a gfx::NativeImage. However, this change would likely be super > controversial so I won't ask you to make it ImageDecoder is not happy about being in the service worker. On top of that, this way of decoding caused weird lags in the UI. Therefore, I've gotten rid of the ImageDecoder and now use the previously implemented SkImage decoding. The difference is that I return early if resizing or encoding is not needed. I am not familiar with gfx::NativeImage so I suggest to post-pone this idea for later. https://codereview.chromium.org/2784233003/diff/20001/components/favicon/core... File components/favicon/core/large_icon_service_unittest.cc (right): https://codereview.chromium.org/2784233003/diff/20001/components/favicon/core... components/favicon/core/large_icon_service_unittest.cc:190: TEST_F(LargeIconServiceTest, RawBitmapSameSize) { On 2017/03/30 19:14:23, pkotwicz wrote: > I don't think that duplicating the tests is worth it. Since > GetLargeIconImageOrFallbackStyle() uses GetLargeIconOrFallbackStyle(), in my > opinion you just need to test GetLargeIconOrFallbackStyle() In the current version, the new function is not a wrapper anymore, so I think the tests are worth it, in the end. https://codereview.chromium.org/2784233003/diff/20001/components/favicon_base... File components/favicon_base/favicon_callback.h (right): https://codereview.chromium.org/2784233003/diff/20001/components/favicon_base... components/favicon_base/favicon_callback.h:37: // TODO(jkrcal): Rename LargeIcon* to LargeIconRawBitmap*. Peter, do you agree with this renaming (in a follow-up CL, sooner or later)? https://codereview.chromium.org/2784233003/diff/20001/components/favicon_base... File components/favicon_base/favicon_types.h (right): https://codereview.chromium.org/2784233003/diff/20001/components/favicon_base... components/favicon_base/favicon_types.h:109: explicit LargeIconImageResult(FallbackIconStyle* fallback_icon_style_in); On 2017/03/30 19:14:23, pkotwicz wrote: > That would be nice. However, it is not very pressing Acknowledged.
The CQ bit was checked by jkrcal@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by jkrcal@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
I posted a suggestion. Let me know what you think https://codereview.chromium.org/2784233003/diff/20001/components/favicon/core... File components/favicon/core/large_icon_service.cc (right): https://codereview.chromium.org/2784233003/diff/20001/components/favicon/core... components/favicon/core/large_icon_service.cc:329: base::Unretained(this), callback)); I was suggesting postponing switching to gfx::NativeImage. https://codereview.chromium.org/2784233003/diff/80001/components/favicon/core... File components/favicon/core/large_icon_service.cc (right): https://codereview.chromium.org/2784233003/diff/80001/components/favicon/core... components/favicon/core/large_icon_service.cc:169: bool return_original_size) { How about something like this: Some things about my approach that I like: - The function parameters have all of the data that the functions need to operate on. This means that the functions can be in the anonymous namespace. I think that this makes the code less confusing. - What ResizeLargeIconOnBackgroundThread() does is clearer now. The function now does just one thing - I removed some of the |in_pixel| suffixes because I *think* that it is obvious that the functions are dealing in pixel void ProcessIconOnBackgroundThread( const favicon_base::FaviconRawBitmapResult& db_result, int min_source_size, int desired_size, favicon_base::FaviconRawBitmapResult* raw_result, SkBitmap* bitmap, favicon_base::FallbackIconStyle* fallback_style) { gfx::Image image; if (IsDbResultAdequate(db_result, min_source_size)) { image = Resize(db_result, desired_size); } if (!image.IsEmpty()) { if (raw_result) { *raw_result = db_result; raw_result->pixel_size = gfx::Size(desired_size, desired_size); raw_result->bitmap_data = image.As1xPNGBytes(); } if (bitmap) { *bitmap = image.AsBitmap(); } return; } if (!fallback_icon_style) return; *fallback_icon_style = favicon_base::FallbackIconStyle(); if (db_result.is_valid()) { favicon_base::SetDominantColorAsBackground( db_result.bitmap_data, fallback_icon_style); } } bool IsDbResultAdequate( const favicon_base::FaviconRawBitmapResult& db_result, int min_source_size) { return db_result.is_valid() && db_result.pixel_size.width() == db_result.pixel_size.height()) && db_result.pixel_size.width() >= min_source_size) { } gfx::Image ResizeLargeIconOnBackgroundThread( const favicon_base::FaviconRawBitmapResult& db_result, int desired_size) { gfx::Image image = gfx::Image::CreateFrom1xPNGBytes(db_result.bitmap_data->front(), db_result.bitmap_data->size()); if (desired_size == 0 || db_result.pixel_size.width() == desired_size) { return image; } SkBitmap resized = skia::ImageOperations::Resize( image.AsBitmap(), skia::ImageOperations::RESIZE_LANCZOS3, desired_size, desired_size); return gfx::Image::CreateFrom1xBitmap(resized); }
The CQ bit was checked by jkrcal@chromium.org to run a CQ dry run
Thanks! PTAL again. https://codereview.chromium.org/2784233003/diff/80001/components/favicon/core... File components/favicon/core/large_icon_service.cc (right): https://codereview.chromium.org/2784233003/diff/80001/components/favicon/core... components/favicon/core/large_icon_service.cc:169: bool return_original_size) { On 2017/04/02 20:46:52, pkotwicz wrote: > How about something like this: > Some things about my approach that I like: > - The function parameters have all of the data that the functions need to > operate on. This means that the functions can be in the anonymous namespace. I > think that this makes the code less confusing. > - What ResizeLargeIconOnBackgroundThread() does is clearer now. The function now > does just one thing > - I removed some of the |in_pixel| suffixes because I *think* that it is obvious > that the functions are dealing in pixel > > void ProcessIconOnBackgroundThread( > const favicon_base::FaviconRawBitmapResult& db_result, > int min_source_size, > int desired_size, > favicon_base::FaviconRawBitmapResult* raw_result, > SkBitmap* bitmap, > favicon_base::FallbackIconStyle* fallback_style) { > gfx::Image image; > if (IsDbResultAdequate(db_result, min_source_size)) { > image = Resize(db_result, desired_size); > } > > if (!image.IsEmpty()) { > if (raw_result) { > *raw_result = db_result; > raw_result->pixel_size = gfx::Size(desired_size, desired_size); > raw_result->bitmap_data = image.As1xPNGBytes(); > } > if (bitmap) { > *bitmap = image.AsBitmap(); > } > return; > } > > if (!fallback_icon_style) > return; > > *fallback_icon_style = favicon_base::FallbackIconStyle(); > if (db_result.is_valid()) { > favicon_base::SetDominantColorAsBackground( > db_result.bitmap_data, fallback_icon_style); > } > } > > bool IsDbResultAdequate( > const favicon_base::FaviconRawBitmapResult& db_result, > int min_source_size) { > return db_result.is_valid() && > db_result.pixel_size.width() == db_result.pixel_size.height()) && > db_result.pixel_size.width() >= min_source_size) { > } > > gfx::Image ResizeLargeIconOnBackgroundThread( > const favicon_base::FaviconRawBitmapResult& db_result, > int desired_size) { > gfx::Image image = > gfx::Image::CreateFrom1xPNGBytes(db_result.bitmap_data->front(), > db_result.bitmap_data->size()); > > if (desired_size == 0 || db_result.pixel_size.width() == desired_size) { > return image; > } > > SkBitmap resized = skia::ImageOperations::Resize( > image.AsBitmap(), skia::ImageOperations::RESIZE_LANCZOS3, > desired_size, desired_size); > return gfx::Image::CreateFrom1xBitmap(resized); > } > Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, build hasn't started yet, builder probably lacks capacity)
Mostly nits https://codereview.chromium.org/2784233003/diff/100001/components/favicon/cor... File components/favicon/core/large_icon_service.cc (right): https://codereview.chromium.org/2784233003/diff/100001/components/favicon/cor... components/favicon/core/large_icon_service.cc:65: Nit: Please add a comment to this function https://codereview.chromium.org/2784233003/diff/100001/components/favicon/cor... components/favicon/core/large_icon_service.cc:94: if (!image.IsEmpty()) { Nit: You can nest this if() inside of the if(IsDbResultAdequate()) {} block and move the gfx::Image declaration from line 89 to line 91 https://codereview.chromium.org/2784233003/diff/100001/components/favicon/cor... components/favicon/core/large_icon_service.cc:153: SkBitmap image_result_; Nit: Rename this to |bitmap_result_| https://codereview.chromium.org/2784233003/diff/100001/components/favicon/cor... components/favicon/core/large_icon_service.cc:154: std::unique_ptr<favicon_base::FallbackIconStyle> fallback_icon_style_; Nit: You might as well make this: favicon_base::FallbackIconStyle fallback_icon_style_; https://codereview.chromium.org/2784233003/diff/100001/components/favicon/cor... components/favicon/core/large_icon_service.cc:179: const favicon_base::FaviconRawBitmapResult& raw_bitmap_result) { Can you please rename |raw_bitmap_result| to |db_result| for the sake of clarity? This will make the difference between |raw_bitmap_result| and |raw_bitmap_result_| more obvious. https://codereview.chromium.org/2784233003/diff/100001/components/favicon/cor... components/favicon/core/large_icon_service.cc:195: } For the sake of simplicity, perhaps do if (raw_bitmap_callback_) { if (raw_bitmap_result_.is_valid()) { raw_bitmap_callback_.Run( favicon_base::LargeIconResult(raw_bitmap_result_)); return; } raw_bitmap_callback_.Run( favicon_base::LargeIconResult(fallback_icon_style_.release())); return; } ... https://codereview.chromium.org/2784233003/diff/100001/components/favicon/cor... components/favicon/core/large_icon_service.cc:305: } Please add a new function GetLargeIconImageOrFallbackStyleImpl() which takes in both a favicon_base::LargeIconCallback and a favicon_base::LargeIconImageCallback in order to avoid duplicating code https://codereview.chromium.org/2784233003/diff/100001/components/favicon/cor... File components/favicon/core/large_icon_service_unittest.cc (right): https://codereview.chromium.org/2784233003/diff/100001/components/favicon/cor... components/favicon/core/large_icon_service_unittest.cc:279: TEST_F(LargeIconServiceTest, RawBitmapFallbackSinceIconTooSmall) { I don't think that it is worth duplicating the tests for GetLargeIconOrFallbackStyle() and GetLargeIconImageOrFallbackStyle() where LargeIconResult and LargeIconImageResult wrap a FallbackIconStyle If you must, a single FallbackIconStyle test for GetLargeIconImageOrFallbackStyle() should suffice. TEST_P() should help reduce the number of new code for duplicated tests
The CQ bit was checked by jkrcal@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks! PTAL, again. https://codereview.chromium.org/2784233003/diff/100001/components/favicon/cor... File components/favicon/core/large_icon_service.cc (right): https://codereview.chromium.org/2784233003/diff/100001/components/favicon/cor... components/favicon/core/large_icon_service.cc:65: On 2017/04/04 02:34:40, pkotwicz wrote: > Nit: Please add a comment to this function Done. https://codereview.chromium.org/2784233003/diff/100001/components/favicon/cor... components/favicon/core/large_icon_service.cc:94: if (!image.IsEmpty()) { On 2017/04/04 02:34:40, pkotwicz wrote: > Nit: You can nest this if() inside of the if(IsDbResultAdequate()) {} block > and move the gfx::Image declaration from line 89 to line 91 Done. https://codereview.chromium.org/2784233003/diff/100001/components/favicon/cor... components/favicon/core/large_icon_service.cc:153: SkBitmap image_result_; On 2017/04/04 02:34:41, pkotwicz wrote: > Nit: Rename this to |bitmap_result_| I'd like to keep this as there is an analogy with |raw_bitmap_callback| and |image_callback| (and the desired GetLargeIcon*RawBitmap*OrFallbackStyle and GetLargeIcon*Image*OrFallbackStyle). https://codereview.chromium.org/2784233003/diff/100001/components/favicon/cor... components/favicon/core/large_icon_service.cc:154: std::unique_ptr<favicon_base::FallbackIconStyle> fallback_icon_style_; On 2017/04/04 02:34:40, pkotwicz wrote: > Nit: You might as well make this: > > favicon_base::FallbackIconStyle fallback_icon_style_; If I drop the unique_ptr, I would have to have a "new" statement somewhere, instead because the favicon_base::LargeIconResult constructor takes ownership of the provided pointer. I prefer it this way. https://codereview.chromium.org/2784233003/diff/100001/components/favicon/cor... components/favicon/core/large_icon_service.cc:179: const favicon_base::FaviconRawBitmapResult& raw_bitmap_result) { On 2017/04/04 02:34:41, pkotwicz wrote: > Can you please rename |raw_bitmap_result| to |db_result| for the sake of > clarity? > > This will make the difference between |raw_bitmap_result| and > |raw_bitmap_result_| more obvious. Makes sense! Done. https://codereview.chromium.org/2784233003/diff/100001/components/favicon/cor... components/favicon/core/large_icon_service.cc:195: } On 2017/04/04 02:34:41, pkotwicz wrote: > For the sake of simplicity, perhaps do > > if (raw_bitmap_callback_) { > if (raw_bitmap_result_.is_valid()) { > raw_bitmap_callback_.Run( > favicon_base::LargeIconResult(raw_bitmap_result_)); > return; > } > raw_bitmap_callback_.Run( > favicon_base::LargeIconResult(fallback_icon_style_.release())); > return; > } > ... Ok, done. https://codereview.chromium.org/2784233003/diff/100001/components/favicon/cor... components/favicon/core/large_icon_service.cc:305: } On 2017/04/04 02:34:40, pkotwicz wrote: > Please add a new function GetLargeIconImageOrFallbackStyleImpl() which takes in > both a favicon_base::LargeIconCallback and a > favicon_base::LargeIconImageCallback in order to avoid duplicating code Done. (Named GetLargeIcon**OrFallbackStyleImpl to match to both GetLargeIcon*RawBitmap*OrFallbackStyle and GetLargeIcon*Image*OrFallbackStyle in the future.) https://codereview.chromium.org/2784233003/diff/100001/components/favicon/cor... File components/favicon/core/large_icon_service_unittest.cc (right): https://codereview.chromium.org/2784233003/diff/100001/components/favicon/cor... components/favicon/core/large_icon_service_unittest.cc:279: TEST_F(LargeIconServiceTest, RawBitmapFallbackSinceIconTooSmall) { On 2017/04/04 02:34:41, pkotwicz wrote: > I don't think that it is worth duplicating the tests for > GetLargeIconOrFallbackStyle() and GetLargeIconImageOrFallbackStyle() where > LargeIconResult and LargeIconImageResult wrap a FallbackIconStyle > > If you must, a single FallbackIconStyle test for > GetLargeIconImageOrFallbackStyle() should suffice. > > TEST_P() should help reduce the number of new code for duplicated tests Okay, I moved the getter tests into a subclass, using TEST_P. I still want to keep all the tests duplicated, with the TEST_P, I think it makes sense.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Mostly test nits https://codereview.chromium.org/2784233003/diff/100001/components/favicon/cor... File components/favicon/core/large_icon_service.cc (right): https://codereview.chromium.org/2784233003/diff/100001/components/favicon/cor... components/favicon/core/large_icon_service.cc:153: SkBitmap image_result_; It is confusing to have a variable which stores an SkBitmap be name *image* https://codereview.chromium.org/2784233003/diff/100001/components/favicon/cor... components/favicon/core/large_icon_service.cc:154: std::unique_ptr<favicon_base::FallbackIconStyle> fallback_icon_style_; I understand now https://codereview.chromium.org/2784233003/diff/120001/components/favicon/cor... File components/favicon/core/large_icon_service.cc (right): https://codereview.chromium.org/2784233003/diff/120001/components/favicon/cor... components/favicon/core/large_icon_service.cc:71: // as a decoded bitmap. How about: "Wraps the PNG data in |db_result| in a gfx::Image. If |desired_size| is not 0, the image gets decoded and resized to |desired_size| (in px). Must run on a background thread in production." My version: - Makes clear what the function does in the first sentence of the comment. It is possible to read only the first few sentences and mostly understand what the function does https://codereview.chromium.org/2784233003/diff/120001/components/favicon/cor... components/favicon/core/large_icon_service.cc:203: // Return the large icon, if we have the image. This comment is redundant. It does not say anything that is not obvious from the code https://codereview.chromium.org/2784233003/diff/120001/components/favicon/cor... components/favicon/core/large_icon_service.cc:209: // Return fallback style, otherwise This comment is redundant. It does not say anything that is not obvious from the code https://codereview.chromium.org/2784233003/diff/120001/components/favicon/cor... components/favicon/core/large_icon_service.cc:215: // Otherwise we assume |image_callback_| is provided. Return decoded results. This comment is redundant. It does not say anything that is not obvious from the code https://codereview.chromium.org/2784233003/diff/120001/components/favicon/cor... components/favicon/core/large_icon_service.cc:297: LargeIconService::GetLargeIconOrFallbackStyleImpl( Please reorder this function to match order in .h file https://codereview.chromium.org/2784233003/diff/120001/components/favicon/cor... File components/favicon/core/large_icon_service.h (right): https://codereview.chromium.org/2784233003/diff/120001/components/favicon/cor... components/favicon/core/large_icon_service.h:59: // Behaves the same as GetLargeIconOrFallbackStyle, only returns the large Nit: GetLargeIconOrFallbackStyle -> GetLargeIconOrFallbackStyle() https://codereview.chromium.org/2784233003/diff/120001/components/favicon/cor... File components/favicon/core/large_icon_service_unittest.cc (right): https://codereview.chromium.org/2784233003/diff/120001/components/favicon/cor... components/favicon/core/large_icon_service_unittest.cc:246: bool get_raw_bitmap) { - Maybe pass in |expected_bitmap| and |expected_bitmap| as parameters and call this method RunTest() - You should be able to remove the |get_raw_bitmap| parameter. And call GetParam() within this method https://codereview.chromium.org/2784233003/diff/120001/components/favicon/cor... components/favicon/core/large_icon_service_unittest.cc:260: base::RunLoop().RunUntilIdle(); Nit: Make this base::RunLoop().Run() instead and make RawBitmapResultCallback() and ImageResultCallback() call the base::RunLoop::QuitClosure(). This will enable you to get rid of |is_callback_invoked_| https://codereview.chromium.org/2784233003/diff/120001/components/favicon/cor... components/favicon/core/large_icon_service_unittest.cc:295: } Nit: Can you extract the common functionality of RawBitmapResultCallback() and ImageResultCallback() into a shared function? e.g. void CheckCallback(bool is_bitmap_valid, const gfx::Size& bitmap_size, favicon_base::FallbackIconStyle* fallback_style) {} https://codereview.chromium.org/2784233003/diff/120001/components/favicon/cor... components/favicon/core/large_icon_service_unittest.cc:301: EXPECT_CALL(mock_favicon_service_, Can this be ON_CALL() and WillByDefault() instead?
PTAL, again! https://codereview.chromium.org/2784233003/diff/100001/components/favicon/cor... File components/favicon/core/large_icon_service.cc (right): https://codereview.chromium.org/2784233003/diff/100001/components/favicon/cor... components/favicon/core/large_icon_service.cc:153: SkBitmap image_result_; On 2017/04/04 12:43:42, pkotwicz wrote: > It is confusing to have a variable which stores an SkBitmap be name *image* Ok, done. https://codereview.chromium.org/2784233003/diff/120001/components/favicon/cor... File components/favicon/core/large_icon_service.cc (right): https://codereview.chromium.org/2784233003/diff/120001/components/favicon/cor... components/favicon/core/large_icon_service.cc:71: // as a decoded bitmap. On 2017/04/04 12:43:42, pkotwicz wrote: > How about: "Wraps the PNG data in |db_result| in a gfx::Image. If |desired_size| > is not 0, the image gets decoded and resized to |desired_size| (in px). Must run > on a background thread in production." > > My version: > - Makes clear what the function does in the first sentence of the comment. It is > possible to read only the first few sentences and mostly understand what the > function does Ok, done. https://codereview.chromium.org/2784233003/diff/120001/components/favicon/cor... components/favicon/core/large_icon_service.cc:203: // Return the large icon, if we have the image. On 2017/04/04 12:43:43, pkotwicz wrote: > This comment is redundant. It does not say anything that is not obvious from the > code Done. https://codereview.chromium.org/2784233003/diff/120001/components/favicon/cor... components/favicon/core/large_icon_service.cc:209: // Return fallback style, otherwise On 2017/04/04 12:43:43, pkotwicz wrote: > This comment is redundant. It does not say anything that is not obvious from the > code Done. https://codereview.chromium.org/2784233003/diff/120001/components/favicon/cor... components/favicon/core/large_icon_service.cc:215: // Otherwise we assume |image_callback_| is provided. Return decoded results. On 2017/04/04 12:43:42, pkotwicz wrote: > This comment is redundant. It does not say anything that is not obvious from the > code Done. https://codereview.chromium.org/2784233003/diff/120001/components/favicon/cor... components/favicon/core/large_icon_service.cc:297: LargeIconService::GetLargeIconOrFallbackStyleImpl( On 2017/04/04 12:43:42, pkotwicz wrote: > Please reorder this function to match order in .h file Done. https://codereview.chromium.org/2784233003/diff/120001/components/favicon/cor... File components/favicon/core/large_icon_service.h (right): https://codereview.chromium.org/2784233003/diff/120001/components/favicon/cor... components/favicon/core/large_icon_service.h:59: // Behaves the same as GetLargeIconOrFallbackStyle, only returns the large On 2017/04/04 12:43:43, pkotwicz wrote: > Nit: GetLargeIconOrFallbackStyle -> GetLargeIconOrFallbackStyle() Done. https://codereview.chromium.org/2784233003/diff/120001/components/favicon/cor... File components/favicon/core/large_icon_service_unittest.cc (right): https://codereview.chromium.org/2784233003/diff/120001/components/favicon/cor... components/favicon/core/large_icon_service_unittest.cc:246: bool get_raw_bitmap) { On 2017/04/04 12:43:43, pkotwicz wrote: > - Maybe pass in |expected_bitmap| and |expected_bitmap| as parameters and call > this method RunTest() > - You should be able to remove the |get_raw_bitmap| parameter. And call > GetParam() within this method As regards the first point: I want to have some expectations in the tests. I've rewritten the tests so that the callback only stores the results and the tests now verify expectations. In my personal opinion, it is much clearer now. As regards the second point: done! https://codereview.chromium.org/2784233003/diff/120001/components/favicon/cor... components/favicon/core/large_icon_service_unittest.cc:260: base::RunLoop().RunUntilIdle(); On 2017/04/04 12:43:43, pkotwicz wrote: > Nit: Make this base::RunLoop().Run() instead and make RawBitmapResultCallback() > and ImageResultCallback() call the base::RunLoop::QuitClosure(). This will > enable you to get rid of |is_callback_invoked_| I got rid of the |is_callback_invoked| without this change. I do not see any other benefit of this change (subject to personal taste). https://codereview.chromium.org/2784233003/diff/120001/components/favicon/cor... components/favicon/core/large_icon_service_unittest.cc:295: } On 2017/04/04 12:43:43, pkotwicz wrote: > Nit: Can you extract the common functionality of RawBitmapResultCallback() and > ImageResultCallback() into a shared function? > > e.g. > void CheckCallback(bool is_bitmap_valid, const gfx::Size& bitmap_size, > favicon_base::FallbackIconStyle* fallback_style) {} I extracted a common part of that. https://codereview.chromium.org/2784233003/diff/120001/components/favicon/cor... components/favicon/core/large_icon_service_unittest.cc:301: EXPECT_CALL(mock_favicon_service_, On 2017/04/04 12:43:43, pkotwicz wrote: > Can this be ON_CALL() and WillByDefault() instead? Done.
Just test nits! https://codereview.chromium.org/2784233003/diff/120001/components/favicon/cor... File components/favicon/core/large_icon_service_unittest.cc (right): https://codereview.chromium.org/2784233003/diff/120001/components/favicon/cor... components/favicon/core/large_icon_service_unittest.cc:246: bool get_raw_bitmap) { I like your new version too! https://codereview.chromium.org/2784233003/diff/120001/components/favicon/cor... components/favicon/core/large_icon_service_unittest.cc:260: base::RunLoop().RunUntilIdle(); Cool: looks like using the QuitClosure is unnecessary. https://codereview.chromium.org/2784233003/diff/140001/components/favicon/cor... File components/favicon/core/large_icon_service_unittest.cc (right): https://codereview.chromium.org/2784233003/diff/140001/components/favicon/cor... components/favicon/core/large_icon_service_unittest.cc:320: EXPECT_THAT(*returned_bitmap_size_, Eq(gfx::Size(24, 24))); Nit: Can you use the more ubiquitous EXPECT_EQ(*returned_bitmap_size_, gfx::Size(24, 24)); here? https://codereview.chromium.org/2784233003/diff/140001/components/favicon/cor... components/favicon/core/large_icon_service_unittest.cc:321: EXPECT_THAT(returned_fallback_style_, IsNull()); Nit: Can you use the more ubiquitous EXPECT_EQ(returned_fallback_style_.get(), nullptr); here? https://codereview.chromium.org/2784233003/diff/140001/components/favicon/cor... components/favicon/core/large_icon_service_unittest.cc:353: EXPECT_THAT(*returned_fallback_style_, HasBackgroundColor(kTestColor)); Can we do EXPECT_TRUE(HasNonDefaultBackgroundColor(*returned_fallback_style_)); instead? https://codereview.chromium.org/2784233003/diff/140001/components/favicon/cor... components/favicon/core/large_icon_service_unittest.cc:367: EXPECT_THAT(*returned_fallback_style_, HasDefaultBackgroundColor()); Can we do: EXPECT_TRUE(returned_fallback_style_->is_default_background_color) instead?
The CQ bit was checked by jkrcal@chromium.org to run a CQ dry run
Peter, could you PTAL, again? https://codereview.chromium.org/2784233003/diff/120001/components/favicon/cor... File components/favicon/core/large_icon_service_unittest.cc (right): https://codereview.chromium.org/2784233003/diff/120001/components/favicon/cor... components/favicon/core/large_icon_service_unittest.cc:246: bool get_raw_bitmap) { On 2017/04/04 17:47:15, pkotwicz wrote: > I like your new version too! Acknowledged. https://codereview.chromium.org/2784233003/diff/120001/components/favicon/cor... components/favicon/core/large_icon_service_unittest.cc:260: base::RunLoop().RunUntilIdle(); On 2017/04/04 17:47:15, pkotwicz wrote: > Cool: looks like using the QuitClosure is unnecessary. Acknowledged. https://codereview.chromium.org/2784233003/diff/140001/components/favicon/cor... File components/favicon/core/large_icon_service_unittest.cc (right): https://codereview.chromium.org/2784233003/diff/140001/components/favicon/cor... components/favicon/core/large_icon_service_unittest.cc:320: EXPECT_THAT(*returned_bitmap_size_, Eq(gfx::Size(24, 24))); On 2017/04/04 17:47:15, pkotwicz wrote: > Nit: Can you use the more ubiquitous > EXPECT_EQ(*returned_bitmap_size_, gfx::Size(24, 24)); > here? Done. https://codereview.chromium.org/2784233003/diff/140001/components/favicon/cor... components/favicon/core/large_icon_service_unittest.cc:321: EXPECT_THAT(returned_fallback_style_, IsNull()); On 2017/04/04 17:47:15, pkotwicz wrote: > Nit: Can you use the more ubiquitous > EXPECT_EQ(returned_fallback_style_.get(), nullptr); > here? Done. https://codereview.chromium.org/2784233003/diff/140001/components/favicon/cor... components/favicon/core/large_icon_service_unittest.cc:353: EXPECT_THAT(*returned_fallback_style_, HasBackgroundColor(kTestColor)); On 2017/04/04 17:47:15, pkotwicz wrote: > Can we do > > EXPECT_TRUE(HasNonDefaultBackgroundColor(*returned_fallback_style_)); > > instead? I'd rather make the color explicit. Anyway, rewritten to EXPECT_TRUE. https://codereview.chromium.org/2784233003/diff/140001/components/favicon/cor... components/favicon/core/large_icon_service_unittest.cc:367: EXPECT_THAT(*returned_fallback_style_, HasDefaultBackgroundColor()); On 2017/04/04 17:47:15, pkotwicz wrote: > Can we do: > > EXPECT_TRUE(returned_fallback_style_->is_default_background_color) > > instead? Yep, this sounds simpler!
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
LGTM!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by jkrcal@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 160001, "attempt_start_ts": 1491411530489760,
"parent_rev": "b329f4acd50f60d7e98ecb29f4a9356f2e1a3414", "commit_rev":
"5fb02f69ac851beaeafbfcc84fb649744bc0944d"}
Message was sent while issue was closed.
Description was changed from ========== [LargeIconService] Allow decoding of images in the service Previously, LargeIconService only returned raw results. This is advantagous for some clients that cache (or in other way need to use) raw image data as encoding back is expensive. Still, many clients of the service need to do the decoding on their side. This CL adds another API function with the same behaviour that returns a decoded gfx::Image if available. It leaves renaming the existing function, callback, and result struct to a follow-up CL. BUG=706787 ========== to ========== [LargeIconService] Allow decoding of images in the service Previously, LargeIconService only returned raw results. This is advantagous for some clients that cache (or in other way need to use) raw image data as encoding back is expensive. Still, many clients of the service need to do the decoding on their side. This CL adds another API function with the same behaviour that returns a decoded gfx::Image if available. It leaves renaming the existing function, callback, and result struct to a follow-up CL. BUG=706787 Review-Url: https://codereview.chromium.org/2784233003 Cr-Commit-Position: refs/heads/master@{#462125} Committed: https://chromium.googlesource.com/chromium/src/+/5fb02f69ac851beaeafbfcc84fb6... ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001) as https://chromium.googlesource.com/chromium/src/+/5fb02f69ac851beaeafbfcc84fb6... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
