Chromium Code Reviews
[email protected] (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(281)

Issue 6597043: Convert IconLoader and IconManager to deal with gfx::Image rather than SkBitmap. (Closed)

Created:
9 years, 9 months ago by Robert Sesek
Modified:
9 years, 7 months ago
Reviewers:
Nico, Evan Stade
CC:
chromium-reviews, pam+watch_chromium.org, rdsmith+dwatch_chromium.org, Paweł Hajdan Jr.
Visibility:
Public.

Description

Convert IconLoader and IconManager to deal with gfx::Image rather than SkBitmap. This allows loading of icons in the platform format, avoiding unnecessary conversions if the image is going to be used with the platform toolkit. In other cases, this just pushes image conversion to the callsite rather than the actual image load. BUG=19685 TEST=unit_tests and visual inspection of icons in the download shelf Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=76743

Patch Set 1 #

Patch Set 2 : Rebase ToT #

Patch Set 3 : Fix linux_view #

Total comments: 9

Patch Set 4 : Address thakis' comments #

Total comments: 4
Unified diffs Side-by-side diffs Delta from patch set Stats (+166 lines, -377 lines) Patch
M chrome/browser/download/download_util.h View 3 chunks +3 lines, -2 lines 0 comments Download
M chrome/browser/download/download_util.cc View 1 2 4 chunks +4 lines, -3 lines 2 comments Download
M chrome/browser/icon_loader.h View 5 chunks +5 lines, -7 lines 0 comments Download
M chrome/browser/icon_loader.cc View 1 2 3 3 chunks +7 lines, -4 lines 0 comments Download
M chrome/browser/icon_loader_linux.cc View 3 chunks +7 lines, -25 lines 0 comments Download
M chrome/browser/icon_loader_mac.mm View 2 chunks +4 lines, -10 lines 0 comments Download
M chrome/browser/icon_loader_win.cc View 2 chunks +3 lines, -2 lines 0 comments Download
M chrome/browser/icon_manager.h View 1 2 5 chunks +7 lines, -7 lines 0 comments Download
M chrome/browser/icon_manager.cc View 4 chunks +5 lines, -5 lines 0 comments Download
M chrome/browser/ui/cocoa/download/download_item_mac.h View 1 2 3 chunks +7 lines, -2 lines 0 comments Download
M chrome/browser/ui/cocoa/download/download_item_mac.mm View 4 chunks +8 lines, -11 lines 0 comments Download
M chrome/browser/ui/cocoa/download/download_util_mac.mm View 4 chunks +4 lines, -7 lines 0 comments Download
M chrome/browser/ui/gtk/custom_drag.h View 3 chunks +13 lines, -6 lines 0 comments Download
M chrome/browser/ui/gtk/custom_drag.cc View 1 2 6 chunks +10 lines, -13 lines 2 comments Download
M chrome/browser/ui/gtk/download_item_gtk.h View 1 3 chunks +9 lines, -4 lines 0 comments Download
M chrome/browser/ui/gtk/download_item_gtk.cc View 1 2 chunks +5 lines, -4 lines 0 comments Download
M chrome/browser/ui/views/download_item_view.h View 1 2 3 chunks +6 lines, -2 lines 0 comments Download
M chrome/browser/ui/views/download_item_view.cc View 1 2 3 4 chunks +12 lines, -6 lines 0 comments Download
M chrome/browser/ui/webui/downloads_dom_handler.cc View 1 2 3 2 chunks +3 lines, -2 lines 0 comments Download
M chrome/browser/ui/webui/fileicon_source.h View 1 2 chunks +5 lines, -1 line 0 comments Download
M chrome/browser/ui/webui/fileicon_source.cc View 1 2 4 chunks +6 lines, -5 lines 0 comments Download
M ui/gfx/gfx.gyp View 2 chunks +0 lines, -2 lines 0 comments Download
M ui/gfx/image.h View 1 chunk +3 lines, -0 lines 0 comments Download
M ui/gfx/image.cc View 1 2 3 2 chunks +7 lines, -0 lines 0 comments Download
M ui/gfx/image_unittest.cc View 1 chunk +21 lines, -0 lines 0 comments Download
D ui/gfx/scoped_image.h View 1 chunk +0 lines, -147 lines 0 comments Download
D ui/gfx/scoped_image_unittest.cc View 1 chunk +0 lines, -98 lines 0 comments Download
M views/drag_utils.h View 1 chunk +1 line, -1 line 0 comments Download
M views/drag_utils.cc View 1 chunk +1 line, -1 line 0 comments Download

Messages

Total messages: 5 (0 generated)
Robert Sesek
estade: overall IconManager/Loader changes, Linux changes thakis: Mac changes
9 years, 9 months ago (2011-03-01 17:25:05 UTC) #1
Nico
LG http://codereview.chromium.org/6597043/diff/3030/chrome/browser/icon_loader.cc File chrome/browser/icon_loader.cc (right): http://codereview.chromium.org/6597043/diff/3030/chrome/browser/icon_loader.cc#newcode43 chrome/browser/icon_loader.cc:43: image = image_.release(); // Can't ignore return value. ...
9 years, 9 months ago (2011-03-02 02:31:33 UTC) #2
Robert Sesek
http://codereview.chromium.org/6597043/diff/3030/chrome/browser/icon_loader.cc File chrome/browser/icon_loader.cc (right): http://codereview.chromium.org/6597043/diff/3030/chrome/browser/icon_loader.cc#newcode43 chrome/browser/icon_loader.cc:43: image = image_.release(); // Can't ignore return value. On ...
9 years, 9 months ago (2011-03-02 14:05:36 UTC) #3
Evan Stade
cool, lgtm http://codereview.chromium.org/6597043/diff/3030/chrome/browser/icon_loader.cc File chrome/browser/icon_loader.cc (right): http://codereview.chromium.org/6597043/diff/3030/chrome/browser/icon_loader.cc#newcode43 chrome/browser/icon_loader.cc:43: image = image_.release(); // Can't ignore return ...
9 years, 9 months ago (2011-03-02 22:48:57 UTC) #4
Robert Sesek
9 years, 9 months ago (2011-03-03 15:48:46 UTC) #5
Submitting.

http://codereview.chromium.org/6597043/diff/3030/chrome/browser/icon_loader.cc
File chrome/browser/icon_loader.cc (right):

http://codereview.chromium.org/6597043/diff/3030/chrome/browser/icon_loader.c...
chrome/browser/icon_loader.cc:43: image = image_.release();  // Can't ignore
return value.
On 2011/03/02 22:48:57, Evan Stade wrote:
> On 2011/03/02 14:05:36, rsesek wrote:
> > On 2011/03/02 02:31:33, Nico wrote:
> > > The usual pattern for this is
> > >   (void)image_.release();
> > 
> > This doesn't suppress the warning.
> 
> ignore_result(image_.release());
> 
> (defined in basictypes.h)

Done.

http://codereview.chromium.org/6597043/diff/8001/chrome/browser/download/down...
File chrome/browser/download/download_util.cc (right):

http://codereview.chromium.org/6597043/diff/8001/chrome/browser/download/down...
chrome/browser/download/download_util.cc:57: #include "ui/gfx/image.h"
On 2011/03/02 22:48:57, Evan Stade wrote:
> alphabet

Done.

http://codereview.chromium.org/6597043/diff/8001/chrome/browser/ui/gtk/custom...
File chrome/browser/ui/gtk/custom_drag.cc (right):

http://codereview.chromium.org/6597043/diff/8001/chrome/browser/ui/gtk/custom...
chrome/browser/ui/gtk/custom_drag.cc:110: if (icon) {
On 2011/03/02 22:48:57, Evan Stade wrote:
> nit: no curlies

Done.

Powered by Google App Engine
This is Rietveld 408576698