Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(737)

Issue 10701087: chromeos: Fix pixelated icons in app list and launcher (part 2) (Closed)

Created:
8 years, 5 months ago by xiyuan
Modified:
8 years, 4 months ago
CC:
chromium-reviews, mihaip-chromium-reviews_chromium.org, Ben Goodger (Google)
Visibility:
Public.

Description

chromeos: Fix pixelated icons in app list and launcher (part 2) Add an ExtensionIconImage, which loads image that supports DIP by having a special ImageSkiaSource that makes ImageLoadingTracker to load image for additional DIP scale. BUG=131738, 131739 TEST=None. Wait for the last CL to update laucher/app list code to use LoadImageInDIP to verify.

Patch Set 1 #

Patch Set 2 : #

Patch Set 3 : address oshima's comments from http://codereview.chromium.org/10699065/ #

Total comments: 7

Patch Set 4 : for comments in #3, add an ImageSource and makes ImageLoadingTracker auto load image for additional… #

Total comments: 34

Patch Set 5 : for pkotwicz and oshima's comments in #4 #

Patch Set 6 : Defer actual resource loading until ImageSource gets request #

Total comments: 15

Patch Set 7 : addres comments in #6 #

Patch Set 8 : add more tests #

Total comments: 17

Patch Set 9 : for pkotwicz's comments in #8 #

Patch Set 10 : rebase and refactor #

Total comments: 6

Patch Set 11 : for comments in #10 #

Total comments: 17
Unified diffs Side-by-side diffs Delta from patch set Stats (+687 lines, -113 lines) Patch
M chrome/browser/extensions/app_shortcut_manager.cc View 1 2 3 4 5 6 7 8 9 2 chunks +11 lines, -5 lines 0 comments Download
A chrome/browser/extensions/extension_icon_image.h View 1 2 3 4 5 6 7 8 9 10 1 chunk +87 lines, -0 lines 3 comments Download
A chrome/browser/extensions/extension_icon_image.cc View 1 2 3 4 5 6 7 8 9 10 1 chunk +175 lines, -0 lines 5 comments Download
A chrome/browser/extensions/extension_icon_image_delegate.h View 1 2 3 4 5 6 7 8 9 1 chunk +19 lines, -0 lines 1 comment Download
A chrome/browser/extensions/extension_icon_image_unittest.cc View 1 2 3 4 5 6 7 8 9 10 1 chunk +217 lines, -0 lines 1 comment Download
M chrome/browser/extensions/image_loading_tracker.h View 1 2 3 4 5 6 7 8 9 6 chunks +53 lines, -18 lines 4 comments Download
M chrome/browser/extensions/image_loading_tracker.cc View 1 2 3 4 5 6 7 8 9 9 chunks +111 lines, -86 lines 2 comments Download
M chrome/browser/extensions/image_loading_tracker_unittest.cc View 1 2 3 4 5 6 7 8 9 2 chunks +10 lines, -4 lines 1 comment Download
M chrome/chrome_browser_extensions.gypi View 1 2 3 4 5 6 7 8 9 1 chunk +3 lines, -0 lines 0 comments Download
M chrome/chrome_tests.gypi View 1 2 3 4 5 6 7 8 9 1 chunk +1 line, -0 lines 0 comments Download

Messages

