MenuButton: support Mac look & feel

MenuButton: support Mac look & feel

This change:
* Factors arrow width calculation out of Combobox
* Generalizes ComboboxBackgroundMac so it can be used on any control
* Refactor MenuButton to use PlatformStyle for background and arrow

The ability to set the arrow image on MenuButton is removed; MenuButton will
now always use the appropriate PlatformStyle image.

To test this change, build views_examples_with_content_exe and choose Menu
from the combobox.

BUG=605161

[Elly Fong-Jones, 2016-04-20 17:22:39]

Description was changed from

==========
MenuButton: support Mac look & feel

This change:
* Factors arrow width calculation out of Combobox
* Generalizes ComboboxBackgroundMac so it can be used on any control
* Refactor MenuButton to use PlatformStyle for background and arrow

The ability to set the arrow image on MenuButton is removed; MenuButton will
now always use the appropriate PlatformStyle image.

To test this change, build views_examples_with_content_exe and choose Menu
from the combobox.

BUG=605161
==========

to

==========
MenuButton: support Mac look & feel

This change:
* Factors arrow width calculation out of Combobox
* Generalizes ComboboxBackgroundMac so it can be used on any control
* Refactor MenuButton to use PlatformStyle for background and arrow

The ability to set the arrow image on MenuButton is removed; MenuButton will
now always use the appropriate PlatformStyle image.

To test this change, build views_examples_with_content_exe and choose Menu
from the combobox.

BUG=605161
==========

[Elly Fong-Jones, 2016-04-20 17:22:39]

ellyjones@chromium.org changed reviewers:
+ sky@chromium.org

[Elly Fong-Jones, 2016-04-20 17:22:48]

ellyjones@chromium.org changed reviewers:
+ tapted@chromium.org
- sky@chromium.org

[Elly Fong-Jones, 2016-04-20 17:23:02]

tapted: ptal? :)

[tapted, 2016-04-21 06:06:46]

There's some quirky stuff in mac_views_browser -- see http://crbug.com/605161#c3
- there might be an easy fix below.

https://codereview.chromium.org/1904753002/diff/20001/ui/gfx/BUILD.gn
File ui/gfx/BUILD.gn (right):

https://codereview.chromium.org/1904753002/diff/20001/ui/gfx/BUILD.gn#newcode585
ui/gfx/BUILD.gn:585: "vector_icons/menu_button_arrow_mac.icon",
`git add` this?

https://codereview.chromium.org/1904753002/diff/20001/ui/views/controls/butto...
File ui/views/controls/button/menu_button.cc (right):

https://codereview.chromium.org/1904753002/diff/20001/ui/views/controls/butto...
ui/views/controls/button/menu_button.cc:85:
set_background(PlatformStyle::CreateMenuButtonBackground().release());
e.g. maybe this just needs to be conditional on `show_menu_marker_`

https://codereview.chromium.org/1904753002/diff/20001/ui/views/controls/butto...
ui/views/controls/button/menu_button.cc:202: if (show_menu_marker_) {
nit: no curlies

https://codereview.chromium.org/1904753002/diff/20001/ui/views/controls/butto...
ui/views/controls/button/menu_button.cc:321: gfx::ImageSkia marker =
PlatformStyle::CreateMenuButtonArrow(enabled());
`Create` here makes it sound more heavyweight than it is.. maybe `Get`, but the
same applies for the combo arrow. Maybe this is fine.

https://codereview.chromium.org/1904753002/diff/20001/ui/views/controls/butto...
ui/views/controls/button/menu_button.cc:330: if (show_menu_marker_) {
nit: no curlies

https://codereview.chromium.org/1904753002/diff/20001/ui/views/controls/butto...
File ui/views/controls/button/menu_button.h (left):

https://codereview.chromium.org/1904753002/diff/20001/ui/views/controls/butto...
ui/views/controls/button/menu_button.h:131: bool show_menu_marker_;
can this be declared const? that might make it easier to reason about the rest
of the code

https://codereview.chromium.org/1904753002/diff/20001/ui/views/controls/combo...
File ui/views/controls/combobox/combobox.cc (right):

https://codereview.chromium.org/1904753002/diff/20001/ui/views/controls/combo...
ui/views/controls/combobox/combobox.cc:459: return
PlatformStyle::GetComboboxArrowWidth(style_);
Maybe "ShoulderWidth"

https://codereview.chromium.org/1904753002/diff/20001/ui/views/style/mac/comb...
File ui/views/style/mac/combobox_background_mac.cc (right):

https://codereview.chromium.org/1904753002/diff/20001/ui/views/style/mac/comb...
ui/views/style/mac/combobox_background_mac.cc:12: #include
"ui/views/controls/combobox/combobox.h"
can this be removed now? (or replaced with view.h?)

https://codereview.chromium.org/1904753002/diff/20001/ui/views/style/mac/comb...
ui/views/style/mac/combobox_background_mac.cc:27: if (base::i18n::IsRTL()) {
Should we plonk this on gfx::ScopedCanvas - Maybe inherit from ScopedCanvas with
a ScopedRTLFlippedCanvas? I feel like this pattern comes up a whole bunch, and
the code is a bit like voodoo. 

gfx::Canvas already has a base::i18n::IsRTL dependency, so I don't think there
will be any complaints putting it into scoped_canvas.h

https://codereview.chromium.org/1904753002/diff/20001/ui/views/style/mac/comb...
File ui/views/style/mac/combobox_background_mac.h (right):

https://codereview.chromium.org/1904753002/diff/20001/ui/views/style/mac/comb...
ui/views/style/mac/combobox_background_mac.h:28: ComboboxBackgroundMac(int
width);
explicit. Also perhaps |width| -> |shoulder_width|? Or something better than
"shoulder" - my brain is struggling with this one, but `width` immediately makes
me think of the entire background width, which of course needs to come from the
view. The data member should change too.

https://codereview.chromium.org/1904753002/diff/20001/ui/views/style/mac/comb...
ui/views/style/mac/combobox_background_mac.h:33: private:
nit: blank line before

https://codereview.chromium.org/1904753002/diff/20001/ui/views/style/platform...
File ui/views/style/platform_style.cc (right):

https://codereview.chromium.org/1904753002/diff/20001/ui/views/style/platform...
ui/views/style/platform_style.cc:107: const int kNormalRightPadding = 7;
I think we can collapse the left/right and just call it kNormalPadding. Same for
Action. Then multiply by 2. Same below.

https://codereview.chromium.org/1904753002/diff/20001/ui/views/style/platform...
File ui/views/style/platform_style.h (right):

https://codereview.chromium.org/1904753002/diff/20001/ui/views/style/platform...
ui/views/style/platform_style.h:72: static int
GetComboboxArrowWidth(Combobox::Style style);
At this point "shoulder" is really starting to grow on me :)

https://codereview.chromium.org/1904753002/diff/20001/ui/views/style/platform...
File ui/views/style/platform_style_mac.mm (right):

https://codereview.chromium.org/1904753002/diff/20001/ui/views/style/platform...
ui/views/style/platform_style_mac.mm:42: const int kComboboxArrowWidth = 13;
kMenuButtonArrowWidth

https://codereview.chromium.org/1904753002/diff/20001/ui/views/style/platform...
ui/views/style/platform_style_mac.mm:56: new
ComboboxBackgroundMac(GetComboboxArrowWidth(Combobox::STYLE_NORMAL)));
comment above about why STYLE_NORMAL is right here?

[Elly Fong-Jones, 2016-04-21 15:30:27]

tapted: ptal? :)

