Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
553233002:
Dynamically calculate the number of extension icons to show per row in overflow (Closed)
DescriptionDynamically calculate the number of extension icons to show per row in overflow
Determine how many extension icons to show on each row of the overflow menu based on the width of other menu items.
Screenshot: http://imgur.com/mPQAbxs
BUG=412503
Committed: https://crrev.com/9d543b5777b347d6071692a4b09effbeff838526
Cr-Commit-Position: refs/heads/master@{#294944}
Patch Set 1 #
Total comments: 10
Patch Set 2 : s/icons_per_menu_row/icons_per_overflow_menu_row #
Total comments: 6
Patch Set 3 : #Patch Set 4 : Don't resize menu to accommodate #
Total comments: 14
Patch Set 5 : #Patch Set 6 : Test fix #
Total comments: 2
Patch Set 7 : #
Total comments: 2
Patch Set 8 : interactive uitest -> unittest #
Total comments: 4
Patch Set 9 : #Messages
Total messages: 29 (4 generated)
rdevlin.cronin@chromium.org changed reviewers: + finnur@chromium.org
Please take a look. :) I can add screenshots tomorrow morning (ainslie@ has already signed off OTR).
https://codereview.chromium.org/553233002/diff/1/chrome/browser/ui/views/tool... File chrome/browser/ui/views/toolbar/browser_actions_container.cc (right): https://codereview.chromium.org/553233002/diff/1/chrome/browser/ui/views/tool... chrome/browser/ui/views/toolbar/browser_actions_container.cc:518: drop_position_->row * icons_per_menu_row + drop_position_->icon_in_row; Danger! Danger! Drop can be broken if the wrench menu is opened, triggering a change to this static value and then someone tries to drag to the main container. Would have been nasty to track down. :) https://codereview.chromium.org/553233002/diff/1/chrome/browser/ui/views/tool... File chrome/browser/ui/views/toolbar/browser_actions_container.h (right): https://codereview.chromium.org/553233002/diff/1/chrome/browser/ui/views/tool... chrome/browser/ui/views/toolbar/browser_actions_container.h:260: static int icons_per_menu_row; So, two things I'm concerned about here: 1) Most worryingly is that this variable guides both types of containers and I'm worried someone might use it to calculate something for the main container _after_ it has been modified to fit an overflow container. In fact, I've already uncovered what I think is one such bug (see comment on DragDrop). I think I would like to see this renamed so that it is clear that this variable should only be used when in overflow. Maybe icons_per_overflow_menu_row? That way we can have code like: in_overflow() ? icons_per_overflow_menu_row : 1 (or a helper function that does that, if we do it more than often). 2) It feels very icky to expose a global static like that. I wonder if we can communicate this value through the ExtensionToolbarModel, instead? ExtensionToolbarMenuView can get at that through the browser_->profile() pointer it has and this class already has a direct pointer to the model. https://codereview.chromium.org/553233002/diff/1/chrome/browser/ui/views/tool... chrome/browser/ui/views/toolbar/browser_actions_container.h:264: static const int kItemSpacing; This is only used internally. Doesn't need to be public.
Couple of questions. :) Also, as promised, screenshot added to CL description. https://codereview.chromium.org/553233002/diff/1/chrome/browser/ui/views/tool... File chrome/browser/ui/views/toolbar/browser_actions_container.cc (right): https://codereview.chromium.org/553233002/diff/1/chrome/browser/ui/views/tool... chrome/browser/ui/views/toolbar/browser_actions_container.cc:518: drop_position_->row * icons_per_menu_row + drop_position_->icon_in_row; On 2014/09/10 10:48:27, Finnur wrote: > Danger! Danger! Drop can be broken if the wrench menu is opened, triggering a > change to this static value and then someone tries to drag to the main > container. Would have been nasty to track down. :) Can it, though? In the main container, there are no "rows", and, as such, drop_position_->row is always 0, so the value of icons_per_menu_row doesn't matter at all. The only time it matters is for the overflow container (in which case it's correct by definition, since the overflow container uses this to determine how many icons it has). I think this is actually safe. (I've verified by hand that dragging still works, just to be sure.) https://codereview.chromium.org/553233002/diff/1/chrome/browser/ui/views/tool... File chrome/browser/ui/views/toolbar/browser_actions_container.h (right): https://codereview.chromium.org/553233002/diff/1/chrome/browser/ui/views/tool... chrome/browser/ui/views/toolbar/browser_actions_container.h:260: static int icons_per_menu_row; On 2014/09/10 10:48:28, Finnur wrote: > So, two things I'm concerned about here: > > 1) Most worryingly is that this variable guides both types of containers and I'm > worried someone might use it to calculate something for the main container > _after_ it has been modified to fit an overflow container. In fact, I've already > uncovered what I think is one such bug (see comment on DragDrop). > > I think I would like to see this renamed so that it is clear that this variable > should only be used when in overflow. Maybe icons_per_overflow_menu_row? That > way we can have code like: > in_overflow() ? icons_per_overflow_menu_row : 1 > (or a helper function that does that, if we do it more than often). > > 2) It feels very icky to expose a global static like that. I wonder if we can > communicate this value through the ExtensionToolbarModel, instead? > ExtensionToolbarMenuView can get at that through the browser_->profile() pointer > it has and this class already has a direct pointer to the model. 1) It's not really used to guide both types of containers, since there are no menu rows in the main container. The only places it's used are in overflow menu code, or where it "just does the right thing" (like when row is always 0 in your other comment). As such, doing something like "in_overflow() ? icons_per_menu_row : 1" isn't helpful, since we don't assume it to be 1 anywhere (if you notice, it used to be 7/8 in the old code). 1.5) I'm not really opposed to saying icons_per_overflow_menu_row, but I'm slightly in favor of keeping the current name, just because "overflow_menu_row" feels redundant. After all, the only time we show icons in a menu is in overflow. 2) I'm kinda torn on whether or not to have it be a static like this. It's modeled after many of the constants in menu code, which are static yet modified based on the menu shown. [1] The logic is that there's only ever one menu shown, so it can be shared variable. I'd be willing to put it as a member of BrowserActionsContainer, but that seems a little odd since it's only used in overflow, and it would mean we always have to recalculate it. I don't really like the idea of having it on ExtensionToolbarModel, because this really is pure UI code (and platform-dependent UI code, at that). It seems like a conceptual layering violation to put the burden of it on the Model. [1] https://code.google.com/p/chromium/codesearch#chromium/src/ui/views/controls/... https://codereview.chromium.org/553233002/diff/1/chrome/browser/ui/views/tool... chrome/browser/ui/views/toolbar/browser_actions_container.h:264: static const int kItemSpacing; On 2014/09/10 10:48:28, Finnur wrote: > This is only used internally. Doesn't need to be public. This is also now used in ExtensionToolbarMenuView in order to properly calculate padding.
Screenshot looks good, both left and right icons properly aligned to the menu. https://codereview.chromium.org/553233002/diff/1/chrome/browser/ui/views/tool... File chrome/browser/ui/views/toolbar/browser_actions_container.cc (right): https://codereview.chromium.org/553233002/diff/1/chrome/browser/ui/views/tool... chrome/browser/ui/views/toolbar/browser_actions_container.cc:518: drop_position_->row * icons_per_menu_row + drop_position_->icon_in_row; Good. https://codereview.chromium.org/553233002/diff/1/chrome/browser/ui/views/tool... File chrome/browser/ui/views/toolbar/browser_actions_container.h (right): https://codereview.chromium.org/553233002/diff/1/chrome/browser/ui/views/tool... chrome/browser/ui/views/toolbar/browser_actions_container.h:260: static int icons_per_menu_row; How about we compromise on this by doing step 1.5 (naming the variable icons_per_overflow_menu_row, so that it is obvious what it should not be used for)? :) https://codereview.chromium.org/553233002/diff/1/chrome/browser/ui/views/tool... chrome/browser/ui/views/toolbar/browser_actions_container.h:264: static const int kItemSpacing; Huh. I see that now.
https://codereview.chromium.org/553233002/diff/1/chrome/browser/ui/views/tool... File chrome/browser/ui/views/toolbar/browser_actions_container.h (right): https://codereview.chromium.org/553233002/diff/1/chrome/browser/ui/views/tool... chrome/browser/ui/views/toolbar/browser_actions_container.h:260: static int icons_per_menu_row; On 2014/09/11 09:12:43, Finnur wrote: > How about we compromise on this by doing step 1.5 (naming the variable > icons_per_overflow_menu_row, so that it is obvious what it should not be used > for)? :) Done. :)
rdevlin.cronin@chromium.org changed reviewers: + sky@chromium.org
Scott, if you have time, would you mind looking at this one, too?
https://codereview.chromium.org/553233002/diff/20001/chrome/browser/ui/views/... File chrome/browser/ui/views/toolbar/browser_actions_container.h (right): https://codereview.chromium.org/553233002/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/browser_actions_container.h:260: static int icons_per_overflow_menu_row; style guide says no public members. https://codereview.chromium.org/553233002/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/browser_actions_container.h:264: static const int kItemSpacing; nit: constants should be first in the section (see style guide for exact order). https://codereview.chromium.org/553233002/diff/20001/chrome/browser/ui/views/... File chrome/browser/ui/views/toolbar/extension_toolbar_menu_view.cc (right): https://codereview.chromium.org/553233002/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/extension_toolbar_menu_view.cc:42: void ExtensionToolbarMenuView::Init(views::MenuItemView* menu_root) { Can't you do this in Layout()? At Layout() time you'll know the size. Maybe you need to know the width to calculate the preferred height? If that's the case I would rather you change SubmenuView to make a pass using GetHeightForWidth to calculate the necessary height.
https://codereview.chromium.org/553233002/diff/20001/chrome/browser/ui/views/... File chrome/browser/ui/views/toolbar/browser_actions_container.h (right): https://codereview.chromium.org/553233002/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/browser_actions_container.h:260: static int icons_per_overflow_menu_row; On 2014/09/11 19:42:27, sky wrote: > style guide says no public members. Done. https://codereview.chromium.org/553233002/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/browser_actions_container.h:264: static const int kItemSpacing; On 2014/09/11 19:42:27, sky wrote: > nit: constants should be first in the section (see style guide for exact order). Done. https://codereview.chromium.org/553233002/diff/20001/chrome/browser/ui/views/... File chrome/browser/ui/views/toolbar/extension_toolbar_menu_view.cc (right): https://codereview.chromium.org/553233002/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/extension_toolbar_menu_view.cc:42: void ExtensionToolbarMenuView::Init(views::MenuItemView* menu_root) { On 2014/09/11 19:42:27, sky wrote: > Can't you do this in Layout()? At Layout() time you'll know the size. Maybe you > need to know the width to calculate the preferred height? If that's the case I > would rather you change SubmenuView to make a pass using GetHeightForWidth to > calculate the necessary height. The problem is that we need to not just know the width of the menu, but potentially _alter_ the width of the menu. The behavior should be that we fill a row with icons and, if an even number doesn't fit, we extend the menu to make them fit perfectly (with a constant padding). So we need to calculate and set our width prior to the menu setting its own bounds. Unfortunately, MenuController::CalculateMenuBounds() is called before Layout(), so we need to do this work sooner.
On Thu, Sep 11, 2014 at 1:32 PM, <rdevlin.cronin@chromium.org> wrote: > > https://codereview.chromium.org/553233002/diff/20001/chrome/browser/ui/views/... > File chrome/browser/ui/views/toolbar/browser_actions_container.h > (right): > > https://codereview.chromium.org/553233002/diff/20001/chrome/browser/ui/views/... > chrome/browser/ui/views/toolbar/browser_actions_container.h:260: static > int icons_per_overflow_menu_row; > On 2014/09/11 19:42:27, sky wrote: >> >> style guide says no public members. > > > Done. > > https://codereview.chromium.org/553233002/diff/20001/chrome/browser/ui/views/... > chrome/browser/ui/views/toolbar/browser_actions_container.h:264: static > const int kItemSpacing; > On 2014/09/11 19:42:27, sky wrote: >> >> nit: constants should be first in the section (see style guide for > > exact order). > > Done. > > https://codereview.chromium.org/553233002/diff/20001/chrome/browser/ui/views/... > File chrome/browser/ui/views/toolbar/extension_toolbar_menu_view.cc > (right): > > https://codereview.chromium.org/553233002/diff/20001/chrome/browser/ui/views/... > chrome/browser/ui/views/toolbar/extension_toolbar_menu_view.cc:42: void > ExtensionToolbarMenuView::Init(views::MenuItemView* menu_root) { > On 2014/09/11 19:42:27, sky wrote: >> >> Can't you do this in Layout()? At Layout() time you'll know the size. > > Maybe you >> >> need to know the width to calculate the preferred height? If that's > > the case I >> >> would rather you change SubmenuView to make a pass using > > GetHeightForWidth to >> >> calculate the necessary height. > > > The problem is that we need to not just know the width of the menu, but > potentially _alter_ the width of the menu. The behavior should be that > we fill a row with icons and, if an even number doesn't fit, we extend > the menu to make them fit perfectly (with a constant padding). So we > need to calculate and set our width prior to the menu setting its own > bounds. Unfortunately, MenuController::CalculateMenuBounds() is called > before Layout(), so we need to do this work sooner. Dancing layouts are evil. What happens if someone else comes along and decides they need to enforce something similar on the wrench menu but the sizes don't line up? IMO this is a bad design. You should fill the available space and be ok if you can't fit at all entirely. If you really must do this, then I think it better to have a specific call to the delegate that allows enforcing a size on a submenu. That way it's a bit clearer that this operation effects the whole menu size. -Scott > > https://codereview.chromium.org/553233002/ To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
> Dancing layouts are evil. What happens if someone else comes along and > decides they need to enforce something similar on the wrench menu but > the sizes don't line up? IMO this is a bad design. You should fill the > available space and be ok if you can't fit at all entirely. > > If you really must do this, then I think it better to have a specific > call to the delegate that allows enforcing a size on a submenu. That > way it's a bit clearer that this operation effects the whole menu > size. > > -Scott This was discussed offline, and it was concluded that we should avoid modifying the menu size at all, and should also not worry about right-justifying the icons. We do, however, still need to dynamically calculate the number of icons on a row, so we can account for font settings, languages, OS, etc.
Scott, mind taking a look at this version?
https://codereview.chromium.org/553233002/diff/60001/chrome/browser/ui/views/... File chrome/browser/ui/views/toolbar/browser_actions_container.h (right): https://codereview.chromium.org/553233002/diff/60001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/browser_actions_container.h:430: static int icons_per_overflow_menu_row_; Why does this need to be static? https://codereview.chromium.org/553233002/diff/60001/ui/views/controls/menu/m... File ui/views/controls/menu/menu_item_view.cc (right): https://codereview.chromium.org/553233002/diff/60001/ui/views/controls/menu/m... ui/views/controls/menu/menu_item_view.cc:417: if (!IsContainer()) If !container, can't you just use GetPreferredSize().height()? I'm ok with being inconsistent for the icon_view_. Layout doesn't ask for the heightforwidth either, so it's ok not to here too. https://codereview.chromium.org/553233002/diff/60001/ui/views/controls/menu/m... ui/views/controls/menu/menu_item_view.cc:418: height = icon_view_ ? icon_view_->GetHeightForWidth(width) : 0; nit: spacing. https://codereview.chromium.org/553233002/diff/60001/ui/views/controls/menu/m... ui/views/controls/menu/menu_item_view.cc:428: if (is_dimensions_valid() && height != GetDimensions().height) I don't understand the need for this. Can you elaborate? MenuItemDimensions should only matter for the !container case, which I think you should early out of here. https://codereview.chromium.org/553233002/diff/60001/ui/views/controls/menu/m... File ui/views/controls/menu/menu_item_view.h (right): https://codereview.chromium.org/553233002/diff/60001/ui/views/controls/menu/m... ui/views/controls/menu/menu_item_view.h:427: void invalidate_dimensions() const { dimensions_.height = 0; } This shouldn't be const. It's modifying a member. That said, dimensions_ is const, so you shouldn't need the const. Maybe I'm missing something. https://codereview.chromium.org/553233002/diff/60001/ui/views/controls/menu/s... File ui/views/controls/menu/submenu_view.cc (right): https://codereview.chromium.org/553233002/diff/60001/ui/views/controls/menu/s... ui/views/controls/menu/submenu_view.cc:175: return gfx::Size(width, height + insets.height()); Can you write test coverage of this?
https://codereview.chromium.org/553233002/diff/60001/chrome/browser/ui/views/... File chrome/browser/ui/views/toolbar/browser_actions_container.h (right): https://codereview.chromium.org/553233002/diff/60001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/browser_actions_container.h:430: static int icons_per_overflow_menu_row_; On 2014/09/12 20:09:37, sky wrote: > Why does this need to be static? It doesn't really, but there should only be one menu at a time showing, so it kind of made sense to me (in the same way that certain MenuItemView members are static). I'm happy to make it a private member, though. Of course, if we do that, we'll have to either cheat and make it mutable or use something slightly less elegant in GetHeightForWidth() (which is const). https://codereview.chromium.org/553233002/diff/60001/ui/views/controls/menu/m... File ui/views/controls/menu/menu_item_view.cc (right): https://codereview.chromium.org/553233002/diff/60001/ui/views/controls/menu/m... ui/views/controls/menu/menu_item_view.cc:417: if (!IsContainer()) On 2014/09/12 20:09:37, sky wrote: > If !container, can't you just use GetPreferredSize().height()? I'm ok with being > inconsistent for the icon_view_. Layout doesn't ask for the heightforwidth > either, so it's ok not to here too. As long as we don't mind being inconsistent for icon_view_, yeah. Done. https://codereview.chromium.org/553233002/diff/60001/ui/views/controls/menu/m... ui/views/controls/menu/menu_item_view.cc:418: height = icon_view_ ? icon_view_->GetHeightForWidth(width) : 0; On 2014/09/12 20:09:37, sky wrote: > nit: spacing. moot (line gone). https://codereview.chromium.org/553233002/diff/60001/ui/views/controls/menu/m... ui/views/controls/menu/menu_item_view.cc:428: if (is_dimensions_valid() && height != GetDimensions().height) On 2014/09/12 20:09:37, sky wrote: > I don't understand the need for this. Can you elaborate? MenuItemDimensions > should only matter for the !container case, which I think you should early out > of here. But the Dimensions are used in GetPreferredSize(), so if our preferred size changed here (as is the case with the extension action overflow container), we need to invalidate those dimensions. Otherwise they'll never be recalculated in GetPreferredSize(), and we use the old dimensions. https://codereview.chromium.org/553233002/diff/60001/ui/views/controls/menu/m... File ui/views/controls/menu/menu_item_view.h (right): https://codereview.chromium.org/553233002/diff/60001/ui/views/controls/menu/m... ui/views/controls/menu/menu_item_view.h:427: void invalidate_dimensions() const { dimensions_.height = 0; } On 2014/09/12 20:09:37, sky wrote: > This shouldn't be const. It's modifying a member. That said, dimensions_ is > const, so you shouldn't need the const. Maybe I'm missing something. |dimensions_| are mutable, because it's just a value cached for performance reasons. But we need to invalidate them in GetHeightForWidth() (see other comment), and, if we want to use this function (rather than just saying "dimensions_.height = 0;"), we need to add the const here. https://codereview.chromium.org/553233002/diff/60001/ui/views/controls/menu/s... File ui/views/controls/menu/submenu_view.cc (right): https://codereview.chromium.org/553233002/diff/60001/ui/views/controls/menu/s... ui/views/controls/menu/submenu_view.cc:175: return gfx::Size(width, height + insets.height()); On 2014/09/12 20:09:38, sky wrote: > Can you write test coverage of this? But of course! Done. :)
https://codereview.chromium.org/553233002/diff/60001/ui/views/controls/menu/m... File ui/views/controls/menu/menu_item_view.cc (right): https://codereview.chromium.org/553233002/diff/60001/ui/views/controls/menu/m... ui/views/controls/menu/menu_item_view.cc:428: if (is_dimensions_valid() && height != GetDimensions().height) On 2014/09/12 22:41:37, Devlin wrote: > On 2014/09/12 20:09:37, sky wrote: > > I don't understand the need for this. Can you elaborate? MenuItemDimensions > > should only matter for the !container case, which I think you should early out > > of here. > > But the Dimensions are used in GetPreferredSize(), so if our preferred size > changed here (as is the case with the extension action overflow container), we > need to invalidate those dimensions. Otherwise they'll never be recalculated in > GetPreferredSize(), and we use the old dimensions. Why does that matter though? GetPreferredSize and GetHeightForWidth need not return the same thing. There is no guarantee or implication that one influences the other.
https://codereview.chromium.org/553233002/diff/60001/ui/views/controls/menu/m... File ui/views/controls/menu/menu_item_view.cc (right): https://codereview.chromium.org/553233002/diff/60001/ui/views/controls/menu/m... ui/views/controls/menu/menu_item_view.cc:428: if (is_dimensions_valid() && height != GetDimensions().height) On 2014/09/15 15:29:54, sky wrote: > On 2014/09/12 22:41:37, Devlin wrote: > > On 2014/09/12 20:09:37, sky wrote: > > > I don't understand the need for this. Can you elaborate? MenuItemDimensions > > > should only matter for the !container case, which I think you should early > out > > > of here. > > > > But the Dimensions are used in GetPreferredSize(), so if our preferred size > > changed here (as is the case with the extension action overflow container), we > > need to invalidate those dimensions. Otherwise they'll never be recalculated > in > > GetPreferredSize(), and we use the old dimensions. > > Why does that matter though? GetPreferredSize and GetHeightForWidth need not > return the same thing. There is no guarantee or implication that one influences > the other. In SubmenuView::GetPreferredSize(), we loop over the MenuItemView children in two passes - the first to get the width (and find the maximum width for the menu), and the second to GetHeightForWidth(). Unfortunately, if a menu item has flexible width (like the extension action overflow container), then the dimensions initially calculated in the first call to GetPreferredSize() will be incorrect (since the width isn't set at that time - we're still calculating it), but they will still be "valid". As such, we need to invalidate them before GetPreferredSize() is called again (since the preferred size is cached as part of GetPreferredSize()) when we draw the menu items. It seemed simplest to me to do this here, so that we can confine the logic to MenuItemView (the thing that's actually changing), but we could also potentially do it SubmenuView() after calling GetHeightForWidth(), or we could make the first loop SubmenuView::GetPreferredSize() have more complex checks so that it doesn't call GetPreferredSize() on the MenuItemViews that have flexible widths.
https://codereview.chromium.org/553233002/diff/100001/ui/views/controls/menu/... File ui/views/controls/menu/submenu_view.cc (right): https://codereview.chromium.org/553233002/diff/100001/ui/views/controls/menu/... ui/views/controls/menu/submenu_view.cc:117: gfx::Size child_pref_size = child->GetPreferredSize(); Is this the bug you're working around? Now that we're using GetHeightForWidth for sizing we need to use GetHeightForWidth for layout too.
Patchset #7 (id:120001) has been deleted
https://codereview.chromium.org/553233002/diff/100001/ui/views/controls/menu/... File ui/views/controls/menu/submenu_view.cc (right): https://codereview.chromium.org/553233002/diff/100001/ui/views/controls/menu/... ui/views/controls/menu/submenu_view.cc:117: gfx::Size child_pref_size = child->GetPreferredSize(); On 2014/09/15 16:21:24, sky wrote: > Is this the bug you're working around? Now that we're using GetHeightForWidth > for sizing we need to use GetHeightForWidth for layout too. Yeah, if we change this to GetHeightForWidth(), it should work. My only hesitance in doing that was that then calling GetPreferredSize() on the MenuItemView containing a view like the action overflow is kind of misleading. But no more so, I suppose, than invalidating dimensions as part of finding the height for a width. So changed.
https://codereview.chromium.org/553233002/diff/140001/chrome/browser/ui/views... File chrome/browser/ui/views/menu_item_view_interactive_uitest.cc (right): https://codereview.chromium.org/553233002/diff/140001/chrome/browser/ui/views... chrome/browser/ui/views/menu_item_view_interactive_uitest.cc:347: // Test that MenuItemViews can have flexible sizes if they are containers. Isn't there a way to test your new code without an interactive ui test?
https://codereview.chromium.org/553233002/diff/140001/chrome/browser/ui/views... File chrome/browser/ui/views/menu_item_view_interactive_uitest.cc (right): https://codereview.chromium.org/553233002/diff/140001/chrome/browser/ui/views... chrome/browser/ui/views/menu_item_view_interactive_uitest.cc:347: // Test that MenuItemViews can have flexible sizes if they are containers. On 2014/09/15 17:14:56, sky wrote: > Isn't there a way to test your new code without an interactive ui test? Ah, I suppose none of this really needs interaction. Made it a unittest. :)
https://codereview.chromium.org/553233002/diff/160001/chrome/browser/ui/views... File chrome/browser/ui/views/menu_item_view_unittest.cc (right): https://codereview.chromium.org/553233002/diff/160001/chrome/browser/ui/views... chrome/browser/ui/views/menu_item_view_unittest.cc:1: // Copyright 2014 The Chromium Authors. All rights reserved. Is there a reason you put this in chrome and not views tests? https://codereview.chromium.org/553233002/diff/160001/chrome/browser/ui/views... chrome/browser/ui/views/menu_item_view_unittest.cc:42: scoped_ptr<TestMenuItemView> root_menu(new TestMenuItemView()); nit: AFAICT there is no need to wrap in a scoped_ptr. Can you declare on the stack?
https://codereview.chromium.org/553233002/diff/160001/chrome/browser/ui/views... File chrome/browser/ui/views/menu_item_view_unittest.cc (right): https://codereview.chromium.org/553233002/diff/160001/chrome/browser/ui/views... chrome/browser/ui/views/menu_item_view_unittest.cc:1: // Copyright 2014 The Chromium Authors. All rights reserved. On 2014/09/15 18:19:13, sky wrote: > Is there a reason you put this in chrome and not views tests? Only because that's where the MenuItemView interactive ui tests were. Moved. https://codereview.chromium.org/553233002/diff/160001/chrome/browser/ui/views... chrome/browser/ui/views/menu_item_view_unittest.cc:42: scoped_ptr<TestMenuItemView> root_menu(new TestMenuItemView()); On 2014/09/15 18:19:12, sky wrote: > nit: AFAICT there is no need to wrap in a scoped_ptr. Can you declare on the > stack? Good point. Done.
LGTM
The CQ bit was checked by rdevlin.cronin@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patchset/553233002/180001
Message was sent while issue was closed.
Committed patchset #9 (id:180001) as eb76312bb6ec1d8bcdc88f82642bc46459806e51
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/9d543b5777b347d6071692a4b09effbeff838526 Cr-Commit-Position: refs/heads/master@{#294944} |
