Views: Custom drawing for GAIA avatar pictures

Views: Custom drawing for GAIA avatar pictures 

This CL updates the way we draw GAIA avatar pictures in the title bar and in the avatar menu bubble. 

Screenshots:
  normal: http://imgur.com/zkzl1,6tZfR#0
  maximized: http://imgur.com/zkzl1,6tZfR#1

BUG=91241
TEST=

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=112171

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=112177

[sail, 2011-11-26 06:59:04]

http://codereview.chromium.org/8709001/diff/1/chrome/browser/ui/views/avatar_...
File chrome/browser/ui/views/avatar_menu_bubble_view.cc (left):

http://codereview.chromium.org/8709001/diff/1/chrome/browser/ui/views/avatar_...
chrome/browser/ui/views/avatar_menu_bubble_view.cc:236: icon.width(),
icon.height(), 0, 0, kIconWidth, height());
This old code had a bug in it. It assumed that the image_view just contained the
avatar icon and was the icon width and height.

For active profiles this was not the case. For active profiles the view
contained the avatar icon and the badge on the bottom corner. This mean that the
view was wider than the icon width.

This bug resulted in the image being scaled and blurry. To fix this I'm forcing
the code to use the image view's preferred size for the active profile. This is
safe since the we explicitly scale the avatar icon before badging it.

[Peter Kasting, 2011-11-28 22:01:36]

LGTM

http://codereview.chromium.org/8709001/diff/1/chrome/browser/ui/views/avatar_...
File chrome/browser/ui/views/avatar_menu_button.cc (right):

http://codereview.chromium.org/8709001/diff/1/chrome/browser/ui/views/avatar_...
chrome/browser/ui/views/avatar_menu_button.cc:60: int x = 2;
Nit: Wouldn't it make more sense to do something like:

      int min_dimension =
          std::min(profiles::kAvatarIconWidth, profiles::kAvatarIconHeight);
      bitmap->extractSubset(&squarer_bitmap, SkIRect::MakeXYWH(
          (profiles::kAvatarIconWidth - min_dimension) / 2,
          (profiles::kAvatarIconHeight - min_dimension) / 2, min_dimension,
          min_dimension));

This would be robust against any size changes.

http://codereview.chromium.org/8709001/diff/1/chrome/browser/ui/views/avatar_...
chrome/browser/ui/views/avatar_menu_button.cc:148: icon_.reset(new
gfx::Image(icon));
Nit: Why use a scoped_ptr here?  Why not just use an object and say "icon_ =
icon;"?

[sail, 2011-11-28 23:09:36]

http://codereview.chromium.org/8709001/diff/1/chrome/browser/ui/views/avatar_...
File chrome/browser/ui/views/avatar_menu_button.cc (right):

http://codereview.chromium.org/8709001/diff/1/chrome/browser/ui/views/avatar_...
chrome/browser/ui/views/avatar_menu_button.cc:60: int x = 2;
On 2011/11/28 22:01:36, Peter Kasting wrote:
> Nit: Wouldn't it make more sense to do something like:
> 
>       int min_dimension =
>           std::min(profiles::kAvatarIconWidth, profiles::kAvatarIconHeight);
>       bitmap->extractSubset(&squarer_bitmap, SkIRect::MakeXYWH(
>           (profiles::kAvatarIconWidth - min_dimension) / 2,
>           (profiles::kAvatarIconHeight - min_dimension) / 2, min_dimension,
>           min_dimension));
> 
> This would be robust against any size changes.

I think that's a good idea. I'm working on a separate CL to clean up this
function. Mind if I do this change in that CL?

http://codereview.chromium.org/8709001/diff/1/chrome/browser/ui/views/avatar_...
chrome/browser/ui/views/avatar_menu_button.cc:148: icon_.reset(new
gfx::Image(icon));
On 2011/11/28 22:01:36, Peter Kasting wrote:
> Nit: Why use a scoped_ptr here?  Why not just use an object and say "icon_ =
> icon;"?

gfx::Image doesn't have a default constructor so it's hard to use a object
variable here.

[Peter Kasting, 2011-11-29 00:49:48]

On 2011/11/28 23:09:36, sail wrote:
> On 2011/11/28 22:01:36, Peter Kasting wrote:
> > This would be robust against any size changes.
> 
> I think that's a good idea. I'm working on a separate CL to clean up this
> function. Mind if I do this change in that CL?

Whatever you like.

> On 2011/11/28 22:01:36, Peter Kasting wrote:
> > Nit: Why use a scoped_ptr here?  Why not just use an object and say "icon_ =
> > icon;"?
> 
> gfx::Image doesn't have a default constructor so it's hard to use a object
> variable here.

Ah.

I wonder if it would be sane to try and create a NULL constructor.  SkBitmap has
one so it seems like gfx::Image could have one that meant something similar. 
Outside the scope of this change.  Up to you whether you want to comment on the
declaration why we use a scoped_ptr.

[Peter Kasting, 2011-11-29 00:49:48]

On 2011/11/28 23:09:36, sail wrote:
> On 2011/11/28 22:01:36, Peter Kasting wrote:
> > This would be robust against any size changes.
> 
> I think that's a good idea. I'm working on a separate CL to clean up this
> function. Mind if I do this change in that CL?

Whatever you like.

> On 2011/11/28 22:01:36, Peter Kasting wrote:
> > Nit: Why use a scoped_ptr here?  Why not just use an object and say "icon_ =
> > icon;"?
> 
> gfx::Image doesn't have a default constructor so it's hard to use a object
> variable here.

Ah.

I wonder if it would be sane to try and create a NULL constructor.  SkBitmap has
one so it seems like gfx::Image could have one that meant something similar. 
Outside the scope of this change.  Up to you whether you want to comment on the
declaration why we use a scoped_ptr.