MacViews: draw Mac combobox arrows

MacViews: draw Mac combobox arrows

This CL adds Mac-specific combobox arrow drawing code to NativeThemeMac.

BUG=543683
TEST=adhoc
Build views_examples_with_content_exe and look at the Combobox page.

Committed: https://crrev.com/0cdd64944cabbca4072e01f0da8326eea9b1cc9e
Cr-Commit-Position: refs/heads/master@{#385759}

[Elly Fong-Jones, 2016-03-18 19:51:27]

Description was changed from

==========
MacViews: draw Mac combobox arrows

This CL adds Mac-specific combobox arrow drawing code to NativeThemeMac.

BUG=543683
TEST=adhoc
Build views_examples_with_content_exe and look at the Combobox page.
==========

to

==========
MacViews: draw Mac combobox arrows

This CL adds Mac-specific combobox arrow drawing code to NativeThemeMac.

BUG=543683
TEST=adhoc
Build views_examples_with_content_exe and look at the Combobox page.
==========

[Elly Fong-Jones, 2016-03-18 19:51:27]

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

[Elly Fong-Jones, 2016-03-18 19:51:35]

tapted: ptal? :)

[tapted, 2016-03-21 06:58:45]

https://codereview.chromium.org/1817613002/diff/1/ui/native_theme/native_them...
File ui/native_theme/native_theme_mac.mm (right):

https://codereview.chromium.org/1817613002/diff/1/ui/native_theme/native_them...
ui/native_theme/native_theme_mac.mm:277: // TODO(ellyjones): These colors aren't
exactly right.
What's the change that needs to be made? (why can't we do it now?)

https://codereview.chromium.org/1817613002/diff/1/ui/native_theme/native_them...
ui/native_theme/native_theme_mac.mm:281: paint.setAntiAlias(true);
I think we need a setStrokeCap(SkPaint::kRound_Cap); too (but that's default for
vector icons - see below)

https://codereview.chromium.org/1817613002/diff/1/ui/native_theme/native_them...
ui/native_theme/native_theme_mac.mm:288: SkPoint arrow_rect_left_mid =
My feeling is that this is something that should be done by
`gfx::PaintVectorIcon(canvas, gfx::VectorIconId::COMBOBOX_ARROW,
kComboboxArrowHeight, color);`  (i.e. by adding these LINE_TO, etc., calls to a
new vector icon file, and using it here). There isn't a precedent for using
vector icons in ui/native_theme yet, but vector icons haven't been around that
long.

I think a vector icon file would be easier to maintain than explicit drawing
code, and you'd only need one constant in the "actual" code rather than a bunch.

We could possibly even merge the code to use a vector asset on other platforms
after making a vector image for IDR_MENU_DROPARROW also.


Maybe there's a reason for not using vector icons..... but I don't know it. I
think that would be my preference - what are your thoughts?

https://codereview.chromium.org/1817613002/diff/1/ui/native_theme/native_them...
ui/native_theme/native_theme_mac.mm:321: return gfx::Size(kComboboxArrowWidth,
kComboboxArrowHeight);
To keep the control size / dialog layout / etc. consistent across platforms, I
think it would be better to return the same thing as other platforms. I.e.
"whatever it is now" - i.e. the size of the IDR_MENU_DROPARROW asset. then just
always draw inside a (square) square the same size, at the appropriate place

[Elly Fong-Jones, 2016-03-21 21:04:23]

tapted: ptal?

https://codereview.chromium.org/1817613002/diff/1/ui/native_theme/native_them...
File ui/native_theme/native_theme_mac.mm (right):

https://codereview.chromium.org/1817613002/diff/1/ui/native_theme/native_them...
ui/native_theme/native_theme_mac.mm:277: // TODO(ellyjones): These colors aren't
exactly right.
On 2016/03/21 06:58:45, tapted wrote:
> What's the change that needs to be made? (why can't we do it now?)

Done.

