Draw nicer arrows when the scrollbar buttons are not square.

Draw nicer arrows when the scrollbar buttons are not square.

Before they would look squished or messed up, but with this patch
their bounding box is always square so we'll get the nice pixel pyramids
that we want. This shouldn't change anything visually for the normal
square scrollbar buttons.

BUG=614297
Committed: https://crrev.com/91cbc6d7c2f02771ee3640a4b2f3376029b3bab0
Cr-Commit-Position: refs/heads/master@{#399626}

[Bret, 2016-06-07 00:24:09]

Description was changed from

==========
Draw nicer arrows when the scrollbar buttons are not square.

Before they would kinda look squished or messed up, but with this patch
their bounding box is alway square so we'll get the nice pixel pyramids
that we want. This shouldn't change anything visually for the normal
scrollbar buttons.

BUG=614297
==========

to

==========
Draw nicer arrows when the scrollbar buttons are not square.

Before they would look squished or messed up, but with this patch
their bounding box is always square so we'll get the nice pixel pyramids
that we want. This shouldn't change anything visually for the normal
square scrollbar buttons.

BUG=614297
==========

[Bret, 2016-06-07 00:25:04]

bsep@chromium.org changed reviewers:
+ estade@chromium.org

[Bret, 2016-06-09 17:38:58]

bsep@chromium.org changed reviewers:
+ pkasting@google.com
- estade@chromium.org

[Bret, 2016-06-09 17:43:50]

Changing reviewers because Evan is on vacation.

[Peter Kasting, 2016-06-10 01:03:22]

pkasting@chromium.org changed reviewers:
+ pkasting@chromium.org

[Peter Kasting, 2016-06-10 01:03:23]

https://codereview.chromium.org/2009733002/diff/20001/ui/native_theme/native_...
File ui/native_theme/native_theme_base.cc (left):

https://codereview.chromium.org/2009733002/diff/20001/ui/native_theme/native_...
ui/native_theme/native_theme_base.cc:388: path.offset(0, -1);
What were these offsets for?

https://codereview.chromium.org/2009733002/diff/20001/ui/native_theme/native_...
File ui/native_theme/native_theme_base.cc (right):

https://codereview.chromium.org/2009733002/diff/20001/ui/native_theme/native_...
ui/native_theme/native_theme_base.cc:402: gfx::RectF
NativeThemeBase::BoundingRectForArrow(const gfx::Rect& rect) const {
It seems like this whole function just does something like the following (except
hopefully this is a little less inaccurate rounding-wise due to not ceil()ing
multiple times):

  std::pair<int> sides = std::minmax(rect.width(), rect.height());
  float side_length =
      std::min(static_cast<float>(sides.first), sides.second * 0.75f);
  gfx::RectF bounding_rect(rect.x() + (rect.width() - side_length) / 2,
                           rect.y() + (rect.height() - side_length) / 2,
                           side_length, side_length);
  return gfx::ToEnclosedRect(bounding_rect);

https://codereview.chromium.org/2009733002/diff/20001/ui/native_theme/native_...
File ui/native_theme/native_theme_base.h (right):

https://codereview.chromium.org/2009733002/diff/20001/ui/native_theme/native_...
ui/native_theme/native_theme_base.h:182: BoundingRectSnappedToWholePixels);
When there are this many FRIEND_TESTs, consider whether it would make sense to
have something like "friend class NativeThemeAuraTest", make that the base for
your tests, and give it public accessors that plumb through the relevant values
on this class to the individual testcases.

Whether this is better depends on how much of the surface area of this class
you're accessing.

[Bret, 2016-06-11 00:11:43]

I also added the testsuite to GN because I noticed it was missing.

https://codereview.chromium.org/2009733002/diff/20001/ui/native_theme/native_...
File ui/native_theme/native_theme_base.cc (left):

https://codereview.chromium.org/2009733002/diff/20001/ui/native_theme/native_...
ui/native_theme/native_theme_base.cc:388: path.offset(0, -1);
On 2016/06/10 01:03:23, Peter Kasting wrote:
> What were these offsets for?

Before this change the bounding box was an integer Rect. When it got the center
point (line 375 above) it would be rounded, so down and left arrows were
slightly off center. But since I changed the bounding box to a RectF, that
problem went away.

https://codereview.chromium.org/2009733002/diff/20001/ui/native_theme/native_...
File ui/native_theme/native_theme_base.cc (right):

https://codereview.chromium.org/2009733002/diff/20001/ui/native_theme/native_...
ui/native_theme/native_theme_base.cc:402: gfx::RectF
NativeThemeBase::BoundingRectForArrow(const gfx::Rect& rect) const {
On 2016/06/10 01:03:23, Peter Kasting wrote:
> It seems like this whole function just does something like the following
(except
> hopefully this is a little less inaccurate rounding-wise due to not ceil()ing
> multiple times):
> 
>   std::pair<int> sides = std::minmax(rect.width(), rect.height());
>   float side_length =
>       std::min(static_cast<float>(sides.first), sides.second * 0.75f);
>   gfx::RectF bounding_rect(rect.x() + (rect.width() - side_length) / 2,
>                            rect.y() + (rect.height() - side_length) / 2,
>                            side_length, side_length);
>   return gfx::ToEnclosedRect(bounding_rect);

I managed to synthesize your suggestion with what I want and I think it ended up
fairly nice. The fact that we do the extra ceils is important to maintain the
same look when the buttons are square.

https://codereview.chromium.org/2009733002/diff/20001/ui/native_theme/native_...
File ui/native_theme/native_theme_base.h (right):

https://codereview.chromium.org/2009733002/diff/20001/ui/native_theme/native_...
ui/native_theme/native_theme_base.h:182: BoundingRectSnappedToWholePixels);
On 2016/06/10 01:03:23, Peter Kasting wrote:
> When there are this many FRIEND_TESTs, consider whether it would make sense to
> have something like "friend class NativeThemeAuraTest", make that the base for
> your tests, and give it public accessors that plumb through the relevant
values
> on this class to the individual testcases.
> 
> Whether this is better depends on how much of the surface area of this class
> you're accessing.

Since I only need PathForArrow and BoundRectForArrow I made a friend class like
you suggested.

[Peter Kasting, 2016-06-11 00:14:48]

pkasting@chromium.org changed reviewers:
- pkasting@google.com

[Peter Kasting, 2016-06-11 02:19:27]

LGTM

https://codereview.chromium.org/2009733002/diff/40001/ui/native_theme/native_...
File ui/native_theme/native_theme_base.cc (right):

https://codereview.chromium.org/2009733002/diff/40001/ui/native_theme/native_...
ui/native_theme/native_theme_base.cc:414:
bounding_rect.set_y(ceil(bounding_rect.y()));
After thinking about this for a while, I think the ceil() calls here are
equivalent to round() calls.  This is because the X and Y coordinates are either
on .0 or .5.  Right?

Also, it seems odd to return a RectF here when we want an int-snapped rect, just
because the caller wants to call CenterPoint (*).  That's sort of a caller-side
detail.

I might do this:

  ...
  const int side_length = ...;
  // When there are an odd number of pixels, put the extra on the top/left.
  return gfx::Rect(rect.x() + (rect.width() - side_length + 1) / 2,
                   rect.y() + (rect.height() - side_length + 1) / 2,
                   side_length, side_length);
}