Total messages: 43 (0 generated)
xiyuan
Splitted from http://codereview.chromium.org/10699065/ so that we have a more focused review on ImageLoadingTracker.
8 years, 5 months ago (2012-07-03 22:13:43 UTC) #1
pkotwicz
At a high level, I believe that you should follow the same pattern as http://codereview.chromium.org/10500015/ ...
8 years, 5 months ago (2012-07-11 20:44:03 UTC) #2
pkotwicz
Can you sync with Oshima, Ben, and Aaron as to whether or not we want ...
8 years, 5 months ago (2012-07-11 21:01:26 UTC) #3
pkotwicz
http://codereview.chromium.org/10701087/diff/6002/chrome/browser/extensions/image_loading_tracker.h File chrome/browser/extensions/image_loading_tracker.h (right): http://codereview.chromium.org/10701087/diff/6002/chrome/browser/extensions/image_loading_tracker.h#newcode100 chrome/browser/extensions/image_loading_tracker.h:100: void LoadDIPImage(const extensions::Extension* extension, Nit: I am not a ...
8 years, 5 months ago (2012-07-11 21:02:25 UTC) #4
oshima
http://codereview.chromium.org/10701087/diff/6002/chrome/browser/extensions/image_loading_tracker.cc File chrome/browser/extensions/image_loading_tracker.cc (right): http://codereview.chromium.org/10701087/diff/6002/chrome/browser/extensions/image_loading_tracker.cc#newcode222 chrome/browser/extensions/image_loading_tracker.cc:222: } instead of having different code path, won't this ...
8 years, 5 months ago (2012-07-11 21:09:33 UTC) #5
xiyuan
On 2012/07/11 20:44:03, pkotwicz wrote: > At a high level, I believe that you should ...
8 years, 5 months ago (2012-07-11 21:58:12 UTC) #6
xiyuan
I have updated the CL to follow pattern in http://codereview.chromium.org/10500015/. LoadImageInDIP loads images for interesting ...
8 years, 5 months ago (2012-07-13 18:35:46 UTC) #7
pkotwicz
I will review tonight
8 years, 5 months ago (2012-07-13 23:02:11 UTC) #8
xiyuan
+sky
8 years, 5 months ago (2012-07-16 16:30:03 UTC) #9
oshima
http://codereview.chromium.org/10701087/diff/9001/chrome/browser/extensions/image_loading_tracker.cc File chrome/browser/extensions/image_loading_tracker.cc (right): http://codereview.chromium.org/10701087/diff/9001/chrome/browser/extensions/image_loading_tracker.cc#newcode90 chrome/browser/extensions/image_loading_tracker.cc:90: BrowserThread::FILE, FROM_HERE, this is not the scope of this ...
8 years, 5 months ago (2012-07-16 17:22:33 UTC) #10
pkotwicz
It looks pretty good. Can you do me a huge favor and clean up the ...
8 years, 5 months ago (2012-07-16 17:33:52 UTC) #11
xiyuan
http://codereview.chromium.org/10701087/diff/9001/chrome/browser/extensions/app_shortcut_manager.cc File chrome/browser/extensions/app_shortcut_manager.cc (right): http://codereview.chromium.org/10701087/diff/9001/chrome/browser/extensions/app_shortcut_manager.cc#newcode117 chrome/browser/extensions/app_shortcut_manager.cc:117: ui::SCALE_FACTOR_NONE)); On 2012/07/16 17:33:52, pkotwicz wrote: > ui::SCALE_FACTOR_100P Changed ...
8 years, 5 months ago (2012-07-16 20:11:27 UTC) #12
xiyuan
CL updated to defer resource loading until ImageSource is asked for a representation per dicussion ...
8 years, 5 months ago (2012-07-17 21:53:22 UTC) #13
pkotwicz
http://codereview.chromium.org/10701087/diff/17004/chrome/browser/extensions/image_loading_tracker.cc File chrome/browser/extensions/image_loading_tracker.cc (right): http://codereview.chromium.org/10701087/diff/17004/chrome/browser/extensions/image_loading_tracker.cc#newcode296 chrome/browser/extensions/image_loading_tracker.cc:296: observer_->OnImageLoaded(gfx::Image(load_info.image_skia), You need to validate that you are capable ...
8 years, 5 months ago (2012-07-18 00:07:25 UTC) #14
pkotwicz
The validation is super cheap so you can also validate whether you can generate a ...
8 years, 5 months ago (2012-07-18 14:16:26 UTC) #15
pkotwicz
http://codereview.chromium.org/10701087/diff/17004/chrome/browser/extensions/image_loading_tracker.cc File chrome/browser/extensions/image_loading_tracker.cc (right): http://codereview.chromium.org/10701087/diff/17004/chrome/browser/extensions/image_loading_tracker.cc#newcode296 chrome/browser/extensions/image_loading_tracker.cc:296: observer_->OnImageLoaded(gfx::Image(load_info.image_skia), The validation is super cheap so you can ...
8 years, 5 months ago (2012-07-18 14:17:53 UTC) #16
oshima
approach lg. a few nits, + please address peter's comment. http://codereview.chromium.org/10701087/diff/17004/chrome/browser/extensions/image_loading_tracker.cc File chrome/browser/extensions/image_loading_tracker.cc (right): http://codereview.chromium.org/10701087/diff/17004/chrome/browser/extensions/image_loading_tracker.cc#newcode391 ...
8 years, 5 months ago (2012-07-18 15:16:27 UTC) #17
xiyuan
http://codereview.chromium.org/10701087/diff/17004/chrome/browser/extensions/image_loading_tracker.cc File chrome/browser/extensions/image_loading_tracker.cc (right): http://codereview.chromium.org/10701087/diff/17004/chrome/browser/extensions/image_loading_tracker.cc#newcode296 chrome/browser/extensions/image_loading_tracker.cc:296: observer_->OnImageLoaded(gfx::Image(load_info.image_skia), On 2012/07/18 14:17:53, pkotwicz wrote: > The validation ...
8 years, 5 months ago (2012-07-18 17:28:32 UTC) #18
oshima
http://codereview.chromium.org/10701087/diff/17004/chrome/browser/extensions/image_loading_tracker.cc File chrome/browser/extensions/image_loading_tracker.cc (right): http://codereview.chromium.org/10701087/diff/17004/chrome/browser/extensions/image_loading_tracker.cc#newcode438 chrome/browser/extensions/image_loading_tracker.cc:438: if (bitmap) { On 2012/07/18 17:28:32, xiyuan wrote: > ...
8 years, 5 months ago (2012-07-18 20:41:49 UTC) #19
xiyuan
http://codereview.chromium.org/10701087/diff/17004/chrome/browser/extensions/image_loading_tracker.cc File chrome/browser/extensions/image_loading_tracker.cc (right): http://codereview.chromium.org/10701087/diff/17004/chrome/browser/extensions/image_loading_tracker.cc#newcode438 chrome/browser/extensions/image_loading_tracker.cc:438: if (bitmap) { On 2012/07/18 20:41:49, oshima wrote: > ...
8 years, 5 months ago (2012-07-18 20:45:01 UTC) #20
oshima
On Wed, Jul 18, 2012 at 1:45 PM, <xiyuan@chromium.org> wrote: > > http://codereview.chromium.**org/10701087/diff/17004/** > chrome/browser/extensions/**image_loading_tracker.cc<http://codereview.chromium.org/10701087/diff/17004/chrome/browser/extensions/image_loading_tracker.cc> ...
8 years, 5 months ago (2012-07-18 20:49:48 UTC) #21
xiyuan
On 2012/07/18 20:49:48, oshima wrote: > Sounds to me that it shouldn't use ImageSkia for ...
8 years, 5 months ago (2012-07-18 20:57:40 UTC) #22
oshima
On 2012/07/18 20:57:40, xiyuan wrote: > On 2012/07/18 20:49:48, oshima wrote: > > Sounds to ...
8 years, 5 months ago (2012-07-18 22:32:45 UTC) #23
sky
I'm not an OWNER in this directory, so you don't really need my to review ...
8 years, 5 months ago (2012-07-18 23:50:57 UTC) #24
pkotwicz
This CL changes launcher behavior slightly. Currently, we display a default icon in the mean ...
8 years, 5 months ago (2012-07-19 01:26:15 UTC) #25
pkotwicz
http://codereview.chromium.org/10701087/diff/27003/chrome/browser/extensions/image_loading_tracker.cc File chrome/browser/extensions/image_loading_tracker.cc (right): http://codereview.chromium.org/10701087/diff/27003/chrome/browser/extensions/image_loading_tracker.cc#newcode37 chrome/browser/extensions/image_loading_tracker.cc:37: } const kSpecialComponentExtensionResources[] = { Nit: New line after ...
8 years, 5 months ago (2012-07-19 01:37:45 UTC) #26
pkotwicz
Xiyuan, thank you for your patience. I think it might be time to ping Aaron ...
8 years, 5 months ago (2012-07-19 01:48:46 UTC) #27
Aaron Boodman
I wanted to ping that I am following this and am really sorry I've not ...
8 years, 5 months ago (2012-07-19 01:50:10 UTC) #28
Aaron Boodman
ImageSkia/ImageSkiaSource is really cool. Can we separate concerns here and break this into two features: ...
8 years, 5 months ago (2012-07-19 05:02:45 UTC) #29
Aaron Boodman
Nevermind, I misunderstood what ImageSkia/ImageSkiaSource are doing. I thought they would support an async impl ...
8 years, 5 months ago (2012-07-19 05:22:56 UTC) #30
pkotwicz
Aaron please ping me as to how you envision an async ImageSkiaSource working. The challenge ...
8 years, 5 months ago (2012-07-19 13:39:55 UTC) #31
pkotwicz
Just realized that my suggestion in #30 will not work
8 years, 5 months ago (2012-07-19 13:48:26 UTC) #32
xiyuan
On 2012/07/19 05:22:56, Aaron Boodman wrote: > Nevermind, I misunderstood what ImageSkia/ImageSkiaSource are doing. I ...
8 years, 5 months ago (2012-07-19 16:41:22 UTC) #33
xiyuan
Per yesterday's discussion, I will refactoring the CL to extract ImageSource into its own class ...
8 years, 5 months ago (2012-07-20 15:00:13 UTC) #34
xiyuan
CL refactored. Wrapped the ImageSkiaSource and load image for scale factor logic into an ExtensionIconImage ...
8 years, 5 months ago (2012-07-20 21:21:14 UTC) #35
Aaron Boodman
http://codereview.chromium.org/10701087/diff/30012/chrome/browser/extensions/extension_icon_image.cc File chrome/browser/extensions/extension_icon_image.cc (right): http://codereview.chromium.org/10701087/diff/30012/chrome/browser/extensions/extension_icon_image.cc#newcode83 chrome/browser/extensions/extension_icon_image.cc:83: // Check whether extension has at least 1x resource. ...
8 years, 5 months ago (2012-07-20 22:12:39 UTC) #36
xiyuan
http://codereview.chromium.org/10701087/diff/30012/chrome/browser/extensions/extension_icon_image.cc File chrome/browser/extensions/extension_icon_image.cc (right): http://codereview.chromium.org/10701087/diff/30012/chrome/browser/extensions/extension_icon_image.cc#newcode83 chrome/browser/extensions/extension_icon_image.cc:83: // Check whether extension has at least 1x resource. ...
8 years, 5 months ago (2012-07-20 22:43:16 UTC) #37
Aaron Boodman
On 2012/07/20 22:43:16, xiyuan wrote: > http://codereview.chromium.org/10701087/diff/30012/chrome/browser/extensions/extension_icon_image.cc > File chrome/browser/extensions/extension_icon_image.cc (right): > > http://codereview.chromium.org/10701087/diff/30012/chrome/browser/extensions/extension_icon_image.cc#newcode83 > ...
8 years, 5 months ago (2012-07-20 22:50:36 UTC) #38
pkotwicz
LGTM with nits
8 years, 5 months ago (2012-07-24 15:06:59 UTC) #39
tbarzic
On 2012/07/24 15:06:59, pkotwicz wrote: > LGTM with nits pkotwicz: what nits (did you forget ...
8 years, 5 months ago (2012-07-24 17:53:54 UTC) #40
pkotwicz
http://codereview.chromium.org/10701087/diff/30014/chrome/browser/extensions/extension_icon_image.cc File chrome/browser/extensions/extension_icon_image.cc (right): http://codereview.chromium.org/10701087/diff/30014/chrome/browser/extensions/extension_icon_image.cc#newcode157 chrome/browser/extensions/extension_icon_image.cc:157: gfx::ImageSkiaRep rep = image.ToImageSkia()->GetRepresentation(scale_factor); Nit: You can DCHECK on ...
8 years, 5 months ago (2012-07-25 02:00:39 UTC) #41
Aaron Boodman
http://codereview.chromium.org/10701087/diff/30014/chrome/browser/extensions/extension_icon_image.cc File chrome/browser/extensions/extension_icon_image.cc (right): http://codereview.chromium.org/10701087/diff/30014/chrome/browser/extensions/extension_icon_image.cc#newcode76 chrome/browser/extensions/extension_icon_image.cc:76: const ExtensionIconSet& icon_set, Since currently the EIS that is ...
8 years, 4 months ago (2012-08-10 20:49:34 UTC) #42
xiyuan
8 years, 4 months ago (2012-08-10 21:07:43 UTC) #43
Sorry for the confusion. I'll close this CL to avoid future confusions.

Tony has taken over this change. His CL is here:
http://codereview.chromium.org/10825012/

Powered by Google App Engine
This is Rietveld 408576698