Reverse bookmark buttons and menus in RTL

Reverse bookmark buttons and menus in RTL

This looks a little odd to me when the text is LTR with the Material
Design ellipsis cut-off (doesn't seem like Views has adopted this
yet) but RTL text looks good.

Also includes a method to flip directional images (arrows, etc.)

BUG=648561, 648560

[lgrey, 2016-11-17 22:09:04]

lgrey@chromium.org changed reviewers:
+ ellyjones@chromium.org, shrike@chromium.org

[lgrey, 2016-11-17 22:09:04]

PTAL :)

[lgrey, 2016-11-17 22:09:36]

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

[commit-bot: I haz the power, 2016-11-17 22:10:55]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-17 23:36:06]

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

[commit-bot: I haz the power, 2016-11-17 23:36:06]

Dry run: This issue passed the CQ dry run.

[Elly Fong-Jones, 2016-11-18 18:42:52]

lgtm :)

[shrike, 2016-11-19 00:37:37]

The Apps item's title looks like it's too far to the right in LTR mode (the very
end appears to be clipped). When running in RTL it seems that this causes me to
get an ellipsis. I also have the About page bookmarked and it seems to have the
same ellipsis problem. It looks like the titles being too far to the right is a
regression.

Also, if you create a bookmark that has no title, there's too much space to the
right of the favicon in the mouse hover state.

[lgrey, 2016-11-28 15:42:34]

On 2016/11/19 00:37:37, shrike wrote:
> The Apps item's title looks like it's too far to the right in LTR mode (the
very
> end appears to be clipped). When running in RTL it seems that this causes me
to
> get an ellipsis. I also have the About page bookmarked and it seems to have
the
> same ellipsis problem. It looks like the titles being too far to the right is
a
> regression.
> 
> Also, if you create a bookmark that has no title, there's too much space to
the
> right of the favicon in the mouse hover state.

shrike@ I had some issues like the first two you mentioned during development,
but can't repro on desktop or laptop. What hardware/OS are you using?

(I do see the issue with a no title bookmark, fix coming soon)

[shrike, 2016-11-28 18:19:14]

On 2016/11/28 15:42:34, lgrey wrote:
> shrike@ I had some issues like the first two you mentioned during development,
> but can't repro on desktop or laptop. What hardware/OS are you using?
> 
> (I do see the issue with a no title bookmark, fix coming soon)

This is my desktop Mac with macOS 10.11. I will post a screenshot to 648561.

[lgrey, 2016-11-28 22:53:04]

On 2016/11/28 18:19:14, shrike wrote:
> On 2016/11/28 15:42:34, lgrey wrote:
> > shrike@ I had some issues like the first two you mentioned during
development,
> > but can't repro on desktop or laptop. What hardware/OS are you using?
> > 
> > (I do see the issue with a no title bookmark, fix coming soon)
> 
> This is my desktop Mac with macOS 10.11. I will post a screenshot to 648561.

Updated, PTAL

The underlying issue is that we were calculating the size of the cell based on
the 
contents, but the superclass draws it with a 1px inset. The current
implementation
ignores the trailing padding when drawing the text.

This change adds 2px to the calculated cell size to account for the 1px inset,
so it's
not pixel perfect with the current implementation, but IMO reflects the intent
of
the constants better. What do you think?

[Elly Fong-Jones, 2016-11-29 13:51:48]

On 2016/11/28 22:53:04, lgrey wrote:
> On 2016/11/28 18:19:14, shrike wrote:
> > On 2016/11/28 15:42:34, lgrey wrote:
> > > shrike@ I had some issues like the first two you mentioned during
> development,
> > > but can't repro on desktop or laptop. What hardware/OS are you using?
> > > 
> > > (I do see the issue with a no title bookmark, fix coming soon)
> > 
> > This is my desktop Mac with macOS 10.11. I will post a screenshot to 648561.
> 
> Updated, PTAL
> 
> The underlying issue is that we were calculating the size of the cell based on
> the 
> contents, but the superclass draws it with a 1px inset. The current
> implementation
> ignores the trailing padding when drawing the text.
> 
> This change adds 2px to the calculated cell size to account for the 1px inset,
> so it's
> not pixel perfect with the current implementation, but IMO reflects the intent
> of
> the constants better. What do you think?

lgtm :)

[lgrey, 2016-11-29 19:34:26]

