Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
16977007:
Only load theme images for the scale factors that use them (Closed)
|
Created:
7 years, 6 months ago by sschmitz Modified:
7 years, 6 months ago Reviewers:
pkotwicz CC:
chromium-reviews Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionOnly load theme images for the scale factors that use them
This CL implements delayed decoding of PNG data for some theme images and delayed
scaling for other scale factors until needed. Created new child of ImageSkiaSource
in BrowserThemePack for this purpose.
BUG=243831
R=pkotwicz@chromium.org
TEST=manual
out/Debug/unit_tests --gtest_filter='BrowserThemePackTest.HiDpiThemeTest'
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=207727
Patch Set 1 #
Total comments: 1
Patch Set 2 : delay GetRawMemory #
Total comments: 24
Patch Set 3 : review #Patch Set 4 : store png in source #
Total comments: 12
Patch Set 5 : add unit test #Patch Set 6 : combine unit tests #
Total comments: 1
Patch Set 7 : revert unit test #Messages
Total messages: 23 (0 generated)
Comment on the bug when you want a review :)
I realized that comment #2 is misleading. Comment on the CL when you want a review.
On 2013/06/14 15:17:05, pkotwicz wrote: > I realized that comment #2 is misleading. Comment on the CL when you want a > review. Yes, please review. I updated the description. I think that was misleading.
Looks pretty good. Please ping me if you want my (maybe wrong) opinion on different approaches in implementing my request https://codereview.chromium.org/16977007/diff/1/chrome/browser/themes/browser... File chrome/browser/themes/browser_theme_pack.cc (right): https://codereview.chromium.org/16977007/diff/1/chrome/browser/themes/browser... chrome/browser/themes/browser_theme_pack.cc:490: ThemeImagePngSource() {} Ideally we should not even cache the PNG data for the unused scale factors. You should be able to do this by passing a weak ptr to BrowserThemePack and the idr_id (BrowserThemePack does not support weak ptrs so you will have to make it do so). - There may be a good way of splitting up BrowserThemePack to avoid the nastiness of it being both ref counted and support weak ptrs but I cannot think of one right now. You should be able to use ImageSkia(ImageSkiaSource* source, ui::ScaleFactor scale_factor); in BrowserThemePack::GetImageNamed().
To be more specific, you should be able to use ImageSkia::ImageSkia(ImageSkiaSource* source, ui::SCALE_FACTOR_100P) in BrowserThemePack::GetImageNamed()
LGTM with nits https://codereview.chromium.org/16977007/diff/11001/chrome/browser/themes/bro... File chrome/browser/themes/browser_theme_pack.cc (right): https://codereview.chromium.org/16977007/diff/11001/chrome/browser/themes/bro... chrome/browser/themes/browser_theme_pack.cc:483: // An ImageSkiaSouce that delays decoding PNG data into bitmaps until Nit: ImageSkiaSource https://codereview.chromium.org/16977007/diff/11001/chrome/browser/themes/bro... chrome/browser/themes/browser_theme_pack.cc:500: BitmapMap::const_iterator exact_bitmap = bitmap_map_.find(scale_factor); I personally like suffixing iterator variable names with "_it" to make it clear that they are iterators. For instance, I find it confusing that |available_bitmap| is an iterator but |available_scale_factor| is not https://codereview.chromium.org/16977007/diff/11001/chrome/browser/themes/bro... chrome/browser/themes/browser_theme_pack.cc:505: return gfx::ImageSkiaRep(SkBitmap(), scale_factor); Nit: just use the empty constructor for ImageSkiaRep() https://codereview.chromium.org/16977007/diff/11001/chrome/browser/themes/bro... chrome/browser/themes/browser_theme_pack.cc:507: // Look up the scale factor in the browser theme pack. If found, How about: "Look up the raw PNG data for |idr_id_| and |scale_factor| in the browser theme pack. If found ..." https://codereview.chromium.org/16977007/diff/11001/chrome/browser/themes/bro... chrome/browser/themes/browser_theme_pack.cc:517: return gfx::ImageSkiaRep(SkBitmap(), scale_factor); Nit: use the empty constructor https://codereview.chromium.org/16977007/diff/11001/chrome/browser/themes/bro... chrome/browser/themes/browser_theme_pack.cc:523: // No exact match was found, find an available png with highest scale. Nit: the highest scale factor https://codereview.chromium.org/16977007/diff/11001/chrome/browser/themes/bro... chrome/browser/themes/browser_theme_pack.cc:534: // the corresponding png and store the result in bitmap map. Nit: "the bitmap map" or "|bitmap_map_|". https://codereview.chromium.org/16977007/diff/11001/chrome/browser/themes/bro... chrome/browser/themes/browser_theme_pack.cc:551: gfx::Size scaled_size = gfx::ToCeiledSize( Nit: Pull out the resizing code into a function in the anonymous namespace. Call the method CreateLowQualityResizedBitmap(); https://codereview.chromium.org/16977007/diff/11001/chrome/browser/themes/bro... chrome/browser/themes/browser_theme_pack.cc:574: BitmapMap bitmap_map_; Nit: Move the typedef to the line above |bitmap_map_|. https://codereview.chromium.org/16977007/diff/11001/chrome/browser/themes/bro... chrome/browser/themes/browser_theme_pack.cc:856: gfx::ImageSkia image_skia( Nit: Comment that the gfx::ImageSkia machinery will make |ret| IsEmpty() if there is no data for |idr_id| in the data pack. https://codereview.chromium.org/16977007/diff/11001/chrome/browser/themes/bro... chrome/browser/themes/browser_theme_pack.cc:885: base::RefCountedMemory* BrowserThemePack::GetRawDataWithHighestScale( This is only used by ThemeImagePngSource. Can this be a member of ThemeImagePNGSource? You may need to add an accessor for |scale_factors_|. https://codereview.chromium.org/16977007/diff/11001/chrome/browser/themes/bro... chrome/browser/themes/browser_theme_pack.cc:938: scale_factors_ = ui::GetSupportedScaleFactors(); Nit: The result of ui::GetSupportedScaleFactors() is already sorted in ascending order.
Thanks for your quick and excellent review. I implemented all suggested changes. However, (as I indicated in a separate email), the WeakPtr machinery has a problem. We may need to go back to storing all available PNGs in the Source. https://codereview.chromium.org/16977007/diff/11001/chrome/browser/themes/bro... File chrome/browser/themes/browser_theme_pack.cc (right): https://codereview.chromium.org/16977007/diff/11001/chrome/browser/themes/bro... chrome/browser/themes/browser_theme_pack.cc:483: // An ImageSkiaSouce that delays decoding PNG data into bitmaps until On 2013/06/18 15:23:43, pkotwicz wrote: > Nit: ImageSkiaSource Done. https://codereview.chromium.org/16977007/diff/11001/chrome/browser/themes/bro... chrome/browser/themes/browser_theme_pack.cc:500: BitmapMap::const_iterator exact_bitmap = bitmap_map_.find(scale_factor); On 2013/06/18 15:23:43, pkotwicz wrote: > I personally like suffixing iterator variable names with "_it" to make it clear > that they are iterators. > > For instance, I find it confusing that |available_bitmap| is an iterator but > |available_scale_factor| is not Done. https://codereview.chromium.org/16977007/diff/11001/chrome/browser/themes/bro... chrome/browser/themes/browser_theme_pack.cc:505: return gfx::ImageSkiaRep(SkBitmap(), scale_factor); On 2013/06/18 15:23:43, pkotwicz wrote: > Nit: just use the empty constructor for ImageSkiaRep() Done. https://codereview.chromium.org/16977007/diff/11001/chrome/browser/themes/bro... chrome/browser/themes/browser_theme_pack.cc:507: // Look up the scale factor in the browser theme pack. If found, On 2013/06/18 15:23:43, pkotwicz wrote: > How about: "Look up the raw PNG data for |idr_id_| and |scale_factor| in the > browser theme pack. If found ..." Done. https://codereview.chromium.org/16977007/diff/11001/chrome/browser/themes/bro... chrome/browser/themes/browser_theme_pack.cc:517: return gfx::ImageSkiaRep(SkBitmap(), scale_factor); On 2013/06/18 15:23:43, pkotwicz wrote: > Nit: use the empty constructor Done. https://codereview.chromium.org/16977007/diff/11001/chrome/browser/themes/bro... chrome/browser/themes/browser_theme_pack.cc:523: // No exact match was found, find an available png with highest scale. On 2013/06/18 15:23:43, pkotwicz wrote: > Nit: the highest scale factor I changed to: "for the scale factor that corresponds to the highest scale." https://codereview.chromium.org/16977007/diff/11001/chrome/browser/themes/bro... chrome/browser/themes/browser_theme_pack.cc:534: // the corresponding png and store the result in bitmap map. On 2013/06/18 15:23:43, pkotwicz wrote: > Nit: "the bitmap map" or "|bitmap_map_|". Done. https://codereview.chromium.org/16977007/diff/11001/chrome/browser/themes/bro... chrome/browser/themes/browser_theme_pack.cc:551: gfx::Size scaled_size = gfx::ToCeiledSize( On 2013/06/18 15:23:43, pkotwicz wrote: > Nit: Pull out the resizing code into a function in the anonymous namespace. Call > the method CreateLowQualityResizedBitmap(); Done. https://codereview.chromium.org/16977007/diff/11001/chrome/browser/themes/bro... chrome/browser/themes/browser_theme_pack.cc:574: BitmapMap bitmap_map_; On 2013/06/18 15:23:43, pkotwicz wrote: > Nit: Move the typedef to the line above |bitmap_map_|. Done. https://codereview.chromium.org/16977007/diff/11001/chrome/browser/themes/bro... chrome/browser/themes/browser_theme_pack.cc:856: gfx::ImageSkia image_skia( On 2013/06/18 15:23:43, pkotwicz wrote: > Nit: Comment that the gfx::ImageSkia machinery will make |ret| IsEmpty() if > there is no data for |idr_id| in the data pack. Done. https://codereview.chromium.org/16977007/diff/11001/chrome/browser/themes/bro... chrome/browser/themes/browser_theme_pack.cc:885: base::RefCountedMemory* BrowserThemePack::GetRawDataWithHighestScale( On 2013/06/18 15:23:43, pkotwicz wrote: > This is only used by ThemeImagePngSource. Can this be a member of > ThemeImagePNGSource? You may need to add an accessor for |scale_factors_|. Done. https://codereview.chromium.org/16977007/diff/11001/chrome/browser/themes/bro... chrome/browser/themes/browser_theme_pack.cc:938: scale_factors_ = ui::GetSupportedScaleFactors(); On 2013/06/18 15:23:43, pkotwicz wrote: > Nit: The result of ui::GetSupportedScaleFactors() is already sorted in ascending > order. Done.
After some thought, I do not think there is an easy way of avoiding storing all of the PNGs in the source. Let's go back to that
ok... png_map_ inside the source is back.
LGTM https://codereview.chromium.org/16977007/diff/21001/chrome/browser/themes/bro... File chrome/browser/themes/browser_theme_pack.cc (right): https://codereview.chromium.org/16977007/diff/21001/chrome/browser/themes/bro... chrome/browser/themes/browser_theme_pack.cc:446: Nit: Add a comment for this method https://codereview.chromium.org/16977007/diff/21001/chrome/browser/themes/bro... chrome/browser/themes/browser_theme_pack.cc:447: SkBitmap CreateLowQualityResizedBitmap(const SkBitmap& available_bitmap, Nit: Name this |source_bitmap| https://codereview.chromium.org/16977007/diff/21001/chrome/browser/themes/bro... chrome/browser/themes/browser_theme_pack.cc:485: resized_bitmap.setConfig(SkBitmap::kARGB_8888_Config, size.width(), You should be able to invoke CreateLowQualityResizedBitmap() here too https://codereview.chromium.org/16977007/diff/21001/chrome/browser/themes/bro... chrome/browser/themes/browser_theme_pack.cc:509: ThemeImagePngSource() {} Nit: Could you pass std::map<ui::ScaleFactor, base::RefCountedMemory> in the constructor? (That would allow you to get rid of AddPng() ) https://codereview.chromium.org/16977007/diff/21001/chrome/browser/themes/bro... chrome/browser/themes/browser_theme_pack.cc:523: BitmapMap::const_iterator exact_bitmap_it = bitmap_map_.find(scale_factor); Technically, this check here is unnecessary because GetImageForScale() will only be called once per scale factor.
https://codereview.chromium.org/16977007/diff/21001/chrome/browser/themes/bro... File chrome/browser/themes/browser_theme_pack.cc (right): https://codereview.chromium.org/16977007/diff/21001/chrome/browser/themes/bro... chrome/browser/themes/browser_theme_pack.cc:555: Shouldn't you check if there is already a bitmap in |bitmap_map_|?
Implemented review and added a unit test. The unit test is disabled until the test data is available. https://codereview.chromium.org/16977007/diff/21001/chrome/browser/themes/bro... File chrome/browser/themes/browser_theme_pack.cc (right): https://codereview.chromium.org/16977007/diff/21001/chrome/browser/themes/bro... chrome/browser/themes/browser_theme_pack.cc:446: On 2013/06/19 16:20:32, pkotwicz wrote: > Nit: Add a comment for this method Done. https://codereview.chromium.org/16977007/diff/21001/chrome/browser/themes/bro... chrome/browser/themes/browser_theme_pack.cc:447: SkBitmap CreateLowQualityResizedBitmap(const SkBitmap& available_bitmap, On 2013/06/19 16:20:32, pkotwicz wrote: > Nit: Name this |source_bitmap| Done. https://codereview.chromium.org/16977007/diff/21001/chrome/browser/themes/bro... chrome/browser/themes/browser_theme_pack.cc:485: resized_bitmap.setConfig(SkBitmap::kARGB_8888_Config, size.width(), On 2013/06/19 16:20:32, pkotwicz wrote: > You should be able to invoke CreateLowQualityResizedBitmap() here too Done. https://codereview.chromium.org/16977007/diff/21001/chrome/browser/themes/bro... chrome/browser/themes/browser_theme_pack.cc:509: ThemeImagePngSource() {} On 2013/06/19 16:20:32, pkotwicz wrote: > Nit: Could you pass std::map<ui::ScaleFactor, base::RefCountedMemory> in the > constructor? (That would allow you to get rid of AddPng() ) Done. https://codereview.chromium.org/16977007/diff/21001/chrome/browser/themes/bro... chrome/browser/themes/browser_theme_pack.cc:523: BitmapMap::const_iterator exact_bitmap_it = bitmap_map_.find(scale_factor); On 2013/06/19 16:20:32, pkotwicz wrote: > Technically, this check here is unnecessary because GetImageForScale() will only > be called once per scale factor. It feel safer to keep it. Then we're less coupled to how this class is used. https://codereview.chromium.org/16977007/diff/21001/chrome/browser/themes/bro... chrome/browser/themes/browser_theme_pack.cc:555: On 2013/06/19 16:23:28, pkotwicz wrote: > Shouldn't you check if there is already a bitmap in |bitmap_map_|? Yes. Done.
Combined the two unit test. Once the needed image data (binary) lands, I will uncomment some lines in the unittest and update the manifest to activate the new testing. (It works on my desktop)
https://codereview.chromium.org/16977007/diff/39001/chrome/browser/themes/bro... File chrome/browser/themes/browser_theme_pack.cc (right): https://codereview.chromium.org/16977007/diff/39001/chrome/browser/themes/bro... chrome/browser/themes/browser_theme_pack.cc:531: return gfx::ImageSkiaRep(bitmap, scale_factor); After reading through the unittests, I would rather keep things simple and resize in this case too. I do not see a really compelling case for not resizing here.
Also, I believe that you are missing the modified json file in this CL
On 2013/06/20 02:00:23, pkotwicz wrote: > https://codereview.chromium.org/16977007/diff/39001/chrome/browser/themes/bro... > File chrome/browser/themes/browser_theme_pack.cc (right): > > https://codereview.chromium.org/16977007/diff/39001/chrome/browser/themes/bro... > chrome/browser/themes/browser_theme_pack.cc:531: return > gfx::ImageSkiaRep(bitmap, scale_factor); > After reading through the unittests, I would rather keep things simple and > resize in this case too. > I do not see a really compelling case for not resizing here. I don't understand. What do you mean by resizing?
On 2013/06/20 14:16:05, pkotwicz wrote: > Also, I believe that you are missing the modified json file in this CL You are correct. However, I can only modify the manifest once the new image file has landed. (Otherwise the parsing of the manifest fails). I plan to land this CL and dcommit the image file in parallel. Once the image file is in the repo, I will upload a CL with the manifest change and uncommenting stuff in the unit test.
On 2013/06/20 14:21:40, sschmitz wrote: > On 2013/06/20 14:16:05, pkotwicz wrote: > > Also, I believe that you are missing the modified json file in this CL > > You are correct. However, I can only modify the manifest once the new image file > has landed. (Otherwise the parsing of the manifest fails). > > I plan to land this CL and dcommit the image file in parallel. Once the image > file is in the repo, I will upload a CL with the manifest change and > uncommenting stuff in the unit test. I plan to remove the unit-test completely and not do the resizing. Maybe this needs more thought at a later time. Troublesome scenario: Consider this case where user specifies an image for two scales. Let's say w is the image width and x is the x-coordinate for some feature (Imagine height and y are similar) 100% w=120 x=6 200% w=180 x=12 So the DIP-w is 120. So the DIP-x is 6. If we resize the 200% image to its DIP equivalent width, we'd scale it to 240. That would move x from 12 to 16 which is a DIP-x of 8 (different from 6). I'm not sure that is what we want. Ideally the author's will size scaled images properly, then it does not matter. But if they don't we have to competing principles: 1. scale only based on scale factor, then features are preserved (so to speak) 2. scale by DIP size, then features may be stretched or squasched (so to speak)
reverted the unit test.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sschmitz@chromium.org/16977007/53001
Message was sent while issue was closed.
Change committed as 207727 |
