[Mac] Reverse the omnibox in RTL

[Mac] Reverse the omnibox in RTL

BUG=648554, 648557, 673362
Committed: https://crrev.com/753efdea7319cbdca59df1e1d7b458c4eaa3c479
Cr-Commit-Position: refs/heads/master@{#440891}

[lgrey, 2016-12-13 19:35:20]

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

[commit-bot: I haz the power, 2016-12-13 19:36:13]

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-13 20:09:37]

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

[commit-bot: I haz the power, 2016-12-13 20:09:38]

Dry run: This issue passed the CQ dry run.

[lgrey, 2016-12-13 20:51:41]

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

[lgrey, 2016-12-13 20:51:41]

PTAL :)

[Sidney San Martín, 2016-12-16 00:05:53]

Functional issues (I wish there was a good way to attach screenshots to CL
comments...):

- When I hover over the origin chip on a secure page, the background is
mis-aligned.

- The flag on a wide (secure) origin chip doesn't point at the lock icon.

- (Not sure if in scope for this CL?) Bubbles (origin info bubble, bookmark
bubble) are aligned in such a way that they fall off the edge of the window.

https://codereview.chromium.org/2576563002/diff/1/chrome/browser/ui/cocoa/loc...
File chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_cell.mm
(right):

https://codereview.chromium.org/2576563002/diff/1/chrome/browser/ui/cocoa/loc...
chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_cell.mm:39: // How
much the text frame needs to overlap the front-most leading
The use of “front” and “back” in this CL are confusing to me. Maybe “outermost”
and “innermost” would be more clear, or you can think of something even better?

https://codereview.chromium.org/2576563002/diff/1/chrome/browser/ui/cocoa/loc...
chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_cell.mm:118: // the
index of the first right-hand decoration.
This comment needs to be updated too.

https://codereview.chromium.org/2576563002/diff/1/chrome/browser/ui/cocoa/loc...
chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_cell.mm:129: BOOL
is_rtl = cocoa_l10n_util::ShouldDoExperimentalRTLLayout();
The large number of is_rtl checks in the CL smell funny to me, and it's hard for
me to keep track of when we're working in leading/trailing space versus
left/right space.

I would probably approach this by converting to "real" coordinates as late as
possible. For example, imagine adding this utility function:

    CGRect CGRectFlipXInRect(CGRect rect, CGRect inRect) {
      CGAffineTransform transform = CGAffineTransformScale(
          CGAffineTransformMakeTranslation(inRect.size.width, 0), -1, 1);
      return CGRectApplyAffineTransform(rect, transform);
    }

Instead of all of the new checks, you could then just have something like this
at the end of CalculatePositionInFrame:

    if (cocoa_l10n_util::ShouldDoExperimentalRTLLayout()) {
      for (NSRect& decorationFrame : *decoration_frames) {
        decorationFrame = CGRectFlipXInRect(decorationFrame, frame);
      }
      *text_frame = CGRectFlipXInRect(*text_frame, frame);
    }

Maybe there’s a way to do it even later, or a different approach. The one in
this CL just seems hard to reason about/maintain, though :/.

https://codereview.chromium.org/2576563002/diff/1/chrome/browser/ui/cocoa/loc...
chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_cell.mm:140: const
size_t left_count = decorations->size();
It was unexpected to see "left" pop up here. It looks like it's only used by one
caller (textCursorFrameForFrame:), and it's unclear if it's the correct behavior
for RTL. Is there any way to make that code direction-agnostic and/or get rid of
the need for this one-off return value? It's not a huge deal, though… I won't
push back if you think it's fine.

https://codereview.chromium.org/2576563002/diff/1/chrome/browser/ui/cocoa/loc...
chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_cell.mm:160:
frame.origin.x -= kTextFrameDecorationOverlap;
This is also a good example. It takes some thought to understand why this tweak
is skipped in RTL. If we were staying in leading/trailing space until later, it
wouldn't be needed.

https://codereview.chromium.org/2576563002/diff/1/chrome/browser/ui/cocoa/loc...
chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_cell.mm:427: BOOL
isRTL = cocoa_l10n_util::ShouldDoExperimentalRTLLayout();
is_rtl — but it would be nicer if it could be eliminated.

https://codereview.chromium.org/2576563002/diff/1/chrome/browser/ui/cocoa/loc...
File
chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_cell_unittest.mm
(right):

