ShortcutInfo::favicon is now a gfx::ImageFamily instead of gfx::Image.

ShortcutInfo::favicon is now a gfx::ImageFamily instead of gfx::Image.

This fixes the bad usage where a single Image contained all icon images at the
"100P" scale factor. Now each Image in the family contains a single
representation.

Updated all usages of ShortcutInfo::favicon to use the ImageFamily. Several
places where the "get best size icon" algorithm was duplicated have been
replaced with a call to ImageFamily::GetBest.

BUG=189137
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=193558

[benwells, 2013-03-17 22:42:36]

https://codereview.chromium.org/12881003/diff/3001/ui/gfx/icon_family.h
File ui/gfx/icon_family.h (right):

https://codereview.chromium.org/12881003/diff/3001/ui/gfx/icon_family.h#newco...
ui/gfx/icon_family.h:40: // The icon SHOULD be square; if not, its size will be
whichever is smaller
What's the reasoning behind this recommendation-but-not-requirement? I can
imagine wanting a class like this to handle a bunch of related images, not
necessarily all square. Maybe it should be ImageSet and use both dimensions as a
key?

If you did that you could have three Gets:
Get(int w, int h) with a variant taking a gfx::Size
GetWithWidth(int w)
GetWithHeight(int h)

[Matt Giuca, 2013-03-17 22:51:18]

https://codereview.chromium.org/12881003/diff/3001/ui/gfx/icon_family.h
File ui/gfx/icon_family.h (right):

https://codereview.chromium.org/12881003/diff/3001/ui/gfx/icon_family.h#newco...
ui/gfx/icon_family.h:40: // The icon SHOULD be square; if not, its size will be
whichever is smaller
On 2013/03/17 22:42:36, benwells wrote:
> What's the reasoning behind this recommendation-but-not-requirement?

Basically that it works fine if images are not square, but it becomes a little
bit more arbitrarily (since what does it mean to say "bigger than" in two
dimensions? -- I took it to mean "the minimum of the width or height").

> I can
> imagine wanting a class like this to handle a bunch of related images, not
> necessarily all square. Maybe it should be ImageSet and use both dimensions as
a
> key?
> 
> If you did that you could have three Gets:
> Get(int w, int h) with a variant taking a gfx::Size

But what would this mean if there is an image with a bigger width but smaller
height? I suppose I would take this to mean "find the image where the minimum of
the width and height is greater than the maximum of w and h" -- but that is
quite a mouthful. I'd probably just have Get() take a single parameter, int
size.

> GetWithWidth(int w)
> GetWithHeight(int h)

OK other than my comments above, this could work.

[pkotwicz, 2013-03-18 03:22:43]

Thanks a lot for looking into this!
I have added a few comments. The comments which are labeled "Comment:" are
things I would ask you over lunch were we working out of the same office.

https://codereview.chromium.org/12881003/diff/3001/chrome/browser/ui/views/cr...
File chrome/browser/ui/views/create_application_shortcut_view.cc (right):

https://codereview.chromium.org/12881003/diff/3001/chrome/browser/ui/views/cr...
chrome/browser/ui/views/create_application_shortcut_view.cc:182:
icon_->SetImage(gfx::ImageSkia::CreateFrom1xBitmap(bitmap));
Comment: In a separate CL, it would be nice to call SetImage() with a
multiresolution image. (We end up showing the image in the UI).

https://codereview.chromium.org/12881003/diff/3001/chrome/browser/ui/web_appl...
File chrome/browser/ui/web_applications/web_app_ui.cc (right):

https://codereview.chromium.org/12881003/diff/3001/chrome/browser/ui/web_appl...
chrome/browser/ui/web_applications/web_app_ui.cc:381:
info->favicon.Add(favicon_tab_helper->GetFavicon());
Comment: Please let me know if you want to fix this. (Not your diff, but the
fact that we call GetFavicon() here). We can do a lot better. I have just not
gotten around to implementing the "a lot better".

https://codereview.chromium.org/12881003/diff/3001/chrome/browser/ui/web_appl...
chrome/browser/ui/web_applications/web_app_ui.cc:419: // individually inserting
them into an IconFamily.
Comment: It would be nice to get rid of this hack in a separate CL :)

https://codereview.chromium.org/12881003/diff/3001/ui/gfx/icon_family.cc
File ui/gfx/icon_family.cc (right):

https://codereview.chromium.org/12881003/diff/3001/ui/gfx/icon_family.cc#newc...
ui/gfx/icon_family.cc:19: const gfx::ImageSkia* imageskia = icon.ToImageSkia();
Nit: You can use AsImageSkia() instead.

https://codereview.chromium.org/12881003/diff/3001/ui/gfx/icon_family.h
File ui/gfx/icon_family.h (right):

https://codereview.chromium.org/12881003/diff/3001/ui/gfx/icon_family.h#newco...
ui/gfx/icon_family.h:62: std::map<int, gfx::ImageSkia> map_;
If you want, you can make this into std::map<int, SkBitmap>.
I may be wrong, but I do not think that there are any situations where we will
be storing different bitmaps for 32x32 DIP @ 1x and 16x16 DIP @ 2x.
In the future we will potentially want to support this scenario but I think that
is quite a ways off

[Matt Giuca, 2013-03-18 07:41:16]

https://codereview.chromium.org/12881003/diff/3001/chrome/browser/ui/web_appl...
File chrome/browser/ui/web_applications/web_app_ui.cc (right):

https://codereview.chromium.org/12881003/diff/3001/chrome/browser/ui/web_appl...
chrome/browser/ui/web_applications/web_app_ui.cc:381:
info->favicon.Add(favicon_tab_helper->GetFavicon());
On 2013/03/18 03:22:43, pkotwicz wrote:
> Comment: Please let me know if you want to fix this. (Not your diff, but the
> fact that we call GetFavicon() here). We can do a lot better. I have just not
> gotten around to implementing the "a lot better".

Sorry, I'm not really sure what this code is doing. I'm just updating it to use
the new API.

