Change return type of FaviconTabHelper::GetFavicon() to gfx::Image.

Change return type of FaviconTabHelper::GetFavicon() to gfx::Image.

No intended behavior change for now.
Depends on http://codereview.chromium.org/10832128/
Part of supporting hidpi favicons in the tab strip.

BUG=138550
TBR=joi

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

[jennb, 2012-08-03 16:55:36]

http://codereview.chromium.org/10837090/diff/6009/chrome/browser/ui/panels/pa...
File chrome/browser/ui/panels/panel_host.cc (right):

http://codereview.chromium.org/10837090/diff/6009/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_host.cc:66: // TODO: Make this function return
gfx::Image.
What's needed to make this return gfx::Image? Simple enough to do in this patch?
Would really appreciate that - I've been waiting for the favicon SkBitmap->Image
changeover...

[pkotwicz, 2012-08-03 17:07:43]

LGTM with nits

http://codereview.chromium.org/10837090/diff/6009/chrome/browser/ui/views/hun...
File chrome/browser/ui/views/hung_renderer_view.cc (right):

http://codereview.chromium.org/10837090/diff/6009/chrome/browser/ui/views/hun...
chrome/browser/ui/views/hung_renderer_view.cc:123: return
*tab_observers_[row]->favicon_tab_helper()->GetFavicon().ToImageSkia();
Use AsImageSkia() instead to be consistent?

[sky, 2012-08-03 17:32:02]

LGTM

[Nico, 2012-08-03 19:09:53]

Thanks!

http://codereview.chromium.org/10837090/diff/6009/chrome/browser/ui/panels/pa...
File chrome/browser/ui/panels/panel_host.cc (right):

http://codereview.chromium.org/10837090/diff/6009/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_host.cc:66: // TODO: Make this function return
gfx::Image.
On 2012/08/03 16:55:36, jennb wrote:
> What's needed to make this return gfx::Image? Simple enough to do in this
patch?
> Would really appreciate that - I've been waiting for the favicon
SkBitmap->Image
> changeover...

I don't know how involved that is. I'd prefer not to do it in this patch ('cause
I don't know how much work it'd be).

http://codereview.chromium.org/10837090/diff/6009/chrome/browser/ui/views/hun...
File chrome/browser/ui/views/hung_renderer_view.cc (right):

http://codereview.chromium.org/10837090/diff/6009/chrome/browser/ui/views/hun...
chrome/browser/ui/views/hung_renderer_view.cc:123: return
*tab_observers_[row]->favicon_tab_helper()->GetFavicon().ToImageSkia();
On 2012/08/03 17:07:44, pkotwicz wrote:
> Use AsImageSkia() instead to be consistent?

Done.

