Add avatar decoration to the windows7 taskbar

Add avatar decoration to the windows7 taskbar

-So you can find your window without playing wack-a-mole.

The change is windows 7 specific, because it is easy, we don't have to
to create .ico files, manage them, jump to the file thread to read
them etc.

BUG=94808
TEST=see bug
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=99272

[sail, 2011-08-31 14:55:02]

Awesome! Adding Peter for the Windows UI review.

As for the profiles stuff, I think you need to make two changes:
  #1 I don't think putting this in the menu button class will work. The button
won't be created in various cases (i.e. the browser window is a popup, or
incognito, etc...) I'm not sure what a better place would be, maybe
browser_view.cc?
  #2 You need to handle the case of the icon changing. This is really simple,
just observe chrome::NOTIFICATION_PROFILE_CACHED_INFO_CHANGED. See the following
for an example:
 
http://codesearch.google.com/codesearch#OAMlx_jo-ck/src/chrome/browser/ui/vie...

[sail, 2011-08-31 14:58:14]

Also, if you move this out for avatar button class then you'll have to add some
tests to make sure that you want to badge the icon. I think the following should
be sufficient:
  ProfileManager::IsMultipleProfilesEnabled() &&
 
g_browser_process->profile_manager()->GetProfileInfoCache().GetNumberOfProfiles()
> 1


Oh, a quick question. With this patch, will all the browser windows of a single
profile still get grouped correctly?

Thanks!

[Peter Kasting, 2011-08-31 16:59:46]

LGTM but sail's comments seem important

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

http://codereview.chromium.org/7778025/diff/1/chrome/browser/ui/views/avatar_...
chrome/browser/ui/views/avatar_menu_button.cc:39: HRESULT result =
taskbar.CreateInstance(CLSID_TaskbarList, NULL,
Nit: Shorter:

  if (FAILED(taskbar.CreateInstance(...)) ||
      FAILED(taskbar->HrInit()))
    return;

http://codereview.chromium.org/7778025/diff/1/chrome/browser/ui/views/avatar_...
chrome/browser/ui/views/avatar_menu_button.cc:46: BrowserWindow* bw =
browser->window();
Nit: Do we really need to check window() and GetNativeHandle()?  Seems like they
should be guaranteed to be non-NULL if we only call this when painting the
button?

http://codereview.chromium.org/7778025/diff/1/chrome/browser/ui/views/avatar_...
chrome/browser/ui/views/avatar_menu_button.cc:61: ::DestroyIcon(icon);
Nit: I think you can get away without :: here

http://codereview.chromium.org/7778025/diff/1/chrome/browser/ui/views/avatar_...
chrome/browser/ui/views/avatar_menu_button.cc:102: if (first_time_) {
Nit: Sounds like this variable should be named |should_draw_taskbar_decoration_|
or similar.

[cpu_(ooo_6.6-7.5), 2011-08-31 18:45:43]

On 2011/08/31 14:55:02, sail wrote:
> Awesome! Adding Peter for the Windows UI review.
> 
> As for the profiles stuff, I think you need to make two changes:
>   #1 I don't think putting this in the menu button class will work. The button
> won't be created in various cases (i.e. the browser window is a popup, or
> incognito, etc...) I'm not sure what a better place would be, maybe
> browser_view.cc?

>   #2 You need to handle the case of the icon changing. This is really simple,
> just observe chrome::NOTIFICATION_PROFILE_CACHED_INFO_CHANGED. See the
following
> for an example:
>  

Good observations. Answers and more questions here:

1) Popups : We don't have avatar/profile visual cues for them anyway. What is
the plan here?
2) Grouping: Yes grouping works with my current patch. In fact this sort of
works for the popups as well since they get grouped into the decorated stack.
3) Incognito: Yes it works[1], currently the incognito decoration is the only
one shown (see screenshot in the bug), ideally you have both but I don't want
that kind of complexity on the first patch.
4) icon changing: good point I'll try a fix.

I'll try to move the code somewhere else.

[1] and in fact even you don't have multiple profiles it shows the incognito
decoration!




>
http://codesearch.google.com/codesearch#OAMlx_jo-ck/src/chrome/browser/ui/vie...

[sail, 2011-08-31 18:57:53]

> 1) Popups : We don't have avatar/profile visual cues for them anyway. What is
> the plan here?
I think we want popups, normal windows, and incognito windows all to be badged
with the profile's avatar icon.
Actually, I think once you move this out of the menu button class you'll get
this for free. Given a browser window, you can get the profile's avatar icon
like this:
  ProfileInfoCache& cache =
g_browser_process->profile_manager()->GetProfileInfoCache();
  size_t profile_index =
cache.GetIndexOfProfileWithPath(browser()->profile()->GetPath())
  if (profile_index != std::string::npos)
    x = cache.GetAvatarIconOfProfileAtIndex(profile_index)

> 2) Grouping: Yes grouping works with my current patch. In fact this sort of
> works for the popups as well since they get grouped into the decorated stack.
Awesome.

> 3) Incognito: Yes it works[1], currently the incognito decoration is the only
> one shown (see screenshot in the bug), ideally you have both but I don't want
> that kind of complexity on the first patch.
As above, I think you'll get this for free when you move the code out of the
menu button class.

[cpu_(ooo_6.6-7.5), 2011-09-01 00:51:22]

Patch updated with Peter comments and with the following fixes:

1- decorations change when the avatar changes
2- decoration removed when the avatar is removed

I think this hits the big concerns, to do would be some popup cases.

[Peter Kasting, 2011-09-01 00:54:23]

I kinda feel like the idea I had on the other CL of doing this stuff in the
window code would be better than putting it here,  but I don't feel so strongly
that I'd block this change.

[sail, 2011-09-01 21:16:18]

LGTM

This won't badge popup windows but I think that's ok for now.

[cpu_(ooo_6.6-7.5), 2011-09-01 21:34:36]

On 2011/09/01 21:16:18, sail wrote:
> LGTM
> 
> This won't badge popup windows but I think that's ok for now.

thanks. I have a follow on patch because we are actually leaking the avatar menu
buttons. but that is an easy one.