https://codereview.chromium.org/2576563002/diff/1/chrome/browser/ui/cocoa/loc...
chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_cell_unittest.mm:251:
// Trailing decorations are ordered from front to back..
Ditto — "outermost to innermost", or whatever you think makes more sense.

https://codereview.chromium.org/2576563002/diff/1/chrome/browser/ui/cocoa/loc...
chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_cell_unittest.mm:302:
class AutocompleteTextFieldCellTestRTL : public AutocompleteTextFieldCellTest {
Would it be possible to use the same basic test for LTR and RTL by using a
similar frame flipping strategy? i.e. lay everything out, then fetch the
interesting frames, flip them if in RTL mode, and then do sanity checks.

https://codereview.chromium.org/2576563002/diff/1/chrome/browser/ui/cocoa/loc...
File chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_editor.mm
(right):

https://codereview.chromium.org/2576563002/diff/1/chrome/browser/ui/cocoa/loc...
chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_editor.mm:99: :
NSLeftTextAlignment;
The docs suggest that NSNaturalTextAlignment would work for both… Does it?

https://developer.apple.com/library/content/documentation/MacOSX/Conceptual/B...

[Sidney San Martín, 2016-12-16 00:09:40]

https://codereview.chromium.org/2576563002/diff/1/chrome/browser/ui/cocoa/loc...
File chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_cell.mm
(right):

https://codereview.chromium.org/2576563002/diff/1/chrome/browser/ui/cocoa/loc...
chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_cell.mm:39: // How
much the text frame needs to overlap the front-most leading
On 2016/12/16 00:05:54, Sidney San Martín wrote:
> The use of “front” and “back” in this CL are confusing to me. Maybe
“outermost”
> and “innermost” would be more clear, or you can think of something even
better?

Sorry, I didn't mean to imply "front" → "outermost". I just meant "front-most" →
"outermost".

[lgrey, 2016-12-20 19:41:04]

https://codereview.chromium.org/2576563002/diff/1/chrome/browser/ui/cocoa/loc...
File chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_cell.mm
(right):

https://codereview.chromium.org/2576563002/diff/1/chrome/browser/ui/cocoa/loc...
chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_cell.mm:39: // How
much the text frame needs to overlap the front-most leading
On 2016/12/16 00:05:54, Sidney San Martín wrote:
> The use of “front” and “back” in this CL are confusing to me. Maybe
“outermost”
> and “innermost” would be more clear, or you can think of something even
better?

Done.

https://codereview.chromium.org/2576563002/diff/1/chrome/browser/ui/cocoa/loc...
chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_cell.mm:118: // the
index of the first right-hand decoration.
On 2016/12/16 00:05:54, Sidney San Martín wrote:
> This comment needs to be updated too.

Done.

https://codereview.chromium.org/2576563002/diff/1/chrome/browser/ui/cocoa/loc...
chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_cell.mm:129: BOOL
is_rtl = cocoa_l10n_util::ShouldDoExperimentalRTLLayout();
On 2016/12/16 00:05:54, Sidney San Martín wrote:
> The large number of is_rtl checks in the CL smell funny to me, and it's hard
for
> me to keep track of when we're working in leading/trailing space versus
> left/right space.
> 
> I would probably approach this by converting to "real" coordinates as late as
> possible. For example, imagine adding this utility function:
> 
>     CGRect CGRectFlipXInRect(CGRect rect, CGRect inRect) {
>       CGAffineTransform transform = CGAffineTransformScale(
>           CGAffineTransformMakeTranslation(inRect.size.width, 0), -1, 1);
>       return CGRectApplyAffineTransform(rect, transform);
>     }
> 
> Instead of all of the new checks, you could then just have something like this
> at the end of CalculatePositionInFrame:
> 
>     if (cocoa_l10n_util::ShouldDoExperimentalRTLLayout()) {
>       for (NSRect& decorationFrame : *decoration_frames) {
>         decorationFrame = CGRectFlipXInRect(decorationFrame, frame);
>       }
>       *text_frame = CGRectFlipXInRect(*text_frame, frame);
>     }
> 
> Maybe there’s a way to do it even later, or a different approach. The one in
> this CL just seems hard to reason about/maintain, though :/.

Done.

https://codereview.chromium.org/2576563002/diff/1/chrome/browser/ui/cocoa/loc...
chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_cell.mm:140: const
size_t left_count = decorations->size();
On 2016/12/16 00:05:54, Sidney San Martín wrote:
> It was unexpected to see "left" pop up here. It looks like it's only used by
one
> caller (textCursorFrameForFrame:), and it's unclear if it's the correct
behavior
> for RTL. Is there any way to make that code direction-agnostic and/or get rid
of
> the need for this one-off return value? It's not a huge deal, though… I won't
> push back if you think it's fine.

Done.

https://codereview.chromium.org/2576563002/diff/1/chrome/browser/ui/cocoa/loc...
chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_cell.mm:160:
frame.origin.x -= kTextFrameDecorationOverlap;
On 2016/12/16 00:05:54, Sidney San Martín wrote:
> This is also a good example. It takes some thought to understand why this
tweak
> is skipped in RTL. If we were staying in leading/trailing space until later,
it
> wouldn't be needed.

Done.

https://codereview.chromium.org/2576563002/diff/1/chrome/browser/ui/cocoa/loc...
chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_cell.mm:427: BOOL
isRTL = cocoa_l10n_util::ShouldDoExperimentalRTLLayout();
On 2016/12/16 00:05:54, Sidney San Martín wrote:
> is_rtl — but it would be nicer if it could be eliminated.

Done.

https://codereview.chromium.org/2576563002/diff/1/chrome/browser/ui/cocoa/loc...
File
chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_cell_unittest.mm
(right):

https://codereview.chromium.org/2576563002/diff/1/chrome/browser/ui/cocoa/loc...
chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_cell_unittest.mm:251:
// Trailing decorations are ordered from front to back..
On 2016/12/16 00:05:54, Sidney San Martín wrote:
> Ditto — "outermost to innermost", or whatever you think makes more sense.

Done.

https://codereview.chromium.org/2576563002/diff/1/chrome/browser/ui/cocoa/loc...
chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_cell_unittest.mm:302:
class AutocompleteTextFieldCellTestRTL : public AutocompleteTextFieldCellTest {
On 2016/12/16 00:05:54, Sidney San Martín wrote:
> Would it be possible to use the same basic test for LTR and RTL by using a
> similar frame flipping strategy? i.e. lay everything out, then fetch the
> interesting frames, flip them if in RTL mode, and then do sanity checks.

I think this has some practical issues (RTL scoping needs to happen before the
views load) and theoretical issues (if I'm doing work to massage data
pre-assertion, how do I know my tests are correct? Also, increased complexity.)

https://codereview.chromium.org/2576563002/diff/1/chrome/browser/ui/cocoa/loc...
File chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_editor.mm
(right):

https://codereview.chromium.org/2576563002/diff/1/chrome/browser/ui/cocoa/loc...
chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_editor.mm:99: :
NSLeftTextAlignment;
On 2016/12/16 00:05:54, Sidney San Martín wrote:
> The docs suggest that NSNaturalTextAlignment would work for both… Does it?
> 
>
https://developer.apple.com/library/content/documentation/MacOSX/Conceptual/B...

At least in forced RTL, this doesn't hold for the empty string.  OTOH, it looks
like crrev.com/2555783002 was sufficient for changing this by setting the
field's alignment (vs. the editors), so reverting.

[Sidney San Martín, 2016-12-21 22:52:03]

https://codereview.chromium.org/2576563002/diff/20001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_editor.mm
(right):

https://codereview.chromium.org/2576563002/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_editor.mm:14:
#include "chrome/browser/ui/cocoa/l10n_util.h"
Still needed?

[Sidney San Martín, 2016-12-21 23:15:49]

lgtm with functional nit.

As discussed, there's a rendering issue with some decorations which you have a
fix for.

[lgrey, 2016-12-21 23:29:47]

On 2016/12/21 23:15:49, sdy (OOO Dec. 23-31) wrote:
> lgtm with functional nit.
> 
> As discussed, there's a rendering issue with some decorations which you have a
> fix for.

Fixed, thanks for catching that!

[lgrey, 2016-12-28 17:28:41]

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

[lgrey, 2016-12-28 17:28:42]

Avi, PTAL for OWNERS :)

https://codereview.chromium.org/2576563002/diff/20001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_editor.mm
(right):

https://codereview.chromium.org/2576563002/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_editor.mm:14:
#include "chrome/browser/ui/cocoa/l10n_util.h"
On 2016/12/21 22:52:03, sdy (OOO Dec. 23-31) wrote:
> Still needed?

Done.

[lgrey, 2016-12-28 17:28:43]

Avi, PTAL for OWNERS :)