This is not only shorter, the integer division is a lot clearer to me about what
kind of rounding can practically end up occurring.  Maybe that's just me.

Then on the caller side:

  const gfx::PointF center = gfx::RectF(bounding_rect).CenterPoint();

(*) One other complication is the "rLineTo(... / 2.0f)" calls.  It seems like
the right thing ought to be doable with ints here, but I'm not sure.

[Bret, 2016-06-13 18:02:51]

https://codereview.chromium.org/2009733002/diff/40001/ui/native_theme/native_...
File ui/native_theme/native_theme_base.cc (right):

https://codereview.chromium.org/2009733002/diff/40001/ui/native_theme/native_...
ui/native_theme/native_theme_base.cc:414:
bounding_rect.set_y(ceil(bounding_rect.y()));
On 2016/06/11 02:19:27, Peter Kasting wrote:
> After thinking about this for a while, I think the ceil() calls here are
> equivalent to round() calls.  This is because the X and Y coordinates are
either
> on .0 or .5.  Right?
> 
> Also, it seems odd to return a RectF here when we want an int-snapped rect,
just
> because the caller wants to call CenterPoint (*).  That's sort of a
caller-side
> detail.
> 
> I might do this:
> 
>   ...
>   const int side_length = ...;
>   // When there are an odd number of pixels, put the extra on the top/left.
>   return gfx::Rect(rect.x() + (rect.width() - side_length + 1) / 2,
>                    rect.y() + (rect.height() - side_length + 1) / 2,
>                    side_length, side_length);
> }
> 
> This is not only shorter, the integer division is a lot clearer to me about
what
> kind of rounding can practically end up occurring.  Maybe that's just me.
> 
> Then on the caller side:
> 
>   const gfx::PointF center = gfx::RectF(bounding_rect).CenterPoint();
> 
> (*) One other complication is the "rLineTo(... / 2.0f)" calls.  It seems like
> the right thing ought to be doable with ints here, but I'm not sure.

