Profiles: Fixed crash when avatar bubble is launched using ctrl+shift+m with only default profile

Profiles: Fixed crash when avatar bubble is launched using ctrl+shift+m key press when there is only one default profile. Incorporated necessary condition check before launching avatar bubble.

BUG=105700
TEST=None


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

[NaveenBobbili (Motorola), 2011-11-29 12:26:14]

Please review

[Miranda Callahan, 2011-11-29 13:54:57]

On 2011/11/29 12:26:14, NaveenBobbili (Motorola) wrote:
> Please review

We should not show the avatar button if there is only one default profile;
surely there is another fix for this problem?

[sail, 2011-11-29 17:08:21]

On 2011/11/29 13:54:57, Miranda Callahan wrote:
> On 2011/11/29 12:26:14, NaveenBobbili (Motorola) wrote:
> > Please review
> 
> We should not show the avatar button if there is only one default profile;
> surely there is another fix for this problem?

Yea, you should be able to fix the real problem from here:
http://codesearch.google.com/codesearch#OAMlx_jo-ck/src/chrome/browser/ui/gtk...

[NaveenBobbili (Motorola), 2011-11-30 05:00:42]

Thanks for your comments. I have made changes accordingly. Please take a look.

[sail, 2011-11-30 05:08:52]

lgtm

[NaveenBobbili (Motorola), 2011-11-30 05:11:09]

On 2011/11/30 05:08:52, sail wrote:
> lgtm

Thanks. I will wait for others also before landing this patch.

[sail, 2011-11-30 05:17:33]

I think the root problem here is that the button isn't inside a browser window.
Another solution to this bug would be to detect that inside
AvatarButton::ShowAvatarBubble() and ignore the call in that case.

[NaveenBobbili (Motorola), 2011-11-30 06:27:55]

On 2011/11/30 05:17:33, sail wrote:
> I think the root problem here is that the button isn't inside a browser
window.
> Another solution to this bug would be to detect that inside
> AvatarButton::ShowAvatarBubble() and ignore the call in that case.

Can you please tell me which is preferred solution?

[sail, 2011-11-30 06:30:07]

On 2011/11/30 06:27:55, NaveenBobbili (Motorola) wrote:
> On 2011/11/30 05:17:33, sail wrote:
> > I think the root problem here is that the button isn't inside a browser
> window.
> > Another solution to this bug would be to detect that inside
> > AvatarButton::ShowAvatarBubble() and ignore the call in that case.
> 
> Can you please tell me which is preferred solution?

I'm ok with your current solution. Maybe erg has a preference?

[James Hawkins, 2011-11-30 16:24:24]

On 2011/11/30 06:30:07, sail wrote:
> On 2011/11/30 06:27:55, NaveenBobbili (Motorola) wrote:
> > On 2011/11/30 05:17:33, sail wrote:
> > > I think the root problem here is that the button isn't inside a browser
> > window.
> > > Another solution to this bug would be to detect that inside
> > > AvatarButton::ShowAvatarBubble() and ignore the call in that case.
> > 
> > Can you please tell me which is preferred solution?
> 
> I'm ok with your current solution. Maybe erg has a preference?

I prefer the proposed solution over the current solution. When I first looked at
this change, it made me cringe.

[NaveenBobbili (Motorola), 2011-12-01 06:06:21]

I implemented as per proposed approach. Please review the same.

[James Hawkins, 2011-12-01 17:15:54]

http://codereview.chromium.org/8664028/diff/10001/chrome/browser/ui/gtk/avata...
File chrome/browser/ui/gtk/avatar_menu_button_gtk.cc (right):

http://codereview.chromium.org/8664028/diff/10001/chrome/browser/ui/gtk/avata...
chrome/browser/ui/gtk/avatar_menu_button_gtk.cc:52: if
(gtk_widget_get_parent_window(widget_.get()))
This isn't the same logic as ShouldDisplayAvatar().

[sail, 2011-12-01 19:12:23]

http://codereview.chromium.org/8664028/diff/10001/chrome/browser/ui/gtk/avata...
File chrome/browser/ui/gtk/avatar_menu_button_gtk.cc (right):

http://codereview.chromium.org/8664028/diff/10001/chrome/browser/ui/gtk/avata...
chrome/browser/ui/gtk/avatar_menu_button_gtk.cc:52: if
(gtk_widget_get_parent_window(widget_.get()))
On 2011/12/01 17:15:55, James Hawkins wrote:
> This isn't the same logic as ShouldDisplayAvatar().

I think this is ok. ShoudDisplayAvatar decides if the button gets placed in the
title bar.

At this point we just want to know if we're in the title bar or not. If we're
not then we shouldn't show the bubble view.

I think a comment should be added to this code. Maybe something like:
// Only show the avatar bubble if the avatar button is in the title bar.

[NaveenBobbili (Motorola), 2011-12-02 07:39:20]

Added comment in the code. Please take a look now.

[sail, 2011-12-02 16:31:43]

lgtm

[James Hawkins, 2011-12-02 16:32:34]

lgtm

[commit-bot: I haz the power, 2011-12-03 01:11:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/qghc36@motorola.com/8664028/15002

[commit-bot: I haz the power, 2011-12-03 04:18:54]

Change committed as 112864