Issue metadata
Sign in to add a comment
|
Regression: ‘Force UI direction’,’Force text direction’ and ‘Enable RTL’ flags is not working.
Reported by
abom...@etouch.net,
May 29 2017
|
||||||||||||||||||||||
Issue descriptionChrome Version: 61.0.3114.0 (Official Build) cf4f838225c132dee06cc21dd6a576347734589b-refs/heads/master@{#475229} OS: Mac(10.12.3,10.11.6) What steps will reproduce the problem? 1. Launch chrome and navigate to chrome://flags 2. Select ‘Right-to-left’ direction for Force UI direction flag and click on ‘Relaunch’ button, enter text in omnibox 3. Observe, Similarly for other two flags. Actual: ‘Force UI direction’,’Force text direction’ and ‘Enable RTL’ flags is not working.(Refer screencast) Expected: ‘Force UI direction’,’Force text direction’ and ‘Enable RTL’ flags should work. This is regression issue, broken in ‘M 60’ and below is manual bisect info: Good build:60.0.3103.0 (472588) Bad build:60.0.3104.0(473014) Note: Issue is not seen on Windows and Linux OS.
,
May 30 2017
CL touches only ios/web/web_state/ui/crw_web_controller.mm which is not even compiled on mac
,
May 30 2017
+lgrey@ (not sure if Mac RTL changed the behavior of this flag) Possibly 89c24d4 [TSAN] Fix TSAN error in i18n RTL. by mbjorge · 12 days ago
,
May 30 2017
,
May 30 2017
Re #3, Mac RTL shouldn't affect this flag.
,
May 30 2017
The video doesn't show the use of --force-text-direction=rtl. Was it working in 60.0.3103.0 ?
,
May 30 2017
Ah, yeah it could definitely been my CL, though it's not obvious to me why it would be macOS specific
,
May 31 2017
My guess is that the g_icu_text_direction is never getting set. Probably the windows/linux paths are doing a call to 'SetICUDefaultLocale' at some point and the mac path is not? Just speculation though, I don't have a mac setup to test with. Will put up a revert
,
May 31 2017
Revert going through CQ: https://codereview.chromium.org/2917523002/
,
Jun 1 2017
Waiting on a comitter+1 on the CL from #9
,
Jun 5 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/27175dace3576a3e351348cf8272c20b0d3aceec commit 27175dace3576a3e351348cf8272c20b0d3aceec Author: mbjorge <mbjorge@chromium.org> Date: Mon Jun 05 20:12:55 2017 Revert of [TSAN] Fix TSAN error in i18n RTL. (patchset #3 id:40001 of https://codereview.chromium.org/2877983004/ ) Reason for revert: Appears to break the Force UI Direction / Force Text Direction / FOrce RTL flags on mac. BUG= 727229 Original issue's description: > [TSAN] Fix TSAN error in i18n RTL. > > Use an atomic to protect the global g_icu_text_direction cached value > from multiple readers/writers. > > Also reduce acccess to g_icu_text_diretion so it only has a single write > location (in SetICUDefaultLocal) and a single read location (ICUIsRTL), > to simplify the race-y-ness. > > Outside of test code, SetICUDefaultLocal is only used in two locations: > ppapi_plugin_main.cc and l10n_util.cc, whereas IsRTL is used in >100 > places outside of test code, so it seemed reasonable to move the heavy > logic (updating g_icu_text_direction when it changes) into what I imagine to be > a function that is called rarely and let the one that is used a lot > (IsRTL/ICUIsRTL) just do a simple read. > > This still still leaves a race condition in that IsRTL may give in > inaccurate result if SetICUDefaultLocale is executed at the same time. > In particular, IsRTL may return the value for the previously set default > locale after the default locale has been updated. > > BUG=695929 > TEST=cast_shell_browsertests under TSAN before/after > > Review-Url: https://codereview.chromium.org/2877983004 > Cr-Commit-Position: refs/heads/master@{#472890} > Committed: https://chromium.googlesource.com/chromium/src/+/89c24d4a46c1f0e7ad03891a659e6d23dc776c36 TBR=jshin@chromium.org # Not skipping CQ checks because original CL landed more than 1 days ago. BUG=695929 Review-Url: https://codereview.chromium.org/2917523002 Cr-Commit-Position: refs/heads/master@{#477064} [modify] https://crrev.com/27175dace3576a3e351348cf8272c20b0d3aceec/base/i18n/rtl.cc
,
Jun 5 2017
Not sure how to CP to the release branch though? Can anyone point me to some docs for that process?
,
Jun 6 2017
rbasuvula@ are you able to confirm that this is fixed with the revert? I do not have a mac setup to test with
,
Jun 6 2017
Thanks for the revert. abombde@ Can you please verify in latest canary.
,
Jun 6 2017
,
Jun 6 2017
,
Jun 6 2017
ah, sorry, will add Merge-Request-60 label for the revert after it's verified
,
Jun 7 2017
With respect to comment #14: Above issue seems to be fixed on Latest Canary Version:61.0.3123.0 (Official Build)193094009c210267cfa2c194ec9875711ccaf647-refs/heads/master@{#477506} 64 bit using Mac Pro(10.12.3)
,
Jun 7 2017
As per comment #18. Adding TE-Verified labels for M-61. @mbjorge: Please merge in to M-60. Thank You!
,
Jun 7 2017
,
Jun 7 2017
This bug requires manual review: Reverts referenced in bugdroid comments after merge request. Please contact the milestone owner if you have questions. Owners: amineer@(Android), cmasso@(iOS), josafat@(ChromeOS), bustamante@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Jun 9 2017
,
Jun 12 2017
Fix has been verified in canary, and marked as rb-stable. Approving merge for M60.
,
Jun 12 2017
I have a CR+1 and Merge-Approved now, but I don't have permissions to `git cl land` or to even mark CQ+2 on the CL. Let me know if there is something else I need to do, otherwise I'll trust that the powers-that-be will be able to submit the M60 CL
,
Jun 12 2017
I'm not sure what is the cl in c#20, but here are instructions you need to cherry-pick your revert in c#11 back to M60: https://sites.google.com/a/chromium.org/dev/developers/how-tos/drover
,
Jun 12 2017
Please merge the patch to M60 branch(3112),Beta RC cut is scheduled @ 4.00 PM PST tomorrow(06/13).
,
Jun 14 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/eeb12a2d4f56714eafd9e71dba80ca378ace13e9 commit eeb12a2d4f56714eafd9e71dba80ca378ace13e9 Author: Mike Bjorge <mbjorge@chromium.org> Date: Wed Jun 14 20:44:20 2017 Revert of [TSAN] Fix TSAN error in i18n RTL. (patchset #3 id:40001 of https://codereview.chromium.org/2877983004/ ) Reason for revert: Appears to break the Force UI Direction / Force Text Direction / FOrce RTL flags on mac. BUG= 727229 Original issue's description: > [TSAN] Fix TSAN error in i18n RTL. > > Use an atomic to protect the global g_icu_text_direction cached value > from multiple readers/writers. > > Also reduce acccess to g_icu_text_diretion so it only has a single write > location (in SetICUDefaultLocal) and a single read location (ICUIsRTL), > to simplify the race-y-ness. > > Outside of test code, SetICUDefaultLocal is only used in two locations: > ppapi_plugin_main.cc and l10n_util.cc, whereas IsRTL is used in >100 > places outside of test code, so it seemed reasonable to move the heavy > logic (updating g_icu_text_direction when it changes) into what I imagine to be > a function that is called rarely and let the one that is used a lot > (IsRTL/ICUIsRTL) just do a simple read. > > This still still leaves a race condition in that IsRTL may give in > inaccurate result if SetICUDefaultLocale is executed at the same time. > In particular, IsRTL may return the value for the previously set default > locale after the default locale has been updated. > > BUG=695929 > TEST=cast_shell_browsertests under TSAN before/after > > Review-Url: https://codereview.chromium.org/2877983004 > Cr-Commit-Position: refs/heads/master@{#472890} > Committed: https://chromium.googlesource.com/chromium/src/+/89c24d4a46c1f0e7ad03891a659e6d23dc776c36 BUG=695929 (cherry picked from commit 27175dace3576a3e351348cf8272c20b0d3aceec) Review-Url: https://codereview.chromium.org/2917523002 Cr-Original-Commit-Position: refs/heads/master@{#477064} Change-Id: I7e06fc64d20b673766f2bed3f084326e590024f1 Reviewed-on: https://chromium-review.googlesource.com/527328 Reviewed-by: Jungshik Shin <jshin@chromium.org> Reviewed-by: Daniel Cheng <dcheng@chromium.org> Cr-Commit-Position: refs/branch-heads/3112@{#345} Cr-Branched-From: b6460e24cf59f429d69de255538d0fc7a425ccf9-refs/heads/master@{#474897} [modify] https://crrev.com/eeb12a2d4f56714eafd9e71dba80ca378ace13e9/base/i18n/rtl.cc
,
Jun 28 2017
Above issue seems to be fixed on Latest Beta Version:60.0.3112.50 (Official Build) 1b8bb7c2c2f4bd3a42ec5a63db827ad9930f2536-refs/branch-heads/3112@{#485} 64 bit using Mac Pro(10.12.3)
,
Jun 28 2017
Marking as verified per comment #28; bustamante@ please correct me if I'm wrong. |
|||||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||||
Comment 1 by rbasuvula@chromium.org
, May 29 2017Labels: hasbisect
Owner: eugene...@chromium.org
Status: Assigned (was: Unconfirmed)