Update drop down menu look & feel

Update drop down menu look & feel


BUG=122908
TBR=oshima@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=151596

[Harry McCleave, 2012-08-06 22:24:46]

Could you take a look at this when you get a chance.

[sky, 2012-08-06 22:57:59]

https://chromiumcodereview.appspot.com/10829176/diff/14001/ui/views/controls/...
File ui/views/controls/combobox/native_combobox_views.cc (left):

https://chromiumcodereview.appspot.com/10829176/diff/14001/ui/views/controls/...
ui/views/controls/combobox/native_combobox_views.cc:60:
IDR_DISCLOSURE_ARROW).ToImageSkia()),
Update the grd file and remove the image too.

https://chromiumcodereview.appspot.com/10829176/diff/14001/ui/views/controls/...
ui/views/controls/combobox/native_combobox_views.cc:331:
menu->set_menu_position(views::MenuItemView::POSITION_OVER_BOUNDS);
This code only exists for aura. IF it's not needed, remove set_menu_position and
related code.

[Harry McCleave, 2012-08-06 23:41:27]

https://chromiumcodereview.appspot.com/10829176/diff/14001/ui/views/controls/...
File ui/views/controls/combobox/native_combobox_views.cc (left):

https://chromiumcodereview.appspot.com/10829176/diff/14001/ui/views/controls/...
ui/views/controls/combobox/native_combobox_views.cc:60:
IDR_DISCLOSURE_ARROW).ToImageSkia()),
On 2012/08/06 22:57:59, sky wrote:
> Update the grd file and remove the image too.

Done.

https://chromiumcodereview.appspot.com/10829176/diff/14001/ui/views/controls/...
ui/views/controls/combobox/native_combobox_views.cc:331:
menu->set_menu_position(views::MenuItemView::POSITION_OVER_BOUNDS);
On 2012/08/06 22:57:59, sky wrote:
> This code only exists for aura. IF it's not needed, remove set_menu_position
and
> related code.