https://codereview.chromium.org/12881003/diff/3001/chrome/browser/ui/web_appl...
chrome/browser/ui/web_applications/web_app_ui.cc:419: // individually inserting
them into an IconFamily.
On 2013/03/18 03:22:43, pkotwicz wrote:
> Comment: It would be nice to get rid of this hack in a separate CL :)

Yeah, although I think that will require refactoring in ImageLoader. It would
need a new method which takes a vector of ImageRepresentations and returns a
vector of ImageSkias.

https://codereview.chromium.org/12881003/diff/3001/ui/gfx/icon_family.cc
File ui/gfx/icon_family.cc (right):

https://codereview.chromium.org/12881003/diff/3001/ui/gfx/icon_family.cc#newc...
ui/gfx/icon_family.cc:19: const gfx::ImageSkia* imageskia = icon.ToImageSkia();
On 2013/03/18 03:22:43, pkotwicz wrote:
> Nit: You can use AsImageSkia() instead.

I looked at that, but it doesn't return a reference (it returns a copy) so it
seems inefficient. Also, I don't want to go adding an empty ImageSkia if it
fails; I'd rather have the if (imageskia) check and then add nothing if it
fails.

https://codereview.chromium.org/12881003/diff/3001/ui/gfx/icon_family.h
File ui/gfx/icon_family.h (right):

https://codereview.chromium.org/12881003/diff/3001/ui/gfx/icon_family.h#newco...
ui/gfx/icon_family.h:40: // The icon SHOULD be square; if not, its size will be
whichever is smaller
Note: I chatted with Ben about this some more.

He convinced me that we want to make this more general (probably rename to
ImageSet), and have Get take both a width and height parameter. I would probably
make the lookup algorithm "find the image whose width is >= desired width, and
height >= desired height" and then pick the one with the smallest ratio between
the actual and desired width/height. It's a little arbitrary for non-square
images, but for square images it's the same as this algorithm here.

https://codereview.chromium.org/12881003/diff/3001/ui/gfx/icon_family.h#newco...
ui/gfx/icon_family.h:62: std::map<int, gfx::ImageSkia> map_;
On 2013/03/18 03:22:43, pkotwicz wrote:
> If you want, you can make this into std::map<int, SkBitmap>.
> I may be wrong, but I do not think that there are any situations where we will
> be storing different bitmaps for 32x32 DIP @ 1x and 16x16 DIP @ 2x.
> In the future we will potentially want to support this scenario but I think
that
> is quite a ways off

Well we (Ben and I) designed it so that we could have multi-resolution images
for each icon size. I agree that the possibility of allowing users specifying
high-DPI versions of icons is quite far off (e.g., a 16x16 @ 2x distinct from a
32x32 @ 1x). But it is at least feasible and sensible to grab the 32x32 icon and
store it in the 16x16 @ 2x representation. That way, when Chrome asks "get me
the 16x16 icon", we will return an ImageSkia that can be shown crisply in a 2x
DPI display. So I think we want to keep the possibility open, and keep it as a
vector of ImageSkia.

Ben, what do you think?

[pkotwicz, 2013-03-18 16:30:23]

https://codereview.chromium.org/12881003/diff/3001/ui/gfx/icon_family.cc
File ui/gfx/icon_family.cc (right):

https://codereview.chromium.org/12881003/diff/3001/ui/gfx/icon_family.cc#newc...
ui/gfx/icon_family.cc:19: const gfx::ImageSkia* imageskia = icon.ToImageSkia();
ImageSkia is ref counted so a copy is not that expensive

https://codereview.chromium.org/12881003/diff/3001/ui/gfx/icon_family.h
File ui/gfx/icon_family.h (right):

https://codereview.chromium.org/12881003/diff/3001/ui/gfx/icon_family.h#newco...
ui/gfx/icon_family.h:62: std::map<int, gfx::ImageSkia> map_;
Comment: Right now I don't think we 2x images into this object. So we would need
some special logic such that storing a 32x32 @ 1x bitmap would store it as both
the 2x representation for the 16x16 ImageSkia and the 1x representation for the
32x32 ImageSkia.

[sail, 2013-03-18 17:38:22]

+rsesek who might be interested

[Matt Giuca, 2013-03-18 22:45:20]

https://codereview.chromium.org/12881003/diff/3001/ui/gfx/icon_family.cc
File ui/gfx/icon_family.cc (right):

https://codereview.chromium.org/12881003/diff/3001/ui/gfx/icon_family.cc#newc...
ui/gfx/icon_family.cc:19: const gfx::ImageSkia* imageskia = icon.ToImageSkia();
On 2013/03/18 16:30:23, pkotwicz wrote:
> ImageSkia is ref counted so a copy is not that expensive