[Avi (use Gerrit), 2016-12-28 17:58:59]

lgtm

Just nits.

https://codereview.chromium.org/2576563002/diff/40001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_cell.mm
(right):

https://codereview.chromium.org/2576563002/diff/40001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_cell.mm:131: //
Layout |leading_decorations| against the LHS.
LHS -> leading side?

https://codereview.chromium.org/2576563002/diff/40001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_cell.mm:147: //
Layout |trailing_decorations| against the RHS.
RHS -> trailing side?

https://codereview.chromium.org/2576563002/diff/40001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_cell.mm:158: //
Flip all frames in RTL.
Blank line before this to start a new commented block.

https://codereview.chromium.org/2576563002/diff/40001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_cell.mm:166: return
leading_count;
This return isn't part of the comment above, so give it a blank line.

[lgrey, 2016-12-28 20:07:58]

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

[commit-bot: I haz the power, 2016-12-28 20:08:15]

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

[lgrey, 2016-12-28 20:15:02]

Thanks!

https://codereview.chromium.org/2576563002/diff/40001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_cell.mm
(right):

https://codereview.chromium.org/2576563002/diff/40001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_cell.mm:131: //
Layout |leading_decorations| against the LHS.
On 2016/12/28 17:58:59, Avi wrote:
> LHS -> leading side?