Ok, I like this suggestion.

[Bret, 2016-06-13 18:03:01]

The CQ bit was checked by bsep@chromium.org

[Bret, 2016-06-13 18:03:02]

The patchset sent to the CQ was uploaded after l-g-t-m from
pkasting@chromium.org
Link to the patchset: https://codereview.chromium.org/2009733002/#ps60001
(title: "no floats allowed")

[commit-bot: I haz the power, 2016-06-13 18:03:51]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2009733002/60001

[commit-bot: I haz the power, 2016-06-13 21:01:31]

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

[commit-bot: I haz the power, 2016-06-13 21:01:32]

Try jobs failed on following builders:
  linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Bret, 2016-06-13 22:16:27]

The CQ bit was checked by bsep@chromium.org

[commit-bot: I haz the power, 2016-06-13 22:17:03]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2009733002/60001

[commit-bot: I haz the power, 2016-06-14 00:52:04]

Description was changed from

==========
Draw nicer arrows when the scrollbar buttons are not square.

Before they would look squished or messed up, but with this patch
their bounding box is always square so we'll get the nice pixel pyramids
that we want. This shouldn't change anything visually for the normal
square scrollbar buttons.

BUG=614297
==========

to

==========
Draw nicer arrows when the scrollbar buttons are not square.

Before they would look squished or messed up, but with this patch
their bounding box is always square so we'll get the nice pixel pyramids
that we want. This shouldn't change anything visually for the normal
square scrollbar buttons.

BUG=614297
==========

[commit-bot: I haz the power, 2016-06-14 00:52:07]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-06-14 00:52:24]

CQ bit was unchecked

[commit-bot: I haz the power, 2016-06-14 00:53:36]

Description was changed from

==========
Draw nicer arrows when the scrollbar buttons are not square.

Before they would look squished or messed up, but with this patch
their bounding box is always square so we'll get the nice pixel pyramids
that we want. This shouldn't change anything visually for the normal
square scrollbar buttons.

BUG=614297
==========

to

==========
Draw nicer arrows when the scrollbar buttons are not square.

Before they would look squished or messed up, but with this patch
their bounding box is always square so we'll get the nice pixel pyramids
that we want. This shouldn't change anything visually for the normal
square scrollbar buttons.

BUG=614297

Committed: https://crrev.com/91cbc6d7c2f02771ee3640a4b2f3376029b3bab0
Cr-Commit-Position: refs/heads/master@{#399626}
==========

[commit-bot: I haz the power, 2016-06-14 00:53:37]

Patchset 4 (id:??) landed as
https://crrev.com/91cbc6d7c2f02771ee3640a4b2f3376029b3bab0
Cr-Commit-Position: refs/heads/master@{#399626}