Fair enough, but also the second reason (don't want to insert empty ImageSkias).
Unless you disagree and think that if a user added an empty image it should just
be inserted? (I suppose the Add(ImageSkia) overload behaves that way.)

https://codereview.chromium.org/12881003/diff/3001/ui/gfx/icon_family.h
File ui/gfx/icon_family.h (right):

https://codereview.chromium.org/12881003/diff/3001/ui/gfx/icon_family.h#newco...
ui/gfx/icon_family.h:62: std::map<int, gfx::ImageSkia> map_;
On 2013/03/18 16:30:23, pkotwicz wrote:
> Comment: Right now I don't think we 2x images into this object. So we would
need
> some special logic such that storing a 32x32 @ 1x bitmap would store it as
both
> the 2x representation for the 16x16 ImageSkia and the 1x representation for
the
> 32x32 ImageSkia.

Correct. But this representation would easily allow for the possibility of us
doing so (for example, there could be a method GenerateHighDPIRepresentations()
that goes through the map and then builds high-DPI versions of every image using
the larger images as a source).

[Matt Giuca, 2013-03-19 07:58:12]

pkotwicz: I've hit a stumbling block, and maybe you have an opinion about this.
In web_app_mac, there is a call to
image_skia.EnsureRepsForSupportedScaleFactors(). This call no longer makes sense
due to the IconFamily, but I thought about what it would mean if I took it out.

I'm not really sure if it works at all or what it's doing. Ben tells me that Mac
doesn't currently have high-DPI display -- is this true? If it did have more
supported scale factors than 100%, I'm not sure what it would mean since the
favicon already has all of the images loaded into the 100% version. So perhaps
it shouldn't be there in the first place.

But assuming we needed it, it's going to be a problem, because in this CL when I
create the IconFamily (in web_app_ui.cc OnImageLoaded), I lose the
ImageSkiaStorage object (if any) in the original ImageSkia created by
ImageLoader. So it is too late to call EnsureRepsForSupportedScaleFactors. If
anything, I should be calling EnsureRepsForSupportedScaleFactors on the
ImageSkia returned by ImageLoader before I split it up. Does that seem sensible?
Right now (and I don't have a full understanding of what it is doing, or what
ImageSkiaStorage is actually in this object, if any), it seems nonsensical for
that method to be called on shortcut_info.favicon at all, and I feel like I
should just delete it. Maybe you can give some advice.

[Robert Sesek, 2013-03-19 14:49:24]

I think it's good to abstract away multiresoultion file icons, but I think this
problem has some sharp edges. This looks like it's taking native images,
converting them to Skia for placement in IconFamily, and then converting them
back to native format. (Apologies if that's not correct, I'm not familiar with
these chrome/ components). If so, gfx::Image exists to avoid exactly those
conversions. Have you thought about storing gfx::Image in the map instead, so
that any other converted representations are kept?

This seems similar to gfx::ImageSkia, which is a collection of SkBitmaps that is
intelligent about scale factor. But rather than dealing with ui::ScaleFactor,
this deals with actual icon sizes. Correct?

Also, yes, Mac has high DPI displays.

[Matt Giuca, 2013-03-20 00:31:34]

On 2013/03/19 14:49:24, rsesek wrote:
> I think it's good to abstract away multiresoultion file icons, but I think
this
> problem has some sharp edges. This looks like it's taking native images,
> converting them to Skia for placement in IconFamily, and then converting them
> back to native format. (Apologies if that's not correct, I'm not familiar with
> these chrome/ components).

Well if you give it an Image it converts it to an ImageSkia, but you can also
give it an ImageSkia and it won't do any conversion. In this CL, there is no
extra conversion taking place, since I believe the ImageLoader gives it back to
me as an ImageSkia in the first place.

> If so, gfx::Image exists to avoid exactly those
> conversions. Have you thought about storing gfx::Image in the map instead, so
> that any other converted representations are kept?

That's certainly a possibility. What are the overheads associated with a
gfx::Image versus an ImageSkia? You're probably right that it's better to keep
it general.

> This seems similar to gfx::ImageSkia, which is a collection of SkBitmaps that
is
> intelligent about scale factor. But rather than dealing with ui::ScaleFactor,
> this deals with actual icon sizes. Correct?

Yes, so it avoids the awkward scenario we currently have where a single
ImageSkia holds all of the different icon sizes, and all of them are stored in
the 100P representation.

> Also, yes, Mac has high DPI displays.

OK thanks. Do you know whether it is appropriate for me to drop the call to
EnsureRepsForSupportedScaleFactors as I mentioned in the previous email? Even if
Mac has high DPI displays, I don't think we currently support high DPI favicons,
either for web pages or apps.

[Matt Giuca, 2013-03-20 08:46:07]

Hello again.

I'm just thinking about the gfx::Image thing. The problem with it is that
gfx::Image does not have a width or height. I'm not sure if this is for a
technical reason or just an oversight, but I imagine that it delegates to its
storage to deal with such things as image size, and doesn't expose those details
externally.

I won't be able to change IconFamily to use Image instead of ImageSkia unless
Image gets a public getter for width and height (since I do matching based on
the size and aspect ratio of the image).

[pkotwicz, 2013-03-20 14:12:19]

gfx::Image should have a public getter for width and height. It is something
that I mean to add when I have time.

[Robert Sesek, 2013-03-20 15:35:29]

On 2013/03/20 00:31:34, Matt Giuca wrote:
> Well if you give it an Image it converts it to an ImageSkia, but you can also
> give it an ImageSkia and it won't do any conversion. In this CL, there is no
> extra conversion taking place, since I believe the ImageLoader gives it back
to
> me as an ImageSkia in the first place.

It's unnecessary to perform an image conversion just to get the width and height
of the image. If getters for the width and height are added, as both you and
Peter suggest, then IconFamily could store gfx::Image and potentially avoid
a conversion.

> > If so, gfx::Image exists to avoid exactly those
> > conversions. Have you thought about storing gfx::Image in the map instead,
so
> > that any other converted representations are kept?
> 
> That's certainly a possibility. What are the overheads associated with a
> gfx::Image versus an ImageSkia? You're probably right that it's better to keep
> it general.

The overhead is pretty negligible -- just a map lookup if it avoids conversions.

> > Also, yes, Mac has high DPI displays.
> 
> OK thanks. Do you know whether it is appropriate for me to drop the call to
> EnsureRepsForSupportedScaleFactors as I mentioned in the previous email? Even
if
> Mac has high DPI displays, I don't think we currently support high DPI
favicons,
> either for web pages or apps.

I do not know the answer to this, know. I'd blame the file to find out who added
it
and ask them.

On 2013/03/20 14:12:19, pkotwicz wrote:
> > On 2013/03/20 08:46:07, Matt Giuca wrote:
> > Hello again.
> > 
> > I'm just thinking about the gfx::Image thing. The problem with it is that
> > gfx::Image does not have a width or height. I'm not sure if this is for a
> > technical reason or just an oversight, but I imagine that it delegates to
its
> > storage to deal with such things as image size, and doesn't expose those
details
> > externally.
> > 
> > I won't be able to change IconFamily to use Image instead of ImageSkia
unless
> > Image gets a public getter for width and height (since I do matching based
on
> > the size and aspect ratio of the image).
> 
> gfx::Image should have a public getter for width and height. It is something
> that I mean to add when I have time.

Yes, this would require adding GetHeight and GetWidth. If this operated on the
DefaultRepresentationType() ImageRep, then a conversion to ImageSkia could be
avoided.

[pkotwicz, 2013-03-20 22:19:52]

https://codereview.chromium.org/12881003/diff/35015/chrome/browser/web_applic...
File chrome/browser/web_applications/web_app_mac.mm (right):

https://codereview.chromium.org/12881003/diff/35015/chrome/browser/web_applic...
chrome/browser/web_applications/web_app_mac.mm:211: NSBitmapImageRep* image_rep
= SkBitmapToImageRep(bitmap);
There is one case where |favicon| can have an ImageSkiaRep with a scale factor
other than 1x.
FaviconTabHelper::GetFavicon() in GetShortcutInfoForTab() will return a favicon
with all of the supported scale factors.
The rule of thumb is that each call to ImageSkia::image_reps() requires a
preceding call to ImageSkia::EnsureRepsForSupportedScaleFactors().

[Matt Giuca, 2013-03-22 07:55:49]

OK I am adding support for Width and Height to gfx::Image in a separate CL (not
yet complete). Then I will change this to use Image instead of ImageSkia.

On 2013/03/20 22:19:52, pkotwicz wrote:
> There is one case where |favicon| can have an ImageSkiaRep with a scale factor
> other than 1x.
> FaviconTabHelper::GetFavicon() in GetShortcutInfoForTab() will return a
favicon
> with all of the supported scale factors.
> The rule of thumb is that each call to ImageSkia::image_reps() requires a
> preceding call to ImageSkia::EnsureRepsForSupportedScaleFactors().

Oh OK I think I've got it now. Following that rule of thumb, the call to
EnsureRepsForSupportedScaleFactors would stay in web_app_mac, but move inside
the for loop. So in the case of the Chrome app icon images (family with multiple
ImageSkias each with a single SkBitmap), EnsureRepsForSupportedScaleFactors
would not have any effect. But in the case of a tab icon (family with a single
ImageSkia with multiple DPI SkBitmaps), it would work the same way it currently
does.

Note that I would *not* follow this rule of thumb when I call image_reps() in
web_app_ui:OnImageLoaded (when I am taking the hacky ImageSkia with all images
at 100P and converting it into an ImageFamily), because it is not really a
multi-DPI ImageSkia; the ImageSkia is just being used as a convenient way to get
ImageLoader to load multiple images. Sound OK?

One last thing: I think that given how big this CL has become, I will split it
into two parts: one to introduce the new ImageFamily class (but not use it
anywhere), and another to convert the ShortcutInfo::favicon to ImageFamily (and
the many resulting changes that will ensure). Sound OK?

Thanks. Have a good weekend.

Matt

[pkotwicz, 2013-03-22 13:44:03]

The OnImageLoaded() exception sounds ok.

To give some more background:
EnsureRepsForSupportedScaleFactors() is used as protection in case the ImageSkia
was constructed using an ImageSkiaSource*. An ImageSkiaSource* allows an
ImageSkia to be lazy loaded. If the ImageSkia is lazy loaded, and no
ImageSkiaReps have been generated yet, then calling ImageSkia::image_reps() will
return an empty list regardless of whether any ImageSkiaRep could have been
generated.
Given that the hack in web_app_ui::OnImageLoaded() would not work if the
ImageSkia was lazy loaded, I am fine with not calling
EnsureRepsForSupportedScaleFactors() there.

[Matt Giuca, 2013-03-27 03:25:44]

pkotwicz: I'm still unsure about the multiple image representations in
web_app_mac. I have restored the call to EnsureRepsForSupportedScaleFactors in
Patch Set 12. However, it doesn't make sense at the moment because the very next
thing it does after that is call it->ToSkBitmap, which takes only the 100P
representation and discards the rest.

We are now dealing with a two-dimensional data structure (set of images of
different sizes x set of image representations at different DPI). Should I be
iterating over both and then exporting them all into a single set? Like:

for each Image in ImageFamily {
  for each ImageRep in Image {
    AddBitmapImageToIconFamily(ImageRep as NSBitmapImageRep)
  }
}

[pkotwicz, 2013-03-27 04:35:05]

Yes, you should be exporting them both into a single image set.
However, AddBitmapImageRepToIconFamily() does not support density information
yet so I am fine with a TODO.

[Matt Giuca, 2013-03-27 04:51:05]

On 2013/03/27 04:35:05, pkotwicz wrote:
> Yes, you should be exporting them both into a single image set.
> However, AddBitmapImageRepToIconFamily() does not support density information
> yet so I am fine with a TODO.

To clarify: do you mean that I should put them both into a single set in this
CL, and add a TODO to add density information to AddBitmapImageRepToIconFamily?
Or some other course of action?

[pkotwicz, 2013-03-27 05:16:56]

Sorry for not being clear and for the lengthiness of all of this.
I am suggesting that a TODO to add density information to
AddBitmapImageRepToIconFamily is sufficient. You do not need to do anything
else.
As you mentioned, because we are using ToSkBitmap(), it is unnecessary to use
EnsureRepsForSupportedScaleFactors().

[pkotwicz, 2013-03-27 05:20:22]

Given that we do not support density information in
AddBitmapImageRepToIconFamily(), there is no point in iterating over each of the
ImageSkiaReps in the gfx::Image.

[Matt Giuca, 2013-03-27 05:44:48]

On 2013/03/27 05:20:22, pkotwicz wrote:
> Given that we do not support density information in
> AddBitmapImageRepToIconFamily(), there is no point in iterating over each of
the
> ImageSkiaReps in the gfx::Image.

But this would seem to be a regression from the current behaviour (for tab
icons). You were saying above that "FaviconTabHelper::GetFavicon() in
GetShortcutInfoForTab() will return a favicon with all of the supported scale
factors." Which means that the current code on Mac will create an icon
containing an image for each DPI. With my current code, it will only contain the
100P image for the tab icon. Are you saying that those extra images wouldn't be
useful anyway because we don't have the density information in the image file?

I was thinking that the two-level loop approach would at least maintain parity
with the current implementation, but you seem to be suggesting we shouldn't do
that.

[pkotwicz, 2013-03-27 14:48:44]

1) I think that I am failing to communicate. Please ping me if you still have
questions.
2) I just realized that I was wrong. GetShortcutInfoForTab() is not called on
mac

