Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
8907001:
Update avatar bubble if profile info changes (Closed)
|
Created:
9 years ago by sail Modified:
9 years ago CC:
chromium-reviews, sky Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionUpdate avatar bubble if profile info changes
Showing the avatar menu for the first time with two or more GAIA photos would cause the menu to look forzen.
The problem was that we the menu wasn't being resized and repositioned.
Fix was to explicitly resize and reposition the bubble.
BUG=106989
TEST=Reproduced the bug. Applied my fix and verified that the menu updated correctly.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=114330
Patch Set 1 #
Total comments: 2
Patch Set 2 : address review comments #
Total comments: 10
Patch Set 3 : address review comments #
Total comments: 3
Messages
Total messages: 17 (0 generated)
http://codereview.chromium.org/8907001/diff/1/chrome/browser/ui/views/avatar_... File chrome/browser/ui/views/avatar_menu_bubble_view.cc (right): http://codereview.chromium.org/8907001/diff/1/chrome/browser/ui/views/avatar_... chrome/browser/ui/views/avatar_menu_bubble_view.cc:501: if (GetWidget()) { This is actually kind of worrisome. If we can reach here without GetWidget() being true that means that we could probably leak the views created above. Normally we only create and add child views in response to ourselves being added to a view hierarchy, i.e. in an OnViewHierarchyChanged() override. This class does it in the constructor instead which is bad form because it means whoever constructs this class needs to ensure it gets properly destroyed if it is never added to a view hierarchy. If we fix this problem then perhaps the original bug goes away as well?
http://codereview.chromium.org/8907001/diff/1/chrome/browser/ui/views/avatar_... File chrome/browser/ui/views/avatar_menu_bubble_view.cc (right): http://codereview.chromium.org/8907001/diff/1/chrome/browser/ui/views/avatar_... chrome/browser/ui/views/avatar_menu_bubble_view.cc:501: if (GetWidget()) { On 2011/12/12 21:43:43, Peter Kasting wrote: > This is actually kind of worrisome. If we can reach here without GetWidget() > being true that means that we could probably leak the views created above. > Normally we only create and add child views in response to ourselves being added > to a view hierarchy, i.e. in an OnViewHierarchyChanged() override. This class > does it in the constructor instead which is bad form because it means whoever > constructs this class needs to ensure it gets properly destroyed if it is never > added to a view hierarchy. > > If we fix this problem then perhaps the original bug goes away as well? Done. I still needed the Layout/SizeToContents() call to get the button to reposition.
http://codereview.chromium.org/8907001/diff/4001/chrome/browser/ui/views/avat... File chrome/browser/ui/views/avatar_menu_bubble_view.cc (right): http://codereview.chromium.org/8907001/diff/4001/chrome/browser/ui/views/avat... chrome/browser/ui/views/avatar_menu_bubble_view.cc:360: if (!add_profile_link_) Is this really necessary? Normally it's safe to assume the object is in a hierarchy when this function is reached. http://codereview.chromium.org/8907001/diff/4001/chrome/browser/ui/views/avat... chrome/browser/ui/views/avatar_menu_bubble_view.cc:440: // Build the menu for the first time. Nit: I think normally we do class-specific stuff before calling the superclass version, but I'm not 100% sure. http://codereview.chromium.org/8907001/diff/4001/chrome/browser/ui/views/avat... chrome/browser/ui/views/avatar_menu_bubble_view.cc:511: // If the bubble has already been shown then resize and reposition the bubble. It still seems strange to me that we need this. I think you should get Scott to comment here. Perhaps something in the bubble should be causing this to happen that currently isn't. Or perhaps we do need to call something here, but only InvalidateLayout() or something. I'm not sure. http://codereview.chromium.org/8907001/diff/4001/chrome/browser/ui/views/avat... File chrome/browser/ui/views/avatar_menu_bubble_view.h (right): http://codereview.chromium.org/8907001/diff/4001/chrome/browser/ui/views/avat... chrome/browser/ui/views/avatar_menu_bubble_view.h:45: OVERRIDE; Nit: The wrapping scheme we've been using for const, which I think is the same as for override, is that in cases like this we should give each arg its own line and put const on the same line as the last one.
http://codereview.chromium.org/8907001/diff/4001/chrome/browser/ui/views/avat... File chrome/browser/ui/views/avatar_menu_bubble_view.cc (right): http://codereview.chromium.org/8907001/diff/4001/chrome/browser/ui/views/avat... chrome/browser/ui/views/avatar_menu_bubble_view.cc:360: if (!add_profile_link_) On 2011/12/13 02:25:11, Peter Kasting wrote: > Is this really necessary? Normally it's safe to assume the object is in a > hierarchy when this function is reached. This is currently necessary. I think it's called by the bubble delegate code (didn't get a chance to check). http://codereview.chromium.org/8907001/diff/4001/chrome/browser/ui/views/avat... chrome/browser/ui/views/avatar_menu_bubble_view.cc:440: // Build the menu for the first time. On 2011/12/13 02:25:11, Peter Kasting wrote: > Nit: I think normally we do class-specific stuff before calling the superclass > version, but I'm not 100% sure. Done. http://codereview.chromium.org/8907001/diff/4001/chrome/browser/ui/views/avat... chrome/browser/ui/views/avatar_menu_bubble_view.cc:511: // If the bubble has already been shown then resize and reposition the bubble. On 2011/12/13 02:25:11, Peter Kasting wrote: > It still seems strange to me that we need this. I think you should get Scott to > comment here. Perhaps something in the bubble should be causing this to happen > that currently isn't. Or perhaps we do need to call something here, but only > InvalidateLayout() or something. I'm not sure. Adding Scott and msw to the review. If this requires changes to the views::Bubble code then I'd prefer to do this separately since this CL needs to get merged into M17.
http://codereview.chromium.org/8907001/diff/4001/chrome/browser/ui/views/avat... File chrome/browser/ui/views/avatar_menu_bubble_view.cc (right): http://codereview.chromium.org/8907001/diff/4001/chrome/browser/ui/views/avat... chrome/browser/ui/views/avatar_menu_bubble_view.cc:511: // If the bubble has already been shown then resize and reposition the bubble. On 2011/12/13 02:31:40, sail wrote: > On 2011/12/13 02:25:11, Peter Kasting wrote: > > It still seems strange to me that we need this. I think you should get Scott > to > > comment here. Perhaps something in the bubble should be causing this to > happen > > that currently isn't. Or perhaps we do need to call something here, but only > > InvalidateLayout() or something. I'm not sure. > > Adding Scott and msw to the review. If this requires changes to the > views::Bubble code then I'd prefer to do this separately since this CL needs to > get merged into M17. Doing a synchronous Layout and then calling SizeToContents (to resize and potentially move the bubble widget[s]) seems reasonable for now. http://codereview.chromium.org/8907001/diff/10001/chrome/browser/ui/views/ava... File chrome/browser/ui/views/avatar_menu_bubble_view.cc (right): http://codereview.chromium.org/8907001/diff/10001/chrome/browser/ui/views/ava... chrome/browser/ui/views/avatar_menu_bubble_view.cc:439: if (!add_profile_link_ && is_add && child == this) I don't think |this| will be added as a child more than once. Is there a reason why you're checking whether |add_profile_link_| has been initialized?
http://codereview.chromium.org/8907001/diff/4001/chrome/browser/ui/views/avat... File chrome/browser/ui/views/avatar_menu_bubble_view.cc (right): http://codereview.chromium.org/8907001/diff/4001/chrome/browser/ui/views/avat... chrome/browser/ui/views/avatar_menu_bubble_view.cc:511: // If the bubble has already been shown then resize and reposition the bubble. On 2011/12/13 02:25:11, Peter Kasting wrote: > It still seems strange to me that we need this. I think you should get Scott to > comment here. Perhaps something in the bubble should be causing this to happen > that currently isn't. Or perhaps we do need to call something here, but only > InvalidateLayout() or something. I'm not sure. We could make whatever the content view is for the bubble override ChildPreferredSizeChanged and do the resize/layout then. But I'm not sure that we want that for all bubbles.
http://codereview.chromium.org/8907001/diff/4001/chrome/browser/ui/views/avat... File chrome/browser/ui/views/avatar_menu_bubble_view.cc (right): http://codereview.chromium.org/8907001/diff/4001/chrome/browser/ui/views/avat... chrome/browser/ui/views/avatar_menu_bubble_view.cc:511: // If the bubble has already been shown then resize and reposition the bubble. On 2011/12/13 05:18:46, sky wrote: > On 2011/12/13 02:25:11, Peter Kasting wrote: > > It still seems strange to me that we need this. I think you should get Scott > to > > comment here. Perhaps something in the bubble should be causing this to > happen > > that currently isn't. Or perhaps we do need to call something here, but only > > InvalidateLayout() or something. I'm not sure. > > We could make whatever the content view is for the bubble override > ChildPreferredSizeChanged and do the resize/layout then. But I'm not sure that > we want that for all bubbles. AvatarMenuBubbleView is the content view (a BubbleDelegateView). So maybe you could override ChildPreferredSizeChanged here for now. Maybe add a TODO to consider the same behavior for other bubbles. Changing all bubbles seems okay for a followup change that won't be merged.
http://codereview.chromium.org/8907001/diff/10001/chrome/browser/ui/views/ava... File chrome/browser/ui/views/avatar_menu_bubble_view.cc (right): http://codereview.chromium.org/8907001/diff/10001/chrome/browser/ui/views/ava... chrome/browser/ui/views/avatar_menu_bubble_view.cc:439: if (!add_profile_link_ && is_add && child == this) On 2011/12/13 03:20:23, msw wrote: > I don't think |this| will be added as a child more than once. Is there a reason > why you're checking whether |add_profile_link_| has been initialized? It doesn't make sense to call OnAvatarMenuModelChanged() more than once. add_profile_link_ will only be NULL if OnAvatarMenuModelChanged() has not been called yet. From what I understand if the view removed and added again then |this| will be added as a child again.
BTW, LGTM once you've done whatever you/msw/sky decide is the right thing to do.
LGTM; overriding ChildPreferredSizeChanged is optional for me (adding a TODO doesn't hurt). I'm not sure if Sky wants you to pursue that for this CL. http://codereview.chromium.org/8907001/diff/10001/chrome/browser/ui/views/ava... File chrome/browser/ui/views/avatar_menu_bubble_view.cc (right): http://codereview.chromium.org/8907001/diff/10001/chrome/browser/ui/views/ava... chrome/browser/ui/views/avatar_menu_bubble_view.cc:439: if (!add_profile_link_ && is_add && child == this) > From what I understand if the view removed and added again then > |this| will be added as a child again. I'd just DCHECK(!add_profile_link_) in OnAvatarMenuModelChanged, because the same BubbleDelegateView shouldn't be removed and re-added in practice. But I guess this extra resilience might be good for merging.
Merging as is for now. Will add TODO's on trunk in a separate CL.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sail@chromium.org/8907001/10001
Do we generally allow landing via the CQ without tryjobs?
Try job failure for 8907001-10001 (retry) on mac_rel for step "compile" (clobber build). It's a second try, previously, step "compile" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sail@chromium.org/8907001/10001
Change committed as 114330 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
