[Mac] Reverse tabs in RTL mode

[Mac] Reverse tabs in RTL mode

Also exposes the kExperimentalMacRTL flag publicly so that it can be
enabled in tests.

BUG=648559
Committed: https://crrev.com/052b36d8d021d293f9cbd8c32b1ddee5c99ade8d
Cr-Commit-Position: refs/heads/master@{#431004}

[lgrey, 2016-11-08 21:11:00]

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

[lgrey, 2016-11-08 21:11:01]

Elly PTAL :)

[Elly Fong-Jones, 2016-11-09 15:11:36]

lgtm :)

Nice work!

[lgrey, 2016-11-09 15:19:28]

Thanks, Elly!

Nico PTAL for owners

[Nico, 2016-11-09 15:27:34]

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

[Nico, 2016-11-09 15:27:35]

lgtm, thanks.

If I read this right, this makes it so that the 0th tab is now on the right, the
first to the left of the 0th, etc, yes? So cmd-1 will select the rightmost tab,
cmd-2 the second-from-right, and cmd-9 the leftmost? If so, is that what we
want? (What does RTL chrome on Windows do, what does Safari do?)

https://codereview.chromium.org/2488833002/diff/1/chrome/browser/ui/cocoa/tab...
File chrome/browser/ui/cocoa/tabs/tab_controller.mm (right):

https://codereview.chromium.org/2488833002/diff/1/chrome/browser/ui/cocoa/tab...
chrome/browser/ui/cocoa/tabs/tab_controller.mm:124: ? NSViewMaxXMargin |
NSViewMinYMargin
why is the y autoresizing mask different in rtl mode?

[lgrey, 2016-11-09 15:36:34]

On 2016/11/09 15:27:35, Nico wrote:
> lgtm, thanks.
> 
> If I read this right, this makes it so that the 0th tab is now on the right,
the
> first to the left of the 0th, etc, yes? So cmd-1 will select the rightmost
tab,
> cmd-2 the second-from-right, and cmd-9 the leftmost? If so, is that what we
> want? (What does RTL chrome on Windows do, what does Safari do?)

Nope, that part is already done by Elly a while back
(https://codereview.chromium.org/2313723004/). FWIW this is how Safari and Linux
Chrome behave (not sure the best way to check Windows).

This reverses the layout of individual tabs to put the icon on the right and the
alert/close buttons on the left, hence the autoresizing mask change you asked
about.

[Nico, 2016-11-09 15:44:07]

But why does autosizing in Y change? LTR should only affect X?

On Wed, Nov 9, 2016 at 10:36 AM, <lgrey@chromium.org> wrote:

> On 2016/11/09 15:27:35, Nico wrote:
> > lgtm, thanks.
> >
> > If I read this right, this makes it so that the 0th tab is now on the
> right,
> the
> > first to the left of the 0th, etc, yes? So cmd-1 will select the
> rightmost
> tab,
> > cmd-2 the second-from-right, and cmd-9 the leftmost? If so, is that what
> we
> > want? (What does RTL chrome on Windows do, what does Safari do?)
>
> Nope, that part is already done by Elly a while back
> (https://codereview.chromium.org/2313723004/). FWIW this is how Safari
> and Linux
> Chrome behave (not sure the best way to check Windows).
>
> This reverses the layout of individual tabs to put the icon on the right
> and the
> alert/close buttons on the left, hence the autoresizing mask change you
> asked
> about.
>
>
> https://codereview.chromium.org/2488833002/
>

-- 
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.

[lgrey, 2016-11-09 15:48:51]

https://codereview.chromium.org/2488833002/diff/1/chrome/browser/ui/cocoa/tab...
File chrome/browser/ui/cocoa/tabs/tab_controller.mm (right):

https://codereview.chromium.org/2488833002/diff/1/chrome/browser/ui/cocoa/tab...
chrome/browser/ui/cocoa/tabs/tab_controller.mm:124: ? NSViewMaxXMargin |
NSViewMinYMargin
On 2016/11/09 15:27:34, Nico wrote:
> why is the y autoresizing mask different in rtl mode?

It doesn't. I decided (kind of arbitrarily), to flip the entire masks on the
leftmost elements (see line 104) vs. just flipping the X. To be honest, I'm not
really sure why the Y is necessary: do tabs ever change height?

Happy to change it to keep the Y mask on the icon only if you think that's
better

[Nico, 2016-11-09 15:52:29]

https://codereview.chromium.org/2488833002/diff/1/chrome/browser/ui/cocoa/tab...
File chrome/browser/ui/cocoa/tabs/tab_controller.mm (right):

https://codereview.chromium.org/2488833002/diff/1/chrome/browser/ui/cocoa/tab...
chrome/browser/ui/cocoa/tabs/tab_controller.mm:124: ? NSViewMaxXMargin |
NSViewMinYMargin
On 2016/11/09 15:48:51, lgrey wrote:
> On 2016/11/09 15:27:34, Nico wrote:
> > why is the y autoresizing mask different in rtl mode?
> 
> It doesn't. I decided (kind of arbitrarily), to flip the entire masks on the
> leftmost elements (see line 104) vs. just flipping the X. To be honest, I'm
not
> really sure why the Y is necessary: do tabs ever change height?

No.
 
> Happy to change it to keep the Y mask on the icon only if you think that's
> better

I don't understand why the change was made (and if it's made, why it's only made
in LTR mode). If there's a good reason for that change, I'm fine with keeping
it, I'd just like to understand why it was done.

[lgrey, 2016-11-09 16:17:54]

On 2016/11/09 15:52:29, Nico wrote:
>
https://codereview.chromium.org/2488833002/diff/1/chrome/browser/ui/cocoa/tab...
> File chrome/browser/ui/cocoa/tabs/tab_controller.mm (right):
> 
>
https://codereview.chromium.org/2488833002/diff/1/chrome/browser/ui/cocoa/tab...
> chrome/browser/ui/cocoa/tabs/tab_controller.mm:124: ? NSViewMaxXMargin |
> NSViewMinYMargin
> On 2016/11/09 15:48:51, lgrey wrote:
> > On 2016/11/09 15:27:34, Nico wrote:
> > > why is the y autoresizing mask different in rtl mode?
> > 
> > It doesn't. I decided (kind of arbitrarily), to flip the entire masks on the
> > leftmost elements (see line 104) vs. just flipping the X. To be honest, I'm
> not
> > really sure why the Y is necessary: do tabs ever change height?
> 
> No.
>  
> > Happy to change it to keep the Y mask on the icon only if you think that's
> > better
> 
> I don't understand why the change was made (and if it's made, why it's only
made
> in LTR mode). If there's a good reason for that change, I'm fine with keeping
> it, I'd just like to understand why it was done.