https://codereview.chromium.org/1904753002/diff/20001/ui/gfx/BUILD.gn
File ui/gfx/BUILD.gn (right):

https://codereview.chromium.org/1904753002/diff/20001/ui/gfx/BUILD.gn#newcode585
ui/gfx/BUILD.gn:585: "vector_icons/menu_button_arrow_mac.icon",
On 2016/04/21 06:06:45, tapted wrote:
> `git add` this?

Done.

https://codereview.chromium.org/1904753002/diff/20001/ui/views/controls/butto...
File ui/views/controls/button/menu_button.cc (right):

https://codereview.chromium.org/1904753002/diff/20001/ui/views/controls/butto...
ui/views/controls/button/menu_button.cc:85:
set_background(PlatformStyle::CreateMenuButtonBackground().release());
On 2016/04/21 06:06:46, tapted wrote:
> e.g. maybe this just needs to be conditional on `show_menu_marker_`

Done.

https://codereview.chromium.org/1904753002/diff/20001/ui/views/controls/butto...
ui/views/controls/button/menu_button.cc:202: if (show_menu_marker_) {
On 2016/04/21 06:06:46, tapted wrote:
> nit: no curlies

Done.

https://codereview.chromium.org/1904753002/diff/20001/ui/views/controls/butto...
ui/views/controls/button/menu_button.cc:321: gfx::ImageSkia marker =
PlatformStyle::CreateMenuButtonArrow(enabled());
On 2016/04/21 06:06:46, tapted wrote:
> `Create` here makes it sound more heavyweight than it is.. maybe `Get`, but
the
> same applies for the combo arrow. Maybe this is fine.

I named it with "Create" in case at some point it did become heavyweight. I
could change it but I don't really have a strong opinion either way.

https://codereview.chromium.org/1904753002/diff/20001/ui/views/controls/butto...
ui/views/controls/button/menu_button.cc:330: if (show_menu_marker_) {
On 2016/04/21 06:06:46, tapted wrote:
> nit: no curlies

Done.

https://codereview.chromium.org/1904753002/diff/20001/ui/views/controls/butto...
File ui/views/controls/button/menu_button.h (left):

https://codereview.chromium.org/1904753002/diff/20001/ui/views/controls/butto...
ui/views/controls/button/menu_button.h:131: bool show_menu_marker_;
On 2016/04/21 06:06:46, tapted wrote:
> can this be declared const? that might make it easier to reason about the rest
> of the code

Done.

https://codereview.chromium.org/1904753002/diff/20001/ui/views/controls/combo...
File ui/views/controls/combobox/combobox.cc (right):

https://codereview.chromium.org/1904753002/diff/20001/ui/views/controls/combo...
ui/views/controls/combobox/combobox.cc:459: return
PlatformStyle::GetComboboxArrowWidth(style_);
On 2016/04/21 06:06:46, tapted wrote:
> Maybe "ShoulderWidth"

Done.

https://codereview.chromium.org/1904753002/diff/20001/ui/views/style/mac/comb...
File ui/views/style/mac/combobox_background_mac.cc (right):

https://codereview.chromium.org/1904753002/diff/20001/ui/views/style/mac/comb...
ui/views/style/mac/combobox_background_mac.cc:12: #include
"ui/views/controls/combobox/combobox.h"
On 2016/04/21 06:06:46, tapted wrote:
> can this be removed now? (or replaced with view.h?)

Done.

https://codereview.chromium.org/1904753002/diff/20001/ui/views/style/mac/comb...
ui/views/style/mac/combobox_background_mac.cc:27: if (base::i18n::IsRTL()) {
On 2016/04/21 06:06:46, tapted wrote:
> Should we plonk this on gfx::ScopedCanvas - Maybe inherit from ScopedCanvas
with
> a ScopedRTLFlippedCanvas? I feel like this pattern comes up a whole bunch, and
> the code is a bit like voodoo. 
> 
> gfx::Canvas already has a base::i18n::IsRTL dependency, so I don't think there
> will be any complaints putting it into scoped_canvas.h

Done.

https://codereview.chromium.org/1904753002/diff/20001/ui/views/style/mac/comb...
File ui/views/style/mac/combobox_background_mac.h (right):

https://codereview.chromium.org/1904753002/diff/20001/ui/views/style/mac/comb...
ui/views/style/mac/combobox_background_mac.h:28: ComboboxBackgroundMac(int
width);
On 2016/04/21 06:06:46, tapted wrote:
> explicit. Also perhaps |width| -> |shoulder_width|? Or something better than
> "shoulder" - my brain is struggling with this one, but `width` immediately
makes
> me think of the entire background width, which of course needs to come from
the
> view. The data member should change too.

Done.

https://codereview.chromium.org/1904753002/diff/20001/ui/views/style/mac/comb...
ui/views/style/mac/combobox_background_mac.h:33: private:
On 2016/04/21 06:06:46, tapted wrote:
> nit: blank line before

Done.

https://codereview.chromium.org/1904753002/diff/20001/ui/views/style/platform...
File ui/views/style/platform_style.cc (right):

https://codereview.chromium.org/1904753002/diff/20001/ui/views/style/platform...
ui/views/style/platform_style.cc:107: const int kNormalRightPadding = 7;
On 2016/04/21 06:06:46, tapted wrote:
> I think we can collapse the left/right and just call it kNormalPadding. Same
for
> Action. Then multiply by 2. Same below.

Done.

https://codereview.chromium.org/1904753002/diff/20001/ui/views/style/platform...
File ui/views/style/platform_style.h (right):

https://codereview.chromium.org/1904753002/diff/20001/ui/views/style/platform...
ui/views/style/platform_style.h:72: static int
GetComboboxArrowWidth(Combobox::Style style);
On 2016/04/21 06:06:46, tapted wrote:
> At this point "shoulder" is really starting to grow on me :)

Done.

https://codereview.chromium.org/1904753002/diff/20001/ui/views/style/platform...
File ui/views/style/platform_style_mac.mm (right):

https://codereview.chromium.org/1904753002/diff/20001/ui/views/style/platform...
ui/views/style/platform_style_mac.mm:42: const int kComboboxArrowWidth = 13;
On 2016/04/21 06:06:46, tapted wrote:
> kMenuButtonArrowWidth

Done.

https://codereview.chromium.org/1904753002/diff/20001/ui/views/style/platform...
ui/views/style/platform_style_mac.mm:56: new
ComboboxBackgroundMac(GetComboboxArrowWidth(Combobox::STYLE_NORMAL)));
On 2016/04/21 06:06:46, tapted wrote:
> comment above about why STYLE_NORMAL is right here?

It's only okay because of an implementation detail of Combobox. I put a comment
to that effect.

[tapted, 2016-04-22 13:46:52]

ran out of time to try this out today. Main question is about how the origin
info bubble might be affected -- permission_view and all that - I don't think
they should be getting borders.

otherwise i think it looks good - fine to send to OWNERS after addressing
comments. And perhaps see whether sky has an opinion on keeping a copy of the
ImageSkia around to avoid lots of calls to CreateFoo (and move
GetFooShoulderWidth onto the Foo classes, since they can get the arrow icon
width from the cached image)

https://codereview.chromium.org/1904753002/diff/40001/ui/gfx/BUILD.gn
File ui/gfx/BUILD.gn (right):

https://codereview.chromium.org/1904753002/diff/40001/ui/gfx/BUILD.gn#newcode165
ui/gfx/BUILD.gn:165: "scoped_canvas.h",
scoped_canvas.cc probably needs to be here too

https://codereview.chromium.org/1904753002/diff/40001/ui/gfx/scoped_canvas.h
File ui/gfx/scoped_canvas.h (right):

https://codereview.chromium.org/1904753002/diff/40001/ui/gfx/scoped_canvas.h#...
ui/gfx/scoped_canvas.h:10: #include "ui/gfx/geometry/rect.h"
forward-declare?

https://codereview.chromium.org/1904753002/diff/40001/ui/gfx/scoped_canvas.h#...
ui/gfx/scoped_canvas.h:34: // drawn inside the supplied rect will be flipped
RTL.
nit: replace `flipped RTL` with `horizontally flipped`.

(to me at least, "RTL" seems like a concept of it's own, rather than something
that should be spoken "right to left")

https://codereview.chromium.org/1904753002/diff/40001/ui/gfx/scoped_canvas.h#...
ui/gfx/scoped_canvas.h:35: 
nit: remove blank line

https://codereview.chromium.org/1904753002/diff/40001/ui/gfx/scoped_canvas.h#...
ui/gfx/scoped_canvas.h:38: ScopedRTLFlipCanvas(gfx::Canvas* canvas, gfx::Rect
rect);
nit: const-ref on `rect`

https://codereview.chromium.org/1904753002/diff/40001/ui/views/controls/butto...
File ui/views/controls/button/menu_button.cc (right):

https://codereview.chromium.org/1904753002/diff/40001/ui/views/controls/butto...
ui/views/controls/button/menu_button.cc:84:
SetBorder(PlatformStyle::CreateMenuButtonBorder());
won't this still affect the OIB in a way that we don't want it to? I think we
want those to be borderless regardless of platform

https://codereview.chromium.org/1904753002/diff/40001/ui/views/style/mac/comb...
File ui/views/style/mac/combobox_background_mac.h (right):

https://codereview.chromium.org/1904753002/diff/40001/ui/views/style/mac/comb...
ui/views/style/mac/combobox_background_mac.h:36: int width_;
shoulder_width_

https://codereview.chromium.org/1904753002/diff/40001/ui/views/style/platform...
File ui/views/style/platform_style.cc (right):

https://codereview.chromium.org/1904753002/diff/40001/ui/views/style/platform...
ui/views/style/platform_style.cc:110: int image_width =
CreateComboboxArrow(true, style).size().width();
I don't think the .size() is needed, same below. But, I also think we should
hard-code this, and perhaps DCHECK against the width(), since this gets called
quite a bit, and a resource bundle lookup has some cost (as does copying the
ImageSkia - it's ref-counted, but thread safe so there's special instructions
and impact to the cpu cache and stuff).

but of course that would need different widths based on the platform. (maybe
it's better to keep the ImageSkia on the Combobox instance and initialize it in
the constructor)

[Elly Fong-Jones, 2016-04-25 14:30:51]

tapted: ptal? should be last round I think

https://codereview.chromium.org/1904753002/diff/40001/ui/gfx/BUILD.gn
File ui/gfx/BUILD.gn (right):

https://codereview.chromium.org/1904753002/diff/40001/ui/gfx/BUILD.gn#newcode165
ui/gfx/BUILD.gn:165: "scoped_canvas.h",
On 2016/04/22 13:46:52, tapted wrote:
> scoped_canvas.cc probably needs to be here too

Done.

https://codereview.chromium.org/1904753002/diff/40001/ui/gfx/scoped_canvas.h
File ui/gfx/scoped_canvas.h (right):

https://codereview.chromium.org/1904753002/diff/40001/ui/gfx/scoped_canvas.h#...
ui/gfx/scoped_canvas.h:10: #include "ui/gfx/geometry/rect.h"
On 2016/04/22 13:46:52, tapted wrote:
> forward-declare?

Done.

https://codereview.chromium.org/1904753002/diff/40001/ui/gfx/scoped_canvas.h#...
ui/gfx/scoped_canvas.h:34: // drawn inside the supplied rect will be flipped
RTL.
On 2016/04/22 13:46:52, tapted wrote:
> nit: replace `flipped RTL` with `horizontally flipped`.
> 
> (to me at least, "RTL" seems like a concept of it's own, rather than something
> that should be spoken "right to left")

Done.

https://codereview.chromium.org/1904753002/diff/40001/ui/gfx/scoped_canvas.h#...
ui/gfx/scoped_canvas.h:35: 
On 2016/04/22 13:46:52, tapted wrote:
> nit: remove blank line

Done.

https://codereview.chromium.org/1904753002/diff/40001/ui/gfx/scoped_canvas.h#...
ui/gfx/scoped_canvas.h:38: ScopedRTLFlipCanvas(gfx::Canvas* canvas, gfx::Rect
rect);
On 2016/04/22 13:46:52, tapted wrote:
> nit: const-ref on `rect`

Done.

https://codereview.chromium.org/1904753002/diff/40001/ui/views/controls/butto...
File ui/views/controls/button/menu_button.cc (right):

https://codereview.chromium.org/1904753002/diff/40001/ui/views/controls/butto...
ui/views/controls/button/menu_button.cc:84:
SetBorder(PlatformStyle::CreateMenuButtonBorder());
On 2016/04/22 13:46:52, tapted wrote:
> won't this still affect the OIB in a way that we don't want it to? I think we
> want those to be borderless regardless of platform

What is an OIB?

https://codereview.chromium.org/1904753002/diff/40001/ui/views/style/mac/comb...
File ui/views/style/mac/combobox_background_mac.h (right):

https://codereview.chromium.org/1904753002/diff/40001/ui/views/style/mac/comb...
ui/views/style/mac/combobox_background_mac.h:36: int width_;
On 2016/04/22 13:46:52, tapted wrote:
> shoulder_width_

Done.

https://codereview.chromium.org/1904753002/diff/40001/ui/views/style/platform...
File ui/views/style/platform_style.cc (right):

https://codereview.chromium.org/1904753002/diff/40001/ui/views/style/platform...
ui/views/style/platform_style.cc:110: int image_width =
CreateComboboxArrow(true, style).size().width();
On 2016/04/22 13:46:52, tapted wrote:
> I don't think the .size() is needed, same below. But, I also think we should
> hard-code this, and perhaps DCHECK against the width(), since this gets called
> quite a bit, and a resource bundle lookup has some cost (as does copying the
> ImageSkia - it's ref-counted, but thread safe so there's special instructions
> and impact to the cpu cache and stuff).
> 
> but of course that would need different widths based on the platform. (maybe
> it's better to keep the ImageSkia on the Combobox instance and initialize it
in
> the constructor)

Okay.

The reason why it wasn't statically computed right now is because SetStyle() can
get called on the combobox later on. I'm refactoring Combobox so that the style
is passed into the constructor and is const, and then I can cache the arrow
image and width inside the combobox. I made the style const and cached the image
inside Combobox/MenuButton, then kicked the Get*ShoulderWidth() methods back
into the classes themselves, so the only thing PlatformStyle exports is the
Get*ArrowImage() methods, which are called only once in the constructors. This
also makes the code easier to follow, so win/win.

[tapted, 2016-04-26 05:05:58]

https://codereview.chromium.org/1904753002/diff/40001/ui/views/controls/butto...
File ui/views/controls/button/menu_button.cc (right):

https://codereview.chromium.org/1904753002/diff/40001/ui/views/controls/butto...
ui/views/controls/button/menu_button.cc:84:
SetBorder(PlatformStyle::CreateMenuButtonBorder());
On 2016/04/25 14:30:50, Elly Jones wrote:
> On 2016/04/22 13:46:52, tapted wrote:
> > won't this still affect the OIB in a way that we don't want it to? I think
we
> > want those to be borderless regardless of platform
> 
> What is an OIB?

OIB is origin info bubble :). Which is the "official" name for the site settings
dialog AKA "that dialog you get when you click the padlock icon in the location
bar".

