Check string and UI direction in GetDisplayStringInLTRDirectionality, etc.

Check string and UI direction in GetDisplayStringInLTRDirectionality, etc.

Continue always wrapping the string in RTL UI.
(in case it's appended to an RTL string later, etc.)

Additionally wrap strings with RTL first strong characters in LTR UI.
(previously, text inherited UI base direction, now it uses the first strong char)

Add, expand upon, and cleanup unit tests.

BUG=144541
TEST=Download shelf (and other UI) file paths are shown as LTR (without stray rendered glyphs or other regressions).

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=156459

[msw, 2012-09-07 05:37:58]

Hey Xiaomei, please take a look; thanks!

[xji, 2012-09-10 18:07:26]

https://chromiumcodereview.appspot.com/10910128/diff/2003/base/i18n/rtl.cc
File base/i18n/rtl.cc (right):

https://chromiumcodereview.appspot.com/10910128/diff/2003/base/i18n/rtl.cc#ne...
base/i18n/rtl.cc:324: if (!IsRTL() && GetFirstStrongCharacterDirection(text) ==
LEFT_TO_RIGHT)
why RTL locale is specially handled (always wrap in LRE/PDF)?

for rtl locale (in other cases), are we still displaying text in RTL
directionality or we are also using first-strong-character's directionality?

[msw, 2012-09-10 18:37:31]

https://chromiumcodereview.appspot.com/10910128/diff/2003/base/i18n/rtl.cc
File base/i18n/rtl.cc (right):

https://chromiumcodereview.appspot.com/10910128/diff/2003/base/i18n/rtl.cc#ne...
base/i18n/rtl.cc:324: if (!IsRTL() && GetFirstStrongCharacterDirection(text) ==
LEFT_TO_RIGHT)
On 2012/09/10 18:07:26, xji wrote:
> why RTL locale is specially handled (always wrap in LRE/PDF)?
> 
> for rtl locale (in other cases), are we still displaying text in RTL
> directionality or we are also using first-strong-character's directionality?

I don't know about Mac/GTK, but Win and CrOs use first-strong-char's dir.
I left the check to not regress additional cases (perhaps in Mac/GTK); also:

In RTL UI, |text| is more likely to be concatenated w/RTL text: "OPENING file".
So we must also wrap "file" if it starts with weak directionality characters.
Otherwise, concatenating "[weak text][ltr text]" to "[RTL TEXT]" would yield:
Logical text: "[RTL TEXT][weak text][ltr text]"
Display text: "[ltr text][weak text][RTL TEXT]" < wrong, fixed by wrapping.

Perhaps we must wrap if the first char isn't strong LTR? Or always wrap?
(just in case LTR UI uses RTL strings somewhere like the example above?)

[xji, 2012-09-10 22:27:57]

lgtm

https://chromiumcodereview.appspot.com/10910128/diff/2003/base/i18n/rtl.cc
File base/i18n/rtl.cc (right):

https://chromiumcodereview.appspot.com/10910128/diff/2003/base/i18n/rtl.cc#ne...
base/i18n/rtl.cc:324: if (!IsRTL() && GetFirstStrongCharacterDirection(text) ==
LEFT_TO_RIGHT)
On 2012/09/10 18:37:31, msw wrote:
> On 2012/09/10 18:07:26, xji wrote:
> > why RTL locale is specially handled (always wrap in LRE/PDF)?
> > 
> > for rtl locale (in other cases), are we still displaying text in RTL
> > directionality or we are also using first-strong-character's directionality?
> 
> I don't know about Mac/GTK, but Win and CrOs use first-strong-char's dir.
> I left the check to not regress additional cases (perhaps in Mac/GTK); also:
> 
> In RTL UI, |text| is more likely to be concatenated w/RTL text: "OPENING
file".
> So we must also wrap "file" if it starts with weak directionality characters.
> Otherwise, concatenating "[weak text][ltr text]" to "[RTL TEXT]" would yield:
> Logical text: "[RTL TEXT][weak text][ltr text]"
> Display text: "[ltr text][weak text][RTL TEXT]" < wrong, fixed by wrapping.
> 
> Perhaps we must wrap if the first char isn't strong LTR? Or always wrap?
> (just in case LTR UI uses RTL strings somewhere like the example above?)

In English windows (windows xp), if a user does not install the RTL language
package, LRE/PDF characters render as empty square. So, we do not want always
wrapping.

[msw, 2012-09-10 22:46:12]

https://chromiumcodereview.appspot.com/10910128/diff/2003/base/i18n/rtl.cc
File base/i18n/rtl.cc (right):

https://chromiumcodereview.appspot.com/10910128/diff/2003/base/i18n/rtl.cc#ne...
base/i18n/rtl.cc:324: if (!IsRTL() && GetFirstStrongCharacterDirection(text) ==
LEFT_TO_RIGHT)
On 2012/09/10 22:27:57, xji wrote:
> On 2012/09/10 18:37:31, msw wrote:
> > On 2012/09/10 18:07:26, xji wrote:
> > > why RTL locale is specially handled (always wrap in LRE/PDF)?
> > > 
> > > for rtl locale (in other cases), are we still displaying text in RTL
> > > directionality or we are also using first-strong-character's
directionality?
> > 
> > I don't know about Mac/GTK, but Win and CrOs use first-strong-char's dir.
> > I left the check to not regress additional cases (perhaps in Mac/GTK); also:
> > 
> > In RTL UI, |text| is more likely to be concatenated w/RTL text: "OPENING
> file".
> > So we must also wrap "file" if it starts with weak directionality
characters.
> > Otherwise, concatenating "[weak text][ltr text]" to "[RTL TEXT]" would
yield:
> > Logical text: "[RTL TEXT][weak text][ltr text]"
> > Display text: "[ltr text][weak text][RTL TEXT]" < wrong, fixed by wrapping.
> > 
> > Perhaps we must wrap if the first char isn't strong LTR? Or always wrap?
> > (just in case LTR UI uses RTL strings somewhere like the example above?)
> 
> In English windows (windows xp), if a user does not install the RTL language
> package, LRE/PDF characters render as empty square. So, we do not want always
> wrapping.

Cool, this still avoids adding them in LTR for LTR text.
( I cc'ed myself on that issue http://crbug.com/99300 )
I guess those users see RTL text as empty squares anyway.

[commit-bot: I haz the power, 2012-09-10 22:47:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/msw@chromium.org/10910128/2003

[commit-bot: I haz the power, 2012-09-10 22:47:12]

Presubmit check for 10910128-2003 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    base/i18n

[msw, 2012-09-10 23:38:28]

Hey Jungshik, please take a look for OWNERS; thanks!

[msw, 2012-09-12 19:37:49]

Friendly ping; jshin, can you take a look?

[jungshik at Google, 2012-09-12 20:24:34]

Sorry for the delay. I have a couple of minor comments. Otherwise, looks good.

http://codereview.chromium.org/10910128/diff/2003/base/i18n/rtl.cc
File base/i18n/rtl.cc (right):

http://codereview.chromium.org/10910128/diff/2003/base/i18n/rtl.cc#newcode324
base/i18n/rtl.cc:324: if (!IsRTL() && GetFirstStrongCharacterDirection(text) ==
LEFT_TO_RIGHT)
Can we match the comment and if-statement? At first, I was confused because the
comment and conditions in if are opposite of each other. 

if (IsRTL() || GetFirst...() == RIGHT_TO_LEFT) {
   string16 text_mutable...
   ....
   return text_mutable;
}

return text;

}

BTW, I wonder about TODO comment in RenderText (from
https://chromiumcodereview.appspot.com/10693061 )

// Use the first strong character direction as the base text direction.
 601   // TODO(msw): Use the application text direction instead of LTR by
default?

> I guess those users see RTL text as empty squares anyway.

Actually, I believe Arial/Times New Roman in XP do have Hebrew (and possibly
Arabic) covered.

http://codereview.chromium.org/10910128/diff/2003/base/i18n/rtl_unittest.cc
File base/i18n/rtl_unittest.cc (right):

http://codereview.chromium.org/10910128/diff/2003/base/i18n/rtl_unittest.cc#n...
base/i18n/rtl_unittest.cc:298: EXPECT_EQ(test_case, adjusted_string) << " for
test case " << test_case;
nit: shouldn't we indicate whether it's RTL or LTR here (in place of 'locale' in
the current code)?

[msw, 2012-09-12 20:46:52]

Comments addressed, please take another look; thanks!

https://chromiumcodereview.appspot.com/10910128/diff/2003/base/i18n/rtl.cc
File base/i18n/rtl.cc (right):

https://chromiumcodereview.appspot.com/10910128/diff/2003/base/i18n/rtl.cc#ne...
base/i18n/rtl.cc:156: return LEFT_TO_RIGHT;
FYI: This is what my tangential RenderTextWin TODO is about.
base::i18n::GetFirstStrongCharacterDirection() returns LTR if there are no
strong characters, but perhaps should return RTL if the UI is RTL? I'm happy to
address this in a separate CL if you have advice.

https://chromiumcodereview.appspot.com/10910128/diff/2003/base/i18n/rtl.cc#ne...
base/i18n/rtl.cc:324: if (!IsRTL() && GetFirstStrongCharacterDirection(text) ==
LEFT_TO_RIGHT)
On 2012/09/12 20:24:35, Jungshik Shin wrote:
> Can we match the comment and if-statement? At first, I was confused because
the
> comment and conditions in if are opposite of each other. 
> 
> if (IsRTL() || GetFirst...() == RIGHT_TO_LEFT) {
>    string16 text_mutable...
>    ....
>    return text_mutable;
> }
> 
> return text;
> 
> }
> 
> BTW, I wonder about TODO comment in RenderText (from
> https://chromiumcodereview.appspot.com/10693061 )
> 
> // Use the first strong character direction as the base text direction.
>  601   // TODO(msw): Use the application text direction instead of LTR by
> default?
> 
> > I guess those users see RTL text as empty squares anyway.
> 
> Actually, I believe Arial/Times New Roman in XP do have Hebrew (and possibly
> Arabic) covered. 

Done, flipped condition & blocks.
See my comment above about the TODO.

https://chromiumcodereview.appspot.com/10910128/diff/2003/base/i18n/rtl_unitt...
File base/i18n/rtl_unittest.cc (right):

https://chromiumcodereview.appspot.com/10910128/diff/2003/base/i18n/rtl_unitt...
base/i18n/rtl_unittest.cc:298: EXPECT_EQ(test_case, adjusted_string) << " for
test case " << test_case;
On 2012/09/12 20:24:35, Jungshik Shin wrote:
> nit: shouldn't we indicate whether it's RTL or LTR here (in place of 'locale'
in
> the current code)? 

Done.

[jungshik at Google, 2012-09-12 22:14:01]

LGTM !


As for the tangential TODO in another file, what you proposed seems to make
sense, but we may have to check all the users of that function before changing
it.

[commit-bot: I haz the power, 2012-09-12 22:16:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/msw@chromium.org/10910128/2007

[commit-bot: I haz the power, 2012-09-13 01:47:38]

Change committed as 156459