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

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

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

Description

taking over xiyuan's patch: http://codereview.chromium.org/10701087/ *************************************************************** 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. *************************************************************** For gypi files (I reckon this part is trivial enough) TBR=ben@chromium.org Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=152407

Patch Set 1 #

Total comments: 22

Patch Set 2 : pkotwicz's comments #

Patch Set 3 : .. #

Patch Set 4 : rebase #

Total comments: 24

Patch Set 5 : aaron's comments #

Patch Set 6 : rebase #

Total comments: 2

Patch Set 7 : rebase #

Total comments: 2

Patch Set 8 : a crash fix #

Patch Set 9 : . #

Patch Set 10 : . #

Patch Set 11 : . #

Total comments: 16

Patch Set 12 : oshima's feedback #

Patch Set 13 : .. #

Total comments: 4

Patch Set 14 : . #

Patch Set 15 : . #

Patch Set 16 : . #

Total comments: 8

Patch Set 17 : feedback #

Patch Set 18 : feedback #

Patch Set 19 : rebase #

Patch Set 20 : new fallback logic #

Patch Set 21 : minor cleanup #

Total comments: 2

Patch Set 22 : rebase #

Patch Set 23 : .. #

Patch Set 24 : .. #

Patch Set 25 : rebase #

Patch Set 26 : passing ref to tmp object is not very safe.. #

Patch Set 27 : rebase #

Patch Set 28 : . #

Unified diffs Side-by-side diffs Delta from patch set Stats (+749 lines, -125 lines) Patch
M chrome/browser/extensions/app_shortcut_manager.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 2 chunks +12 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 11 12 13 14 15 16 17 18 19 20 21 22 1 chunk +98 lines, -0 lines 0 comments Download
A chrome/browser/extensions/extension_icon_image.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 1 chunk +180 lines, -0 lines 0 comments Download
A chrome/browser/extensions/extension_icon_image_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 1 chunk +264 lines, -0 lines 0 comments Download
M chrome/browser/extensions/image_loading_tracker.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 7 chunks +46 lines, -20 lines 0 comments Download
M chrome/browser/extensions/image_loading_tracker.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 9 chunks +138 lines, -96 lines 0 comments Download
M chrome/browser/extensions/image_loading_tracker_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 2 chunks +8 lines, -4 lines 0 comments Download
M chrome/chrome_browser_extensions.gypi View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 1 chunk +2 lines, -0 lines 0 comments Download
M chrome/chrome_tests.gypi View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 1 chunk +1 line, -0 lines 0 comments Download

Messages