Done.

https://codereview.chromium.org/2576563002/diff/40001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_cell.mm:147: //
Layout |trailing_decorations| against the RHS.
On 2016/12/28 17:58:59, Avi wrote:
> RHS -> trailing side?

Done.

https://codereview.chromium.org/2576563002/diff/40001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_cell.mm:158: //
Flip all frames in RTL.
On 2016/12/28 17:58:59, Avi wrote:
> Blank line before this to start a new commented block.

Done.

https://codereview.chromium.org/2576563002/diff/40001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_cell.mm:166: return
leading_count;
On 2016/12/28 17:58:59, Avi wrote:
> This return isn't part of the comment above, so give it a blank line.

Done.

[lgrey, 2016-12-28 20:25:39]

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

[commit-bot: I haz the power, 2016-12-28 20:25: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-12-28 20:54:21]

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

[commit-bot: I haz the power, 2016-12-28 20:54:22]

Dry run: This issue passed the CQ dry run.

[lgrey, 2016-12-28 20:54:58]

The CQ bit was checked by lgrey@chromium.org

[lgrey, 2016-12-28 20:54:58]

The patchset sent to the CQ was uploaded after l-g-t-m from sdy@chromium.org,
avi@chromium.org
Link to the patchset: https://codereview.chromium.org/2576563002/#ps60001
(title: "Review comments")

[commit-bot: I haz the power, 2016-12-28 20:55:16]

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-28 20:59:24]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1482958498024360,
"parent_rev": "db9a4f07703590697e6aacbb68b01f5651e65e9f", "commit_rev":
"2d0076a76b69943eaac34fd38ecf7313912ccda4"}

[commit-bot: I haz the power, 2016-12-28 20:59:51]

Description was changed from

==========
[Mac] Reverse the omnibox in RTL

BUG=648554, 648557, 673362
==========

to

==========
[Mac] Reverse the omnibox in RTL

BUG=648554, 648557, 673362

Review-Url: https://codereview.chromium.org/2576563002
==========

[commit-bot: I haz the power, 2016-12-28 20:59:52]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2017-01-02 15:50:15]

Description was changed from

==========
[Mac] Reverse the omnibox in RTL

BUG=648554, 648557, 673362

Review-Url: https://codereview.chromium.org/2576563002
==========

to

==========
[Mac] Reverse the omnibox in RTL

BUG=648554, 648557, 673362

Committed: https://crrev.com/753efdea7319cbdca59df1e1d7b458c4eaa3c479
Cr-Commit-Position: refs/heads/master@{#440891}
==========

[commit-bot: I haz the power, 2017-01-02 15:50:17]

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