Done? (is the mergeing of [requested|actual]_menu_position into menu_position
still relevant or should that go as well?

[sky, 2012-08-07 00:31:57]

https://chromiumcodereview.appspot.com/10829176/diff/1022/ui/views/controls/m...
File ui/views/controls/menu/menu_controller.cc (right):

https://chromiumcodereview.appspot.com/10829176/diff/1022/ui/views/controls/m...
ui/views/controls/menu/menu_controller.cc:54: static const int
kMenuBorderOffsetY = 2;
This should be moved into menu_config and you'll want to leave the default at 0.

https://chromiumcodereview.appspot.com/10829176/diff/1022/ui/views/controls/m...
ui/views/controls/menu/menu_controller.cc:1542: pref.set_width(pref.width() - (2
* kMenuBorderOffsetX));
Move this into 1545.

https://chromiumcodereview.appspot.com/10829176/diff/1022/ui/views/controls/m...
File ui/views/controls/menu/menu_item_view.h (right):

https://chromiumcodereview.appspot.com/10829176/diff/1022/ui/views/controls/m...
ui/views/controls/menu/menu_item_view.h:474: MenuPosition menu_position_;
I don't like that showing a menu many change the menu_position. That's why the
code has the two fields.

[Harry McCleave, 2012-08-07 01:26:36]

https://chromiumcodereview.appspot.com/10829176/diff/1022/ui/views/controls/m...
File ui/views/controls/menu/menu_controller.cc (right):

https://chromiumcodereview.appspot.com/10829176/diff/1022/ui/views/controls/m...
ui/views/controls/menu/menu_controller.cc:54: static const int
kMenuBorderOffsetY = 2;
On 2012/08/07 00:31:57, sky wrote:
> This should be moved into menu_config and you'll want to leave the default at
0.

Done.

https://chromiumcodereview.appspot.com/10829176/diff/1022/ui/views/controls/m...
ui/views/controls/menu/menu_controller.cc:1542: pref.set_width(pref.width() - (2
* kMenuBorderOffsetX));
On 2012/08/07 00:31:57, sky wrote:
> Move this into 1545.

Done.

https://chromiumcodereview.appspot.com/10829176/diff/1022/ui/views/controls/m...
File ui/views/controls/menu/menu_item_view.h (right):

https://chromiumcodereview.appspot.com/10829176/diff/1022/ui/views/controls/m...
ui/views/controls/menu/menu_item_view.h:474: MenuPosition menu_position_;
On 2012/08/07 00:31:57, sky wrote:
> I don't like that showing a menu many change the menu_position. That's why the
> code has the two fields.

Done.

[sky, 2012-08-07 03:41:53]

https://chromiumcodereview.appspot.com/10829176/diff/1026/ui/views/controls/m...
File ui/views/controls/menu/menu_controller.cc (right):

https://chromiumcodereview.appspot.com/10829176/diff/1026/ui/views/controls/m...
ui/views/controls/menu/menu_controller.cc:1540: (2 *
MenuConfig::instance().menu_border_offset_horizontal),
I'm confused by the usage of this. Here menu_border_offset_horizontal means
shrink the border by a certain amount, and below it means offset the initial
position by it. It should only mean one of these things. IF you need to adjust
the preferred size, it shouldn't be here but instead in where the preferred size
is calculated.

Also, this change is going to effect the placement of *all* menus, not just
those from the comobox. The bug seems to indicate this is only for the placement
of the menu from the combobox. If that's the case, the adjustments shouldn't be
here, but where the size and position are calculated in the combobox.

[Harry McCleave, 2012-08-07 20:16:34]

https://chromiumcodereview.appspot.com/10829176/diff/1026/ui/views/controls/m...
File ui/views/controls/menu/menu_controller.cc (right):

https://chromiumcodereview.appspot.com/10829176/diff/1026/ui/views/controls/m...
ui/views/controls/menu/menu_controller.cc:1540: (2 *
MenuConfig::instance().menu_border_offset_horizontal),
On 2012/08/07 03:41:53, sky wrote:
> I'm confused by the usage of this. Here menu_border_offset_horizontal means
> shrink the border by a certain amount, and below it means offset the initial
> position by it. It should only mean one of these things. IF you need to adjust
> the preferred size, it shouldn't be here but instead in where the preferred
size
> is calculated.
> 
> Also, this change is going to effect the placement of *all* menus, not just
> those from the comobox. The bug seems to indicate this is only for the
placement
> of the menu from the combobox. If that's the case, the adjustments shouldn't
be
> here, but where the size and position are calculated in the combobox.

Done.

[sky, 2012-08-07 20:50:01]

https://chromiumcodereview.appspot.com/10829176/diff/15005/ui/views/controls/...
File ui/views/controls/combobox/native_combobox_views.cc (right):

https://chromiumcodereview.appspot.com/10829176/diff/15005/ui/views/controls/...
ui/views/controls/combobox/native_combobox_views.cc:336:
MenuConfig::instance().menu_border_width_left);
Now that this code is entirely here it doesn't make sense for the constants to
be in MenuCOnfig. Move them back here.

[Harry McCleave, 2012-08-07 22:08:08]

https://chromiumcodereview.appspot.com/10829176/diff/15005/ui/views/controls/...
File ui/views/controls/combobox/native_combobox_views.cc (right):

https://chromiumcodereview.appspot.com/10829176/diff/15005/ui/views/controls/...
ui/views/controls/combobox/native_combobox_views.cc:336:
MenuConfig::instance().menu_border_width_left);
On 2012/08/07 20:50:01, sky wrote:
> Now that this code is entirely here it doesn't make sense for the constants to
> be in MenuCOnfig. Move them back here.

Done.

[sky, 2012-08-07 22:51:29]

https://chromiumcodereview.appspot.com/10829176/diff/5006/ui/views/controls/m...
File ui/views/controls/menu/menu_config_views.cc (right):

https://chromiumcodereview.appspot.com/10829176/diff/5006/ui/views/controls/m...
ui/views/controls/menu/menu_config_views.cc:22:
config->submenu_vertical_margin_size = 0;
I don't think you need this anymore. Is that right?

[Harry McCleave, 2012-08-07 22:55:54]

https://chromiumcodereview.appspot.com/10829176/diff/5006/ui/views/controls/m...
File ui/views/controls/menu/menu_config_views.cc (right):

