Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
7057058:
Convert BrowserActionsContainer context menu from Menu2 to MenuItemView. (Closed)
|
Created:
9 years, 6 months ago by rhashimoto Modified:
9 years, 6 months ago CC:
chromium-reviews Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionConvert BrowserActionsContainer context menu from Menu2 to MenuItemView.
This CL is part of removing GTK dependencies.
BUG=chromium-os:13887
TEST=none
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=88599
Patch Set 1 #Patch Set 2 : Reformat line wrapping. #Patch Set 3 : Fix anchor position for RTL and non-RTL. #
Total comments: 10
Patch Set 4 : Pass invoking button bounds to MenuItemView. Use TOPLEFT anchor. #Patch Set 5 : Initialize pointer to NULL. #
Total comments: 5
Patch Set 6 : Implement reviewer recommendations. Rebase to trunk. #
Total comments: 6
Patch Set 7 : Implemented reviewer recommendations. #Patch Set 8 : Implement reviewer recommendation. #
Total comments: 1
Messages
Total messages: 11 (0 generated)
http://codereview.chromium.org/7057058/diff/4001/chrome/browser/ui/views/brow... File chrome/browser/ui/views/browser_actions_container.cc (right): http://codereview.chromium.org/7057058/diff/4001/chrome/browser/ui/views/brow... chrome/browser/ui/views/browser_actions_container.cc:223: // Align with button left edge for RTL, right edge otherwise. This seems backwards. We want to align to the left edge for LTR. The old code ought to have been correct. http://codereview.chromium.org/7057058/diff/4001/chrome/browser/ui/views/brow... chrome/browser/ui/views/browser_actions_container.cc:271: gfx::Rect(p, gfx::Size()), views::MenuItemView::TOPRIGHT, true); Nit: I mentioned this in the last code review and it apparently hasn't been fixed yet -- why does this API take a rect instead of a point? http://codereview.chromium.org/7057058/diff/4001/chrome/browser/ui/views/brow... chrome/browser/ui/views/browser_actions_container.cc:275: context_menu_menu_.reset(NULL); Do we really need to use a scoped_ptr if we're resetting it here? Can we just use a raw pointer?
http://codereview.chromium.org/7057058/diff/4001/chrome/browser/ui/views/brow... File chrome/browser/ui/views/browser_actions_container.cc (right): http://codereview.chromium.org/7057058/diff/4001/chrome/browser/ui/views/brow... chrome/browser/ui/views/browser_actions_container.cc:223: // Align with button left edge for RTL, right edge otherwise. On 2011/06/08 20:29:43, Peter Kasting wrote: > This seems backwards. We want to align to the left edge for LTR. The old code > ought to have been correct. It looks like we indeed align to the left edge on Windows, but the right edge on Linux (and ChromiumOS), and the mouse location for Mac. Patch 4 makes it TOPLEFT. http://codereview.chromium.org/7057058/diff/4001/chrome/browser/ui/views/brow... chrome/browser/ui/views/browser_actions_container.cc:271: gfx::Rect(p, gfx::Size()), views::MenuItemView::TOPRIGHT, true); On 2011/06/08 20:29:43, Peter Kasting wrote: > Nit: I mentioned this in the last code review and it apparently hasn't been > fixed yet -- why does this API take a rect instead of a point? I believe the API allows the specification of a Rect to represent the invoking object. The menu controller will then choose the bottom left or bottom right corner to place the menu depending on whether the anchor position is TOPLEFT or TOPRIGHT. For context menus a Rect of zero size is usually used, but because we have a invoking object in this case we can pass its bounds. Patch 4 does this. http://codereview.chromium.org/7057058/diff/4001/chrome/browser/ui/views/brow... chrome/browser/ui/views/browser_actions_container.cc:275: context_menu_menu_.reset(NULL); On 2011/06/08 20:29:43, Peter Kasting wrote: > Do we really need to use a scoped_ptr if we're resetting it here? Can we just > use a raw pointer? Done.
LGTM with nits http://codereview.chromium.org/7057058/diff/4001/chrome/browser/ui/views/brow... File chrome/browser/ui/views/browser_actions_container.cc (right): http://codereview.chromium.org/7057058/diff/4001/chrome/browser/ui/views/brow... chrome/browser/ui/views/browser_actions_container.cc:271: gfx::Rect(p, gfx::Size()), views::MenuItemView::TOPRIGHT, true); On 2011/06/09 00:26:23, rhashimoto wrote: > I believe the API allows the specification of a Rect to represent the invoking > object. The menu controller will then choose the bottom left or bottom right > corner to place the menu depending on whether the anchor position is TOPLEFT or > TOPRIGHT. This makes callers who use just a point construct an extra rect with empty size, while saving nothing for callers who have a rect (since it's just as easy for them to pass rect.x() or rect.right(), and they must already know which side they wish to align to). Please fix this API to use a point instead. This can be a separate change. http://codereview.chromium.org/7057058/diff/8001/chrome/browser/ui/views/brow... chrome/browser/ui/views/browser_actions_container.cc:258: if (!extension()->ShowConfigureContextMenus()) Nit: Safer against leaks would be to stack-allocated this object and then set |context_menu_menu_| to point at its address. http://codereview.chromium.org/7057058/diff/8001/chrome/browser/ui/views/brow... File chrome/browser/ui/views/browser_actions_container.h (right): http://codereview.chromium.org/7057058/diff/8001/chrome/browser/ui/views/brow... chrome/browser/ui/views/browser_actions_container.h:134: bool showing_context_menu_; Nit: This variable isn't needed; callers can just check whether context_menu_menu_ is NULL. That's safer against misuse. http://codereview.chromium.org/7057058/diff/8001/chrome/browser/ui/views/brow... chrome/browser/ui/views/browser_actions_container.h:144: views::MenuItemView* context_menu_menu_; Nit: Might want to comment: // Only non-NULL while we're actually displaying the menu. Seems like this could just be named |context_menu_|, too.
http://codereview.chromium.org/7057058/diff/4001/chrome/browser/ui/views/brow... File chrome/browser/ui/views/browser_actions_container.cc (right): http://codereview.chromium.org/7057058/diff/4001/chrome/browser/ui/views/brow... chrome/browser/ui/views/browser_actions_container.cc:271: gfx::Rect(p, gfx::Size()), views::MenuItemView::TOPRIGHT, true); On 2011/06/09 00:39:09, Peter Kasting wrote: > On 2011/06/09 00:26:23, rhashimoto wrote: > > I believe the API allows the specification of a Rect to represent the invoking > > object. The menu controller will then choose the bottom left or bottom right > > corner to place the menu depending on whether the anchor position is TOPLEFT > or > > TOPRIGHT. > > This makes callers who use just a point construct an extra rect with empty size, > while saving nothing for callers who have a rect (since it's just as easy for > them to pass rect.x() or rect.right(), and they must already know which side > they wish to align to). > > Please fix this API to use a point instead. This can be a separate change. I can add convenience API to pass a Point, but I think the Rect version is still useful and should be retained. The menu controller automatically swaps the alignment and anchor position if IsRTL() is true. http://codereview.chromium.org/7057058/diff/8001/chrome/browser/ui/views/brow... File chrome/browser/ui/views/browser_actions_container.h (right): http://codereview.chromium.org/7057058/diff/8001/chrome/browser/ui/views/brow... chrome/browser/ui/views/browser_actions_container.h:134: bool showing_context_menu_; On 2011/06/09 00:39:09, Peter Kasting wrote: > Nit: This variable isn't needed; callers can just check whether > context_menu_menu_ is NULL. That's safer against misuse. Done. http://codereview.chromium.org/7057058/diff/8001/chrome/browser/ui/views/brow... chrome/browser/ui/views/browser_actions_container.h:144: views::MenuItemView* context_menu_menu_; On 2011/06/09 00:39:09, Peter Kasting wrote: > Nit: Might want to comment: > > // Only non-NULL while we're actually displaying the menu. > > Seems like this could just be named |context_menu_|, too. Done.
http://codereview.chromium.org/7057058/diff/4001/chrome/browser/ui/views/brow... File chrome/browser/ui/views/browser_actions_container.cc (right): http://codereview.chromium.org/7057058/diff/4001/chrome/browser/ui/views/brow... chrome/browser/ui/views/browser_actions_container.cc:271: gfx::Rect(p, gfx::Size()), views::MenuItemView::TOPRIGHT, true); On 2011/06/09 01:15:27, rhashimoto wrote: > On 2011/06/09 00:39:09, Peter Kasting wrote: > > Please fix this API to use a point instead. This can be a separate change. > > I can add convenience API to pass a Point, but I think the Rect version is still > useful and should be retained. The menu controller automatically swaps the > alignment and anchor position if IsRTL() is true. This worries me. The old APIs took a point. That means that anywhere we now take a rect, we're making a behavior change. Either the old behavior was actually wrong in RTL mode or the new behavior is. I would like to see concrete examples of the former before I'm OK with making this change. We do enough auto-mirroring that it would not surprise me at all if the old code was correct. Another possibility is that callers should be passing something like GetMirroredX() in cases where neither x() nor right() is correct. This obviates any need to ever take a rect. Please file a bug to change this so we can continue there and stop dragging this code review out with this issue. http://codereview.chromium.org/7057058/diff/5002/chrome/browser/ui/views/brow... chrome/browser/ui/views/browser_actions_container.cc:253: MenuButton::OnKeyReleased(event) : TextButton::OnKeyReleased(event); It looks like this leaks. Should this be a scoped_refptr? http://codereview.chromium.org/7057058/diff/5002/chrome/browser/ui/views/brow... chrome/browser/ui/views/browser_actions_container.cc:256: void BrowserActionButton::ShowContextMenu(const gfx::Point& p, What about that idea of stack-allocating this object?
http://codereview.chromium.org/7057058/diff/4001/chrome/browser/ui/views/brow... File chrome/browser/ui/views/browser_actions_container.cc (right): http://codereview.chromium.org/7057058/diff/4001/chrome/browser/ui/views/brow... chrome/browser/ui/views/browser_actions_container.cc:271: gfx::Rect(p, gfx::Size()), views::MenuItemView::TOPRIGHT, true); On 2011/06/09 17:43:57, Peter Kasting wrote: > On 2011/06/09 01:15:27, rhashimoto wrote: > > On 2011/06/09 00:39:09, Peter Kasting wrote: > > > Please fix this API to use a point instead. This can be a separate change. > > > > I can add convenience API to pass a Point, but I think the Rect version is > still > > useful and should be retained. The menu controller automatically swaps the > > alignment and anchor position if IsRTL() is true. > > This worries me. The old APIs took a point. That means that anywhere we now > take a rect, we're making a behavior change. Either the old behavior was > actually wrong in RTL mode or the new behavior is. I would like to see concrete > examples of the former before I'm OK with making this change. We do enough > auto-mirroring that it would not surprise me at all if the old code was correct. > > Another possibility is that callers should be passing something like > GetMirroredX() in cases where neither x() nor right() is correct. This obviates > any need to ever take a rect. > > Please file a bug to change this so we can continue there and stop dragging this > code review out with this issue. I filed crosbug.com/85585. http://codereview.chromium.org/7057058/diff/5002/chrome/browser/ui/views/brow... chrome/browser/ui/views/browser_actions_container.cc:253: MenuButton::OnKeyReleased(event) : TextButton::OnKeyReleased(event); On 2011/06/09 17:43:57, Peter Kasting wrote: > It looks like this leaks. Should this be a scoped_refptr? It is a scoped_refptr. I thought I would change the operator= to a reset() to make that clear, but apparently it has no reset() method. http://codereview.chromium.org/7057058/diff/5002/chrome/browser/ui/views/brow... chrome/browser/ui/views/browser_actions_container.cc:256: void BrowserActionButton::ShowContextMenu(const gfx::Point& p, On 2011/06/09 17:43:57, Peter Kasting wrote: > What about that idea of stack-allocating this object? Done.
http://codereview.chromium.org/7057058/diff/5002/chrome/browser/ui/views/brow... File chrome/browser/ui/views/browser_actions_container.cc (right): http://codereview.chromium.org/7057058/diff/5002/chrome/browser/ui/views/brow... chrome/browser/ui/views/browser_actions_container.cc:253: context_menu_contents_ = On 2011/06/09 18:19:01, rhashimoto wrote: > On 2011/06/09 17:43:57, Peter Kasting wrote: > > It looks like this leaks. Should this be a scoped_refptr? > > It is a scoped_refptr. I thought I would change the operator= to a reset() to > make that clear, but apparently it has no reset() method. Oh, it's a member. Why is it a member? Seems like it should be a local.
http://codereview.chromium.org/7057058/diff/5002/chrome/browser/ui/views/brow... File chrome/browser/ui/views/browser_actions_container.cc (right): http://codereview.chromium.org/7057058/diff/5002/chrome/browser/ui/views/brow... chrome/browser/ui/views/browser_actions_container.cc:253: context_menu_contents_ = On 2011/06/09 18:20:42, Peter Kasting wrote: > On 2011/06/09 18:19:01, rhashimoto wrote: > > On 2011/06/09 17:43:57, Peter Kasting wrote: > > > It looks like this leaks. Should this be a scoped_refptr? > > > > It is a scoped_refptr. I thought I would change the operator= to a reset() to > > make that clear, but apparently it has no reset() method. > > Oh, it's a member. Why is it a member? Seems like it should be a local. Done.
LGTM http://codereview.chromium.org/7057058/diff/5003/chrome/browser/ui/views/brow... File chrome/browser/ui/views/browser_actions_container.cc (right): http://codereview.chromium.org/7057058/diff/5003/chrome/browser/ui/views/brow... chrome/browser/ui/views/browser_actions_container.cc:266: context_menu_ = NULL; Nit: Move this up to just after RunMenuAt().
Change committed as 88599 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