[Matt Giuca, 2013-03-27 23:44:49]

On 2013/03/27 14:48:44, pkotwicz wrote:
> 1) I think that I am failing to communicate. Please ping me if you still have
> questions.

Time zone differences might make that hard (Sydney here, UTC+11). Don't worry,
this CL is blocked by my other one so it's not urgent.

> 2) I just realized that I was wrong. GetShortcutInfoForTab() is not called on
> mac

OK, if you're sure about that then it should be safe to go ahead and ignore any
non-1x images in the family, and I'll just add a TODO about adding density
information as you suggested.

[Matt Giuca, 2013-04-04 01:24:21]

This is ready for review now. The prerequisite CL
(https://codereview.chromium.org/12704026/) is in the CQ, so just assume that
ImageFamily is available. Thanks.

[benwells, 2013-04-04 04:58:29]

https://codereview.chromium.org/12881003/diff/90001/chrome/browser/ui/views/c...
File chrome/browser/ui/views/create_application_shortcut_view.cc (right):

https://codereview.chromium.org/12881003/diff/90001/chrome/browser/ui/views/c...
chrome/browser/ui/views/create_application_shortcut_view.cc:445: const
gfx::Image* icon = shortcut_info_.favicon.Get(0, 0);
So the way you get the smallest icon is to ask for an icon with size (0, 0)?
Hmm, that doesn't feel that intuitive. Maybe ImageFamily needs a GetSmallest?

https://codereview.chromium.org/12881003/diff/90001/chrome/browser/ui/web_app...
File chrome/browser/ui/web_applications/web_app_ui.cc (right):

https://codereview.chromium.org/12881003/diff/90001/chrome/browser/ui/web_app...
chrome/browser/ui/web_applications/web_app_ui.cc:427: // individually inserting
them into an ImageFamily.
Should we change LoadImagesAsync to use ImageFamily as well?

https://codereview.chromium.org/12881003/diff/90001/chrome/browser/web_applic...
File chrome/browser/web_applications/web_app_win.cc (right):

https://codereview.chromium.org/12881003/diff/90001/chrome/browser/web_applic...
chrome/browser/web_applications/web_app_win.cc:182: const gfx::Image* icon_32 =
image.Get(32, 32);
nit: add kIconSize = 32; and use that.

[Matt Giuca, 2013-04-04 05:15:35]

https://codereview.chromium.org/12881003/diff/90001/chrome/browser/ui/views/c...
File chrome/browser/ui/views/create_application_shortcut_view.cc (right):

https://codereview.chromium.org/12881003/diff/90001/chrome/browser/ui/views/c...
chrome/browser/ui/views/create_application_shortcut_view.cc:445: const
gfx::Image* icon = shortcut_info_.favicon.Get(0, 0);
On 2013/04/04 04:58:30, benwells wrote:
> So the way you get the smallest icon is to ask for an icon with size (0, 0)?
> Hmm, that doesn't feel that intuitive. Maybe ImageFamily needs a GetSmallest?

It feels intuitive to me, when you consider that the semantics of Get are "find
the smallest icon larger than this size"; then "find the smallest icon" is just
a special case where size == 0.

I'm happy to add GetSmallest if you want, but it doesn't seem to be a generally
useful feature (it's only used here because we think that there will only be one
icon).

https://codereview.chromium.org/12881003/diff/90001/chrome/browser/ui/web_app...
File chrome/browser/ui/web_applications/web_app_ui.cc (right):

https://codereview.chromium.org/12881003/diff/90001/chrome/browser/ui/web_app...
chrome/browser/ui/web_applications/web_app_ui.cc:427: // individually inserting
them into an ImageFamily.
On 2013/04/04 04:58:30, benwells wrote:
> Should we change LoadImagesAsync to use ImageFamily as well?

Yes, I intend to do this in a separate CL.

https://codereview.chromium.org/12881003/diff/90001/chrome/browser/web_applic...
File chrome/browser/web_applications/web_app_win.cc (right):

https://codereview.chromium.org/12881003/diff/90001/chrome/browser/web_applic...
chrome/browser/web_applications/web_app_win.cc:182: const gfx::Image* icon_32 =
image.Get(32, 32);
On 2013/04/04 04:58:30, benwells wrote:
> nit: add kIconSize = 32; and use that.

Done.

[pkotwicz, 2013-04-04 18:34:40]

LGTM with nits

https://codereview.chromium.org/12881003/diff/94001/chrome/browser/ui/cocoa/o...
File chrome/browser/ui/cocoa/omnibox/omnibox_view_mac_unittest.mm (right):

https://codereview.chromium.org/12881003/diff/94001/chrome/browser/ui/cocoa/o...
chrome/browser/ui/cocoa/omnibox/omnibox_view_mac_unittest.mm:15: #include
"ui/gfx/image/image.h"
Do you need this change?

https://codereview.chromium.org/12881003/diff/94001/chrome/browser/ui/gtk/cre...
File chrome/browser/ui/gtk/create_application_shortcuts_dialog_gtk.cc (right):

https://codereview.chromium.org/12881003/diff/94001/chrome/browser/ui/gtk/cre...
chrome/browser/ui/gtk/create_application_shortcuts_dialog_gtk.cc:90: if (!icon
|| icon->IsEmpty())
Should we just CHECK() instead?
gdk_pixbuf_get_width(NULL) does not seem to be supported by GTK

https://codereview.chromium.org/12881003/diff/94001/chrome/browser/ui/web_app...
File chrome/browser/ui/web_applications/web_app_ui.cc (right):

https://codereview.chromium.org/12881003/diff/94001/chrome/browser/ui/web_app...
chrome/browser/ui/web_applications/web_app_ui.cc:342: const gfx::ImageSkia&
multires_image_skia = *(image.ToImageSkia());
Nit: AsImageSkia()

https://codereview.chromium.org/12881003/diff/94001/chrome/browser/ui/web_app...
chrome/browser/ui/web_applications/web_app_ui.cc:344: // The image reps here are
not really scale factors (ImageSkia is just being
Nit: How about: "The image reps here are not really for different scale factors.
(ImageSkia is just being used as a handy container for multiple SkBitmaps.)"

https://codereview.chromium.org/12881003/diff/94001/chrome/browser/ui/web_app...
chrome/browser/ui/web_applications/web_app_ui.cc:351: const gfx::ImageSkiaRep&
image = *it;
This should not be a reference?
Nit: |image_rep|, instead of |image|. Otherwise, it is kind of hard to keep
track of what's which image type.

https://codereview.chromium.org/12881003/diff/94001/chrome/browser/web_applic...
File chrome/browser/web_applications/web_app_mac.mm (right):

https://codereview.chromium.org/12881003/diff/94001/chrome/browser/web_applic...
chrome/browser/web_applications/web_app_mac.mm:221:
it->EnsureRepsForSupportedScaleFactors();
EnsureRepsForSupportedScaleFactors() is not necessary.

[Robert Sesek, 2013-04-04 18:39:01]

https://codereview.chromium.org/12881003/diff/94001/chrome/browser/ui/gtk/cre...
File chrome/browser/ui/gtk/create_application_shortcuts_dialog_gtk.cc (right):

https://codereview.chromium.org/12881003/diff/94001/chrome/browser/ui/gtk/cre...
chrome/browser/ui/gtk/create_application_shortcuts_dialog_gtk.cc:94: GdkPixbuf*
pixbuf = gfx::GdkPixbufFromSkBitmap(*icon->ToSkBitmap());
icon->ToGdkPixbuf.

https://codereview.chromium.org/12881003/diff/94001/chrome/browser/ui/views/c...
File chrome/browser/ui/views/create_application_shortcut_view.cc (right):

https://codereview.chromium.org/12881003/diff/94001/chrome/browser/ui/views/c...
chrome/browser/ui/views/create_application_shortcut_view.cc:447: SkBitmap();
Can use AsSkBitmap() to do this.

https://codereview.chromium.org/12881003/diff/94001/chrome/browser/web_applic...
File chrome/browser/web_applications/web_app_mac.mm (right):

https://codereview.chromium.org/12881003/diff/94001/chrome/browser/web_applic...
chrome/browser/web_applications/web_app_mac.mm:224: NSBitmapImageRep* image_rep
= SkBitmapToImageRep(*it->ToSkBitmap());
ToNSImage and get the bitmap reps via the NSImage API?

https://codereview.chromium.org/12881003/diff/94001/chrome/browser/web_applic...
File chrome/browser/web_applications/web_app_mac_unittest.mm (right):

https://codereview.chromium.org/12881003/diff/94001/chrome/browser/web_applic...
chrome/browser/web_applications/web_app_mac_unittest.mm:141:
ui::ResourceBundle::GetSharedInstance().GetImageNamed(
GetNativeImageNamed

https://codereview.chromium.org/12881003/diff/94001/chrome/browser/web_applic...
chrome/browser/web_applications/web_app_mac_unittest.mm:153:
EXPECT_EQ(product_logo.ToSkBitmap()->width(), [image size].width);
Use ->Width() and ->Height()

https://codereview.chromium.org/12881003/diff/94001/chrome/browser/web_applic...
File chrome/browser/web_applications/web_app_win.cc (right):

https://codereview.chromium.org/12881003/diff/94001/chrome/browser/web_applic...
chrome/browser/web_applications/web_app_win.cc:189: SkBitmap();
AsSkBitmap()

[benwells, 2013-04-04 23:52:35]

https://codereview.chromium.org/12881003/diff/90001/chrome/browser/ui/views/c...
File chrome/browser/ui/views/create_application_shortcut_view.cc (right):

https://codereview.chromium.org/12881003/diff/90001/chrome/browser/ui/views/c...
chrome/browser/ui/views/create_application_shortcut_view.cc:445: const
gfx::Image* icon = shortcut_info_.favicon.Get(0, 0);
On 2013/04/04 05:15:36, Matt Giuca wrote:
> On 2013/04/04 04:58:30, benwells wrote:
> > So the way you get the smallest icon is to ask for an icon with size (0, 0)?
> > Hmm, that doesn't feel that intuitive. Maybe ImageFamily needs a
GetSmallest?
> 
> It feels intuitive to me, when you consider that the semantics of Get are
"find
> the smallest icon larger than this size"; then "find the smallest icon" is
just
> a special case where size == 0.
> 
> I'm happy to add GetSmallest if you want, but it doesn't seem to be a
generally
> useful feature (it's only used here because we think that there will only be
one
> icon).

Ah, I see. Perhaps Get is unintuitive then. I would expect it to get me
something at that size, and if there is nothing available resize something to
that size.

Maybe Get should be called GetNearest or GetClosest or GetBestMatching or
something.

[Matt Giuca, 2013-04-05 06:29:54]

On 2013/04/04 23:52:35, benwells wrote:
> Ah, I see. Perhaps Get is unintuitive then. I would expect it to get me
> something at that size, and if there is nothing available resize something to
> that size.
> 
> Maybe Get should be called GetNearest or GetClosest or GetBestMatching or
> something.

Maybe. But that CL is already submitted and I don't think it makes sense to
change it in this CL. I think I'll send out another CL to rename it because I do
think this is important. And it should be submitted before this one because
otherwise renaming it will be a much bigger job.

[Matt Giuca, 2013-04-05 06:30:12]

https://codereview.chromium.org/12881003/diff/94001/chrome/browser/ui/cocoa/o...
File chrome/browser/ui/cocoa/omnibox/omnibox_view_mac_unittest.mm (right):

https://codereview.chromium.org/12881003/diff/94001/chrome/browser/ui/cocoa/o...
chrome/browser/ui/cocoa/omnibox/omnibox_view_mac_unittest.mm:15: #include
"ui/gfx/image/image.h"
On 2013/04/04 18:34:40, pkotwicz wrote:
> Do you need this change?

Yes. This stopped compiling when I removed the image.h include from one of the
other .h files. The correct solution is for this module to include what it uses.

https://codereview.chromium.org/12881003/diff/94001/chrome/browser/ui/gtk/cre...
File chrome/browser/ui/gtk/create_application_shortcuts_dialog_gtk.cc (right):

https://codereview.chromium.org/12881003/diff/94001/chrome/browser/ui/gtk/cre...
chrome/browser/ui/gtk/create_application_shortcuts_dialog_gtk.cc:90: if (!icon
|| icon->IsEmpty())
On 2013/04/04 18:34:40, pkotwicz wrote:
> Should we just CHECK() instead?
> gdk_pixbuf_get_width(NULL) does not seem to be supported by GTK

Done.

Note that the existing code didn't work properly if image was empty, because
ToGdkPixbuf is not supposed to be called when the image is empty. So the CHECK
is the right thing to do. web_app_ui guarantees that the icon will not be empty,
since it will use the placeholder icon for apps without icons.

https://codereview.chromium.org/12881003/diff/94001/chrome/browser/ui/gtk/cre...
chrome/browser/ui/gtk/create_application_shortcuts_dialog_gtk.cc:94: GdkPixbuf*
pixbuf = gfx::GdkPixbufFromSkBitmap(*icon->ToSkBitmap());
On 2013/04/04 18:39:02, rsesek wrote:
> icon->ToGdkPixbuf.

Done.

https://codereview.chromium.org/12881003/diff/94001/chrome/browser/ui/views/c...
File chrome/browser/ui/views/create_application_shortcut_view.cc (right):

https://codereview.chromium.org/12881003/diff/94001/chrome/browser/ui/views/c...
chrome/browser/ui/views/create_application_shortcut_view.cc:447: SkBitmap();
On 2013/04/04 18:39:02, rsesek wrote:
> Can use AsSkBitmap() to do this.

Done.

https://codereview.chromium.org/12881003/diff/94001/chrome/browser/ui/web_app...
File chrome/browser/ui/web_applications/web_app_ui.cc (right):

https://codereview.chromium.org/12881003/diff/94001/chrome/browser/ui/web_app...
chrome/browser/ui/web_applications/web_app_ui.cc:342: const gfx::ImageSkia&
multires_image_skia = *(image.ToImageSkia());
On 2013/04/04 18:34:40, pkotwicz wrote:
> Nit: AsImageSkia()

Done.

https://codereview.chromium.org/12881003/diff/94001/chrome/browser/ui/web_app...
chrome/browser/ui/web_applications/web_app_ui.cc:344: // The image reps here are
not really scale factors (ImageSkia is just being
On 2013/04/04 18:34:40, pkotwicz wrote:
> Nit: How about: "The image reps here are not really for different scale
factors.
> (ImageSkia is just being used as a handy container for multiple SkBitmaps.)"

Done.

https://codereview.chromium.org/12881003/diff/94001/chrome/browser/ui/web_app...
chrome/browser/ui/web_applications/web_app_ui.cc:351: const gfx::ImageSkiaRep&
image = *it;
On 2013/04/04 18:34:40, pkotwicz wrote:
> This should not be a reference?
> Nit: |image_rep|, instead of |image|. Otherwise, it is kind of hard to keep
> track of what's which image type.

I just deleted the variable.

https://codereview.chromium.org/12881003/diff/94001/chrome/browser/web_applic...
File chrome/browser/web_applications/web_app_mac.mm (right):

https://codereview.chromium.org/12881003/diff/94001/chrome/browser/web_applic...
chrome/browser/web_applications/web_app_mac.mm:221:
it->EnsureRepsForSupportedScaleFactors();
On 2013/04/04 18:34:40, pkotwicz wrote:
> EnsureRepsForSupportedScaleFactors() is not necessary.

:O I thought I removed this.

https://codereview.chromium.org/12881003/diff/94001/chrome/browser/web_applic...
chrome/browser/web_applications/web_app_mac.mm:224: NSBitmapImageRep* image_rep
= SkBitmapToImageRep(*it->ToSkBitmap());
On 2013/04/04 18:39:02, rsesek wrote:
> ToNSImage and get the bitmap reps via the NSImage API?

Done. But note that SkBitmapToImageRep called SkBitmapToNSImageWithColorSpace,
whereas ToNSImage simply calls NSImageFromImageSkia. I'm not sure what the
ramifications are of not explicitly specifying a color space. Do you know?

https://codereview.chromium.org/12881003/diff/94001/chrome/browser/web_applic...
File chrome/browser/web_applications/web_app_mac_unittest.mm (right):

https://codereview.chromium.org/12881003/diff/94001/chrome/browser/web_applic...
chrome/browser/web_applications/web_app_mac_unittest.mm:141:
ui::ResourceBundle::GetSharedInstance().GetImageNamed(
On 2013/04/04 18:39:02, rsesek wrote:
> GetNativeImageNamed

Done.

https://codereview.chromium.org/12881003/diff/94001/chrome/browser/web_applic...
chrome/browser/web_applications/web_app_mac_unittest.mm:153:
EXPECT_EQ(product_logo.ToSkBitmap()->width(), [image size].width);
On 2013/04/04 18:39:02, rsesek wrote:
> Use ->Width() and ->Height()

Done.

https://codereview.chromium.org/12881003/diff/94001/chrome/browser/web_applic...
File chrome/browser/web_applications/web_app_win.cc (right):

https://codereview.chromium.org/12881003/diff/94001/chrome/browser/web_applic...
chrome/browser/web_applications/web_app_win.cc:189: SkBitmap();
On 2013/04/04 18:39:02, rsesek wrote:
> AsSkBitmap()

Done.

[benwells, 2013-04-05 06:38:03]

lgtm

[Matt Giuca, 2013-04-06 11:49:58]

Adding sky for OWNERS.

[Matt Giuca, 2013-04-08 09:13:36]

I had a bug in the Mac version which I've fixed in PS 22. Please take a cost
look at the delta in PS 22 since I am new to Obj-C. I have (finally) tested this
on a real Mac and it appears to work.

[Robert Sesek, 2013-04-08 13:16:39]

https://codereview.chromium.org/12881003/diff/158001/chrome/browser/ui/gtk/cr...
File chrome/browser/ui/gtk/create_application_shortcuts_dialog_gtk.cc (right):

https://codereview.chromium.org/12881003/diff/158001/chrome/browser/ui/gtk/cr...
chrome/browser/ui/gtk/create_application_shortcuts_dialog_gtk.cc:92:
CHECK(!icon->IsEmpty());
Can remove this CHECK, since ToGdkPixbuf will do it, too.

https://codereview.chromium.org/12881003/diff/158001/chrome/browser/web_appli...
File chrome/browser/web_applications/web_app_mac.mm (right):

https://codereview.chromium.org/12881003/diff/158001/chrome/browser/web_appli...
chrome/browser/web_applications/web_app_mac.mm:34: NSBitmapImageRep*
NSImageGet100PRepresentation(const NSImage* image) {
NSObjects don't have any correct notion of constness, so just NSImage here.

https://codereview.chromium.org/12881003/diff/158001/chrome/browser/web_appli...
File chrome/browser/web_applications/web_app_win.cc (right):

https://codereview.chromium.org/12881003/diff/158001/chrome/browser/web_appli...
chrome/browser/web_applications/web_app_win.cc:188: SkBitmap bitmap = icon ?
icon->AsBitmap() : SkBitmap();
If the icon is empty, return false early?

[Matt Giuca, 2013-04-09 00:00:45]

https://codereview.chromium.org/12881003/diff/158001/chrome/browser/ui/gtk/cr...
File chrome/browser/ui/gtk/create_application_shortcuts_dialog_gtk.cc (right):

https://codereview.chromium.org/12881003/diff/158001/chrome/browser/ui/gtk/cr...
chrome/browser/ui/gtk/create_application_shortcuts_dialog_gtk.cc:92:
CHECK(!icon->IsEmpty());
On 2013/04/08 13:16:39, rsesek wrote:
> Can remove this CHECK, since ToGdkPixbuf will do it, too.

Done.

https://codereview.chromium.org/12881003/diff/158001/chrome/browser/web_appli...
File chrome/browser/web_applications/web_app_mac.mm (right):

https://codereview.chromium.org/12881003/diff/158001/chrome/browser/web_appli...
chrome/browser/web_applications/web_app_mac.mm:34: NSBitmapImageRep*
NSImageGet100PRepresentation(const NSImage* image) {
On 2013/04/08 13:16:39, rsesek wrote:
> NSObjects don't have any correct notion of constness, so just NSImage here.

Done.

https://codereview.chromium.org/12881003/diff/158001/chrome/browser/web_appli...
File chrome/browser/web_applications/web_app_win.cc (right):

https://codereview.chromium.org/12881003/diff/158001/chrome/browser/web_appli...
chrome/browser/web_applications/web_app_win.cc:188: SkBitmap bitmap = icon ?
icon->AsBitmap() : SkBitmap();
On 2013/04/08 13:16:39, rsesek wrote:
> If the icon is empty, return false early?

Returning false is supposed to occur only if there was a problem. Note that the
caller aborts saving the shortcut if this returns false. Saving an empty icon
isn't a serious problem, so I think we should continue with an empty bitmap.

[Matt Giuca, 2013-04-10 00:17:53]

Robert and Scott: Friendly ping.

[sky, 2013-04-10 00:44:08]

What files do you need me to review?

[Matt Giuca, 2013-04-10 01:07:58]

On 2013/04/10 00:44:08, sky wrote:
> What files do you need me to review?

Unfortunately, all of them except chrome/browser/web_applications (which Ben has
covered).

Note that this CL has already undergone extensive review by benwells, pkotwicz
and rsesek, so there shouldn't be too much to do.

Thanks.

[Robert Sesek, 2013-04-10 13:20:17]

LGTM

https://codereview.chromium.org/12881003/diff/192001/chrome/browser/ui/gtk/cr...
File chrome/browser/ui/gtk/create_application_shortcuts_dialog_gtk.cc (right):

https://codereview.chromium.org/12881003/diff/192001/chrome/browser/ui/gtk/cr...
chrome/browser/ui/gtk/create_application_shortcuts_dialog_gtk.cc:92: GdkPixbuf*
pixbuf = icon->ToGdkPixbuf();
Actually, since you're keeping a reference to this, use CopyGdkPixbuf and remove
line 93.

https://codereview.chromium.org/12881003/diff/192001/chrome/browser/ui/web_ap...
File chrome/browser/ui/web_applications/web_app_ui.cc (right):

https://codereview.chromium.org/12881003/diff/192001/chrome/browser/ui/web_ap...
chrome/browser/ui/web_applications/web_app_ui.cc:343: // As described in
UpdateShortcutInfoAndIconForApp, image contains all of
Would it make sense to fix this in a TODO?

[sky, 2013-04-10 14:29:02]

LGTM

[Matt Giuca, 2013-04-11 00:55:22]

https://codereview.chromium.org/12881003/diff/192001/chrome/browser/ui/gtk/cr...
File chrome/browser/ui/gtk/create_application_shortcuts_dialog_gtk.cc (right):

https://codereview.chromium.org/12881003/diff/192001/chrome/browser/ui/gtk/cr...
chrome/browser/ui/gtk/create_application_shortcuts_dialog_gtk.cc:92: GdkPixbuf*
pixbuf = icon->ToGdkPixbuf();
On 2013/04/10 13:20:17, rsesek wrote:
> Actually, since you're keeping a reference to this, use CopyGdkPixbuf and
remove
> line 93.

Done.

https://codereview.chromium.org/12881003/diff/192001/chrome/browser/ui/web_ap...
File chrome/browser/ui/web_applications/web_app_ui.cc (right):

https://codereview.chromium.org/12881003/diff/192001/chrome/browser/ui/web_ap...
chrome/browser/ui/web_applications/web_app_ui.cc:343: // As described in
UpdateShortcutInfoAndIconForApp, image contains all of
On 2013/04/10 13:20:17, rsesek wrote:
> Would it make sense to fix this in a TODO?

Done.

[commit-bot: I haz the power, 2013-04-11 01:03:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mgiuca@chromium.org/12881003/205001

[commit-bot: I haz the power, 2013-04-11 03:33:03]

Change committed as 193558