https://chromiumcodereview.appspot.com/10829176/diff/5006/ui/views/controls/m...
ui/views/controls/menu/menu_config_views.cc:22:
config->submenu_vertical_margin_size = 0;
On 2012/08/07 22:51:29, sky wrote:
> I don't think you need this anymore. Is that right?

Still required, this is inside the menu margin (between the top/bottom menu
items and the edge/shadow of the menu).

[sky, 2012-08-07 23:19:51]

On Tue, Aug 7, 2012 at 3:55 PM,  <harrym@chromium.org> wrote:
>
>
https://chromiumcodereview.appspot.com/10829176/diff/5006/ui/views/controls/m...
> File ui/views/controls/menu/menu_config_views.cc (right):
>
>
https://chromiumcodereview.appspot.com/10829176/diff/5006/ui/views/controls/m...
> ui/views/controls/menu/menu_config_views.cc:22:
> config->submenu_vertical_margin_size = 0;
> On 2012/08/07 22:51:29, sky wrote:
>>
>> I don't think you need this anymore. Is that right?
>
>
> Still required, this is inside the menu margin (between the top/bottom
> menu items and the edge/shadow of the menu).
>
> https://chromiumcodereview.appspot.com/10829176/

Remember, this effects all menus. I thought you only cared about comboboxs?

  -Scott

[Harry McCleave, 2012-08-08 22:44:12]

On 2012/08/07 23:19:51, sky wrote:
> On Tue, Aug 7, 2012 at 3:55 PM,  <mailto:harrym@chromium.org> wrote:
> >
> >
>
https://chromiumcodereview.appspot.com/10829176/diff/5006/ui/views/controls/m...
> > File ui/views/controls/menu/menu_config_views.cc (right):
> >
> >
>
https://chromiumcodereview.appspot.com/10829176/diff/5006/ui/views/controls/m...
> > ui/views/controls/menu/menu_config_views.cc:22:
> > config->submenu_vertical_margin_size = 0;
> > On 2012/08/07 22:51:29, sky wrote:
> >>
> >> I don't think you need this anymore. Is that right?
> >
> >
> > Still required, this is inside the menu margin (between the top/bottom
> > menu items and the edge/shadow of the menu).
> >
> > https://chromiumcodereview.appspot.com/10829176/
> 
> Remember, this effects all menus. I thought you only cared about comboboxs?
> 
>   -Scott

Conveniently in this case the change is desirable to all menu's however I am now
curious how I would apply specifically to dropdown menu's as it seems awkward to
add any specifier there as none of the code flow determining the margin goes
through the combobox/owner of the menu.

[sky, 2012-08-09 00:20:48]

You would have to expose a setter on MenuItemView to manipulate the value per
menu. But, if you really want this for all (say things like menus on the
bookmark bar, or the wrench), then LGTM

  -Scott

[commit-bot: I haz the power, 2012-08-14 19:23:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/harrym@chromium.org/10829176/16006

[commit-bot: I haz the power, 2012-08-14 19:23:20]

Presubmit check for 10829176-16006 failed and returned exit status 1.


Running presubmit commit checks ...
Traceback (most recent call last):
  File "/b/commit-queue/verification/presubmit_shim.py", line 43, in <module>
    sys.exit(presubmit_support.Main(argv))
  File "/b/depot_tools/presubmit_support.py", line 1258, in Main
    rietveld_obj)
  File "/b/depot_tools/presubmit_support.py", line 1106, in DoPresubmitChecks
    results += executer.ExecPresubmitScript(presubmit_script, filename)
  File "/b/depot_tools/presubmit_support.py", line 1023, in ExecPresubmitScript
    result = eval(function_name + '(*__args)', context)
  File "<string>", line 1, in <module>
  File "<string>", line 19, in CheckChangeOnCommit
  File "<string>", line 43, in _CommonChecks
  File
"/b/commit-queue/workdir/chromium/ui/resources/resource_check/resource_scale_factors.py",
line 93, in RunChecks
    width, height = ImageSize(image_path)
  File
"/b/commit-queue/workdir/chromium/ui/resources/resource_check/resource_scale_factors.py",
line 47, in ImageSize
    assert data[:8] == '\x89PNG\r\n\x1A\n' and data[12:16] == 'IHDR'
AssertionError