https://codereview.chromium.org/1817613002/diff/1/ui/native_theme/native_them...
ui/native_theme/native_theme_mac.mm:281: paint.setAntiAlias(true);
On 2016/03/21 06:58:45, tapted wrote:
> I think we need a setStrokeCap(SkPaint::kRound_Cap); too (but that's default
for
> vector icons - see below)

Done.

https://codereview.chromium.org/1817613002/diff/1/ui/native_theme/native_them...
ui/native_theme/native_theme_mac.mm:288: SkPoint arrow_rect_left_mid =
On 2016/03/21 06:58:45, tapted wrote:
> My feeling is that this is something that should be done by
> `gfx::PaintVectorIcon(canvas, gfx::VectorIconId::COMBOBOX_ARROW,
> kComboboxArrowHeight, color);`  (i.e. by adding these LINE_TO, etc., calls to
a
> new vector icon file, and using it here). There isn't a precedent for using
> vector icons in ui/native_theme yet, but vector icons haven't been around that
> long.
> 
> I think a vector icon file would be easier to maintain than explicit drawing
> code, and you'd only need one constant in the "actual" code rather than a
bunch.
> 
> We could possibly even merge the code to use a vector asset on other platforms
> after making a vector image for IDR_MENU_DROPARROW also.
> 
> 
> Maybe there's a reason for not using vector icons..... but I don't know it. I
> think that would be my preference - what are your thoughts?

Done.

This turned out to be a bit annoying, since we have an SkCanvas* and we need a
Canvas*, but I think I got it to work, and I created a vector icon for this
arrow.

https://codereview.chromium.org/1817613002/diff/1/ui/native_theme/native_them...
ui/native_theme/native_theme_mac.mm:321: return gfx::Size(kComboboxArrowWidth,
kComboboxArrowHeight);
On 2016/03/21 06:58:45, tapted wrote:
> To keep the control size / dialog layout / etc. consistent across platforms, I
> think it would be better to return the same thing as other platforms. I.e.
> "whatever it is now" - i.e. the size of the IDR_MENU_DROPARROW asset. then
just
> always draw inside a (square) square the same size, at the appropriate place

The current size is too small to draw two arrows in, though. I tried this at
first, and it ends up trying to squeeze into an area that's like, 6px by 6px.
Maybe I can make the image *larger* on other platforms, with just more
transparent space around it, to give us room to work on Mac?

[tapted, 2016-03-21 22:59:59]

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

https://codereview.chromium.org/1817613002/diff/20001/ui/views/controls/combo...
ui/views/controls/combobox/combobox.cc:753:
params.combobox_arrow.icon_scale_factor = canvas->image_scale();
I'm pretty sure we don't need to pass this in. When a gfx::Canvas is created,
its constructor does

  SkScalar scale_scalar = SkFloatToScalar(image_scale);
  canvas_->scale(scale_scalar, scale_scalar);

that scale on the SkCanvas should "stick" for its lifetime - even if it's
adopted by a new gfx::Canvas (unless something does
gfx::Canvas::UndoDeviceScaleFactor).

