Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2853523002:
[rendertext,i18n] Added flag to force RTL rendering (Closed)
|
Created:
3 years, 7 months ago by Kevin Bailey Modified:
3 years, 7 months ago CC:
chromium-reviews, danakj+watch_chromium.org, derat+watch_chromium.org, asvitkine+watch_chromium.org, jshin+watch_chromium.org, vmpstr+watch_chromium.org, Matt Giuca Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Description[rendertext,i18n] Added flag to force RTL rendering
Added command line flag to enable testing of RTL rendering of LTR
languages (and vice versa). Complements "--force-ui-direction".
BUG=716660
Review-Url: https://codereview.chromium.org/2853523002
Cr-Commit-Position: refs/heads/master@{#468632}
Committed: https://chromium.googlesource.com/chromium/src/+/6822b60b6967b700212972aefcceda3aad23785c
Patch Set 1 #
Total comments: 14
Patch Set 2 : Corrected ordering and text, added cached look-ups #
Total comments: 12
Patch Set 3 : Absorbed functions and added comments #Patch Set 4 : Missed a comment #
Total comments: 2
Patch Set 5 : Made method stand-alone and anonymous #Patch Set 6 : Forgot to update histograms.xml #
Messages
Total messages: 37 (20 generated)
Description was changed from ========== [rendertext] Added flag to force RTL rendering Added command line flag to enable testing of RTL rendering of LTR languages (and vice versa). Complements "--force-ui-direction". BUG= ========== to ========== [rendertext] Added flag to force RTL rendering Added command line flag to enable testing of RTL rendering of LTR languages (and vice versa). Complements "--force-ui-direction". BUG= ==========
krb@chromium.org changed reviewers: + jshin@chromium.org, msw@chromium.org
Hi, jshin, could you take a look at the i18n changes? msw, could you take a look at the rendertext changes? This flag is strictly for facilitating testing, to complement danak's --force-ui-direction flag. https://codereview.chromium.org/2853523002/diff/1/base/i18n/base_i18n_switches.h File base/i18n/base_i18n_switches.h (right): https://codereview.chromium.org/2853523002/diff/1/base/i18n/base_i18n_switche... base/i18n/base_i18n_switches.h:16: BASE_I18N_EXPORT extern const char kForceUIDirectionLTR[]; What do you think about renaming these to "kForceDirection..." ?
What utility does this offer that isn't covered by existing patterns? Most test
either set the UI locale to one that is RTL (ie. SetICUDefaultLocale("he")) or
supply some RTL/LTR/BiDi text to specific objects in unit tests. I would avoid
adding this functionality unless it's really needed.
On 2017/04/28 20:33:08, msw wrote:
> What utility does this offer that isn't covered by existing patterns? Most
test
> either set the UI locale to one that is RTL (ie. SetICUDefaultLocale("he")) or
> supply some RTL/LTR/BiDi text to specific objects in unit tests. I would avoid
> adding this functionality unless it's really needed.
In fact, I'm not sure why we have --force-ui-direction... can we remove that?
On 2017/04/28 20:34:14, msw wrote:
> What utility does this offer that isn't covered by existing patterns? Most
> test
> either set the UI locale to one that is RTL (ie. SetICUDefaultLocale("he")) or
> supply some RTL/LTR/BiDi text to specific objects in unit tests. I would avoid
> adding this functionality unless it's really needed.
It allows me (and checking it in would allow others) to trivially test RTL text
without knowing an RTL language. I'm using it right now to test results in the
omnibox and seeing if it handles RTL text correctly.
> In fact, I'm not sure why we have --force-ui-direction... can we remove that?
I didn't add it but I find it too to be a crucial test tool.
On 2017/04/28 20:40:45, Kevin Bailey wrote:
> On 2017/04/28 20:34:14, msw wrote:
> > What utility does this offer that isn't covered by existing patterns? Most
> > test
> > either set the UI locale to one that is RTL (ie. SetICUDefaultLocale("he"))
or
> > supply some RTL/LTR/BiDi text to specific objects in unit tests. I would
avoid
> > adding this functionality unless it's really needed.
>
> It allows me (and checking it in would allow others) to trivially test RTL
text
> without knowing an RTL language. I'm using it right now to test results in the
> omnibox and seeing if it handles RTL text correctly.
>
> > In fact, I'm not sure why we have --force-ui-direction... can we remove
that?
>
> I didn't add it but I find it too to be a crucial test tool.
When you're testing RTL text handling, the two currently available tools (1.
find someone who reads Arabic or Hebrew and interrupt what they're doing; 2. cut
and paste text into and out of Google Translate over and over again) produce a
lot of friction and are subject to a lot of errors. Android and iOS developers
seem to run into this issue often enough that those platforms offer the
synthetic language "RTL English". This CL seems like a relatively lightweight
way to accomplish something similar.
Okay I buy it. I'm glad to see additional RTL testing, even if it's not quite realistic. Please set BUG=558208 or file a similar issue and point to that. https://codereview.chromium.org/2853523002/diff/1/base/i18n/base_i18n_switches.h File base/i18n/base_i18n_switches.h (right): https://codereview.chromium.org/2853523002/diff/1/base/i18n/base_i18n_switche... base/i18n/base_i18n_switches.h:16: BASE_I18N_EXPORT extern const char kForceUIDirectionLTR[]; On 2017/04/28 14:38:06, Kevin Bailey wrote: > What do you think about renaming these to "kForceDirection..." ? sgtm https://codereview.chromium.org/2853523002/diff/1/base/i18n/rtl.cc File base/i18n/rtl.cc (right): https://codereview.chromium.org/2853523002/diff/1/base/i18n/rtl.cc#newcode58 base/i18n/rtl.cc:58: base::CommandLine* command_line = base::CommandLine::ForCurrentProcess(); This function is called heavily, cache the value (or at least the presence of a value, so the typical production codepath isn't hit with commandline checks for each call), maybe: namespace { bool HasForcedTextDirection() { static bool value = base::CommandLine::ForCurrentProcess()->HasSwitch(switches::kForceTextDirection); return value; } } // namespace ... if (HasForcedTextDirection()) { std::string force_flag = ... } https://codereview.chromium.org/2853523002/diff/1/chrome/browser/flag_descrip... File chrome/browser/flag_descriptions.cc (right): https://codereview.chromium.org/2853523002/diff/1/chrome/browser/flag_descrip... chrome/browser/flag_descriptions.cc:1600: const char kForceTextDirectionName[] = "Force text direction"; ditto ordering https://codereview.chromium.org/2853523002/diff/1/chrome/browser/flag_descrip... chrome/browser/flag_descriptions.cc:1608: "(RTL) mode, overriding the default direction of the UI language."; "overriding the per-character directionality of UI text." (ie. this doesn't just change the top level directionality of each run of text, it overrides every character, there's no way to test Bidi text in this mode). https://codereview.chromium.org/2853523002/diff/1/chrome/browser/flag_descrip... File chrome/browser/flag_descriptions.h (right): https://codereview.chromium.org/2853523002/diff/1/chrome/browser/flag_descrip... chrome/browser/flag_descriptions.h:1760: // Name for the flag to force a specific text rendering direction. nit: order Ui name and desc together before text name and desc https://codereview.chromium.org/2853523002/diff/1/ui/gfx/render_text_harfbuzz.cc File ui/gfx/render_text_harfbuzz.cc (right): https://codereview.chromium.org/2853523002/diff/1/ui/gfx/render_text_harfbuzz... ui/gfx/render_text_harfbuzz.cc:1400: CheckForcedDirection(&run->level); I'm not sure this is needed; doesn't the underlying value come from GetCharacterDirection? If this is needed; I'd (1) be curious as to why, and (2) need you also cache the (presence of an) override value here too.
https://codereview.chromium.org/2853523002/diff/1/base/i18n/base_i18n_switches.h File base/i18n/base_i18n_switches.h (right): https://codereview.chromium.org/2853523002/diff/1/base/i18n/base_i18n_switche... base/i18n/base_i18n_switches.h:16: BASE_I18N_EXPORT extern const char kForceUIDirectionLTR[]; On 2017/04/28 22:04:03, msw wrote: > On 2017/04/28 14:38:06, Kevin Bailey wrote: > > What do you think about renaming these to "kForceDirection..." ? > > sgtm Done. https://codereview.chromium.org/2853523002/diff/1/base/i18n/rtl.cc File base/i18n/rtl.cc (right): https://codereview.chromium.org/2853523002/diff/1/base/i18n/rtl.cc#newcode58 base/i18n/rtl.cc:58: base::CommandLine* command_line = base::CommandLine::ForCurrentProcess(); On 2017/04/28 22:04:03, msw wrote: > This function is called heavily, cache the value (or at least the presence of a > value, so the typical production codepath isn't hit with commandline checks for > each call), maybe: > > namespace { > > bool HasForcedTextDirection() { > static bool value = > base::CommandLine::ForCurrentProcess()->HasSwitch(switches::kForceTextDirection); > return value; > } > > } // namespace > > ... > > if (HasForcedTextDirection()) { > std::string force_flag = ... > } Done. https://codereview.chromium.org/2853523002/diff/1/chrome/browser/flag_descrip... File chrome/browser/flag_descriptions.cc (right): https://codereview.chromium.org/2853523002/diff/1/chrome/browser/flag_descrip... chrome/browser/flag_descriptions.cc:1600: const char kForceTextDirectionName[] = "Force text direction"; On 2017/04/28 22:04:03, msw wrote: > ditto ordering Done. https://codereview.chromium.org/2853523002/diff/1/chrome/browser/flag_descrip... chrome/browser/flag_descriptions.cc:1608: "(RTL) mode, overriding the default direction of the UI language."; On 2017/04/28 22:04:03, msw wrote: > "overriding the per-character directionality of UI text." (ie. this doesn't just > change the top level directionality of each run of text, it overrides every > character, there's no way to test Bidi text in this mode). Done. https://codereview.chromium.org/2853523002/diff/1/chrome/browser/flag_descrip... File chrome/browser/flag_descriptions.h (right): https://codereview.chromium.org/2853523002/diff/1/chrome/browser/flag_descrip... chrome/browser/flag_descriptions.h:1760: // Name for the flag to force a specific text rendering direction. On 2017/04/28 22:04:03, msw wrote: > nit: order Ui name and desc together before text name and desc Done. https://codereview.chromium.org/2853523002/diff/1/ui/gfx/render_text_harfbuzz.cc File ui/gfx/render_text_harfbuzz.cc (right): https://codereview.chromium.org/2853523002/diff/1/ui/gfx/render_text_harfbuzz... ui/gfx/render_text_harfbuzz.cc:1400: CheckForcedDirection(&run->level); On 2017/04/28 22:04:03, msw wrote: > I'm not sure this is needed; doesn't the underlying value come from > GetCharacterDirection? If this is needed; I'd (1) be curious as to why, and (2) > need you also cache the (presence of an) override value here too. 'GetTextDirection()' is returning RTL, but, when the text contains 2 runs - a bold one and a regular one - 'pBiDi->levels' is full of 2's, so 'run->level' is always 2. I didn't dig deep enough into i18n to figure out why. Done.
Description was changed from ========== [rendertext] Added flag to force RTL rendering Added command line flag to enable testing of RTL rendering of LTR languages (and vice versa). Complements "--force-ui-direction". BUG= ========== to ========== [rendertext,i18n] Added flag to force RTL rendering Added command line flag to enable testing of RTL rendering of LTR languages (and vice versa). Complements "--force-ui-direction". BUG=716660 ==========
LGTM I thought a pseudo-RTL locale had been added for testing, but I couldn't find it. Anyway, force-rtl-text-rendering can have a different utility.
lgtm with nits; thanks. https://codereview.chromium.org/2853523002/diff/1/ui/gfx/render_text_harfbuzz.cc File ui/gfx/render_text_harfbuzz.cc (right): https://codereview.chromium.org/2853523002/diff/1/ui/gfx/render_text_harfbuzz... ui/gfx/render_text_harfbuzz.cc:1400: CheckForcedDirection(&run->level); On 2017/04/29 04:24:03, Kevin Bailey wrote: > On 2017/04/28 22:04:03, msw wrote: > > I'm not sure this is needed; doesn't the underlying value come from > > GetCharacterDirection? If this is needed; I'd (1) be curious as to why, and > (2) > > need you also cache the (presence of an) override value here too. > > 'GetTextDirection()' is returning RTL, but, when the text contains 2 runs - a > bold one and a regular one - 'pBiDi->levels' is full of 2's, so 'run->level' is > always 2. I didn't dig deep enough into i18n to figure out why. > > Done. Acknowledged. https://codereview.chromium.org/2853523002/diff/20001/base/i18n/base_i18n_swi... File base/i18n/base_i18n_switches.cc (right): https://codereview.chromium.org/2853523002/diff/20001/base/i18n/base_i18n_swi... base/i18n/base_i18n_switches.cc:18: const char kForceDirectionLTR[] = "ltr"; optional nit: shrink the spacing before '=' here and above to make them match with one space after the longest prefix. https://codereview.chromium.org/2853523002/diff/20001/base/i18n/base_i18n_swi... File base/i18n/base_i18n_switches.h (right): https://codereview.chromium.org/2853523002/diff/20001/base/i18n/base_i18n_swi... base/i18n/base_i18n_switches.h:15: // kForce*Direction choices. nit: "kForce*Direction choices for the switches above." https://codereview.chromium.org/2853523002/diff/20001/base/i18n/rtl.cc File base/i18n/rtl.cc (right): https://codereview.chromium.org/2853523002/diff/20001/base/i18n/rtl.cc#newcode54 base/i18n/rtl.cc:54: bool HasForcedTextDirectionSwitch() { nit: "// Get the cached switch presence; avoids repeated lookup in the common case." (or you can inline this static bool in the function below) https://codereview.chromium.org/2853523002/diff/20001/chrome/browser/flag_des... File chrome/browser/flag_descriptions.h (right): https://codereview.chromium.org/2853523002/diff/20001/chrome/browser/flag_des... chrome/browser/flag_descriptions.h:1769: // Name for the option to force left-to-right UI direction mode. nit: remove "UI" here and below. (or "UI or text") https://codereview.chromium.org/2853523002/diff/20001/ui/gfx/render_text_harf... File ui/gfx/render_text_harfbuzz.cc (right): https://codereview.chromium.org/2853523002/diff/20001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:611: bool HasForcedTextDirectionSwitch() { ditto nit: "// Get the cached switch presence; avoids repeated lookup in the common case." (or you can inline this static bool in the function below) https://codereview.chromium.org/2853523002/diff/20001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:1350: void RenderTextHarfBuzz::CheckForcedDirection(UBiDiLevel* level) { nit: "// Applies a forced direction if specified by a commandline switch."
https://codereview.chromium.org/2853523002/diff/20001/base/i18n/base_i18n_swi... File base/i18n/base_i18n_switches.cc (right): https://codereview.chromium.org/2853523002/diff/20001/base/i18n/base_i18n_swi... base/i18n/base_i18n_switches.cc:18: const char kForceDirectionLTR[] = "ltr"; On 2017/05/01 20:56:18, msw wrote: > optional nit: shrink the spacing before '=' here and above to make them match > with one space after the longest prefix. Done. Agreed, looks better. https://codereview.chromium.org/2853523002/diff/20001/base/i18n/base_i18n_swi... File base/i18n/base_i18n_switches.h (right): https://codereview.chromium.org/2853523002/diff/20001/base/i18n/base_i18n_swi... base/i18n/base_i18n_switches.h:15: // kForce*Direction choices. On 2017/05/01 20:56:18, msw wrote: > nit: "kForce*Direction choices for the switches above." Done. https://codereview.chromium.org/2853523002/diff/20001/base/i18n/rtl.cc File base/i18n/rtl.cc (right): https://codereview.chromium.org/2853523002/diff/20001/base/i18n/rtl.cc#newcode54 base/i18n/rtl.cc:54: bool HasForcedTextDirectionSwitch() { On 2017/05/01 20:56:18, msw wrote: > nit: "// Get the cached switch presence; avoids repeated lookup in the common > case." (or you can inline this static bool in the function below) Done. Seemed like a bit much overhead for a whole function. https://codereview.chromium.org/2853523002/diff/20001/chrome/browser/flag_des... File chrome/browser/flag_descriptions.h (right): https://codereview.chromium.org/2853523002/diff/20001/chrome/browser/flag_des... chrome/browser/flag_descriptions.h:1769: // Name for the option to force left-to-right UI direction mode. On 2017/05/01 20:56:18, msw wrote: > nit: remove "UI" here and below. (or "UI or text") Done. https://codereview.chromium.org/2853523002/diff/20001/ui/gfx/render_text_harf... File ui/gfx/render_text_harfbuzz.cc (right): https://codereview.chromium.org/2853523002/diff/20001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:611: bool HasForcedTextDirectionSwitch() { On 2017/05/01 20:56:18, msw wrote: > ditto nit: "// Get the cached switch presence; avoids repeated lookup in the > common case." (or you can inline this static bool in the function below) Done. Again, absorbed code. https://codereview.chromium.org/2853523002/diff/20001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:1350: void RenderTextHarfBuzz::CheckForcedDirection(UBiDiLevel* level) { On 2017/05/01 20:56:18, msw wrote: > nit: "// Applies a forced direction if specified by a commandline switch." Done.
Sorry, I missed one nit; still lgtm with that fixed. https://codereview.chromium.org/2853523002/diff/60001/ui/gfx/render_text_harf... File ui/gfx/render_text_harfbuzz.cc (right): https://codereview.chromium.org/2853523002/diff/60001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:1346: void RenderTextHarfBuzz::CheckForcedDirection(UBiDiLevel* level) { nit: make this a static local function in the line 45 anon namespace.
No worries. https://codereview.chromium.org/2853523002/diff/60001/ui/gfx/render_text_harf... File ui/gfx/render_text_harfbuzz.cc (right): https://codereview.chromium.org/2853523002/diff/60001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:1346: void RenderTextHarfBuzz::CheckForcedDirection(UBiDiLevel* level) { On 2017/05/01 21:55:33, msw wrote: > nit: make this a static local function in the line 45 anon namespace. Done.
The CQ bit was checked by msw@chromium.org
lgtm
The patchset sent to the CQ was uploaded after l-g-t-m from jshin@chromium.org Link to the patchset: https://codereview.chromium.org/2853523002/#ps80001 (title: "Made method stand-alone and anonymous")
The CQ bit was unchecked by msw@chromium.org
The CQ bit was checked by krb@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
krb@chromium.org changed reviewers: + mpearson@chromium.org
Hi mpearson@, could you bless histograms.xml ? thx, krb
histograms.xml lgtm
The CQ bit was checked by krb@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by krb@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from msw@chromium.org, jshin@chromium.org Link to the patchset: https://codereview.chromium.org/2853523002/#ps100001 (title: "Forgot to update histograms.xml")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 100001, "attempt_start_ts": 1493736477660060,
"parent_rev": "818ada352c5ec2c3b9da4baf2c9da3472f016fcc", "commit_rev":
"8d2358aef79d3a95da221a18bedc6859c7a42211"}
CQ is committing da patch.
Bot data: {"patchset_id": 100001, "attempt_start_ts": 1493736477660060,
"parent_rev": "e834ce91dc3157be07c109fb1d6c422297ddf0bb", "commit_rev":
"6822b60b6967b700212972aefcceda3aad23785c"}
Message was sent while issue was closed.
Description was changed from ========== [rendertext,i18n] Added flag to force RTL rendering Added command line flag to enable testing of RTL rendering of LTR languages (and vice versa). Complements "--force-ui-direction". BUG=716660 ========== to ========== [rendertext,i18n] Added flag to force RTL rendering Added command line flag to enable testing of RTL rendering of LTR languages (and vice versa). Complements "--force-ui-direction". BUG=716660 Review-Url: https://codereview.chromium.org/2853523002 Cr-Commit-Position: refs/heads/master@{#468632} Committed: https://chromium.googlesource.com/chromium/src/+/6822b60b6967b700212972aefcce... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://chromium.googlesource.com/chromium/src/+/6822b60b6967b700212972aefcce... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
