[Mac] Reverse bookmark buttons and menus in RTL (take 2)

[Mac] Reverse bookmark buttons and menus in RTL (take 2)

LTR image diffs show this as:
- Identical to HEAD in retina
- In non-retina, some bookmarks at the width limit show an additional character vs. HEAD. I think this is more true to the spec and is closer to Views (though it's hard to compare directly since views fades instead of cutting off with an ellipsis)

This is a semi-manual rebase of
https://codereview.chromium.org/2511973002/

BUG=648561, 648560
Review-Url: https://codereview.chromium.org/2845003003
Cr-Commit-Position: refs/heads/master@{#469157}
Committed: https://chromium.googlesource.com/chromium/src/+/e85d0513cdbaeeff936cc30f1d73ae9887d1df43

[lgrey, 2017-04-27 19:55:21]

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

[commit-bot: I haz the power, 2017-04-27 19:56:06]

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, 2017-04-27 20:22:18]

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

[commit-bot: I haz the power, 2017-04-27 20:22:18]

Dry run: This issue passed the CQ dry run.

[lgrey, 2017-05-02 18:08:04]

Description was changed from

==========
[Mac] Reverse bookmark buttons and menus in RTL (take 2)

WIP! TODO: Add screenshots to description after
testing on retina.

This is a semi-manual rebase of
https://codereview.chromium.org/2511973002/

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

to

==========
[Mac] Reverse bookmark buttons and menus in RTL (take 2)

LTR image diffs show this as:
- Identical to HEAD in retina
- In non-retina, some bookmarks at the width limit show an additional character
vs. HEAD. I think this is more true to the spec and is closer to Views (though
it's hard to compare directly since views fades instead of cutting off with an
ellipsis)

This is a semi-manual rebase of
https://codereview.chromium.org/2511973002/

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

[lgrey, 2017-05-02 18:08:59]

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

[lgrey, 2017-05-02 18:09:00]

PTAL :)

[Elly Fong-Jones, 2017-05-03 17:03:14]

lgtm

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

https://codereview.chromium.org/2845003003/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/bookmarks/bookmark_button_cell.mm:133: width +=
kIconLeadingPadding;
why is this leading padding twice? shouldn't it always be kTextTrailingPadding,
even if there is no text?

https://codereview.chromium.org/2845003003/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/bookmarks/bookmark_button_cell.mm:202: [self
setAlignment:NSNaturalTextAlignment];
why did this move? is it important that these be done in this order?

https://codereview.chromium.org/2845003003/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/bookmarks/bookmark_button_cell.mm:383: // Make buttons
without visible titles 20pts wide (18 plus padding).
please delete this comment! comments that duplicate constants like this are
basically pleading to become inaccurate

https://codereview.chromium.org/2845003003/diff/20001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/gradient_button_cell.mm (right):

https://codereview.chromium.org/2845003003/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/gradient_button_cell.mm:1: // Copyright (c) 2012 The
Chromium Authors. All rights reserved.
this might be a dumb question but now that we do MD top chrome on mac do we ever
actually draw gradients? if not can we get rid of this code?

https://codereview.chromium.org/2845003003/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/gradient_button_cell.mm:494: if (!controlView)
can that happen?

[shrike, 2017-05-03 17:35:55]

lgtm

https://codereview.chromium.org/2845003003/diff/20001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/l10n_util.h (right):

https://codereview.chromium.org/2845003003/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/l10n_util.h:55: // TODO(lgrey): Remove these when all
builds are on 10.12 SDK.
Can you add something like a check for min deployment target so that we get a
compilation warning or error when we're finally 10.12+ only?

[lgrey, 2017-05-03 21:32:48]

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

https://codereview.chromium.org/2845003003/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/bookmarks/bookmark_button_cell.mm:133: width +=
kIconLeadingPadding;
On 2017/05/03 17:03:14, Elly Fong-Jones wrote:
> why is this leading padding twice? shouldn't it always be
kTextTrailingPadding,
> even if there is no text?

Done.

https://codereview.chromium.org/2845003003/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/bookmarks/bookmark_button_cell.mm:202: [self
setAlignment:NSNaturalTextAlignment];
On 2017/05/03 17:03:14, Elly Fong-Jones wrote:
> why did this move? is it important that these be done in this order?