So I think we can create a new gfx::Canvas with a scale factor of 1. But then
the problem is some code in PaintVectorIcon which checks for
canvas->image_scale() -- if it's 1x, the method calls
GetPathForVectorIconAt1xScale, but that method is the same as
GetPathForVectorIcon unless a "*.1x.icon" file exists (which it doesn't), so I
think everything will "work". But it's really not very neat :/. 

Currently I think there's some extra scaling that's undone by passing
rect.width(). (or the scale factor is always "1", so it doesn't look strange --
passing --force-device-scale-factor=2.0 in the command line might show up
something if you're not on a retina device).


But, where does this leave us :/. Creating a gfx::Canvas with scale factor of 1
and chancing that everyhing works doesn't feel right.

Currently I'm thinking we should stop using ui::NativeTheme, and instead look
more to how Chrome does GTK styling for Desktop Linux.  This stuff is all in
ui/views/linux_ui (typically it gets used via #ifdefs which is kinda gross, so I
ended up creating ui/views/style/platform_style to lie on top of calls into
linux_ui, as well as the mac-specific stuff, so there's a nice cross-platform
API that doesn't need #ifdefs).

Then, if we don't use NativeTheme, we don't need to worry about these
Canvas/SkCanvas conversions. Otherwise the "right" fix is probably to update the
NativeTheme API to use gfx::Canvas, or update PaintVectorIcon to take in a
SkCanvas and a distinct scale factor argument.

The first step:
 - purge CommonThemePaintComboboxArrow and ui::NativeTheme::kComboboxArrow --
there's a silly amount of plumbing through {native,common}_theme{,_base,win}
already to handle this single call site. It's also the only thing necessitating
some GetPartSize overrides. These can be replaced by a constant on
views::PlatformStyle and something PlatformStyle::PaintComboboxArrow

But! We can probably go even further and make the ComboboxArrow part of the
ComboboxBorder...... That would simplify this function. It's kinda weird that
`PaintText` is responsible for painting an arrow.

[Elly Fong-Jones, 2016-03-24 19:53:54]

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

[Elly Fong-Jones, 2016-03-24 19:53:55]

tapted, sky: ptal?

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

https://codereview.chromium.org/1817613002/diff/20001/ui/views/controls/combo...
ui/views/controls/combobox/combobox.cc:753:
params.combobox_arrow.icon_scale_factor = canvas->image_scale();
On 2016/03/21 22:59:59, tapted (OOO.travel to Apr 5th) wrote:
> I'm pretty sure we don't need to pass this in. When a gfx::Canvas is created,
> its constructor does
> 
>   SkScalar scale_scalar = SkFloatToScalar(image_scale);
>   canvas_->scale(scale_scalar, scale_scalar);
> 
> that scale on the SkCanvas should "stick" for its lifetime - even if it's
> adopted by a new gfx::Canvas (unless something does
> gfx::Canvas::UndoDeviceScaleFactor).
> 
> So I think we can create a new gfx::Canvas with a scale factor of 1. But then
> the problem is some code in PaintVectorIcon which checks for
> canvas->image_scale() -- if it's 1x, the method calls
> GetPathForVectorIconAt1xScale, but that method is the same as
> GetPathForVectorIcon unless a "*.1x.icon" file exists (which it doesn't), so I
> think everything will "work". But it's really not very neat :/. 
> 
> Currently I think there's some extra scaling that's undone by passing
> rect.width(). (or the scale factor is always "1", so it doesn't look strange
--
> passing --force-device-scale-factor=2.0 in the command line might show up
> something if you're not on a retina device).
> 
> 
> But, where does this leave us :/. Creating a gfx::Canvas with scale factor of
1
> and chancing that everyhing works doesn't feel right.
> 
> Currently I'm thinking we should stop using ui::NativeTheme, and instead look
> more to how Chrome does GTK styling for Desktop Linux.  This stuff is all in
> ui/views/linux_ui (typically it gets used via #ifdefs which is kinda gross, so
I
> ended up creating ui/views/style/platform_style to lie on top of calls into
> linux_ui, as well as the mac-specific stuff, so there's a nice cross-platform
> API that doesn't need #ifdefs).
> 
> Then, if we don't use NativeTheme, we don't need to worry about these
> Canvas/SkCanvas conversions. Otherwise the "right" fix is probably to update
the
> NativeTheme API to use gfx::Canvas, or update PaintVectorIcon to take in a
> SkCanvas and a distinct scale factor argument.
> 
> The first step:
>  - purge CommonThemePaintComboboxArrow and ui::NativeTheme::kComboboxArrow --
> there's a silly amount of plumbing through {native,common}_theme{,_base,win}
> already to handle this single call site. It's also the only thing
necessitating
> some GetPartSize overrides. These can be replaced by a constant on
> views::PlatformStyle and something PlatformStyle::PaintComboboxArrow
> 
> But! We can probably go even further and make the ComboboxArrow part of the
> ComboboxBorder...... That would simplify this function. It's kinda weird that
> `PaintText` is responsible for painting an arrow.

I swapped this entire thing over to using:

PlatformStyle::CreateComboboxArrow()

which returns an ImageSkia that Combobox::PaintText() draws into its canvas.

What do you think?

[tapted, 2016-03-29 04:16:37]

Yeah - I think this is a good direction and a nice simplification. (hopefully
sky does too!)

https://codereview.chromium.org/1817613002/diff/40001/ui/native_theme/native_...
File ui/native_theme/native_theme.gyp (right):

https://codereview.chromium.org/1817613002/diff/40001/ui/native_theme/native_...
ui/native_theme/native_theme.gyp:20: '../gfx/gfx.gyp:gfx_vector_icons',
I don't think this is needed any more

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

https://codereview.chromium.org/1817613002/diff/40001/ui/views/controls/combo...
ui/views/controls/combobox/combobox.cc:759:
ui::CommonThemePaintComboboxArrow(canvas->sk_canvas(), arrow_bounds);
I think this is the only caller now, and the method is pretty similar to what's
below.  Except I guess the Mac arrows wouldn't be appropriate for STYLE_ACTION,
(i.e. supposing we do decide to have STYLE_ACTION comboboxes appear on Chrome's
Mac UI, which is unlikely).

Can we remove this method from ui:: by passing `style_` into
PlatformStyle::CreateComboboxArrow? Then have mac use the raster asset for
ACTION_STYLE

Merging the logic here is probably necessary anyway, since |arrow_bounds| is
initialised with Combobox::ArrowSize() which won't be the size of the triangle

https://codereview.chromium.org/1817613002/diff/40001/ui/views/controls/combo...
ui/views/controls/combobox/combobox.cc:764:
image_bounds.Inset((arrow_bounds.width() - arrow_image.width()) / 2,
I think this is the same as

image_bounds.ClampToCenteredSize(arrow_image.size());

But shouldn't arrow_bounds.size() and arrow_image.size() be the same already,
since Combobox::ArrowSize() is now derived the same way

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

https://codereview.chromium.org/1817613002/diff/40001/ui/views/style/platform...
ui/views/style/platform_style.cc:24:
ui::ResourceBundle::GetSharedInstance().GetImageSkiaNamed(
nit: The convention for this elsewhere in Chrome is typically

  ui::ResourceBundle& rb = uiResourceBundle::GetSharedInstance();
  return *rb.GetImageSkiaNamed(IDR_MENU_DROPARROW);

[Elly Fong-Jones, 2016-04-04 17:09:55]

tapted, sky: ptal? :)

https://codereview.chromium.org/1817613002/diff/40001/ui/native_theme/native_...
File ui/native_theme/native_theme.gyp (right):

https://codereview.chromium.org/1817613002/diff/40001/ui/native_theme/native_...
ui/native_theme/native_theme.gyp:20: '../gfx/gfx.gyp:gfx_vector_icons',
On 2016/03/29 04:16:36, tapted (travelling to Apr 5th) wrote:
> I don't think this is needed any more

Done.

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

https://codereview.chromium.org/1817613002/diff/40001/ui/views/controls/combo...
ui/views/controls/combobox/combobox.cc:759:
ui::CommonThemePaintComboboxArrow(canvas->sk_canvas(), arrow_bounds);
On 2016/03/29 04:16:36, tapted (travelling to Apr 5th) wrote:
> I think this is the only caller now, and the method is pretty similar to
what's
> below.  Except I guess the Mac arrows wouldn't be appropriate for
STYLE_ACTION,
> (i.e. supposing we do decide to have STYLE_ACTION comboboxes appear on
Chrome's
> Mac UI, which is unlikely).
> 
> Can we remove this method from ui:: by passing `style_` into
> PlatformStyle::CreateComboboxArrow? Then have mac use the raster asset for
> ACTION_STYLE
> 
> Merging the logic here is probably necessary anyway, since |arrow_bounds| is
> initialised with Combobox::ArrowSize() which won't be the size of the triangle

Done.

https://codereview.chromium.org/1817613002/diff/40001/ui/views/controls/combo...
ui/views/controls/combobox/combobox.cc:764:
image_bounds.Inset((arrow_bounds.width() - arrow_image.width()) / 2,
On 2016/03/29 04:16:36, tapted (travelling to Apr 5th) wrote:
> I think this is the same as
> 
> image_bounds.ClampToCenteredSize(arrow_image.size());
> 
> But shouldn't arrow_bounds.size() and arrow_image.size() be the same already,
> since Combobox::ArrowSize() is now derived the same way

Done.

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

https://codereview.chromium.org/1817613002/diff/40001/ui/views/style/platform...
ui/views/style/platform_style.cc:24:
ui::ResourceBundle::GetSharedInstance().GetImageSkiaNamed(
On 2016/03/29 04:16:36, tapted (travelling to Apr 5th) wrote:
> nit: The convention for this elsewhere in Chrome is typically
> 
>   ui::ResourceBundle& rb = uiResourceBundle::GetSharedInstance();
>   return *rb.GetImageSkiaNamed(IDR_MENU_DROPARROW);

Done.

[sky, 2016-04-04 20:47:55]

LGTM

[tapted, 2016-04-05 08:45:02]

https://codereview.chromium.org/1817613002/diff/60001/ui/gfx/vector_icons/com...
File ui/gfx/vector_icons/combobox_arrow_mac.icon (right):

https://codereview.chromium.org/1817613002/diff/60001/ui/gfx/vector_icons/com...
ui/gfx/vector_icons/combobox_arrow_mac.icon:1: // Copyright 2016 The Chromium
Authors. All rights reserved.
Looks like "vector_icons/combobox_arrow_mac.icon" needs to be added to
ui/gfx/BUILD.gn  (the GYP rule takes `'inputs': [ 'vector_icons/', ],` -- the
whole folder -- but the GN rule needs to be explicit.) - You'll need estade for
OWNERS

https://codereview.chromium.org/1817613002/diff/60001/ui/native_theme/common_...
File ui/native_theme/common_theme.cc (left):

https://codereview.chromium.org/1817613002/diff/60001/ui/native_theme/common_...
ui/native_theme/common_theme.cc:408: gfx::Size
CommonThemeGetPartSize(NativeTheme::Part part,
remove the declaration in common_theme.h

https://codereview.chromium.org/1817613002/diff/60001/ui/native_theme/common_...
File ui/native_theme/common_theme.cc (right):

https://codereview.chromium.org/1817613002/diff/60001/ui/native_theme/common_...
ui/native_theme/common_theme.cc:408: void
CommonThemePaintComboboxArrow(SkCanvas* canvas, const gfx::Rect& rect) {
Can this method (and the declaration) be removed? Pretty sure nothing calls it
now

https://codereview.chromium.org/1817613002/diff/60001/ui/native_theme/native_...
File ui/native_theme/native_theme_win.cc (right):

https://codereview.chromium.org/1817613002/diff/60001/ui/native_theme/native_...
ui/native_theme/native_theme_win.cc:196: gfx::Size part_size =
CommonThemeGetPartSize(part, state, extra);
These 4 lines can be deleted. Same for the bit in
NativeThemeAuraWin::GetPartSize() (native_theme_aurawin.cc)

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

https://codereview.chromium.org/1817613002/diff/60001/ui/views/controls/combo...
ui/views/controls/combobox/combobox.cc:759: canvas->DrawImageInt(arrow_image,
image_bounds.x(), image_bounds.y());
I have a theory, that you could write something like

DCHECK_EQ(image_bounds.x(), arrow_bounds.x());
DCHECK_EQ(image_bounds.y(), arrow_bounds.y());

and it would always be true on all platforms. In which case, we can just pass
arrow_bounds.x/y() and remove lines 757-8.  (don't keep the DCHECK though -
that's just to test the theory :).

And, actually, I wanted to take some screenshots with this patched in, so I
decided just to test that hypothesis in
https://codereview.chromium.org/1863503002/#ps40001 :).  (So, if bots there are
green we can remove lines 758-9 and just use arrow_bounds.{x,y}() here.)

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

https://codereview.chromium.org/1817613002/diff/60001/ui/views/style/platform...
ui/views/style/platform_style.cc:24: ui::ResourceBundle& rb =
ui::ResourceBundle::SharedInstance();
SharedInstance -> GetSharedInstance

[Elly Fong-Jones, 2016-04-05 20:04:24]

tapted: ptal?

https://codereview.chromium.org/1817613002/diff/60001/ui/gfx/vector_icons/com...
File ui/gfx/vector_icons/combobox_arrow_mac.icon (right):

https://codereview.chromium.org/1817613002/diff/60001/ui/gfx/vector_icons/com...
ui/gfx/vector_icons/combobox_arrow_mac.icon:1: // Copyright 2016 The Chromium
Authors. All rights reserved.
On 2016/04/05 08:45:01, tapted wrote:
> Looks like "vector_icons/combobox_arrow_mac.icon" needs to be added to
> ui/gfx/BUILD.gn  (the GYP rule takes `'inputs': [ 'vector_icons/', ],` -- the
> whole folder -- but the GN rule needs to be explicit.) - You'll need estade
for
> OWNERS

Done.

https://codereview.chromium.org/1817613002/diff/60001/ui/native_theme/common_...
File ui/native_theme/common_theme.cc (left):

https://codereview.chromium.org/1817613002/diff/60001/ui/native_theme/common_...
ui/native_theme/common_theme.cc:408: gfx::Size
CommonThemeGetPartSize(NativeTheme::Part part,
On 2016/04/05 08:45:01, tapted wrote:
> remove the declaration in common_theme.h 

Done.

https://codereview.chromium.org/1817613002/diff/60001/ui/native_theme/common_...
File ui/native_theme/common_theme.cc (right):

https://codereview.chromium.org/1817613002/diff/60001/ui/native_theme/common_...
ui/native_theme/common_theme.cc:408: void
CommonThemePaintComboboxArrow(SkCanvas* canvas, const gfx::Rect& rect) {
On 2016/04/05 08:45:02, tapted wrote:
> Can this method (and the declaration) be removed? Pretty sure nothing calls it
> now

Done.

https://codereview.chromium.org/1817613002/diff/60001/ui/native_theme/native_...
File ui/native_theme/native_theme_win.cc (right):

https://codereview.chromium.org/1817613002/diff/60001/ui/native_theme/native_...
ui/native_theme/native_theme_win.cc:196: gfx::Size part_size =
CommonThemeGetPartSize(part, state, extra);
On 2016/04/05 08:45:02, tapted wrote:
> These 4 lines can be deleted. Same for the bit in
> NativeThemeAuraWin::GetPartSize() (native_theme_aurawin.cc)

Done.

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

https://codereview.chromium.org/1817613002/diff/60001/ui/views/controls/combo...
ui/views/controls/combobox/combobox.cc:759: canvas->DrawImageInt(arrow_image,
image_bounds.x(), image_bounds.y());
On 2016/04/05 08:45:02, tapted wrote:
> I have a theory, that you could write something like
> 
> DCHECK_EQ(image_bounds.x(), arrow_bounds.x());
> DCHECK_EQ(image_bounds.y(), arrow_bounds.y());
> 
> and it would always be true on all platforms. In which case, we can just pass
> arrow_bounds.x/y() and remove lines 757-8.  (don't keep the DCHECK though -
> that's just to test the theory :).
> 
> And, actually, I wanted to take some screenshots with this patched in, so I
> decided just to test that hypothesis in
> https://codereview.chromium.org/1863503002/#ps40001 :).  (So, if bots there
are
> green we can remove lines 758-9 and just use arrow_bounds.{x,y}() here.)

It looks like it exploded on the Windows bots.

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

https://codereview.chromium.org/1817613002/diff/60001/ui/views/style/platform...
ui/views/style/platform_style.cc:24: ui::ResourceBundle& rb =
ui::ResourceBundle::SharedInstance();
On 2016/04/05 08:45:02, tapted wrote:
> SharedInstance -> GetSharedInstance

Done.

[tapted, 2016-04-06 00:07:08]

looks like your `git cl upload` flaked out - can you re-upload?

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

https://codereview.chromium.org/1817613002/diff/60001/ui/views/controls/combo...
ui/views/controls/combobox/combobox.cc:759: canvas->DrawImageInt(arrow_image,
image_bounds.x(), image_bounds.y());
On 2016/04/05 20:04:24, Elly Jones wrote:
> On 2016/04/05 08:45:02, tapted wrote:
> > I have a theory, that you could write something like
> > 
> > DCHECK_EQ(image_bounds.x(), arrow_bounds.x());
> > DCHECK_EQ(image_bounds.y(), arrow_bounds.y());
> > 
> > and it would always be true on all platforms. In which case, we can just
pass
> > arrow_bounds.x/y() and remove lines 757-8.  (don't keep the DCHECK though -
> > that's just to test the theory :).
> > 
> > And, actually, I wanted to take some screenshots with this patched in, so I
> > decided just to test that hypothesis in
> > https://codereview.chromium.org/1863503002/#ps40001 :).  (So, if bots there
> are
> > green we can remove lines 758-9 and just use arrow_bounds.{x,y}() here.)
> 
> It looks like it exploded on the Windows bots.

https://codereview.chromium.org/1863503002/#ps60001 has the green runs, and the
DCHECK

The theory is based on a deskcheck of the code here too - 

line 735 does

gfx::Size arrow_size = ArrowSize();

i.e.

gfx::Size arrow_size = CreateComboboxArrow(..).size();

Then its value is unchanged by the time it gets to

gfx::Rect arrow_bounds(.., .., arrow_size.width(), arrow_size.height);

AdjustBoundsForRTLUI shouldn't affect the size, so we're left with

arrow_bounds.size() == CreateComboboxArrow(..).size() == image_bounds.size().

And if the sizes are equal, then ClampToCenteredSize(..) should be a no-op.



If I've missed something, we should put a comment why we need to call
ClampToCenteredSize, since it's not obvious to me why it's needed (and I'm
pretty sure it isn't :).

[Elly Fong-Jones, 2016-04-06 16:20:21]

tapted: ptal? :)

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

https://codereview.chromium.org/1817613002/diff/60001/ui/views/controls/combo...
ui/views/controls/combobox/combobox.cc:759: canvas->DrawImageInt(arrow_image,
image_bounds.x(), image_bounds.y());
On 2016/04/06 00:07:08, tapted wrote:
> On 2016/04/05 20:04:24, Elly Jones wrote:
> > On 2016/04/05 08:45:02, tapted wrote:
> > > I have a theory, that you could write something like
> > > 
> > > DCHECK_EQ(image_bounds.x(), arrow_bounds.x());
> > > DCHECK_EQ(image_bounds.y(), arrow_bounds.y());
> > > 
> > > and it would always be true on all platforms. In which case, we can just
> pass
> > > arrow_bounds.x/y() and remove lines 757-8.  (don't keep the DCHECK though
-
> > > that's just to test the theory :).
> > > 
> > > And, actually, I wanted to take some screenshots with this patched in, so
I
> > > decided just to test that hypothesis in
> > > https://codereview.chromium.org/1863503002/#ps40001 :).  (So, if bots
there
> > are
> > > green we can remove lines 758-9 and just use arrow_bounds.{x,y}() here.)
> > 
> > It looks like it exploded on the Windows bots.
> 
> https://codereview.chromium.org/1863503002/#ps60001 has the green runs, and
the
> DCHECK
> 
> The theory is based on a deskcheck of the code here too - 
> 
> line 735 does
> 
> gfx::Size arrow_size = ArrowSize();
> 
> i.e.
> 
> gfx::Size arrow_size = CreateComboboxArrow(..).size();
> 
> Then its value is unchanged by the time it gets to
> 
> gfx::Rect arrow_bounds(.., .., arrow_size.width(), arrow_size.height);
> 
> AdjustBoundsForRTLUI shouldn't affect the size, so we're left with
> 
> arrow_bounds.size() == CreateComboboxArrow(..).size() == image_bounds.size().
> 
> And if the sizes are equal, then ClampToCenteredSize(..) should be a no-op.
> 
> 
> 
> If I've missed something, we should put a comment why we need to call
> ClampToCenteredSize, since it's not obvious to me why it's needed (and I'm
> pretty sure it isn't :).

Ya, sorry - I should have realized that these two lines:

  gfx::Rect image_bounds(arrow_bounds);
  image_bounds.ClampToCenteredSize(arrow_bounds.size());

are an obvious noop, since if image_bounds == arrow_bounds, it's already
centered inside arrow_bounds. Removed them.

[Elly Fong-Jones, 2016-04-06 16:21:06]

The CQ bit was checked by ellyjones@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-04-06 16:21:25]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1817613002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1817613002/100001

[commit-bot: I haz the power, 2016-04-06 17:28:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-04-06 17:28:54]

Dry run: This issue passed the CQ dry run.

[tapted, 2016-04-06 21:20:17]

lgtm

[Elly Fong-Jones, 2016-04-07 15:03:30]

The CQ bit was checked by ellyjones@chromium.org

[Elly Fong-Jones, 2016-04-07 15:03:30]

The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org
Link to the patchset: https://codereview.chromium.org/1817613002/#ps100001
(title: "remove noop image_bounds")

[commit-bot: I haz the power, 2016-04-07 15:03:42]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1817613002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1817613002/100001

[commit-bot: I haz the power, 2016-04-07 15:08:40]

Description was changed from

==========
MacViews: draw Mac combobox arrows

This CL adds Mac-specific combobox arrow drawing code to NativeThemeMac.

BUG=543683
TEST=adhoc
Build views_examples_with_content_exe and look at the Combobox page.
==========

to

==========
MacViews: draw Mac combobox arrows

This CL adds Mac-specific combobox arrow drawing code to NativeThemeMac.

BUG=543683
TEST=adhoc
Build views_examples_with_content_exe and look at the Combobox page.
==========

[commit-bot: I haz the power, 2016-04-07 15:08:41]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-04-07 15:09:54]

Description was changed from

==========
MacViews: draw Mac combobox arrows

This CL adds Mac-specific combobox arrow drawing code to NativeThemeMac.

BUG=543683
TEST=adhoc
Build views_examples_with_content_exe and look at the Combobox page.
==========

to

==========
MacViews: draw Mac combobox arrows

This CL adds Mac-specific combobox arrow drawing code to NativeThemeMac.

BUG=543683
TEST=adhoc
Build views_examples_with_content_exe and look at the Combobox page.

Committed: https://crrev.com/0cdd64944cabbca4072e01f0da8326eea9b1cc9e
Cr-Commit-Position: refs/heads/master@{#385759}
==========

[commit-bot: I haz the power, 2016-04-07 15:09:56]

Patchset 6 (id:??) landed as
https://crrev.com/0cdd64944cabbca4072e01f0da8326eea9b1cc9e
Cr-Commit-Position: refs/heads/master@{#385759}