Mac (Cocoa) Omnibox: Force text field to LTR context if it is a URL.

Mac (Cocoa) Omnibox: Force text field to LTR context if it is a URL.

This means that URLs will be displayed in a left-to-right paragraph
context. Right-to-left runs are still rendered RTL, but will not flip
the whole URL around. For example (if "ABC" is Hebrew), this will render
"ABC.com" as "CBA.com", rather than "com.CBA".

Affects main text field and suggestions, but not non-URL search text.
Complies with RFC 3987 Section 4.1 and brings the Mac Omnibox in line
with the existing behaviour on Views and Android.

BUG=624213
Committed: https://crrev.com/05baf795316dd430afbd79ee915187bec6bf5f5d
Cr-Commit-Position: refs/heads/master@{#406791}

[Matt Giuca, 2016-07-20 08:42:33]

Description was changed from

==========
Cocoa Omnibox: Set writing direction to LTR.

BUG=624213
==========

to

==========
Mac (Cocoa) Omnibox: Force text field to LTR context if it is a URL.

This means that URLs will be displayed in a left-to-right paragraph
context. Right-to-left runs are still rendered RTL, but will not flip
the whole URL around. For example (if "ABC" is Hebrew), this will render
"ABC.com" as "CBA.com", rather than "com.CBA".

Affects main text field and suggestions, but not non-URL search text.
Complies with RFC 3987 Section 4.1 and brings the Mac Omnibox in line
with the existing behaviour on Views and Android.

BUG=624213
==========

[Matt Giuca, 2016-07-20 08:42:47]

Description was changed from

==========
Mac (Cocoa) Omnibox: Force text field to LTR context if it is a URL.

This means that URLs will be displayed in a left-to-right paragraph
context. Right-to-left runs are still rendered RTL, but will not flip
the whole URL around. For example (if "ABC" is Hebrew), this will render
"ABC.com" as "CBA.com", rather than "com.CBA".

Affects main text field and suggestions, but not non-URL search text.
Complies with RFC 3987 Section 4.1 and brings the Mac Omnibox in line
with the existing behaviour on Views and Android.

BUG=624213
==========

to

==========
Mac (Cocoa) Omnibox: Force text field to LTR context if it is a URL.

This means that URLs will be displayed in a left-to-right paragraph
context. Right-to-left runs are still rendered RTL, but will not flip
the whole URL around. For example (if "ABC" is Hebrew), this will render
"ABC.com" as "CBA.com", rather than "com.CBA".

Affects main text field and suggestions, but not non-URL search text.
Complies with RFC 3987 Section 4.1 and brings the Mac Omnibox in line
with the existing behaviour on Views and Android.

BUG=624213
==========

[Matt Giuca, 2016-07-21 02:03:15]

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

[commit-bot: I haz the power, 2016-07-21 02:03:38]

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

[Matt Giuca, 2016-07-21 02:04:18]

mgiuca@chromium.org changed reviewers:
+ tapted@chromium.org

[commit-bot: I haz the power, 2016-07-21 02:43:53]

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

[commit-bot: I haz the power, 2016-07-21 02:43:54]

Dry run: This issue passed the CQ dry run.

[tapted, 2016-07-21 04:37:05]

https://codereview.chromium.org/2126023003/diff/60001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.mm (right):

https://codereview.chromium.org/2126023003/diff/60001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.mm:155: void
SetTextDirectionForRange(NSMutableAttributedString* attributedString,
This should be something like SetURLTextDirection, and the comment below should
be attached to the function. E.g. there's no point passing in |direction| if
it's always NSWritingDirectionLeftToRight, and it's not really clear why this
function exists without that comment

https://codereview.chromium.org/2126023003/diff/60001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.mm:164: }
nit: any reason to have this declared up here, rather than just above
CreateClassifiedAttributedString?

https://codereview.chromium.org/2126023003/diff/60001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm (right):

https://codereview.chromium.org/2126023003/diff/60001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm:566: if (force_ltr)
optional: I'd probably just use model()->CurrentTextIsURL() here, but this has
merit too

[Matt Giuca, 2016-07-21 04:55:12]

Patchset #5 (id:80001) has been deleted

[Matt Giuca, 2016-07-21 04:55:37]

https://codereview.chromium.org/2126023003/diff/60001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.mm (right):

https://codereview.chromium.org/2126023003/diff/60001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.mm:155: void
SetTextDirectionForRange(NSMutableAttributedString* attributedString,
On 2016/07/21 04:37:05, tapted wrote:
> This should be something like SetURLTextDirection

It isn't necessarily setting the text direction for a URL (it operates on an
arbitrary text). That it is only ever used on URLs is irrelevant to this
function, and potentially confusing (calling it "SetURLTextDirection" might
imply it does nothing if it is not a URL). I'd prefer to not use the word URL
here.

> and the comment below should
> be attached to the function.

That's kind of the same thing as the naming: this function is not specific to
URLs and it just looks confusing to me to have a function that is technically
generic, but named and documented after its intended usage, rather than what it
actually does (which is set the writing direction).

I added a comment, but it doesn't mention URLs.

> E.g. there's no point passing in |direction| if
> it's always NSWritingDirectionLeftToRight

I have a |direction| argument because it seems unnecessarily specific to have a
function that sets it to LTR when this can easily be generalized.

> and it's not really clear why this
> function exists without that comment

I suppose this could go the other way and be FormatTextAsURL, but then I'd
expect it to do all the other things to format like a URL (like making it blue
or whatever). I think it's better to not make this a function about formatting
URLs and have it be about setting the text direction of an AttributedString.

https://codereview.chromium.org/2126023003/diff/60001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.mm:164: }
On 2016/07/21 04:37:05, tapted wrote:
> nit: any reason to have this declared up here, rather than just above
> CreateClassifiedAttributedString?

Well kind of along similar lines, it isn't specific to
CreateClassifiedAttributedString, just gets used there. I can move it there if
you like.

https://codereview.chromium.org/2126023003/diff/60001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm (right):

https://codereview.chromium.org/2126023003/diff/60001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm:566: if (force_ltr)
On 2016/07/21 04:37:05, tapted wrote:
> optional: I'd probably just use model()->CurrentTextIsURL() here, but this has
> merit too

Done.

[tapted, 2016-07-21 05:11:07]

lgtm - I'm still confused why we'd need to ever call the method with
NSWritingDirectionRightToLeft, or for something other than an URL, but it's not
worth a debate :p

[Matt Giuca, 2016-07-21 05:54:51]

On 2016/07/21 05:11:07, tapted wrote:
> lgtm - I'm still confused why we'd need to ever call the method with
> NSWritingDirectionRightToLeft, or for something other than an URL, but it's
not
> worth a debate :p

We don't *need* to call the method, but in principle I think methods should
describe what they *do*, not what they happen to currently be used for
(otherwise, someone comes along and uses it for something else, and then the
documentation magically is out of date). And the direction argument is not
necessary but may as well be there. Anyway thanks!

[Matt Giuca, 2016-07-21 05:54:55]

The CQ bit was checked by mgiuca@chromium.org

[commit-bot: I haz the power, 2016-07-21 05:55:18]

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-07-21 07:07:05]

Description was changed from

==========
Mac (Cocoa) Omnibox: Force text field to LTR context if it is a URL.

This means that URLs will be displayed in a left-to-right paragraph
context. Right-to-left runs are still rendered RTL, but will not flip
the whole URL around. For example (if "ABC" is Hebrew), this will render
"ABC.com" as "CBA.com", rather than "com.CBA".

Affects main text field and suggestions, but not non-URL search text.
Complies with RFC 3987 Section 4.1 and brings the Mac Omnibox in line
with the existing behaviour on Views and Android.

BUG=624213
==========

to

==========
Mac (Cocoa) Omnibox: Force text field to LTR context if it is a URL.

This means that URLs will be displayed in a left-to-right paragraph
context. Right-to-left runs are still rendered RTL, but will not flip
the whole URL around. For example (if "ABC" is Hebrew), this will render
"ABC.com" as "CBA.com", rather than "com.CBA".

Affects main text field and suggestions, but not non-URL search text.
Complies with RFC 3987 Section 4.1 and brings the Mac Omnibox in line
with the existing behaviour on Views and Android.

BUG=624213
==========

[commit-bot: I haz the power, 2016-07-21 07:07:06]

Committed patchset #5 (id:100001)

[commit-bot: I haz the power, 2016-07-21 07:10:43]

Description was changed from

==========
Mac (Cocoa) Omnibox: Force text field to LTR context if it is a URL.

This means that URLs will be displayed in a left-to-right paragraph
context. Right-to-left runs are still rendered RTL, but will not flip
the whole URL around. For example (if "ABC" is Hebrew), this will render
"ABC.com" as "CBA.com", rather than "com.CBA".

Affects main text field and suggestions, but not non-URL search text.
Complies with RFC 3987 Section 4.1 and brings the Mac Omnibox in line
with the existing behaviour on Views and Android.

BUG=624213
==========

to

==========
Mac (Cocoa) Omnibox: Force text field to LTR context if it is a URL.

This means that URLs will be displayed in a left-to-right paragraph
context. Right-to-left runs are still rendered RTL, but will not flip
the whole URL around. For example (if "ABC" is Hebrew), this will render
"ABC.com" as "CBA.com", rather than "com.CBA".

Affects main text field and suggestions, but not non-URL search text.
Complies with RFC 3987 Section 4.1 and brings the Mac Omnibox in line
with the existing behaviour on Views and Android.

BUG=624213

Committed: https://crrev.com/05baf795316dd430afbd79ee915187bec6bf5f5d
Cr-Commit-Position: refs/heads/master@{#406791}
==========

[commit-bot: I haz the power, 2016-07-21 07:10:44]

Patchset 5 (id:??) landed as
https://crrev.com/05baf795316dd430afbd79ee915187bec6bf5f5d
Cr-Commit-Position: refs/heads/master@{#406791}