Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(49)

Issue 1189553002: Omnibox: Force text field to LTR context if it is a URL. (Closed)

Created:
4 years, 3 months ago by Matt Giuca
Modified:
4 years, 3 months ago
Reviewers:
Peter Kasting
CC:
chromium-reviews, tfarina, James Su, chrome-apps-syd-reviews_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

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". This is consistent with the behaviour in the Omnibox drop-down items (OmniboxResultView::CreateClassifiedRenderText) and status bubble (StatusBubbleViews::SetURL). BUG=495933, 421332 Committed: https://crrev.com/1c7d9ce02925cf766fc508d4ee83424369e71548 Cr-Commit-Position: refs/heads/master@{#334537}

Patch Set 1 #

Total comments: 2

Patch Set 2 : Rebase. #

Patch Set 3 : Rewrite comment with an example. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+12 lines, -3 lines) Patch
M chrome/browser/ui/views/omnibox/omnibox_view_views.cc View 1 2 3 chunks +12 lines, -3 lines 0 comments Download

Messages

Total messages: 18 (5 generated)
Matt Giuca
4 years, 3 months ago (2015-06-15 07:48:00 UTC) #2
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1189553002/1
4 years, 3 months ago (2015-06-15 11:57:34 UTC) #4
commit-bot: I haz the power
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_ng/builds/69656)
4 years, 3 months ago (2015-06-15 13:06:39 UTC) #6
Peter Kasting
LGTM https://codereview.chromium.org/1189553002/diff/1/chrome/browser/ui/views/omnibox/omnibox_view_views.cc File chrome/browser/ui/views/omnibox/omnibox_view_views.cc (right): https://codereview.chromium.org/1189553002/diff/1/chrome/browser/ui/views/omnibox/omnibox_view_views.cc#newcode609 chrome/browser/ui/views/omnibox/omnibox_view_views.cc:609: // rendered on the left. This second sentence ...
4 years, 3 months ago (2015-06-15 22:39:24 UTC) #7
Matt Giuca
https://codereview.chromium.org/1189553002/diff/1/chrome/browser/ui/views/omnibox/omnibox_view_views.cc File chrome/browser/ui/views/omnibox/omnibox_view_views.cc (right): https://codereview.chromium.org/1189553002/diff/1/chrome/browser/ui/views/omnibox/omnibox_view_views.cc#newcode609 chrome/browser/ui/views/omnibox/omnibox_view_views.cc:609: // rendered on the left. On 2015/06/15 22:39:24, Peter ...
4 years, 3 months ago (2015-06-16 00:21:32 UTC) #8
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1189553002/40001
4 years, 3 months ago (2015-06-16 01:21:45 UTC) #11
commit-bot: I haz the power
Committed patchset #3 (id:40001)
4 years, 3 months ago (2015-06-16 02:53:17 UTC) #12
commit-bot: I haz the power
Patchset 3 (id:??) landed as https://crrev.com/1c7d9ce02925cf766fc508d4ee83424369e71548 Cr-Commit-Position: refs/heads/master@{#334537}
4 years, 3 months ago (2015-06-16 02:54:14 UTC) #13
Peter Kasting
So is there a testcase I can start typing that will show me how the ...
4 years, 3 months ago (2015-06-16 08:22:50 UTC) #14
Matt Giuca
Yes, it is a legitimate concern. The main problem is that there seems to be ...
4 years, 3 months ago (2015-06-17 03:16:03 UTC) #15
Peter Kasting
On 2015/06/17 03:16:03, Matt Giuca wrote: > Type: "abc.def/ghi" and then type a sequence of ...
4 years, 3 months ago (2015-06-17 04:36:18 UTC) #16
Matt Giuca
OK I've got a bit on my plate at the moment but I might be ...
4 years, 3 months ago (2015-06-17 05:39:47 UTC) #17
Peter Kasting
4 years, 3 months ago (2015-06-17 05:50:54 UTC) #18
Message was sent while issue was closed.
On 2015/06/17 05:39:47, Matt Giuca wrote:
> OK I've got a bit on my plate at the moment but I might be able to help out.
> 
> In the mean time (talking about the M45 branch point), do you think we should
> revert this temporarily or just live with the flipping behaviour for now?

If we don't manage to land anything else, I probably would not ship this to 45. 
Even if we land a fix for that bug we may find the behavior still feels icky,
but we'll see.

It's a security issue, but it's also one we've had for all time, so I'm willing
to take longer to get it right if need be.

Powered by Google App Engine
This is Rietveld 408576698