Badge icons in windows task bar with avatar icon.

Badge icons in windows task bar with avatar icon.

BUG=412383
Committed: https://crrev.com/9e6e5fb1be5ae176050065d7a6c09e4afb44c168
Cr-Commit-Position: refs/heads/master@{#301431}

[Roger Tawa OOO till Jul 10th, 2014-10-24 17:35:17]

Patchset #2 (id:20001) has been deleted

[Roger Tawa OOO till Jul 10th, 2014-10-24 17:48:43]

Patchset #2 (id:40001) has been deleted

[Roger Tawa OOO till Jul 10th, 2014-10-24 17:51:32]

rogerta@chromium.org changed reviewers:
+ noms@chromium.org

[Roger Tawa OOO till Jul 10th, 2014-10-24 17:51:32]

Hi Monica, please take a look.  Thanks,

https://codereview.chromium.org/678553002/diff/60001/chrome/browser/ui/views/...
File chrome/browser/ui/views/frame/browser_non_client_frame_view.cc (right):

https://codereview.chromium.org/678553002/diff/60001/chrome/browser/ui/views/...
chrome/browser/ui/views/frame/browser_non_client_frame_view.cc:68: }
I made this an anonymous function in this file since its pretty specific.

[noms (inactive), 2014-10-24 17:59:19]

https://codereview.chromium.org/678553002/diff/60001/chrome/browser/ui/views/...
File chrome/browser/ui/views/frame/browser_non_client_frame_view.cc (right):