Total messages: 32 (0 generated)
tbarzic
Since xiyuan is ooo until the end of next week, I'll take over his patch ...
8 years, 5 months ago (2012-07-25 19:37:33 UTC) #1
tbarzic
http://codereview.chromium.org/10825012/diff/1/chrome/browser/extensions/extension_icon_image.cc File chrome/browser/extensions/extension_icon_image.cc (right): http://codereview.chromium.org/10825012/diff/1/chrome/browser/extensions/extension_icon_image.cc#newcode157 chrome/browser/extensions/extension_icon_image.cc:157: gfx::ImageSkiaRep rep = image.ToImageSkia()->GetRepresentation(scale_factor); On 2012/07/25 19:37:34, tbarzic wrote: ...
8 years, 5 months ago (2012-07-25 19:53:53 UTC) #2
pkotwicz
Just to confirm: "Wait for the last CL to update laucher/app list code to use ...
8 years, 5 months ago (2012-07-25 19:56:19 UTC) #3
pkotwicz
*assuming just nits
8 years, 5 months ago (2012-07-25 19:56:52 UTC) #4
tbarzic
On 2012/07/25 19:56:19, pkotwicz wrote: > Just to confirm: "Wait for the last CL to ...
8 years, 5 months ago (2012-07-25 20:18:30 UTC) #5
tbarzic
On 2012/07/25 20:18:30, tbarzic wrote: > On 2012/07/25 19:56:19, pkotwicz wrote: > > Just to ...
8 years, 4 months ago (2012-07-30 18:05:35 UTC) #6
pkotwicz
For the sake of moving forward, I am ok with this patch. However, I have ...
8 years, 4 months ago (2012-07-30 21:09:56 UTC) #7
Aaron Boodman
http://codereview.chromium.org/10825012/diff/8004/chrome/browser/extensions/extension_icon_image.h File chrome/browser/extensions/extension_icon_image.h (right): http://codereview.chromium.org/10825012/diff/8004/chrome/browser/extensions/extension_icon_image.h#newcode26 chrome/browser/extensions/extension_icon_image.h:26: class ExtensionIconImageDelegate; Can the delegate be defined as a ...
8 years, 4 months ago (2012-08-02 17:50:56 UTC) #8
tbarzic
PTAL https://chromiumcodereview.appspot.com/10825012/diff/8004/chrome/browser/extensions/extension_icon_image.h File chrome/browser/extensions/extension_icon_image.h (right): https://chromiumcodereview.appspot.com/10825012/diff/8004/chrome/browser/extensions/extension_icon_image.h#newcode26 chrome/browser/extensions/extension_icon_image.h:26: class ExtensionIconImageDelegate; On 2012/08/02 17:50:56, Aaron Boodman wrote: ...
8 years, 4 months ago (2012-08-02 20:12:52 UTC) #9
pkotwicz
Can you please try this with a use case of ImageLoaadingTracker before committing this? My ...
8 years, 4 months ago (2012-08-03 17:57:41 UTC) #10
pkotwicz
https://chromiumcodereview.appspot.com/10825012/diff/13002/chrome/browser/extensions/extension_icon_image.h File chrome/browser/extensions/extension_icon_image.h (right): https://chromiumcodereview.appspot.com/10825012/diff/13002/chrome/browser/extensions/extension_icon_image.h#newcode57 chrome/browser/extensions/extension_icon_image.h:57: const gfx::ImageSkia& image_skia() const { return image_skia_; } I ...
8 years, 4 months ago (2012-08-03 17:58:38 UTC) #11
tbarzic
https://chromiumcodereview.appspot.com/10825012/diff/13002/chrome/browser/extensions/extension_icon_image.h File chrome/browser/extensions/extension_icon_image.h (right): https://chromiumcodereview.appspot.com/10825012/diff/13002/chrome/browser/extensions/extension_icon_image.h#newcode57 chrome/browser/extensions/extension_icon_image.h:57: const gfx::ImageSkia& image_skia() const { return image_skia_; } On ...
8 years, 4 months ago (2012-08-03 18:44:28 UTC) #12
tbarzic
fyi, I tries this with chrome/browser/ui/views/ash/app_list/search_builder.cc and chrome/browser/ui/views/ash/launcher/launcher_app_icon_loader.cc and it seems to work (but we're ...
8 years, 4 months ago (2012-08-06 18:37:43 UTC) #13
tbarzic
https://chromiumcodereview.appspot.com/10825012/diff/11015/chrome/browser/extensions/image_loading_tracker.cc File chrome/browser/extensions/image_loading_tracker.cc (right): https://chromiumcodereview.appspot.com/10825012/diff/11015/chrome/browser/extensions/image_loading_tracker.cc#newcode192 chrome/browser/extensions/image_loading_tracker.cc:192: ResourceBundle::GetSharedInstance().GetBitmapNamed(resource_id); On 2012/08/06 18:37:43, tbarzic wrote: > This is ...
8 years, 4 months ago (2012-08-06 21:37:11 UTC) #14
oshima
looks good. just a several minor questions/suggestions. http://codereview.chromium.org/10825012/diff/9015/chrome/browser/extensions/extension_icon_image.cc File chrome/browser/extensions/extension_icon_image.cc (right): http://codereview.chromium.org/10825012/diff/9015/chrome/browser/extensions/extension_icon_image.cc#newcode143 chrome/browser/extensions/extension_icon_image.cc:143: } you ...
8 years, 4 months ago (2012-08-07 07:43:05 UTC) #15
xiyuan
http://codereview.chromium.org/10825012/diff/9015/chrome/browser/extensions/extension_icon_image_unittest.cc File chrome/browser/extensions/extension_icon_image_unittest.cc (right): http://codereview.chromium.org/10825012/diff/9015/chrome/browser/extensions/extension_icon_image_unittest.cc#newcode89 chrome/browser/extensions/extension_icon_image_unittest.cc:89: content::TestBrowserThread ui_thread_; On 2012/08/07 07:43:05, oshima wrote: > I'm ...
8 years, 4 months ago (2012-08-07 17:32:40 UTC) #16
tbarzic
https://chromiumcodereview.appspot.com/10825012/diff/9015/chrome/browser/extensions/extension_icon_image.cc File chrome/browser/extensions/extension_icon_image.cc (right): https://chromiumcodereview.appspot.com/10825012/diff/9015/chrome/browser/extensions/extension_icon_image.cc#newcode143 chrome/browser/extensions/extension_icon_image.cc:143: } On 2012/08/07 07:43:05, oshima wrote: > you may ...
8 years, 4 months ago (2012-08-07 18:10:26 UTC) #17
oshima
lgtm with nits. you need aa's lgtm though. http://codereview.chromium.org/10825012/diff/7018/chrome/browser/extensions/image_loading_tracker.cc File chrome/browser/extensions/image_loading_tracker.cc (right): http://codereview.chromium.org/10825012/diff/7018/chrome/browser/extensions/image_loading_tracker.cc#newcode175 chrome/browser/extensions/image_loading_tracker.cc:175: true ...
8 years, 4 months ago (2012-08-08 03:27:43 UTC) #18
tbarzic
https://chromiumcodereview.appspot.com/10825012/diff/7018/chrome/browser/extensions/image_loading_tracker.cc File chrome/browser/extensions/image_loading_tracker.cc (right): https://chromiumcodereview.appspot.com/10825012/diff/7018/chrome/browser/extensions/image_loading_tracker.cc#newcode175 chrome/browser/extensions/image_loading_tracker.cc:175: true /* delete bitmap*/); On 2012/08/08 03:27:43, oshima wrote: ...
8 years, 4 months ago (2012-08-08 04:55:45 UTC) #19
tbarzic
Aaron: ping..
8 years, 4 months ago (2012-08-08 17:44:55 UTC) #20
Aaron Boodman
http://codereview.chromium.org/10825012/diff/4019/chrome/browser/extensions/extension_icon_image.cc File chrome/browser/extensions/extension_icon_image.cc (right): http://codereview.chromium.org/10825012/diff/4019/chrome/browser/extensions/extension_icon_image.cc#newcode84 chrome/browser/extensions/extension_icon_image.cc:84: resource_size_in_dip_(resource_size_in_dip), I don't understand why resource_size_in_dip is necessary. Why ...
8 years, 4 months ago (2012-08-10 21:42:07 UTC) #21
xiyuan
http://codereview.chromium.org/10825012/diff/4019/chrome/browser/extensions/extension_icon_image.cc File chrome/browser/extensions/extension_icon_image.cc (right): http://codereview.chromium.org/10825012/diff/4019/chrome/browser/extensions/extension_icon_image.cc#newcode139 chrome/browser/extensions/extension_icon_image.cc:139: if (image.IsEmpty()) { On 2012/08/10 21:42:08, Aaron Boodman wrote: ...
8 years, 4 months ago (2012-08-10 22:03:43 UTC) #22
tbarzic
https://chromiumcodereview.appspot.com/10825012/diff/4019/chrome/browser/extensions/extension_icon_image.cc File chrome/browser/extensions/extension_icon_image.cc (right): https://chromiumcodereview.appspot.com/10825012/diff/4019/chrome/browser/extensions/extension_icon_image.cc#newcode84 chrome/browser/extensions/extension_icon_image.cc:84: resource_size_in_dip_(resource_size_in_dip), On 2012/08/10 21:42:08, Aaron Boodman wrote: > I ...
8 years, 4 months ago (2012-08-10 23:01:23 UTC) #23
Aaron Boodman
OK, I thought about this over the weekend. I think we should simplify the interface ...
8 years, 4 months ago (2012-08-13 23:29:12 UTC) #24
tbarzic
On 2012/08/13 23:29:12, Aaron Boodman wrote: > OK, I thought about this over the weekend. ...
8 years, 4 months ago (2012-08-14 03:09:09 UTC) #25
Aaron Boodman
http://codereview.chromium.org/10825012/diff/18031/chrome/browser/extensions/extension_icon_image.cc File chrome/browser/extensions/extension_icon_image.cc (right): http://codereview.chromium.org/10825012/diff/18031/chrome/browser/extensions/extension_icon_image.cc#newcode112 chrome/browser/extensions/extension_icon_image.cc:112: if (!is_retry && resource_size_in_pixel >= 32) { We can ...
8 years, 4 months ago (2012-08-15 04:30:40 UTC) #26
tbarzic
http://codereview.chromium.org/10825012/diff/18031/chrome/browser/extensions/extension_icon_image.cc File chrome/browser/extensions/extension_icon_image.cc (right): http://codereview.chromium.org/10825012/diff/18031/chrome/browser/extensions/extension_icon_image.cc#newcode112 chrome/browser/extensions/extension_icon_image.cc:112: if (!is_retry && resource_size_in_pixel >= 32) { On 2012/08/15 ...
8 years, 4 months ago (2012-08-15 15:01:36 UTC) #27
Aaron Boodman
lgtm
8 years, 4 months ago (2012-08-16 05:07:01 UTC) #28
tbarzic
On 2012/08/16 05:07:01, Aaron Boodman wrote: > lgtm tbring Ben
8 years, 4 months ago (2012-08-17 20:20:04 UTC) #29
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/tbarzic@chromium.org/10825012/20036
8 years, 4 months ago (2012-08-20 18:01:32 UTC) #30
Ben Goodger (Google)
lgtm
8 years, 4 months ago (2012-08-20 20:07:29 UTC) #31
commit-bot: I haz the power
8 years, 4 months ago (2012-08-20 22:22:44 UTC) #32
Change committed as 152407

Powered by Google App Engine
This is Rietveld 408576698