I think this was an accident, doesn't seem to make a difference. Reverted.

https://codereview.chromium.org/2845003003/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/bookmarks/bookmark_button_cell.mm:383: // Make buttons
without visible titles 20pts wide (18 plus padding).
On 2017/05/03 17:03:14, Elly Fong-Jones wrote:
> please delete this comment! comments that duplicate constants like this are
> basically pleading to become inaccurate

Done.

https://codereview.chromium.org/2845003003/diff/20001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/gradient_button_cell.mm (right):

https://codereview.chromium.org/2845003003/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/gradient_button_cell.mm:1: // Copyright (c) 2012 The
Chromium Authors. All rights reserved.
On 2017/05/03 17:03:14, Elly Fong-Jones wrote:
> this might be a dumb question but now that we do MD top chrome on mac do we
ever
> actually draw gradients? if not can we get rid of this code?

I'd like to but Find Bar still uses it. We discussed moving it over to MD as a
yak shave for this, but came down on the no side.

https://codereview.chromium.org/2845003003/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/gradient_button_cell.mm:494: if (!controlView)
On 2017/05/03 17:03:14, Elly Fong-Jones wrote:
> can that happen?

I think this was vestigial from a class method calling it. Removed.

https://codereview.chromium.org/2845003003/diff/20001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/l10n_util.h (right):

https://codereview.chromium.org/2845003003/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/l10n_util.h:55: // TODO(lgrey): Remove these when all
builds are on 10.12 SDK.
On 2017/05/03 17:35:55, shrike wrote:
> Can you add something like a check for min deployment target so that we get a
> compilation warning or error when we're finally 10.12+ only?

Done

[lgrey, 2017-05-03 21:32:58]

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

[commit-bot: I haz the power, 2017-05-03 21:34:32]

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, 2017-05-03 21:54:48]

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

[commit-bot: I haz the power, 2017-05-03 21:54:49]

Dry run: This issue passed the CQ dry run.

[lgrey, 2017-05-03 21:57:57]

The CQ bit was checked by lgrey@chromium.org

[lgrey, 2017-05-03 21:57:57]

The patchset sent to the CQ was uploaded after l-g-t-m from
ellyjones@chromium.org, shrike@chromium.org
Link to the patchset: https://codereview.chromium.org/2845003003/#ps40001
(title: "CL comments")

[commit-bot: I haz the power, 2017-05-03 21:58:26]

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, 2017-05-03 22:08:01]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1493848677295440,
"parent_rev": "5006a34633088cb6c63850b2bfd0df5ca8fb79f4", "commit_rev":
"e85d0513cdbaeeff936cc30f1d73ae9887d1df43"}

[commit-bot: I haz the power, 2017-05-03 22:08:11]

Description was changed from

==========
[Mac] Reverse bookmark buttons and menus in RTL (take 2)

LTR image diffs show this as:
- Identical to HEAD in retina
- In non-retina, some bookmarks at the width limit show an additional character
vs. HEAD. I think this is more true to the spec and is closer to Views (though
it's hard to compare directly since views fades instead of cutting off with an
ellipsis)

This is a semi-manual rebase of
https://codereview.chromium.org/2511973002/

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

to

==========
[Mac] Reverse bookmark buttons and menus in RTL (take 2)

LTR image diffs show this as:
- Identical to HEAD in retina
- In non-retina, some bookmarks at the width limit show an additional character
vs. HEAD. I think this is more true to the spec and is closer to Views (though
it's hard to compare directly since views fades instead of cutting off with an
ellipsis)

This is a semi-manual rebase of
https://codereview.chromium.org/2511973002/

BUG=648561, 648560

Review-Url: https://codereview.chromium.org/2845003003
Cr-Commit-Position: refs/heads/master@{#469157}
Committed:
https://chromium.googlesource.com/chromium/src/+/e85d0513cdbaeeff936cc30f1d73...
==========

[commit-bot: I haz the power, 2017-05-03 22:08:12]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/e85d0513cdbaeeff936cc30f1d73...