https://codereview.chromium.org/678553002/diff/60001/chrome/browser/ui/views/...
chrome/browser/ui/views/frame/browser_non_client_frame_view.cc:111: if
((browser_view_->IsOffTheRecord() && !browser_view_->IsGuestSession()) ||
I think that's still just !IsRegularOrGuestSession()

https://codereview.chromium.org/678553002/diff/60001/chrome/browser/ui/views/...
chrome/browser/ui/views/frame/browser_non_client_frame_view.cc:173:
DrawTaskbarDecoration(avatar, taskbar_badge_avatar);
Should this also be in UpdateNewStyleAvatarInfo()? Since they're pretty
symmetric functions, I would think it's either it's needed in both, or it's only
needed on OnProfileAvatarChanged(). I think since it's called in
VisibilityChanged() maybe it's needed here at all?

https://codereview.chromium.org/678553002/diff/60001/chrome/browser/ui/views/...
File chrome/browser/ui/views/frame/browser_non_client_frame_view.h (right):

https://codereview.chromium.org/678553002/diff/60001/chrome/browser/ui/views/...
chrome/browser/ui/views/frame/browser_non_client_frame_view.h:78: //
ProfileInfoCacheObserver implementation.
nit: the other override comment is "Overidden from ..."

[Roger Tawa OOO till Jul 10th, 2014-10-24 18:36:07]

Thanks Monica.  Please take another look.

https://codereview.chromium.org/678553002/diff/60001/chrome/browser/ui/views/...
File chrome/browser/ui/views/frame/browser_non_client_frame_view.cc (right):

https://codereview.chromium.org/678553002/diff/60001/chrome/browser/ui/views/...
chrome/browser/ui/views/frame/browser_non_client_frame_view.cc:111: if
((browser_view_->IsOffTheRecord() && !browser_view_->IsGuestSession()) ||
On 2014/10/24 17:59:19, Monica Dinculescu wrote:
> I think that's still just !IsRegularOrGuestSession()

Doh!  Yes it is.  Done.

https://codereview.chromium.org/678553002/diff/60001/chrome/browser/ui/views/...
chrome/browser/ui/views/frame/browser_non_client_frame_view.cc:173:
DrawTaskbarDecoration(avatar, taskbar_badge_avatar);
On 2014/10/24 17:59:19, Monica Dinculescu wrote:
> Should this also be in UpdateNewStyleAvatarInfo()? Since they're pretty
> symmetric functions, I would think it's either it's needed in both, or it's
only
> needed on OnProfileAvatarChanged(). I think since it's called in
> VisibilityChanged() maybe it's needed here at all?

Done.

https://codereview.chromium.org/678553002/diff/60001/chrome/browser/ui/views/...
File chrome/browser/ui/views/frame/browser_non_client_frame_view.h (right):

https://codereview.chromium.org/678553002/diff/60001/chrome/browser/ui/views/...
chrome/browser/ui/views/frame/browser_non_client_frame_view.h:78: //
ProfileInfoCacheObserver implementation.
On 2014/10/24 17:59:19, Monica Dinculescu wrote:
> nit: the other override comment is "Overidden from ..."

Done.

[noms (inactive), 2014-10-24 18:44:57]

profiles lgtm

https://codereview.chromium.org/678553002/diff/100001/chrome/browser/ui/views...
File chrome/browser/ui/views/frame/browser_non_client_frame_view.cc (right):

https://codereview.chromium.org/678553002/diff/100001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_non_client_frame_view.cc:112: // Make sure
the task bar icon is correctly updated  call
nit: two spaces between updated and call :(

[Roger Tawa OOO till Jul 10th, 2014-10-24 19:21:52]

rogerta@chromium.org changed reviewers:
+ sky@chromium.org

[Roger Tawa OOO till Jul 10th, 2014-10-24 19:21:53]

Thanks Monica.

Scott: can you do an owner review for browser_non_client_frame_view.h|cc?

[sky, 2014-10-24 21:18:08]

https://codereview.chromium.org/678553002/diff/120001/chrome/browser/ui/views...
File chrome/browser/ui/views/frame/browser_non_client_frame_view.cc (right):

https://codereview.chromium.org/678553002/diff/120001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_non_client_frame_view.cc:33: void
GetAvatarImage(BrowserView* browser_view,
Seems all this code is platform neutral (code only needs to know the
browser_view type). Does it make sense to move to a common place?

[Roger Tawa OOO till Jul 10th, 2014-10-24 22:48:01]

https://codereview.chromium.org/678553002/diff/120001/chrome/browser/ui/views...
File chrome/browser/ui/views/frame/browser_non_client_frame_view.cc (right):

https://codereview.chromium.org/678553002/diff/120001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_non_client_frame_view.cc:33: void
GetAvatarImage(BrowserView* browser_view,
On 2014/10/24 21:18:08, sky wrote:
> Seems all this code is platform neutral (code only needs to know the
> browser_view type). Does it make sense to move to a common place?

I don't know if there is a good common place to put it.  How about
c/b/profiles/profiles_avatar_icon_util.cc?  Is it OK to include
"chrome/browser/ui/views/frame/browser_view.h" in there?

[sky, 2014-10-25 15:05:30]

https://codereview.chromium.org/678553002/diff/120001/chrome/browser/ui/views...
File chrome/browser/ui/views/frame/browser_non_client_frame_view.cc (right):

https://codereview.chromium.org/678553002/diff/120001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_non_client_frame_view.cc:33: void
GetAvatarImage(BrowserView* browser_view,
On 2014/10/24 22:48:01, Roger Tawa wrote:
> On 2014/10/24 21:18:08, sky wrote:
> > Seems all this code is platform neutral (code only needs to know the
> > browser_view type). Does it make sense to move to a common place?
> 
> I don't know if there is a good common place to put it.  How about
> c/b/profiles/profiles_avatar_icon_util.cc?

Somewhere in c/b/ui/, possibly c/b/ui/profiles may be better.

>   Is it OK to include
> "chrome/browser/ui/views/frame/browser_view.h" in there?

The goal with platform specific ui code is that it's self contained and not
included from non-platform specific ui code. In other words, no you don't want
an include like that. But then aren't the BrowserView functions you're using
here covers for Profile functions? This is why I thought it made sense to move
to a non-platform specific ui location. WDYT?

[Roger Tawa OOO till Jul 10th, 2014-10-27 14:35:48]

Thanks Scott.  Moved the code to the location you suggested.

https://codereview.chromium.org/678553002/diff/120001/chrome/browser/ui/views...
File chrome/browser/ui/views/frame/browser_non_client_frame_view.cc (right):

https://codereview.chromium.org/678553002/diff/120001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_non_client_frame_view.cc:33: void
GetAvatarImage(BrowserView* browser_view,
On 2014/10/25 15:05:29, sky wrote:
> On 2014/10/24 22:48:01, Roger Tawa wrote:
> > On 2014/10/24 21:18:08, sky wrote:
> > > Seems all this code is platform neutral (code only needs to know the
> > > browser_view type). Does it make sense to move to a common place?
> > 
> > I don't know if there is a good common place to put it.  How about
> > c/b/profiles/profiles_avatar_icon_util.cc?
> 
> Somewhere in c/b/ui/, possibly c/b/ui/profiles may be better.
> 
> >   Is it OK to include
> > "chrome/browser/ui/views/frame/browser_view.h" in there?
> 
> The goal with platform specific ui code is that it's self contained and not
> included from non-platform specific ui code. In other words, no you don't want
> an include like that. But then aren't the BrowserView functions you're using
> here covers for Profile functions? This is why I thought it made sense to move
> to a non-platform specific ui location. WDYT?

Done.

[sky, 2014-10-27 14:45:38]

LGTM

https://codereview.chromium.org/678553002/diff/140001/chrome/browser/ui/views...
File chrome/browser/ui/views/profiles/avatar_menu_button.cc (right):

https://codereview.chromium.org/678553002/diff/140001/chrome/browser/ui/views...
chrome/browser/ui/views/profiles/avatar_menu_button.cc:39: void
AvatarMenuButton::GetAvatarImages(Profile* profile,
This looks platform independent. You should you don't want this in
chrome/browser/ui/profile_foo? I'll approve assuming you don't.

https://codereview.chromium.org/678553002/diff/140001/chrome/browser/ui/views...
File chrome/browser/ui/views/profiles/avatar_menu_button.h (right):

https://codereview.chromium.org/678553002/diff/140001/chrome/browser/ui/views...
chrome/browser/ui/views/profiles/avatar_menu_button.h:46:
AvatarMenuButton(Browser* browser, bool disabled);
nit: style guide says constructor/destructor are before any other functions.

[Roger Tawa OOO till Jul 10th, 2014-10-27 18:51:11]

Thanks Scott.

https://codereview.chromium.org/678553002/diff/140001/chrome/browser/ui/views...
File chrome/browser/ui/views/profiles/avatar_menu_button.h (right):

https://codereview.chromium.org/678553002/diff/140001/chrome/browser/ui/views...
chrome/browser/ui/views/profiles/avatar_menu_button.h:46:
AvatarMenuButton(Browser* browser, bool disabled);
On 2014/10/27 14:45:37, sky wrote:
> nit: style guide says constructor/destructor are before any other functions.

Done.

[Roger Tawa OOO till Jul 10th, 2014-10-27 19:33:36]

The CQ bit was checked by rogerta@chromium.org

[commit-bot: I haz the power, 2014-10-27 19:35:07]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/678553002/180001

[commit-bot: I haz the power, 2014-10-27 20:25:25]

Committed patchset #8 (id:180001)

[commit-bot: I haz the power, 2014-10-27 20:26:07]

Patchset 8 (id:??) landed as
https://crrev.com/9e6e5fb1be5ae176050065d7a6c09e4afb44c168
Cr-Commit-Position: refs/heads/master@{#301431}