[commit-bot: I haz the power, 2012-08-03 19:10:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/thakis@chromium.org/10837090/24

[commit-bot: I haz the power, 2012-08-03 19:10:48]

Presubmit check for 10837090-24 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    content/public/browser

Presubmit checks took 1.7s to calculate.

[Nico, 2012-08-03 19:16:52]

joi: another OWNERS stamp, please.

[commit-bot: I haz the power, 2012-08-03 19:19:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/thakis@chromium.org/10837090/3030

[commit-bot: I haz the power, 2012-08-03 19:34:58]

Try job failure for 10837090-3030 (retry) on mac for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac&number...

[Robert Sesek, 2012-08-06 19:04:20]

+1 gfx::Image-ing favicons.

-1 ui/gfx/image changes. This is a lot of unnecessary API surface. A lot of the
places you use this already IsEmpty() check the image. And the places that don't
look temporary until this is fully gfx::Image'd.

[Nico, 2012-08-06 19:26:39]

On 2012/08/06 19:04:20, rsesek wrote:
> +1 gfx::Image-ing favicons.
> 
> -1 ui/gfx/image changes. This is a lot of unnecessary API surface. A lot of
the
> places you use this already IsEmpty() check the image. And the places that
don't
> look temporary until this is fully gfx::Image'd.

If you worry about api surface, i'd say the existing functions should behave
like the current functions instead. Null objects are much nicer to work with
than having to do IsEmpty() calls all over the place.

[Robert Sesek, 2012-08-06 19:36:06]

On 2012/08/06 19:26:39, Nico wrote:
> On 2012/08/06 19:04:20, rsesek wrote:
> > +1 gfx::Image-ing favicons.
> > 
> > -1 ui/gfx/image changes. This is a lot of unnecessary API surface. A lot of
> the
> > places you use this already IsEmpty() check the image. And the places that
> don't
> > look temporary until this is fully gfx::Image'd.
> 
> If you worry about api surface, i'd say the existing functions should behave
> like the current functions instead. Null objects are much nicer to work with
> than having to do IsEmpty() calls all over the place.

Perhaps that change should be made; they CHECK because for a while there was no
concept of an IsEmpty image. But these changes to gfx::Image are not right and
should be reverted before other clients start calling them.

[Nico, 2012-08-06 21:47:47]

On 2012/08/06 19:36:06, rsesek wrote:
> On 2012/08/06 19:26:39, Nico wrote:
> > On 2012/08/06 19:04:20, rsesek wrote:
> > > +1 gfx::Image-ing favicons.
> > > 
> > > -1 ui/gfx/image changes. This is a lot of unnecessary API surface. A lot
of
> > the
> > > places you use this already IsEmpty() check the image. And the places that
> > don't
> > > look temporary until this is fully gfx::Image'd.
> > 
> > If you worry about api surface, i'd say the existing functions should behave
> > like the current functions instead. Null objects are much nicer to work with
> > than having to do IsEmpty() calls all over the place.
> 
> Perhaps that change should be made

I'll look at this. Ping me if it's not done by tomorrow morning.

> they CHECK because for a while there was no
> concept of an IsEmpty image. But these changes to gfx::Image are not right and
> should be reverted before other clients start calling them.

I don't think this is revert-worthy.

[Robert Sesek, 2012-08-09 19:20:27]

On 2012/08/06 21:47:47, Nico wrote:
> On 2012/08/06 19:36:06, rsesek wrote:
> > On 2012/08/06 19:26:39, Nico wrote:
> > > On 2012/08/06 19:04:20, rsesek wrote:
> > > > +1 gfx::Image-ing favicons.
> > > > 
> > > > -1 ui/gfx/image changes. This is a lot of unnecessary API surface. A lot
> of
> > > the
> > > > places you use this already IsEmpty() check the image. And the places
that
> > > don't
> > > > look temporary until this is fully gfx::Image'd.
> > > 
> > > If you worry about api surface, i'd say the existing functions should
behave
> > > like the current functions instead. Null objects are much nicer to work
with
> > > than having to do IsEmpty() calls all over the place.
> > 
> > Perhaps that change should be made
> 
> I'll look at this. Ping me if it's not done by tomorrow morning.

Ping

> 
> > they CHECK because for a while there was no
> > concept of an IsEmpty image. But these changes to gfx::Image are not right
and
> > should be reverted before other clients start calling them.
> 
> I don't think this is revert-worthy.

[Robert Sesek, 2012-08-13 22:12:16]

On 2012/08/09 19:20:27, rsesek wrote:
> On 2012/08/06 21:47:47, Nico wrote:
> > On 2012/08/06 19:36:06, rsesek wrote:
> > > On 2012/08/06 19:26:39, Nico wrote:
> > > > On 2012/08/06 19:04:20, rsesek wrote:
> > > > > +1 gfx::Image-ing favicons.
> > > > > 
> > > > > -1 ui/gfx/image changes. This is a lot of unnecessary API surface. A
lot
> > of
> > > > the
> > > > > places you use this already IsEmpty() check the image. And the places
> that
> > > > don't
> > > > > look temporary until this is fully gfx::Image'd.
> > > > 
> > > > If you worry about api surface, i'd say the existing functions should
> behave
> > > > like the current functions instead. Null objects are much nicer to work
> with
> > > > than having to do IsEmpty() calls all over the place.
> > > 
> > > Perhaps that change should be made
> > 
> > I'll look at this. Ping me if it's not done by tomorrow morning.
> 
> Ping
> 
> > 
> > > they CHECK because for a while there was no
> > > concept of an IsEmpty image. But these changes to gfx::Image are not right
> and
> > > should be reverted before other clients start calling them.
> > 
> > I don't think this is revert-worthy.

* P I N G *

I'm starting to think it is.

[Nico, 2012-08-13 22:15:27]

On Mon, Aug 13, 2012 at 3:12 PM,  <rsesek@chromium.org> wrote:
> On 2012/08/09 19:20:27, rsesek wrote:
>>
>> On 2012/08/06 21:47:47, Nico wrote:
>> > On 2012/08/06 19:36:06, rsesek wrote:
>> > > On 2012/08/06 19:26:39, Nico wrote:
>> > > > On 2012/08/06 19:04:20, rsesek wrote:
>> > > > > +1 gfx::Image-ing favicons.
>> > > > >
>> > > > > -1 ui/gfx/image changes. This is a lot of unnecessary API surface.
>> > > > > A
>
> lot
>>
>> > of
>> > > > the
>> > > > > places you use this already IsEmpty() check the image. And the
>> > > > > places
>> that
>> > > > don't
>> > > > > look temporary until this is fully gfx::Image'd.
>> > > >
>> > > > If you worry about api surface, i'd say the existing functions
>> > > > should
>> behave
>> > > > like the current functions instead. Null objects are much nicer to
>> > > > work
>> with
>> > > > than having to do IsEmpty() calls all over the place.
>> > >
>> > > Perhaps that change should be made
>> >
>> > I'll look at this. Ping me if it's not done by tomorrow morning.
>
>
>> Ping
>
>
>> >
>> > > they CHECK because for a while there was no
>> > > concept of an IsEmpty image. But these changes to gfx::Image are not
>> > > right
>> and
>> > > should be reverted before other clients start calling them.
>> >
>> > I don't think this is revert-worthy.
>
>
> * P I N G *

I have prototyped the "make all the calls explicit", it's somewhere on
my codereview. I have the other variant halfway done in a local branch
(I have another local branch to delete all the CopyToFoo() functions,
which appear even more pointless).

> I'm starting to think it is.

Uh huh.

>
> https://chromiumcodereview.appspot.com/10837090/