I put a screenshot at http://crbug.com/605161#c4 from an earlier patchset. And
it looks that way still in patchset 4 with mac_views_browser=1 or mvb=0. The
problem is that PermissionMenuButton (in permission_selector_view.cc) has a
weird initialization path. I think to fix this we need views::MenuButton to
override LabelButton::UpdateThemedBorder() [and/or OnNativeThemeChanged] and
these method calls need to move into that method.  [OnNativeThemeChanged gets
called when the button is added to a Widget, so we shouldn't have to call
UpdateThemedBorder explicitly in MenuButton's initialization paths]. Then, so
long as PermissionMenuButton calls `SetBorder(<something> -- probably nullptr)`
in its constructor [probably after its call to UpdateThemedBorder()] things
might be "ok". But see http://crbug.com/510747 where I caused a regression here
on GTK - things are a bit fragile, so we need to check each platform :/

Note there's also a faint white border/outline around the bookmarks bar's "Other
Bookmarks" menu button (mac_views_browser=1 only).

https://codereview.chromium.org/1904753002/diff/60001/ui/gfx/scoped_canvas.h
File ui/gfx/scoped_canvas.h (right):

https://codereview.chromium.org/1904753002/diff/60001/ui/gfx/scoped_canvas.h#...
ui/gfx/scoped_canvas.h:36: class ScopedRTLFlipCanvas {
ooh this needs to be GFX_EXPORT (+include gfx_export.h)

https://codereview.chromium.org/1904753002/diff/60001/ui/views/controls/butto...
File ui/views/controls/button/menu_button.cc (right):

https://codereview.chromium.org/1904753002/diff/60001/ui/views/controls/butto...
ui/views/controls/button/menu_button.cc:24: #include
"ui/views/controls/focusable_border.h"
is this still needed?

https://codereview.chromium.org/1904753002/diff/60001/ui/views/controls/butto...
ui/views/controls/button/menu_button.cc:207: int marker_width = enabled() ?
enabled_marker_image_.size().width()
nit: remove ".size()"

I think we can also do


if (!show_menu_marker_)
  return 0;

DCHECK_EQ(enabled_marker_image_.width(), disabled_marker_image_.width());
return enabled_marker_image_.width() + 2 * kPadding;



The early return means we can simplify GetPreferredSize() and
GetChildAreaBounds(), and the DCHECK simplifies things -- if it wasn't true,
we'd need to trigger a re-layout when the enable state changes, which I don't
think we do


[edit: that DCHECK will be unnecessary when we switch to just having one
ImageSkia]

https://codereview.chromium.org/1904753002/diff/60001/ui/views/controls/butto...
ui/views/controls/button/menu_button.cc:341: if (show_menu_marker_)
not needed with the GetShoulderWidth() tweak. In fact the whole method can just
be

  return gfx::Rect(0, 0, width() - GetShoulderWidth(), height());

https://codereview.chromium.org/1904753002/diff/60001/ui/views/controls/butto...
File ui/views/controls/button/menu_button.h (right):

https://codereview.chromium.org/1904753002/diff/60001/ui/views/controls/butto...
ui/views/controls/button/menu_button.h:101: int GetShoulderWidth() const;
can this be made private:?

https://codereview.chromium.org/1904753002/diff/60001/ui/views/controls/butto...
ui/views/controls/button/menu_button.h:148: gfx::ImageSkia
disabled_marker_image_;
Since the default PlatformStyle ignores the `enabled` argument, and it's rare
for a particular combobox to become disabled/enabled after being drawn once, I
think we should just have one marker_image_ - both here and in combobox.
Instead, just update the image it in an override of View::OnEnabledChanged [that
also calls the superclass impl].

also: (nit) move up to the previous location, below |show_menu_marker_|

https://codereview.chromium.org/1904753002/diff/60001/ui/views/controls/combo...
File ui/views/controls/combobox/combobox.cc (right):

https://codereview.chromium.org/1904753002/diff/60001/ui/views/controls/combo...
ui/views/controls/combobox/combobox.cc:388: if (style_ == STYLE_ACTION)
Move this to the top of the constructor.

Then the content_size_ = and PreferredSizeChanged() calls are unnecessary, since
they will be called by ModelChanged()

https://codereview.chromium.org/1904753002/diff/60001/ui/views/controls/combo...
ui/views/controls/combobox/combobox.cc:859: return enabled() ?
enabled_arrow_image_.size() : disabled_arrow_image_.size();
same here - this should DCHECK that the sizes are equal

(edit: except that won't be necessary)

https://codereview.chromium.org/1904753002/diff/60001/ui/views/controls/combo...
File ui/views/controls/combobox/combobox.h (right):

https://codereview.chromium.org/1904753002/diff/60001/ui/views/controls/combo...
ui/views/controls/combobox/combobox.h:57: Combobox(ui::ComboboxModel* model,
Style style);
nit: move before the `explicit` constructor? Or reorder methods in .cc. Or (my
favourite): give `style` a default argument in the definition here (and add
`explicit` to this method). Default arguments are allowed now and this is a
pretty clear-cut case. It's good intrinsic documentation about exactly what
style you will get if you don't specify one.

https://codereview.chromium.org/1904753002/diff/60001/ui/views/controls/combo...
ui/views/controls/combobox/combobox.h:88: // containing the arrow, including any
padding space around it.
nit: remove "space"

https://codereview.chromium.org/1904753002/diff/60001/ui/views/controls/combo...
ui/views/controls/combobox/combobox.h:89: int GetShoulderWidth() const;
Can these be in the private: section? Also make sure the method order in the .cc
matches

https://codereview.chromium.org/1904753002/diff/60001/ui/views/controls/combo...
ui/views/controls/combobox/combobox.h:154: const Style style_;
nice - I like this :)

https://codereview.chromium.org/1904753002/diff/60001/ui/views/controls/combo...
ui/views/controls/combobox/combobox.h:207: gfx::ImageSkia disabled_arrow_image_;
(per prior comment, just one image)

https://codereview.chromium.org/1904753002/diff/60001/ui/views/controls/combo...
File ui/views/controls/combobox/combobox_unittest.cc (right):

https://codereview.chromium.org/1904753002/diff/60001/ui/views/controls/combo...
ui/views/controls/combobox/combobox_unittest.cc:39: : TestCombobox(model,
Combobox::STYLE_NORMAL) {}
In this case I think things are better without the one-arg (or defaulted-arg)
constructor. We can keep the InitCombobox overload if there's a lot of churn

https://codereview.chromium.org/1904753002/diff/60001/ui/views/controls/combo...
ui/views/controls/combobox/combobox_unittest.cc:458: InitCombobox(NULL,
Combobox::STYLE_ACTION);
nit: since it's changing, any added code should be using `nullptr`. More below

https://codereview.chromium.org/1904753002/diff/60001/ui/views/controls/combo...
ui/views/controls/combobox/combobox_unittest.cc:780: // Check that with
STYLE_ACTION, the first item (only) is not shown.
this comment should move be above the next test case. It should also state that
items should not have checkmarks regardless of platform (since that seems to be
the case!)

[Elly Fong-Jones, 2016-04-26 17:41:24]

ellyjones@chromium.org changed reviewers:
+ sky@chromium.org

[Elly Fong-Jones, 2016-04-26 17:41:26]

tapted, sky: ptal? :)

https://codereview.chromium.org/1904753002/diff/60001/ui/gfx/scoped_canvas.h
File ui/gfx/scoped_canvas.h (right):

https://codereview.chromium.org/1904753002/diff/60001/ui/gfx/scoped_canvas.h#...
ui/gfx/scoped_canvas.h:36: class ScopedRTLFlipCanvas {
On 2016/04/26 05:05:57, tapted wrote:
> ooh this needs to be GFX_EXPORT (+include gfx_export.h)

I made this and ScopedCanvas both GFX_EXPORT. I'm surprised that this worked
before...

https://codereview.chromium.org/1904753002/diff/60001/ui/views/controls/butto...
File ui/views/controls/button/menu_button.cc (right):

https://codereview.chromium.org/1904753002/diff/60001/ui/views/controls/butto...
ui/views/controls/button/menu_button.cc:24: #include
"ui/views/controls/focusable_border.h"
On 2016/04/26 05:05:57, tapted wrote:
> is this still needed?

It seems to be needed so that the destructor can delete a
unique_ptr<FocusableBorder>.

https://codereview.chromium.org/1904753002/diff/60001/ui/views/controls/butto...
ui/views/controls/button/menu_button.cc:207: int marker_width = enabled() ?
enabled_marker_image_.size().width()
On 2016/04/26 05:05:57, tapted wrote:
> nit: remove ".size()"
> 
> I think we can also do
> 
> 
> if (!show_menu_marker_)
>   return 0;
> 
> DCHECK_EQ(enabled_marker_image_.width(), disabled_marker_image_.width());
> return enabled_marker_image_.width() + 2 * kPadding;
> 
> 
> 
> The early return means we can simplify GetPreferredSize() and
> GetChildAreaBounds(), and the DCHECK simplifies things -- if it wasn't true,
> we'd need to trigger a re-layout when the enable state changes, which I don't
> think we do
> 
> 
> [edit: that DCHECK will be unnecessary when we switch to just having one
> ImageSkia]

Done.

https://codereview.chromium.org/1904753002/diff/60001/ui/views/controls/butto...
ui/views/controls/button/menu_button.cc:341: if (show_menu_marker_)
On 2016/04/26 05:05:57, tapted wrote:
> not needed with the GetShoulderWidth() tweak. In fact the whole method can
just
> be
> 
>   return gfx::Rect(0, 0, width() - GetShoulderWidth(), height());

Done.

https://codereview.chromium.org/1904753002/diff/60001/ui/views/controls/butto...
File ui/views/controls/button/menu_button.h (right):

https://codereview.chromium.org/1904753002/diff/60001/ui/views/controls/butto...
ui/views/controls/button/menu_button.h:101: int GetShoulderWidth() const;
On 2016/04/26 05:05:57, tapted wrote:
> can this be made private:?

Done.

https://codereview.chromium.org/1904753002/diff/60001/ui/views/controls/butto...
ui/views/controls/button/menu_button.h:148: gfx::ImageSkia
disabled_marker_image_;
On 2016/04/26 05:05:57, tapted wrote:
> Since the default PlatformStyle ignores the `enabled` argument, and it's rare
> for a particular combobox to become disabled/enabled after being drawn once, I
> think we should just have one marker_image_ - both here and in combobox.
> Instead, just update the image it in an override of View::OnEnabledChanged
[that
> also calls the superclass impl].
> 
> also: (nit) move up to the previous location, below |show_menu_marker_|

Done.

https://codereview.chromium.org/1904753002/diff/60001/ui/views/controls/combo...
File ui/views/controls/combobox/combobox.cc (right):

https://codereview.chromium.org/1904753002/diff/60001/ui/views/controls/combo...
ui/views/controls/combobox/combobox.cc:388: if (style_ == STYLE_ACTION)
On 2016/04/26 05:05:57, tapted wrote:
> Move this to the top of the constructor.
> 
> Then the content_size_ = and PreferredSizeChanged() calls are unnecessary,
since
> they will be called by ModelChanged()

Done.

https://codereview.chromium.org/1904753002/diff/60001/ui/views/controls/combo...
ui/views/controls/combobox/combobox.cc:859: return enabled() ?
enabled_arrow_image_.size() : disabled_arrow_image_.size();
On 2016/04/26 05:05:57, tapted wrote:
> same here - this should DCHECK that the sizes are equal
> 
> (edit: except that won't be necessary)

Done.

https://codereview.chromium.org/1904753002/diff/60001/ui/views/controls/combo...
File ui/views/controls/combobox/combobox.h (right):

https://codereview.chromium.org/1904753002/diff/60001/ui/views/controls/combo...
ui/views/controls/combobox/combobox.h:57: Combobox(ui::ComboboxModel* model,
Style style);
On 2016/04/26 05:05:58, tapted wrote:
> nit: move before the `explicit` constructor? Or reorder methods in .cc. Or (my
> favourite): give `style` a default argument in the definition here (and add
> `explicit` to this method). Default arguments are allowed now and this is a
> pretty clear-cut case. It's good intrinsic documentation about exactly what
> style you will get if you don't specify one.

Done.

https://codereview.chromium.org/1904753002/diff/60001/ui/views/controls/combo...
ui/views/controls/combobox/combobox.h:88: // containing the arrow, including any
padding space around it.
On 2016/04/26 05:05:57, tapted wrote:
> nit: remove "space"

Done.

https://codereview.chromium.org/1904753002/diff/60001/ui/views/controls/combo...
ui/views/controls/combobox/combobox.h:89: int GetShoulderWidth() const;
On 2016/04/26 05:05:57, tapted wrote:
> Can these be in the private: section? Also make sure the method order in the
.cc
> matches

Done.

https://codereview.chromium.org/1904753002/diff/60001/ui/views/controls/combo...
ui/views/controls/combobox/combobox.h:154: const Style style_;
On 2016/04/26 05:05:57, tapted wrote:
> nice - I like this :)

Done.

https://codereview.chromium.org/1904753002/diff/60001/ui/views/controls/combo...
ui/views/controls/combobox/combobox.h:207: gfx::ImageSkia disabled_arrow_image_;
On 2016/04/26 05:05:57, tapted wrote:
> (per prior comment, just one image)

Done.

https://codereview.chromium.org/1904753002/diff/60001/ui/views/controls/combo...
File ui/views/controls/combobox/combobox_unittest.cc (right):

https://codereview.chromium.org/1904753002/diff/60001/ui/views/controls/combo...
ui/views/controls/combobox/combobox_unittest.cc:39: : TestCombobox(model,
Combobox::STYLE_NORMAL) {}
On 2016/04/26 05:05:58, tapted wrote:
> In this case I think things are better without the one-arg (or defaulted-arg)
> constructor. We can keep the InitCombobox overload if there's a lot of churn

Done.

https://codereview.chromium.org/1904753002/diff/60001/ui/views/controls/combo...
ui/views/controls/combobox/combobox_unittest.cc:458: InitCombobox(NULL,
Combobox::STYLE_ACTION);
On 2016/04/26 05:05:58, tapted wrote:
> nit: since it's changing, any added code should be using `nullptr`. More below

Done.

https://codereview.chromium.org/1904753002/diff/60001/ui/views/controls/combo...
ui/views/controls/combobox/combobox_unittest.cc:780: // Check that with
STYLE_ACTION, the first item (only) is not shown.
On 2016/04/26 05:05:58, tapted wrote:
> this comment should move be above the next test case. It should also state
that
> items should not have checkmarks regardless of platform (since that seems to
be
> the case!)

Done.

[tapted, 2016-04-27 03:25:11]

https://codereview.chromium.org/1904753002/diff/60001/ui/gfx/scoped_canvas.h
File ui/gfx/scoped_canvas.h (right):

https://codereview.chromium.org/1904753002/diff/60001/ui/gfx/scoped_canvas.h#...
ui/gfx/scoped_canvas.h:36: class ScopedRTLFlipCanvas {
On 2016/04/26 17:41:25, Elly Jones wrote:
> On 2016/04/26 05:05:57, tapted wrote:
> > ooh this needs to be GFX_EXPORT (+include gfx_export.h)
> 
> I made this and ScopedCanvas both GFX_EXPORT. I'm surprised that this worked
> before...

The inlined methods wouldn't have required it, but now there's a .cc component
builds will only get the symbol if its exported

https://codereview.chromium.org/1904753002/diff/60001/ui/views/controls/butto...
File ui/views/controls/button/menu_button.cc (right):

https://codereview.chromium.org/1904753002/diff/60001/ui/views/controls/butto...
ui/views/controls/button/menu_button.cc:24: #include
"ui/views/controls/focusable_border.h"
On 2016/04/26 17:41:25, Elly Jones wrote:
> On 2016/04/26 05:05:57, tapted wrote:
> > is this still needed?
> 
> It seems to be needed so that the destructor can delete a
> unique_ptr<FocusableBorder>.

Ah, that's subtle. I think we could have PlatformStyle::CreateMenuButtonBorder()
return a vanilla views::Border, but that might just require a border.h include
instead. So I think what you have is fine, but could go either way.

https://codereview.chromium.org/1904753002/diff/80001/ui/gfx/scoped_canvas.h
File ui/gfx/scoped_canvas.h (right):

https://codereview.chromium.org/1904753002/diff/80001/ui/gfx/scoped_canvas.h#...
ui/gfx/scoped_canvas.h:9: 
nit: no blank line here

https://codereview.chromium.org/1904753002/diff/80001/ui/views/controls/butto...
File ui/views/controls/button/menu_button.cc (right):

https://codereview.chromium.org/1904753002/diff/80001/ui/views/controls/butto...
ui/views/controls/button/menu_button.cc:84:
SetBorder(PlatformStyle::CreateMenuButtonBorder());
The issue with the MenuButton subclass used for the origin info bubble still
isn't resolved -- patching this in, it still looks like
http://crbug.com/605161#c4, which is a regression we don't want. See my earlier
comment at
https://codereview.chromium.org/1904753002/diff/40001/ui/views/controls/butto...
- it might have gone missing since it was a thread on an older patchset.

https://codereview.chromium.org/1904753002/diff/80001/ui/views/controls/butto...
ui/views/controls/button/menu_button.cc:204: int MenuButton::GetShoulderWidth()
const {
move definition to end of file

https://codereview.chromium.org/1904753002/diff/80001/ui/views/controls/butto...
File ui/views/controls/button/menu_button.h (right):

https://codereview.chromium.org/1904753002/diff/80001/ui/views/controls/butto...
ui/views/controls/button/menu_button.h:47: static const int
kMenuMarkerPaddingLeft;
huh - looks like these are unused now and can be removed. But that might be bad.
They are not even, so I think there there will be a regression in the layout
using ClampToCenteredSize instead of these for non-Mac platforms, so that needs
to be fixed. If you still need values for left/right, they should move into an
anonymous namespace in the .cc.

https://codereview.chromium.org/1904753002/diff/80001/ui/views/controls/butto...
ui/views/controls/button/menu_button.h:114: // Returns the width of the
"shoulder" of the menubutton, which is the area
nit: menubutton -> MenuButton

https://codereview.chromium.org/1904753002/diff/80001/ui/views/controls/butto...
ui/views/controls/button/menu_button.h:129: // The images to draw for the menu
marker, if |show_menu_marker_| is true.
I think we can keep the old comment, and variable name (menu_marker_) - they
don't really need to change. Also nudge down to the prior location (below the
bool). Changing it from a raw pointer to a (wrapper around) a ref-counted
pointer I'd keep

https://codereview.chromium.org/1904753002/diff/80001/ui/views/controls/combo...
File ui/views/controls/combobox/combobox.cc (right):

https://codereview.chromium.org/1904753002/diff/80001/ui/views/controls/combo...
ui/views/controls/combobox/combobox.cc:450: int Combobox::GetShoulderWidth()
const {
nit: move to the end of the file to match declaration order

https://codereview.chromium.org/1904753002/diff/80001/ui/views/controls/combo...
File ui/views/controls/combobox/combobox.h (right):

https://codereview.chromium.org/1904753002/diff/80001/ui/views/controls/combo...
ui/views/controls/combobox/combobox.h:56: Combobox(ui::ComboboxModel* model,
Style style = STYLE_NORMAL);
nit: add explicit - C++ will still allow implicit conversion if it can find a
way to construct with a single argument

[Elly Fong-Jones, 2016-04-28 17:03:12]

tapted: ptal?

https://codereview.chromium.org/1904753002/diff/80001/ui/gfx/scoped_canvas.h
File ui/gfx/scoped_canvas.h (right):

https://codereview.chromium.org/1904753002/diff/80001/ui/gfx/scoped_canvas.h#...
ui/gfx/scoped_canvas.h:9: 
On 2016/04/27 03:25:11, tapted wrote:
> nit: no blank line here

Done.

https://codereview.chromium.org/1904753002/diff/80001/ui/views/controls/butto...
File ui/views/controls/button/menu_button.cc (right):

https://codereview.chromium.org/1904753002/diff/80001/ui/views/controls/butto...
ui/views/controls/button/menu_button.cc:84:
SetBorder(PlatformStyle::CreateMenuButtonBorder());
On 2016/04/27 03:25:11, tapted wrote:
> The issue with the MenuButton subclass used for the origin info bubble still
> isn't resolved -- patching this in, it still looks like
> http://crbug.com/605161#c4, which is a regression we don't want. See my
earlier
> comment at
>
https://codereview.chromium.org/1904753002/diff/40001/ui/views/controls/butto...
> - it might have gone missing since it was a thread on an older patchset.

Ick. Whether the subclasses need borders or not seems to vary individually. I
added a protected virtual method 'ShouldUseBorder' to decide whether to
construct a border, and made it default to false, so we can make individual
subclasses use the border if needed. I also fixed up the layout code in
PaintMenuMarker() to work properly when there's no border.

https://codereview.chromium.org/1904753002/diff/80001/ui/views/controls/butto...
ui/views/controls/button/menu_button.cc:204: int MenuButton::GetShoulderWidth()
const {
On 2016/04/27 03:25:11, tapted wrote:
> move definition to end of file

Done.

https://codereview.chromium.org/1904753002/diff/80001/ui/views/controls/butto...
File ui/views/controls/button/menu_button.h (right):

https://codereview.chromium.org/1904753002/diff/80001/ui/views/controls/butto...
ui/views/controls/button/menu_button.h:47: static const int
kMenuMarkerPaddingLeft;
On 2016/04/27 03:25:11, tapted wrote:
> huh - looks like these are unused now and can be removed. But that might be
bad.
> They are not even, so I think there there will be a regression in the layout
> using ClampToCenteredSize instead of these for non-Mac platforms, so that
needs
> to be fixed. If you still need values for left/right, they should move into an
> anonymous namespace in the .cc.

Done.

https://codereview.chromium.org/1904753002/diff/80001/ui/views/controls/butto...
ui/views/controls/button/menu_button.h:114: // Returns the width of the
"shoulder" of the menubutton, which is the area
On 2016/04/27 03:25:11, tapted wrote:
> nit: menubutton -> MenuButton

Done.

https://codereview.chromium.org/1904753002/diff/80001/ui/views/controls/butto...
ui/views/controls/button/menu_button.h:129: // The images to draw for the menu
marker, if |show_menu_marker_| is true.
On 2016/04/27 03:25:11, tapted wrote:
> I think we can keep the old comment, and variable name (menu_marker_) - they
> don't really need to change. Also nudge down to the prior location (below the
> bool). Changing it from a raw pointer to a (wrapper around) a ref-counted
> pointer I'd keep

Done.

https://codereview.chromium.org/1904753002/diff/80001/ui/views/controls/combo...
File ui/views/controls/combobox/combobox.cc (right):

https://codereview.chromium.org/1904753002/diff/80001/ui/views/controls/combo...
ui/views/controls/combobox/combobox.cc:450: int Combobox::GetShoulderWidth()
const {
On 2016/04/27 03:25:11, tapted wrote:
> nit: move to the end of the file to match declaration order

Done.

https://codereview.chromium.org/1904753002/diff/80001/ui/views/controls/combo...
File ui/views/controls/combobox/combobox.h (right):

https://codereview.chromium.org/1904753002/diff/80001/ui/views/controls/combo...
ui/views/controls/combobox/combobox.h:56: Combobox(ui::ComboboxModel* model,
Style style = STYLE_NORMAL);
On 2016/04/27 03:25:11, tapted wrote:
> nit: add explicit - C++ will still allow implicit conversion if it can find a
> way to construct with a single argument

Done.

[tapted, 2016-04-29 06:53:18]

https://codereview.chromium.org/1904753002/diff/120001/ui/views/controls/butt...
File ui/views/controls/button/menu_button.cc (right):

https://codereview.chromium.org/1904753002/diff/120001/ui/views/controls/butt...
ui/views/controls/button/menu_button.cc:84:
set_background(PlatformStyle::CreateMenuButtonBackground(GetShoulderWidth())
The background will need similar treatment to the border to avoid a regression
like http://crbug.com/605161#c5

I think we should add some simple tests for this, something similar to
DialogButtonBorderMacTest.ChecksButtonStyle and friends

https://codereview.chromium.org/1904753002/diff/120001/ui/views/controls/butt...
ui/views/controls/button/menu_button.cc:330:
arrow_bounds.ClampToCenteredSize(menu_marker_.size());
With kMenuMarkerPaddingLeft/Right gone, won't this change the position on
linux/win/chromeos?

https://codereview.chromium.org/1904753002/diff/120001/ui/views/controls/butt...
ui/views/controls/button/menu_button.cc:376: return false;
nothing overrides this yet. I believe the current target use-case is the
TranslateBubble - we need to prepare a patch to ensure that will work correctly
when this is needed, and to ensure the structure still looks nice. Something in
views_examples would probably help too.

https://codereview.chromium.org/1904753002/diff/120001/ui/views/controls/butt...
File ui/views/controls/button/menu_button.h (right):

https://codereview.chromium.org/1904753002/diff/120001/ui/views/controls/butt...
ui/views/controls/button/menu_button.h:95: virtual bool
ShouldUsePlatformStyleBorder() const;
nit: usually newly introduced methods come before overrides in each section, so
this should move up below PaintMenuMarker. Don't forget to move the method in
the .cc as well.

[Elly Fong-Jones, 2016-05-06 21:30:45]

tapted: ptal? not sure where else to go with the tests.

https://codereview.chromium.org/1904753002/diff/120001/ui/views/controls/butt...
File ui/views/controls/button/menu_button.cc (right):

https://codereview.chromium.org/1904753002/diff/120001/ui/views/controls/butt...
ui/views/controls/button/menu_button.cc:84:
set_background(PlatformStyle::CreateMenuButtonBackground(GetShoulderWidth())
On 2016/04/29 06:53:18, tapted wrote:
> The background will need similar treatment to the border to avoid a regression
> like http://crbug.com/605161#c5
> 
> I think we should add some simple tests for this, something similar to
> DialogButtonBorderMacTest.ChecksButtonStyle and friends

Done.

https://codereview.chromium.org/1904753002/diff/120001/ui/views/controls/butt...
ui/views/controls/button/menu_button.cc:330:
arrow_bounds.ClampToCenteredSize(menu_marker_.size());
On 2016/04/29 06:53:18, tapted wrote:
> With kMenuMarkerPaddingLeft/Right gone, won't this change the position on
> linux/win/chromeos?

I *think* that I accounted for that in GetShoulderWidth()? I don't have Linux or
Windows machines handy to test against.

https://codereview.chromium.org/1904753002/diff/120001/ui/views/controls/butt...
ui/views/controls/button/menu_button.cc:376: return false;
On 2016/04/29 06:53:18, tapted wrote:
> nothing overrides this yet. I believe the current target use-case is the
> TranslateBubble - we need to prepare a patch to ensure that will work
correctly
> when this is needed, and to ensure the structure still looks nice. Something
in
> views_examples would probably help too.

I added some examples in views_examples for this.

https://codereview.chromium.org/1904753002/diff/120001/ui/views/controls/butt...
File ui/views/controls/button/menu_button.h (right):

https://codereview.chromium.org/1904753002/diff/120001/ui/views/controls/butt...
ui/views/controls/button/menu_button.h:95: virtual bool
ShouldUsePlatformStyleBorder() const;
On 2016/04/29 06:53:18, tapted wrote:
> nit: usually newly introduced methods come before overrides in each section,
so
> this should move up below PaintMenuMarker. Don't forget to move the method in
> the .cc as well.

Done.

[tapted, 2016-05-09 08:01:36]

LabelButton seems to be where all tricky stuff is -- how about we split off the
Combobox changes and land that separately.

https://codereview.chromium.org/1904753002/diff/120001/ui/views/controls/butt...
File ui/views/controls/button/menu_button.cc (right):

https://codereview.chromium.org/1904753002/diff/120001/ui/views/controls/butt...
ui/views/controls/button/menu_button.cc:330:
arrow_bounds.ClampToCenteredSize(menu_marker_.size());
On 2016/05/06 21:30:45, Elly Jones wrote:
> On 2016/04/29 06:53:18, tapted wrote:
> > With kMenuMarkerPaddingLeft/Right gone, won't this change the position on
> > linux/win/chromeos?
> 
> I *think* that I accounted for that in GetShoulderWidth()? I don't have Linux
or
> Windows machines handy to test against.

GetShoulderWidth() is the sum though. It might work for combobox, but MenuButton
is less evenly weighted. I'll try this on Windows. my last attempt at a build
ran out of disk space though :|

https://codereview.chromium.org/1904753002/diff/160001/chrome/browser/ui/view...
File chrome/browser/ui/views/translate/translate_bubble_view.cc (right):

https://codereview.chromium.org/1904753002/diff/160001/chrome/browser/ui/view...
chrome/browser/ui/views/translate/translate_bubble_view.cc:420:
views::Combobox::STYLE_ACTION);
heads-up: this will need a rebase

https://codereview.chromium.org/1904753002/diff/160001/ui/gfx/gfx.gyp
File ui/gfx/gfx.gyp (right):

https://codereview.chromium.org/1904753002/diff/160001/ui/gfx/gfx.gyp#newcode267
ui/gfx/gfx.gyp:267: 'scoped_canvas.cc',
nit: sort

https://codereview.chromium.org/1904753002/diff/160001/ui/views/controls/butt...
File ui/views/controls/button/menu_button.cc (right):

https://codereview.chromium.org/1904753002/diff/160001/ui/views/controls/butt...
ui/views/controls/button/menu_button.cc:82: if (show_menu_marker_) {
just call OnEnabledChanged

https://codereview.chromium.org/1904753002/diff/160001/ui/views/controls/butt...
ui/views/controls/button/menu_button.cc:312: menu_marker_ =
PlatformStyle::CreateMenuButtonArrow(enabled());
So now the background is not unexpectedly blue, there is another problem for
permission_selector_view.cc's PermissionMenuButton which I didn't realise would
happen before. No background == white background. But the default "enabled"
arrow is white, so you can't see it on a white background.

I think the right way to fix this is to pass the entire MenuButton into
PlatformStyle -- let PlatformStyle decide whether to create a native or
light-coloured arrow based on whether the button already has a background/border
it recognises. Make sure you call OnEnabledChanged when the background/border
may have changed due to the native theme. (note: unlike SetBorder, there is no
overridable SetBackground -- just set_background -- so I think this is more
reason to use SetBorder in the same way as LabelButton.

https://codereview.chromium.org/1904753002/diff/160001/ui/views/controls/butt...
ui/views/controls/button/menu_button.cc:322: if (ShouldUsePlatformStyleBorder())
There's still an issue with this. MenuButton inherits from LabelButton which
already has a way for instances to effectively override
`ShouldUsePlatformStyleBorder`. Except they do it simply by calling
SetBorder(anything). E.g. the Chrome menu on the toolbar calls
SetBorder(nullptr) to avoid getting a border. With this line here, it will now
get an outline and a focus ring.

An earlier comment has some ideas about this:

On 2016/04/26 05:05:58, tapted wrote:
> I put a screenshot at http://crbug.com/605161#c4 from an earlier patchset. And
> it looks that way still in patchset 4 with mac_views_browser=1 or mvb=0. The
> problem is that PermissionMenuButton (in permission_selector_view.cc) has a
> weird initialization path. I think to fix this we need views::MenuButton to
> override LabelButton::UpdateThemedBorder() [and/or OnNativeThemeChanged] and
> these method calls need to move into that method.  [OnNativeThemeChanged gets
> called when the button is added to a Widget, so we shouldn't have to call
> UpdateThemedBorder explicitly in MenuButton's initialization paths]. Then, so
> long as PermissionMenuButton calls `SetBorder(<something> -- probably
nullptr)`
> in its constructor [probably after its call to UpdateThemedBorder()] things
> might be "ok". But see http://crbug.com/510747 where I caused a regression
here
> on GTK - things are a bit fragile, so we need to check each platform :/
> 
> Note there's also a faint white border/outline around the bookmarks bar's
"Other
> Bookmarks" menu button (mac_views_browser=1 only).

I'm still confident that having views::MenuButton to override
LabelButton::UpdateThemedBorder() will lead to a way to land this without
regressions in the permission selector.

https://codereview.chromium.org/1904753002/diff/160001/ui/views/controls/butt...
File ui/views/controls/button/menu_button.h (right):

https://codereview.chromium.org/1904753002/diff/160001/ui/views/controls/butt...
ui/views/controls/button/menu_button.h:87: virtual bool
ShouldUsePlatformStyleBorder() const;
So I think there is a nicer solution that doesn't use virtual methods to achieve
this. The big problem is that we need MenuButton to fit in with the stuff that
LabelButton is already doing. Except!

LabelButton doesn't have a background -- just a border, even when themed.

So I really think we need to do the same for MenuButton -- incorporate the
shinymodern themed background in with the border. I think it's dangerous to
think we can get away with only "half" being themed -- they need to match.

Yes, drawing background stuff in with the "border" is weird, but doing this
allows the parts of the theme to stay together and preserve consistency.

This approach should let us use the existing plumbing for this, in
LabelButton::border_is_themed_border_ -- maybe we will need an accessor for that
bool (e.g. so PlatformStyle can read it), or maybe we can just do it via an
UpdateThemedBorder override. See what LabelButton does -- I think we need to
align the approaches we use for adding style to these.

https://codereview.chromium.org/1904753002/diff/160001/ui/views/controls/butt...
File ui/views/controls/button/menu_button_unittest.cc (right):

https://codereview.chromium.org/1904753002/diff/160001/ui/views/controls/butt...
ui/views/controls/button/menu_button_unittest.cc:38: bool use_background,
I don't think we need to pass in these arguments -- just add setters for
use_background_ and use_border_. Then we avoid a lot of the default arguments
which can be annoying to refactor around (like, what if we add a new feature
that requires a third?)

https://codereview.chromium.org/1904753002/diff/160001/ui/views/controls/butt...
ui/views/controls/button/menu_button_unittest.cc:48: // Overrides for MenuButton
customization methods.
nit: just
// MenuButton:

https://codereview.chromium.org/1904753002/diff/160001/ui/views/controls/butt...
ui/views/controls/button/menu_button_unittest.cc:63:
DISALLOW_COPY_AND_ASSIGN(TestMenuButton);
nit: blank line before

https://codereview.chromium.org/1904753002/diff/160001/ui/views/controls/butt...
ui/views/controls/button/menu_button_unittest.cc:635: TEST_F(MenuButtonTest,
UseBorderDoesApplyBorder) {
tests should have a comment describing why they exist

https://codereview.chromium.org/1904753002/diff/160001/ui/views/controls/butt...
ui/views/controls/button/menu_button_unittest.cc:637: if
(PlatformStyle::CreateMenuButtonBorder())
I think it would be better to #if defined(platform) for this -- makes it more
obvious that the test is testing specific rather than inferred behavior.

https://codereview.chromium.org/1904753002/diff/160001/ui/views/controls/butt...
ui/views/controls/button/menu_button_unittest.cc:639: }
we also need to test something that calls SetBorder *before* the button is added
to the Widget -- this is the current bug with the Chrome menu button.

https://codereview.chromium.org/1904753002/diff/160001/ui/views/style/mac/com...
File ui/views/style/mac/combobox_background_mac.h (right):

https://codereview.chromium.org/1904753002/diff/160001/ui/views/style/mac/com...
ui/views/style/mac/combobox_background_mac.h:37:
DISALLOW_COPY_AND_ASSIGN(ComboboxBackgroundMac);
nit: blank line before

[tapted, 2016-05-10 08:03:23]

https://codereview.chromium.org/1904753002/diff/120001/ui/views/controls/butt...
File ui/views/controls/button/menu_button.cc (right):

https://codereview.chromium.org/1904753002/diff/120001/ui/views/controls/butt...
ui/views/controls/button/menu_button.cc:330:
arrow_bounds.ClampToCenteredSize(menu_marker_.size());
On 2016/05/09 08:01:35, tapted wrote:
> On 2016/05/06 21:30:45, Elly Jones wrote:
> > On 2016/04/29 06:53:18, tapted wrote:
> > > With kMenuMarkerPaddingLeft/Right gone, won't this change the position on
> > > linux/win/chromeos?
> > 
> > I *think* that I accounted for that in GetShoulderWidth()? I don't have
Linux
> or
> > Windows machines handy to test against.
> 
> GetShoulderWidth() is the sum though. It might work for combobox, but
MenuButton
> is less evenly weighted. I'll try this on Windows. my last attempt at a build
> ran out of disk space though :|

Added Windows screenshots to http://crbug.com/605161#c6 for this. The layout
changes quite a bit (and of course we need to ensure Windows doesn't change at
all).