Allow adjustment of MenuItemView top and bottom margins.

Allow adjustment of MenuItemView top and bottom margins.

BUG=none
TEST=none


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=86298

[rhashimoto, 2011-05-20 21:01:17]

Hi Scott -

Zel requested some tweaks to the ChromiumOS NetworkMenu vertical spacing.  It
didn't feel right to overwrite the global MenuConfig margin values so this CL
allows setting margins explicitly on MenuItemView.  Unset margin values are
"inherited" from the MenuItemView parent (or taken from MenuConfig as before if
none are set).

Roy

[sky, 2011-05-20 21:56:29]

http://codereview.chromium.org/6961032/diff/6/views/controls/menu/menu_item_v...
File views/controls/menu/menu_item_view.cc (right):

http://codereview.chromium.org/6961032/diff/6/views/controls/menu/menu_item_v...
views/controls/menu/menu_item_view.cc:720: while (item) {
I don't like having to have this logic. Can't we set it on each item to make it
nice and simple?

http://codereview.chromium.org/6961032/diff/6/views/controls/menu/menu_item_v...
File views/controls/menu/menu_item_view.h (right):

http://codereview.chromium.org/6961032/diff/6/views/controls/menu/menu_item_v...
views/controls/menu/menu_item_view.h:312: void set_top_margin(int top_margin);
How about set_margins(int top, bottom. And inline the implementation too.

[rhashimoto, 2011-05-21 00:41:17]

http://codereview.chromium.org/6961032/diff/6/views/controls/menu/menu_item_v...
File views/controls/menu/menu_item_view.cc (right):

http://codereview.chromium.org/6961032/diff/6/views/controls/menu/menu_item_v...
views/controls/menu/menu_item_view.cc:720: while (item) {
On 2011/05/20 21:56:29, sky wrote:
> I don't like having to have this logic. Can't we set it on each item to make
it
> nice and simple?

Done.  I added return values to all of the AppendMenu* convenience methods so
the item is easily accessed.

http://codereview.chromium.org/6961032/diff/6/views/controls/menu/menu_item_v...
File views/controls/menu/menu_item_view.h (right):

http://codereview.chromium.org/6961032/diff/6/views/controls/menu/menu_item_v...
views/controls/menu/menu_item_view.h:312: void set_top_margin(int top_margin);
On 2011/05/20 21:56:29, sky wrote:
> How about set_margins(int top, bottom. And inline the implementation too.

Done.

[sky, 2011-05-23 14:28:44]

http://codereview.chromium.org/6961032/diff/1002/views/controls/menu/menu_ite...
File views/controls/menu/menu_item_view.h (right):

http://codereview.chromium.org/6961032/diff/1002/views/controls/menu/menu_ite...
views/controls/menu/menu_item_view.h:312: DCHECK_LE(0, top_margin);
Why do we have this dcheck?

[rhashimoto, 2011-05-23 15:31:59]

http://codereview.chromium.org/6961032/diff/1002/views/controls/menu/menu_ite...
File views/controls/menu/menu_item_view.h (right):

http://codereview.chromium.org/6961032/diff/1002/views/controls/menu/menu_ite...
views/controls/menu/menu_item_view.h:312: DCHECK_LE(0, top_margin);
On 2011/05/23 14:28:45, sky wrote:
> Why do we have this dcheck?

It didn't seem to make sense to set a negative margin, but I guess that prevents
setting a margin and then reverting to the MenuConfig default.  I have removed
the DCHECKs.

[sky, 2011-05-23 16:04:06]

LGTM

[commit-bot: I haz the power, 2011-05-23 17:33:42]

Change committed as 86298