On 2016/11/29 13:51:48, Elly Fong-Jones wrote:
> On 2016/11/28 22:53:04, lgrey wrote:
> > On 2016/11/28 18:19:14, shrike wrote:
> > > On 2016/11/28 15:42:34, lgrey wrote:
> > > > shrike@ I had some issues like the first two you mentioned during
> > development,
> > > > but can't repro on desktop or laptop. What hardware/OS are you using?
> > > > 
> > > > (I do see the issue with a no title bookmark, fix coming soon)
> > > 
> > > This is my desktop Mac with macOS 10.11. I will post a screenshot to
648561.
> > 
> > Updated, PTAL
> > 
> > The underlying issue is that we were calculating the size of the cell based
on
> > the 
> > contents, but the superclass draws it with a 1px inset. The current
> > implementation
> > ignores the trailing padding when drawing the text.
> > 
> > This change adds 2px to the calculated cell size to account for the 1px
inset,
> > so it's
> > not pixel perfect with the current implementation, but IMO reflects the
intent
> > of
> > the constants better. What do you think?
> 
> lgtm :)

shrike@ does this work for you?

[shrike, 2016-12-02 00:28:07]

https://codereview.chromium.org/2511973002/diff/20001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/bookmarks/bookmark_button_cell.mm (right):

https://codereview.chromium.org/2511973002/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/bookmarks/bookmark_button_cell.mm:369: cellSize.width +=
2;
The inset should be a value returned by an instance method, so that
BookmarkButtonCell can access it without needing to know anything about special
constants being used within in GradientButtonCell.

In GradientButtonCell the -inset method (or whatever you call it) should return
a kBlahBlahBlah - style constant rather than a raw int.

[lgrey, 2016-12-02 17:32:10]

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

[commit-bot: I haz the power, 2016-12-02 17:32:28]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-02 17:52:08]

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

[commit-bot: I haz the power, 2016-12-02 17:52:09]

Dry run: This issue passed the CQ dry run.

[lgrey, 2016-12-02 18:07:43]

https://codereview.chromium.org/2511973002/diff/20001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/bookmarks/bookmark_button_cell.mm (right):

https://codereview.chromium.org/2511973002/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/bookmarks/bookmark_button_cell.mm:369: cellSize.width +=
2;
On 2016/12/02 00:28:07, shrike wrote:
> The inset should be a value returned by an instance method, so that
> BookmarkButtonCell can access it without needing to know anything about
special
> constants being used within in GradientButtonCell.
> 
> In GradientButtonCell the -inset method (or whatever you call it) should
return
> a kBlahBlahBlah - style constant rather than a raw int.

Done. shrike@ PTAL

Turns out it's not actually a constant -- I got thrown by a local with a "k"
prefix in the relevant GradientButtonCell method (also renamed this for
posterity.)

[shrike, 2016-12-03 01:51:00]

On 2016/12/02 18:07:43, lgrey wrote:
> Done. shrike@ PTAL
> 
> Turns out it's not actually a constant -- I got thrown by a local with a "k"
> prefix in the relevant GradientButtonCell method (also renamed this for
> posterity.)

Not sure what's going on but if you bookmark the About page, the icon and title
are 1pt further to the right with your patch than without it. I don't know if
other bookmarks are affected (only the About bookmark stood out as looking odd
but maybe all are affected). I will e-mail you a screenshot.

[Elly Fong-Jones, 2017-04-26 13:40:03]

Description was changed from

==========
Reverse bookmark buttons and menus in RTL

This looks a little odd to me when the text is LTR with the Material
Design ellipsis cut-off (doesn't seem like Views has adopted this
yet) but RTL text looks good.

Also includes a method to flip directional images (arrows, etc.)

BUG=648561,648560
==========

to

==========
Reverse bookmark buttons and menus in RTL

This looks a little odd to me when the text is LTR with the Material
Design ellipsis cut-off (doesn't seem like Views has adopted this
yet) but RTL text looks good.

Also includes a method to flip directional images (arrows, etc.)

BUG=648561,648560
==========

[Elly Fong-Jones, 2017-04-26 13:40:03]

ellyjones@chromium.org changed reviewers:
- ellyjones@chromium.org

[lgrey, 2017-05-04 21:22:02]

On 2016/12/03 01:51:00, shrike wrote:
> On 2016/12/02 18:07:43, lgrey wrote:
> > Done. shrike@ PTAL
> > 
> > Turns out it's not actually a constant -- I got thrown by a local with a "k"
> > prefix in the relevant GradientButtonCell method (also renamed this for
> > posterity.)
> 
> Not sure what's going on but if you bookmark the About page, the icon and
title
> are 1pt further to the right with your patch than without it. I don't know if
> other bookmarks are affected (only the About bookmark stood out as looking odd
> but maybe all are affected). I will e-mail you a screenshot.

This was landed as crrev.com/2845003003

Deleting