Done. 

To summarize offline discussion for posterity, the Y mask is necessary because
the icon size varies and the close button is static

[lgrey, 2016-11-09 16:19:12]

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

[commit-bot: I haz the power, 2016-11-09 16:19:30]

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

[Nico, 2016-11-09 16:23:36]

I meant it the other way: Favicons are always 16px, while the button might
be some other size.

On Wed, Nov 9, 2016 at 11:17 AM, <lgrey@chromium.org> wrote:

> On 2016/11/09 15:52:29, Nico wrote:
> >
> https://codereview.chromium.org/2488833002/diff/1/chrome/
> browser/ui/cocoa/tabs/tab_controller.mm
> > File chrome/browser/ui/cocoa/tabs/tab_controller.mm (right):
> >
> >
> https://codereview.chromium.org/2488833002/diff/1/chrome/
> browser/ui/cocoa/tabs/tab_controller.mm#newcode124
> > chrome/browser/ui/cocoa/tabs/tab_controller.mm:124: ? NSViewMaxXMargin |
> > NSViewMinYMargin
> > On 2016/11/09 15:48:51, lgrey wrote:
> > > On 2016/11/09 15:27:34, Nico wrote:
> > > > why is the y autoresizing mask different in rtl mode?
> > >
> > > It doesn't. I decided (kind of arbitrarily), to flip the entire masks
> on the
> > > leftmost elements (see line 104) vs. just flipping the X. To be
> honest, I'm
> > not
> > > really sure why the Y is necessary: do tabs ever change height?
> >
> > No.
> >
> > > Happy to change it to keep the Y mask on the icon only if you think
> that's
> > > better
> >
> > I don't understand why the change was made (and if it's made, why it's
> only
> made
> > in LTR mode). If there's a good reason for that change, I'm fine with
> keeping
> > it, I'd just like to understand why it was done.
>
> Done.
>
> To summarize offline discussion for posterity, the Y mask is necessary
> because
> the icon size varies and the close button is static
>
> https://codereview.chromium.org/2488833002/
>

-- 
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.

[commit-bot: I haz the power, 2016-11-09 17:36:42]

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

[commit-bot: I haz the power, 2016-11-09 17:36:43]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[lgrey, 2016-11-09 18:46:34]

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

[commit-bot: I haz the power, 2016-11-09 18:47:08]

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

[lgrey, 2016-11-09 18:47:16]

PTAL again, fixed the autoresizing mask in one more place and changed default
locale for the RTL test

[Nico, 2016-11-09 19:00:25]

lgtm

[commit-bot: I haz the power, 2016-11-09 19:07:24]

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

[commit-bot: I haz the power, 2016-11-09 19:07:25]

Dry run: This issue passed the CQ dry run.

[lgrey, 2016-11-09 19:11:16]

The CQ bit was checked by lgrey@chromium.org

[lgrey, 2016-11-09 19:11:16]

The patchset sent to the CQ was uploaded after l-g-t-m from
ellyjones@chromium.org
Link to the patchset: https://codereview.chromium.org/2488833002/#ps60001
(title: "Update icon mask and add force RTL direction")

[lgrey, 2016-11-09 19:11:50]

Thanks, Nico!

[commit-bot: I haz the power, 2016-11-09 19:11:54]

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-09 19:18:25]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-11-09 19:44:41]

Description was changed from

==========
[Mac] Reverse tabs in RTL mode

Also exposes the kExperimentalMacRTL flag publicly so that it can be
enabled in tests.

BUG=648559
==========

to

==========
[Mac] Reverse tabs in RTL mode

Also exposes the kExperimentalMacRTL flag publicly so that it can be
enabled in tests.

BUG=648559

Committed: https://crrev.com/052b36d8d021d293f9cbd8c32b1ddee5c99ade8d
Cr-Commit-Position: refs/heads/master@{#431004}
==========

[commit-bot: I haz the power, 2016-11-09 19:44:43]

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