[MD] simplify and unify some ink drop center calculations

[MD] simplify and unify some ink drop center calculations

BUG=none

Committed: https://crrev.com/8ed0887ad87008f19997c226384275d8c7718c95
Cr-Commit-Position: refs/heads/master@{#379596}

[Evan Stade, 2016-03-03 04:27:24]

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

[commit-bot: I haz the power, 2016-03-03 04:28:28]

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

[commit-bot: I haz the power, 2016-03-03 05:18:17]

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

[commit-bot: I haz the power, 2016-03-03 05:18:18]

Dry run: This issue passed the CQ dry run.

[varkha, 2016-03-03 05:51:47]

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

[varkha, 2016-03-03 05:51:48]

https://codereview.chromium.org/1761443003/diff/1/ui/views/controls/button/la...
File ui/views/controls/button/label_button.cc (right):

https://codereview.chromium.org/1761443003/diff/1/ui/views/controls/button/la...
ui/views/controls/button/label_button.cc:441: return GetText().empty() ?
image()->bounds().CenterPoint()
I think you need to do something like this:
  gfx::Point center = image()->bounds().CenterPoint();
  center.set_x(GetMirroredXInView(center.x()));
and then use |center| in place of |image()->bounds().CenterPoint()|.
Please try it with maximized and not maximized browser window with --lang=he as
well as English layout.

[varkha, 2016-03-03 06:11:22]

https://codereview.chromium.org/1761443003/diff/1/chrome/browser/ui/views/too...
File chrome/browser/ui/views/toolbar/app_menu_button.cc (left):

https://codereview.chromium.org/1761443003/diff/1/chrome/browser/ui/views/too...
chrome/browser/ui/views/toolbar/app_menu_button.cc:198:
(GetMirroredXWithWidthInView(0, visible_width) + visible_width) / 2,
I looked at it some more and I understand what happened here. This had a last
minute typo with the extra parentheses. This line was supposed to be simply:
  GetMirroredXWithWidthInView(0, visible_width) + visible_width / 2,
The calculation in back_button didn't have this problem. The mistake is only 1
pixel at 1x so I have missed it. Using 3x shows it nicely.

But doing this is even simpler (this is what I suggested in label_button.cc):
  gfx::Point center = image()->bounds().CenterPoint();
  center.set_x(GetMirroredXInView(center.x()));
  return center;

I have built it and without GetMirrorXxxxx calls RTL layout will be wrong.

[Evan Stade, 2016-03-03 18:14:40]

please hold off on reviewing this cl.

[Evan Stade, 2016-03-03 21:17:35]

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

[Evan Stade, 2016-03-03 21:17:35]

PTAL

[varkha, 2016-03-03 21:34:01]

LGTM, Thanks for doing this!

[Evan Stade, 2016-03-03 21:39:51]

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

[commit-bot: I haz the power, 2016-03-03 21:40:26]

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

[commit-bot: I haz the power, 2016-03-03 22:37:51]

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

[commit-bot: I haz the power, 2016-03-03 22:37:52]

Dry run: This issue passed the CQ dry run.

[sky, 2016-03-03 23:37:16]

LGTM

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

https://codereview.chromium.org/1761443003/diff/20001/ui/views/controls/butto...
ui/views/controls/button/label_button.cc:441: return GetText().empty() ?
image()->GetMirroredBounds().CenterPoint()
It's worth a comment as to why this uses GetMirroredBounds.

[Evan Stade, 2016-03-04 04:07:52]

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

https://codereview.chromium.org/1761443003/diff/20001/ui/views/controls/butto...
ui/views/controls/button/label_button.cc:441: return GetText().empty() ?
image()->GetMirroredBounds().CenterPoint()
On 2016/03/03 23:37:16, sky wrote:
> It's worth a comment as to why this uses GetMirroredBounds.

What comment content would you suggest? I sorta think the documentation of
GetMirroredBounds makes it obvious why it would be used in this case. Not sure
if you have a particular subtlety in mind that warrants clarifying.

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

LGTM

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

https://codereview.chromium.org/1761443003/diff/20001/ui/views/controls/butto...
ui/views/controls/button/label_button.cc:441: return GetText().empty() ?
image()->GetMirroredBounds().CenterPoint()
On 2016/03/04 04:07:52, Evan Stade wrote:
> On 2016/03/03 23:37:16, sky wrote:
> > It's worth a comment as to why this uses GetMirroredBounds.
> 
> What comment content would you suggest? I sorta think the documentation of
> GetMirroredBounds makes it obvious why it would be used in this case. Not sure
> if you have a particular subtlety in mind that warrants clarifying.

Ya, ok, fair enough.

[varkha, 2016-03-04 17:40:26]

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

https://codereview.chromium.org/1761443003/diff/20001/ui/views/controls/butto...
ui/views/controls/button/label_button.cc:441: return GetText().empty() ?
image()->GetMirroredBounds().CenterPoint()
What GetMirroredBounds does is indeed obvious here. There might still be room
for a comment here. In general case we want to target a center of a visible
button and an assumption made here is that a center of a visible portion of a
button is same as a center of the image when there is no text. Usually the image
is centered in a button but could be laid out differently when some inset is
reserved on a side to make a mouse target larger than a button.

[sky, 2016-03-04 18:38:49]

My request for a comment came from it not being overly obvious as to
why the code used GetMirrorBounds() vs bounds(). I get why you want
one vs the other, but it's not overly obvious why the mirroring in
this context.

On Fri, Mar 4, 2016 at 9:40 AM,  <varkha@chromium.org> wrote:
>
>
https://codereview.chromium.org/1761443003/diff/20001/ui/views/controls/butto...
> File ui/views/controls/button/label_button.cc (right):
>
>
https://codereview.chromium.org/1761443003/diff/20001/ui/views/controls/butto...
> ui/views/controls/button/label_button.cc:441: return GetText().empty() ?
> image()->GetMirroredBounds().CenterPoint()
> What GetMirroredBounds does is indeed obvious here. There might still be
> room for a comment here. In general case we want to target a center of a
> visible button and an assumption made here is that a center of a visible
> portion of a button is same as a center of the image when there is no
> text. Usually the image is centered in a button but could be laid out
> differently when some inset is reserved on a side to make a mouse target
> larger than a button.
>
> https://codereview.chromium.org/1761443003/

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[varkha, 2016-03-04 19:39:21]

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

https://codereview.chromium.org/1761443003/diff/20001/ui/views/controls/butto...
ui/views/controls/button/label_button.cc:441: return GetText().empty() ?
image()->GetMirroredBounds().CenterPoint()
Exactly, I would suggest something like:
"Some buttons have |bounds()| that expand past their visible bounds to make
targeting easier. Centering ink drop at the image center works for those buttons
but needs RTL mirroring to be positioned correctly."

[Evan Stade, 2016-03-07 18:06:22]

The CQ bit was checked by estade@chromium.org

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

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

[commit-bot: I haz the power, 2016-03-07 18:52:21]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-03-07 18:56:01]

Description was changed from

==========
[MD] simplify and unify some ink drop center calculations

BUG=none
==========

to

==========
[MD] simplify and unify some ink drop center calculations

BUG=none

Committed: https://crrev.com/8ed0887ad87008f19997c226384275d8c7718c95
Cr-Commit-Position: refs/heads/master@{#379596}
==========

[commit-bot: I haz the power, 2016-03-07 18:56:02]

Patchset 2 (id:??) landed as
https://crrev.com/8ed0887ad87008f19997c226384275d8c7718c95
Cr-Commit-Position: refs/heads/master@{#379596}