Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10532171:
Added support for icon views (view used instead of icon in a menu item). (Closed)
|
Created:
8 years, 6 months ago by yefimt Modified:
8 years, 5 months ago CC:
chromium-reviews, Aaron Boodman, mihaip-chromium-reviews_chromium.org, tfarina, msw, rudygalfi, macourteau Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdded support for icon views (view used instead of icon in a menu item).
BUG=125307
TEST=Make sure menu works as before, especially menu items with icons
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=144787
Patch Set 1 : Added support for icon views (view used instead of icon in a menu item). #
Total comments: 12
Patch Set 2 : Added support for icon views (view used instead of icon in a menu item). #Patch Set 3 : Added support for icon views (view used instead of icon in a menu item). #
Total comments: 25
Patch Set 4 : Added support for icon views (view used instead of icon in a menu item). #
Total comments: 7
Patch Set 5 : Added support for icon views (view used instead of icon in a menu item). #
Total comments: 7
Patch Set 6 : Added support for icon views (view used instead of icon in a menu item). #
Total comments: 6
Patch Set 7 : Added support for icon views (view used instead of icon in a menu item). #
Total comments: 2
Patch Set 8 : Added support for icon views (view used instead of icon in a menu item). #
Total comments: 22
Patch Set 9 : Added support for icon views (view used instead of icon in a menu item). #
Messages
Total messages: 20 (0 generated)
http://codereview.chromium.org/10532171/diff/2017/ui/views/controls/menu/menu... File ui/views/controls/menu/menu_item_view.cc (right): http://codereview.chromium.org/10532171/diff/2017/ui/views/controls/menu/menu... ui/views/controls/menu/menu_item_view.cc:89: has_icon_views_(false), I don't see where has_icon_views_ is ever assigned to. It's always false? http://codereview.chromium.org/10532171/diff/2017/ui/views/controls/menu/menu... ui/views/controls/menu/menu_item_view.cc:365: void MenuItemView::SetIcon(const gfx::ImageSkia& icon) { Is it possible to reimplement SetIcon() in terms of SetIconView()? (e.g., by creating an ImageView and passing it to SetIconView). http://codereview.chromium.org/10532171/diff/2017/ui/views/controls/menu/menu... ui/views/controls/menu/menu_item_view.cc:500: int last_right_side_child = has_icon_view_ ? 1 : 0; naming suggestion: first_non_icon_view. http://codereview.chromium.org/10532171/diff/2017/ui/views/controls/menu/menu... File ui/views/controls/menu/menu_item_view.h (right): http://codereview.chromium.org/10532171/diff/2017/ui/views/controls/menu/menu... ui/views/controls/menu/menu_item_view.h:248: View* GetIconView(); If these are just simple getters and setters, the correct style would be: void set_icon_view(...) View* icon_view() const; http://codereview.chromium.org/10532171/diff/2017/ui/views/controls/menu/menu... File ui/views/controls/menu/menu_item_view_win.cc (right): http://codereview.chromium.org/10532171/diff/2017/ui/views/controls/menu/menu... ui/views/controls/menu/menu_item_view_win.cc:90: int height = has_icon_view_ ? this->height() : font.GetHeight(); Can you combine the two cases by doing max(this->height(), font.GetHeight()) ? http://codereview.chromium.org/10532171/diff/2017/ui/views/controls/menu/menu... ui/views/controls/menu/menu_item_view_win.cc:91: int flags = (has_icon_view_ ? gfx::Canvas::TEXT_VALIGN_MIDDLE : 0) | It seems like it would still be correct to do TEXT_VALIGN_MIDDLE in either case.
http://codereview.chromium.org/10532171/diff/2017/ui/views/controls/menu/menu... File ui/views/controls/menu/menu_item_view.h (right): http://codereview.chromium.org/10532171/diff/2017/ui/views/controls/menu/menu... ui/views/controls/menu/menu_item_view.h:248: View* GetIconView(); On 2012/06/19 00:06:33, Aaron Boodman wrote: > If these are just simple getters and setters, the correct style would be: > > void set_icon_view(...) > View* icon_view() const; Nevermind, I forgot to remove this comment.
http://codereview.chromium.org/10532171/diff/2017/ui/views/controls/menu/menu... File ui/views/controls/menu/menu_item_view.cc (right): http://codereview.chromium.org/10532171/diff/2017/ui/views/controls/menu/menu... ui/views/controls/menu/menu_item_view.cc:89: has_icon_views_(false), There is a set function in an h file On 2012/06/19 00:06:33, Aaron Boodman wrote: > I don't see where has_icon_views_ is ever assigned to. It's always false? http://codereview.chromium.org/10532171/diff/2017/ui/views/controls/menu/menu... ui/views/controls/menu/menu_item_view.cc:365: void MenuItemView::SetIcon(const gfx::ImageSkia& icon) { On 2012/06/19 00:06:33, Aaron Boodman wrote: > Is it possible to reimplement SetIcon() in terms of SetIconView()? (e.g., by > creating an ImageView and passing it to SetIconView). Done. http://codereview.chromium.org/10532171/diff/2017/ui/views/controls/menu/menu... ui/views/controls/menu/menu_item_view.cc:500: int last_right_side_child = has_icon_view_ ? 1 : 0; On 2012/06/19 00:06:33, Aaron Boodman wrote: > naming suggestion: first_non_icon_view. Done. http://codereview.chromium.org/10532171/diff/2017/ui/views/controls/menu/menu... File ui/views/controls/menu/menu_item_view_win.cc (right): http://codereview.chromium.org/10532171/diff/2017/ui/views/controls/menu/menu... ui/views/controls/menu/menu_item_view_win.cc:90: int height = has_icon_view_ ? this->height() : font.GetHeight(); On 2012/06/19 00:06:33, Aaron Boodman wrote: > Can you combine the two cases by doing max(this->height(), font.GetHeight()) ? Done. http://codereview.chromium.org/10532171/diff/2017/ui/views/controls/menu/menu... ui/views/controls/menu/menu_item_view_win.cc:91: int flags = (has_icon_view_ ? gfx::Canvas::TEXT_VALIGN_MIDDLE : 0) | On 2012/06/19 00:06:33, Aaron Boodman wrote: > It seems like it would still be correct to do TEXT_VALIGN_MIDDLE in either case. Done.
http://codereview.chromium.org/10532171/diff/23001/ui/views/controls/menu/men... File ui/views/controls/menu/menu_item_view.cc (right): http://codereview.chromium.org/10532171/diff/23001/ui/views/controls/menu/men... ui/views/controls/menu/menu_item_view.cc:66: //static // static http://codereview.chromium.org/10532171/diff/23001/ui/views/controls/menu/men... ui/views/controls/menu/menu_item_view.cc:372: if (icon_view_type_ == IMAGE_VIEW) { Make this always create a new ImageView, unless the icon is empty in which case it should set it to NULL. http://codereview.chromium.org/10532171/diff/23001/ui/views/controls/menu/men... ui/views/controls/menu/menu_item_view.cc:526: int first_non_icon_view = (icon_view_type_ != NONE) ? 1 : 0; I don't like assuming the icon_view is always first here. When you add the member it'll be easier to know which view is the icon view. http://codereview.chromium.org/10532171/diff/23001/ui/views/controls/menu/men... ui/views/controls/menu/menu_item_view.cc:819: gfx::Size size = child_at(i)->GetPreferredSize(); revert to old here. http://codereview.chromium.org/10532171/diff/23001/ui/views/controls/menu/men... ui/views/controls/menu/menu_item_view.cc:826: // Return a height of 0 to indicate that we should use the title height Update this comment. http://codereview.chromium.org/10532171/diff/23001/ui/views/controls/menu/men... File ui/views/controls/menu/menu_item_view.h (right): http://codereview.chromium.org/10532171/diff/23001/ui/views/controls/menu/men... ui/views/controls/menu/menu_item_view.h:243: const gfx::ImageSkia& GetIcon(); I would just assume remove this. Do we actually need it? http://codereview.chromium.org/10532171/diff/23001/ui/views/controls/menu/men... ui/views/controls/menu/menu_item_view.h:246: // It always be inserted as a first child view. How about: Sets the view used to render the icon. This clobbers any icon set via SetIcon(). MenuItemView takes ownership of |icon_view|. Is it really necessary to mention how the child is added? http://codereview.chromium.org/10532171/diff/23001/ui/views/controls/menu/men... ui/views/controls/menu/menu_item_view.h:318: virtual Border* GetMenuBorder(); Descriptions. http://codereview.chromium.org/10532171/diff/23001/ui/views/controls/menu/men... ui/views/controls/menu/menu_item_view.h:450: static gfx::ImageSkia empty_icon_; No static objects. http://codereview.chromium.org/10532171/diff/23001/ui/views/controls/menu/men... ui/views/controls/menu/menu_item_view.h:463: enum IconViewType { Move enum to top of section. http://codereview.chromium.org/10532171/diff/23001/ui/views/controls/menu/men... ui/views/controls/menu/menu_item_view.h:466: SOME_VIEW HA! Rather than the enum have an explicit icon_view_ member of type view. Better type safety anyway. http://codereview.chromium.org/10532171/diff/23001/ui/views/controls/menu/men... ui/views/controls/menu/menu_item_view.h:474: static int icon_width_; Add a description. http://codereview.chromium.org/10532171/diff/23001/ui/views/controls/menu/men... File ui/views/controls/menu/menu_scroll_view_container.cc (right): http://codereview.chromium.org/10532171/diff/23001/ui/views/controls/menu/men... ui/views/controls/menu/menu_scroll_view_container.cc:168: MenuItemView* root = content_view_->GetMenuItem(); This won't propagate to children and I don't like attempting to share borders/background. If you're looking for a way to propagate menus I think a factory method for creation on the delegate is a better approach.
http://codereview.chromium.org/10532171/diff/19005/ui/views/controls/menu/men... File ui/views/controls/menu/menu_item_view.cc (right): http://codereview.chromium.org/10532171/diff/19005/ui/views/controls/menu/men... ui/views/controls/menu/menu_item_view.cc:70: int MenuItemView::icon_width_ = 0; Having this stuff be static is really weird, but I guess you're just sticking with the current convention. http://codereview.chromium.org/10532171/diff/19005/ui/views/controls/menu/men... ui/views/controls/menu/menu_item_view.cc:526: int first_non_icon_view = (icon_view_type_ != NONE) ? 1 : 0; If you have an icon_view_ member, you can remove this line and just skip the icon_view_ in the loop below. http://codereview.chromium.org/10532171/diff/19005/ui/views/controls/menu/men... File ui/views/controls/menu/menu_item_view.h (right): http://codereview.chromium.org/10532171/diff/19005/ui/views/controls/menu/men... ui/views/controls/menu/menu_item_view.h:466: SOME_VIEW Agree with sky that it would be nicer to have an explicit image_view_ member. http://codereview.chromium.org/10532171/diff/19005/ui/views/controls/menu/men... File ui/views/controls/menu/menu_item_view_win.cc (right): http://codereview.chromium.org/10532171/diff/19005/ui/views/controls/menu/men... ui/views/controls/menu/menu_item_view_win.cc:89: int height = this->height() - GetTopMargin() - GetBottomMargin(); Ah, you were able to normalize the two case here, cool.
http://codereview.chromium.org/10532171/diff/23001/ui/views/controls/menu/men... File ui/views/controls/menu/menu_item_view.cc (right): http://codereview.chromium.org/10532171/diff/23001/ui/views/controls/menu/men... ui/views/controls/menu/menu_item_view.cc:66: //static On 2012/06/20 21:52:40, sky wrote: > // static With GetIcon() gone we dont need an empty icon anymore http://codereview.chromium.org/10532171/diff/23001/ui/views/controls/menu/men... ui/views/controls/menu/menu_item_view.cc:372: if (icon_view_type_ == IMAGE_VIEW) { On 2012/06/20 21:52:40, sky wrote: > Make this always create a new ImageView, unless the icon is empty in which case > it should set it to NULL. Done. http://codereview.chromium.org/10532171/diff/23001/ui/views/controls/menu/men... ui/views/controls/menu/menu_item_view.cc:526: int first_non_icon_view = (icon_view_type_ != NONE) ? 1 : 0; On 2012/06/20 21:52:40, sky wrote: > I don't like assuming the icon_view is always first here. When you add the > member it'll be easier to know which view is the icon view. Done. http://codereview.chromium.org/10532171/diff/23001/ui/views/controls/menu/men... ui/views/controls/menu/menu_item_view.cc:819: gfx::Size size = child_at(i)->GetPreferredSize(); On 2012/06/20 21:52:40, sky wrote: > revert to old here. Done. http://codereview.chromium.org/10532171/diff/23001/ui/views/controls/menu/men... ui/views/controls/menu/menu_item_view.cc:826: // Return a height of 0 to indicate that we should use the title height On 2012/06/20 21:52:40, sky wrote: > Update this comment. Done. http://codereview.chromium.org/10532171/diff/23001/ui/views/controls/menu/men... File ui/views/controls/menu/menu_item_view.h (right): http://codereview.chromium.org/10532171/diff/23001/ui/views/controls/menu/men... ui/views/controls/menu/menu_item_view.h:243: const gfx::ImageSkia& GetIcon(); Removed On 2012/06/20 21:52:40, sky wrote: > I would just assume remove this. Do we actually need it? http://codereview.chromium.org/10532171/diff/23001/ui/views/controls/menu/men... ui/views/controls/menu/menu_item_view.h:246: // It always be inserted as a first child view. On 2012/06/20 21:52:40, sky wrote: > How about: > Sets the view used to render the icon. This clobbers any icon set via SetIcon(). > MenuItemView takes ownership of |icon_view|. > > Is it really necessary to mention how the child is added? Done. http://codereview.chromium.org/10532171/diff/23001/ui/views/controls/menu/men... ui/views/controls/menu/menu_item_view.h:318: virtual Border* GetMenuBorder(); On 2012/06/20 21:52:40, sky wrote: > Descriptions. Done. http://codereview.chromium.org/10532171/diff/23001/ui/views/controls/menu/men... ui/views/controls/menu/menu_item_view.h:450: static gfx::ImageSkia empty_icon_; removed On 2012/06/20 21:52:40, sky wrote: > No static objects. http://codereview.chromium.org/10532171/diff/23001/ui/views/controls/menu/men... ui/views/controls/menu/menu_item_view.h:463: enum IconViewType { removed On 2012/06/20 21:52:40, sky wrote: > Move enum to top of section. http://codereview.chromium.org/10532171/diff/23001/ui/views/controls/menu/men... ui/views/controls/menu/menu_item_view.h:466: SOME_VIEW On 2012/06/20 21:52:40, sky wrote: > HA! Rather than the enum have an explicit icon_view_ member of type view. Better > type safety anyway. Done. http://codereview.chromium.org/10532171/diff/23001/ui/views/controls/menu/men... ui/views/controls/menu/menu_item_view.h:474: static int icon_width_; On 2012/06/20 21:52:40, sky wrote: > Add a description. Done. http://codereview.chromium.org/10532171/diff/19005/ui/views/controls/menu/men... File ui/views/controls/menu/menu_item_view.cc (right): http://codereview.chromium.org/10532171/diff/19005/ui/views/controls/menu/men... ui/views/controls/menu/menu_item_view.cc:526: int first_non_icon_view = (icon_view_type_ != NONE) ? 1 : 0; On 2012/06/21 08:15:30, Aaron Boodman wrote: > If you have an icon_view_ member, you can remove this line and just skip the > icon_view_ in the loop below. Done. http://codereview.chromium.org/10532171/diff/19005/ui/views/controls/menu/men... File ui/views/controls/menu/menu_item_view.h (right): http://codereview.chromium.org/10532171/diff/19005/ui/views/controls/menu/men... ui/views/controls/menu/menu_item_view.h:466: SOME_VIEW On 2012/06/21 08:15:30, Aaron Boodman wrote: > Agree with sky that it would be nicer to have an explicit image_view_ member. Done. http://codereview.chromium.org/10532171/diff/19005/ui/views/controls/menu/men... File ui/views/controls/menu/menu_item_view_win.cc (right): http://codereview.chromium.org/10532171/diff/19005/ui/views/controls/menu/men... ui/views/controls/menu/menu_item_view_win.cc:89: int height = this->height() - GetTopMargin() - GetBottomMargin(); Yeah, found what i was doing wrong before On 2012/06/21 08:15:30, Aaron Boodman wrote: > Ah, you were able to normalize the two case here, cool.
I started on patchset 3 the other day and got distracted. Here's comments from that patchset. I'll look at the latest right now. http://codereview.chromium.org/10532171/diff/32001/ui/views/controls/menu/men... File ui/views/controls/menu/menu_delegate.h (right): http://codereview.chromium.org/10532171/diff/32001/ui/views/controls/menu/men... ui/views/controls/menu/menu_delegate.h:206: // Returns menu border. Applicable only to a root menu item. Creates and returns a new border for the menu, or NULL if no border is needed. Caller owns the returned object. http://codereview.chromium.org/10532171/diff/32001/ui/views/controls/menu/men... ui/views/controls/menu/menu_delegate.h:209: // Returns menu background. Applicable only to a root menu item. Creates and returns a new background for the menu, or NULL if no background is needed. Caller owns the returned object. http://codereview.chromium.org/10532171/diff/32001/ui/views/controls/menu/men... File ui/views/controls/menu/menu_item_view.h (right): http://codereview.chromium.org/10532171/diff/32001/ui/views/controls/menu/men... ui/views/controls/menu/menu_item_view.h:245: View* GetIconView(); GetIconView() -> icon_view() and inline it.
http://codereview.chromium.org/10532171/diff/32001/ui/views/controls/menu/men... File ui/views/controls/menu/menu_delegate.h (right): http://codereview.chromium.org/10532171/diff/32001/ui/views/controls/menu/men... ui/views/controls/menu/menu_delegate.h:206: // Returns menu border. Applicable only to a root menu item. On 2012/06/26 21:42:13, sky wrote: > Creates and returns a new border for the menu, or NULL if no border is needed. > Caller owns the returned object. You should name these to match: CreateMenuBorder and CreateMenuBackground.
http://codereview.chromium.org/10532171/diff/43002/ui/views/controls/menu/men... File ui/views/controls/menu/menu_item_view.cc (left): http://codereview.chromium.org/10532171/diff/43002/ui/views/controls/menu/men... ui/views/controls/menu/menu_item_view.cc:537: void MenuItemView::UpdateMenuPartSizes(bool has_icons) { has_icons is set on the root and expected to percolate down. This is why the function needs to take the boolean and not access this->icon_views_. Add a comment to that effect in here. http://codereview.chromium.org/10532171/diff/43002/ui/views/controls/menu/men... File ui/views/controls/menu/menu_item_view.cc (right): http://codereview.chromium.org/10532171/diff/43002/ui/views/controls/menu/men... ui/views/controls/menu/menu_item_view.cc:841: return child_count() == 1 && title_.empty(); I believe you need to update this. Once you do that you should be able to nuke HasNonIconChildViews. http://codereview.chromium.org/10532171/diff/43002/ui/views/controls/menu/men... File ui/views/controls/menu/menu_item_view.h (right): http://codereview.chromium.org/10532171/diff/43002/ui/views/controls/menu/men... ui/views/controls/menu/menu_item_view.h:245: View* GetIconView(); icon_view and inline it.
http://codereview.chromium.org/10532171/diff/32001/ui/views/controls/menu/men... File ui/views/controls/menu/menu_delegate.h (right): http://codereview.chromium.org/10532171/diff/32001/ui/views/controls/menu/men... ui/views/controls/menu/menu_delegate.h:206: // Returns menu border. Applicable only to a root menu item. On 2012/06/26 21:42:13, sky wrote: > Creates and returns a new border for the menu, or NULL if no border is needed. > Caller owns the returned object. Done. http://codereview.chromium.org/10532171/diff/32001/ui/views/controls/menu/men... ui/views/controls/menu/menu_delegate.h:209: // Returns menu background. Applicable only to a root menu item. On 2012/06/26 21:42:13, sky wrote: > Creates and returns a new background for the menu, or NULL if no background is > needed. Caller owns the returned object. Done. http://codereview.chromium.org/10532171/diff/32001/ui/views/controls/menu/men... File ui/views/controls/menu/menu_item_view.h (right): http://codereview.chromium.org/10532171/diff/32001/ui/views/controls/menu/men... ui/views/controls/menu/menu_item_view.h:245: View* GetIconView(); On 2012/06/26 21:42:13, sky wrote: > GetIconView() -> icon_view() and inline it. Done. http://codereview.chromium.org/10532171/diff/43002/ui/views/controls/menu/men... File ui/views/controls/menu/menu_item_view.cc (left): http://codereview.chromium.org/10532171/diff/43002/ui/views/controls/menu/men... ui/views/controls/menu/menu_item_view.cc:537: void MenuItemView::UpdateMenuPartSizes(bool has_icons) { It used to take has_icons because function was static, now when it is not there is no reason to pass it as a param On 2012/06/26 21:54:21, sky wrote: > has_icons is set on the root and expected to percolate down. This is why the > function needs to take the boolean and not access this->icon_views_. Add a > comment to that effect in here. http://codereview.chromium.org/10532171/diff/43002/ui/views/controls/menu/men... File ui/views/controls/menu/menu_item_view.cc (right): http://codereview.chromium.org/10532171/diff/43002/ui/views/controls/menu/men... ui/views/controls/menu/menu_item_view.cc:841: return child_count() == 1 && title_.empty(); On 2012/06/26 21:54:21, sky wrote: > I believe you need to update this. Once you do that you should be able to nuke > HasNonIconChildViews. Done. http://codereview.chromium.org/10532171/diff/43002/ui/views/controls/menu/men... File ui/views/controls/menu/menu_item_view.h (right): http://codereview.chromium.org/10532171/diff/43002/ui/views/controls/menu/men... ui/views/controls/menu/menu_item_view.h:245: View* GetIconView(); On 2012/06/26 21:54:21, sky wrote: > icon_view and inline it. Done.
http://codereview.chromium.org/10532171/diff/51001/ui/views/controls/menu/men... File ui/views/controls/menu/menu_item_view.cc (right): http://codereview.chromium.org/10532171/diff/51001/ui/views/controls/menu/men... ui/views/controls/menu/menu_item_view.cc:583: for (int i = 0; i < submenu_->GetMenuItemCount(); ++i) { This needs to recurse through submenus.
http://codereview.chromium.org/10532171/diff/51001/ui/views/controls/menu/men... File ui/views/controls/menu/menu_item_view.cc (right): http://codereview.chromium.org/10532171/diff/51001/ui/views/controls/menu/men... ui/views/controls/menu/menu_item_view.cc:583: for (int i = 0; i < submenu_->GetMenuItemCount(); ++i) { On 2012/06/26 23:49:54, sky wrote: > This needs to recurse through submenus. Done.
LGTM
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yefim@chromium.org/10532171/55001
Just some driveby nits; sorry for the delay. http://codereview.chromium.org/10532171/diff/55001/ui/views/controls/menu/men... File ui/views/controls/menu/menu_delegate.cc (right): http://codereview.chromium.org/10532171/diff/55001/ui/views/controls/menu/men... ui/views/controls/menu/menu_delegate.cc:132: MenuConfig::instance().submenu_vertical_margin_size, nit: Indent two more spaces. http://codereview.chromium.org/10532171/diff/55001/ui/views/controls/menu/men... File ui/views/controls/menu/menu_item_view.cc (right): http://codereview.chromium.org/10532171/diff/55001/ui/views/controls/menu/men... ui/views/controls/menu/menu_item_view.cc:515: // Position icon_view nit: vertical bars around "|icon_view|", add punctuation. http://codereview.chromium.org/10532171/diff/55001/ui/views/controls/menu/men... ui/views/controls/menu/menu_item_view.cc:521: int y = (height() + GetTopMargin() - GetBottomMargin() - nit: I'd move the entire value calculation to the second line, indented by 4 spaces. http://codereview.chromium.org/10532171/diff/55001/ui/views/controls/menu/men... ui/views/controls/menu/menu_item_view.cc:830: return NonIconChildViewsCount() == 1 && title_.empty(); nit: add parens around the first conditional. http://codereview.chromium.org/10532171/diff/55001/ui/views/controls/menu/men... File ui/views/controls/menu/menu_item_view.h (right): http://codereview.chromium.org/10532171/diff/55001/ui/views/controls/menu/men... ui/views/controls/menu/menu_item_view.h:410: // Returns number if child views excluding icon_view. nit: "if"->"of". http://codereview.chromium.org/10532171/diff/55001/ui/views/controls/menu/men... ui/views/controls/menu/menu_item_view.h:413: // Get recursively max icon views width. nit: grammar/clarity. Perhaps: "Returns the max icon width; recurses over submenus." http://codereview.chromium.org/10532171/diff/55001/ui/views/controls/menu/men... ui/views/controls/menu/menu_item_view.h:453: // Set if menu has icons or icon_views nit: combine comments onto one line or add punctuation on first. http://codereview.chromium.org/10532171/diff/55001/ui/views/controls/menu/men... File ui/views/controls/menu/menu_item_view_views.cc (right): http://codereview.chromium.org/10532171/diff/55001/ui/views/controls/menu/men... ui/views/controls/menu/menu_item_view_views.cc:26: NonIconChildViewsCount() == 0); nit: add parens around this conditional term. http://codereview.chromium.org/10532171/diff/55001/ui/views/controls/menu/men... File ui/views/controls/menu/menu_item_view_win.cc (right): http://codereview.chromium.org/10532171/diff/55001/ui/views/controls/menu/men... ui/views/controls/menu/menu_item_view_win.cc:26: NonIconChildViewsCount() == 0); nit: add parens around this conditional term. http://codereview.chromium.org/10532171/diff/55001/ui/views/controls/menu/men... ui/views/controls/menu/menu_item_view_win.cc:114: config.arrow_width, this->height()); nit: remove unnecessary explicit "this->" from width() and height().
List of reviewers changed. msw@chromium.org did a drive-by without LGTM'ing!
http://codereview.chromium.org/10532171/diff/55001/ui/views/controls/menu/men... File ui/views/controls/menu/menu_delegate.cc (right): http://codereview.chromium.org/10532171/diff/55001/ui/views/controls/menu/men... ui/views/controls/menu/menu_delegate.cc:132: MenuConfig::instance().submenu_vertical_margin_size, On 2012/06/28 01:56:38, msw wrote: > nit: Indent two more spaces. Done. http://codereview.chromium.org/10532171/diff/55001/ui/views/controls/menu/men... File ui/views/controls/menu/menu_item_view.cc (right): http://codereview.chromium.org/10532171/diff/55001/ui/views/controls/menu/men... ui/views/controls/menu/menu_item_view.cc:515: // Position icon_view On 2012/06/28 01:56:38, msw wrote: > nit: vertical bars around "|icon_view|", add punctuation. Done. http://codereview.chromium.org/10532171/diff/55001/ui/views/controls/menu/men... ui/views/controls/menu/menu_item_view.cc:521: int y = (height() + GetTopMargin() - GetBottomMargin() - On 2012/06/28 01:56:38, msw wrote: > nit: I'd move the entire value calculation to the second line, indented by 4 > spaces. Done. http://codereview.chromium.org/10532171/diff/55001/ui/views/controls/menu/men... ui/views/controls/menu/menu_item_view.cc:830: return NonIconChildViewsCount() == 1 && title_.empty(); On 2012/06/28 01:56:38, msw wrote: > nit: add parens around the first conditional. Done. http://codereview.chromium.org/10532171/diff/55001/ui/views/controls/menu/men... File ui/views/controls/menu/menu_item_view.h (right): http://codereview.chromium.org/10532171/diff/55001/ui/views/controls/menu/men... ui/views/controls/menu/menu_item_view.h:410: // Returns number if child views excluding icon_view. On 2012/06/28 01:56:38, msw wrote: > nit: "if"->"of". Done. http://codereview.chromium.org/10532171/diff/55001/ui/views/controls/menu/men... ui/views/controls/menu/menu_item_view.h:413: // Get recursively max icon views width. On 2012/06/28 01:56:38, msw wrote: > nit: grammar/clarity. Perhaps: "Returns the max icon width; recurses over > submenus." Done. http://codereview.chromium.org/10532171/diff/55001/ui/views/controls/menu/men... ui/views/controls/menu/menu_item_view.h:453: // Set if menu has icons or icon_views On 2012/06/28 01:56:38, msw wrote: > nit: combine comments onto one line or add punctuation on first. Done. http://codereview.chromium.org/10532171/diff/55001/ui/views/controls/menu/men... File ui/views/controls/menu/menu_item_view_views.cc (right): http://codereview.chromium.org/10532171/diff/55001/ui/views/controls/menu/men... ui/views/controls/menu/menu_item_view_views.cc:26: NonIconChildViewsCount() == 0); Interesting, I had it before then removed to be consistent with mode == PB_NORMAL in the same statement. Anyway, done On 2012/06/28 01:56:38, msw wrote: > nit: add parens around this conditional term. http://codereview.chromium.org/10532171/diff/55001/ui/views/controls/menu/men... File ui/views/controls/menu/menu_item_view_win.cc (right): http://codereview.chromium.org/10532171/diff/55001/ui/views/controls/menu/men... ui/views/controls/menu/menu_item_view_win.cc:26: NonIconChildViewsCount() == 0); On 2012/06/28 01:56:38, msw wrote: > nit: add parens around this conditional term. Done. http://codereview.chromium.org/10532171/diff/55001/ui/views/controls/menu/men... ui/views/controls/menu/menu_item_view_win.cc:114: config.arrow_width, this->height()); There are width and height variables defined above, without this-> compiler fails to evaluate width() and height() as functions. On 2012/06/28 01:56:38, msw wrote: > nit: remove unnecessary explicit "this->" from width() and height().
Thanks for addressing nits! LGTM http://codereview.chromium.org/10532171/diff/55001/ui/views/controls/menu/men... File ui/views/controls/menu/menu_item_view_views.cc (right): http://codereview.chromium.org/10532171/diff/55001/ui/views/controls/menu/men... ui/views/controls/menu/menu_item_view_views.cc:26: NonIconChildViewsCount() == 0); On 2012/06/28 16:53:34, yefimt wrote: > Interesting, I had it before then removed to be consistent with mode == > PB_NORMAL in the same statement. > Anyway, done > > On 2012/06/28 01:56:38, msw wrote: > > nit: add parens around this conditional term. > Ah, right. Feel free to ignore style preference nits. http://codereview.chromium.org/10532171/diff/55001/ui/views/controls/menu/men... File ui/views/controls/menu/menu_item_view_win.cc (right): http://codereview.chromium.org/10532171/diff/55001/ui/views/controls/menu/men... ui/views/controls/menu/menu_item_view_win.cc:114: config.arrow_width, this->height()); On 2012/06/28 16:53:34, yefimt wrote: > There are width and height variables defined above, without this-> compiler > fails to evaluate width() and height() as functions. > > On 2012/06/28 01:56:38, msw wrote: > > nit: remove unnecessary explicit "this->" from width() and height(). > Gotcha, thanks.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yefim@chromium.org/10532171/66002
Change committed as 144787 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
