Fixed bidi RTL check failures on font settings page.

Fixed bidi RTL check failures on font settings page.


BUG=118420
TEST=browser_tests --gtest_filter=*.TestSettingsFrameFonts


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

[Danh Nguyen, 2012-03-23 14:17:29]

Hi Chris,

This would be my last fix on bidi test failures on the uber frames.  Please take
a look whenever you have a chance.

Thanks,
Danh

[Sheridan Rawlins, 2012-03-23 15:56:57]

http://codereview.chromium.org/9838055/diff/1/chrome/browser/resources/option...
File chrome/browser/resources/options2/font_settings.js (right):

http://codereview.chromium.org/9838055/diff/1/chrome/browser/resources/option...
chrome/browser/resources/options2/font_settings.js:176: option.dir = 'ltr';
How do we know it's ltr?  Is it ever bidi or rtl?

[Danh Nguyen, 2012-03-23 16:28:21]

Thanks Sheridan for taking a look.

Danh

http://codereview.chromium.org/9838055/diff/1/chrome/browser/resources/option...
File chrome/browser/resources/options2/font_settings.js (right):

http://codereview.chromium.org/9838055/diff/1/chrome/browser/resources/option...
chrome/browser/resources/options2/font_settings.js:176: option.dir = 'ltr';
On 2012/03/23 15:56:57, Sheridan Rawlins wrote:
> How do we know it's ltr?  Is it ever bidi or rtl?

Font names are not translated.  So I think they should be displayed as is, i.e.
'ltr'.  Their characters should not be reversed on a page with RTL language.

[csilv, 2012-04-10 23:35:32]

http://codereview.chromium.org/9838055/diff/1/chrome/browser/resources/option...
File chrome/browser/resources/options2/font_settings.js (right):

http://codereview.chromium.org/9838055/diff/1/chrome/browser/resources/option...
chrome/browser/resources/options2/font_settings.js:176: option.dir = 'ltr';
On 2012/03/23 16:28:21, Danh Nguyen wrote:
> On 2012/03/23 15:56:57, Sheridan Rawlins wrote:
> > How do we know it's ltr?  Is it ever bidi or rtl?
> 
> Font names are not translated.  So I think they should be displayed as is,
i.e.
> 'ltr'.  Their characters should not be reversed on a page with RTL language.

I'm not sure if this is correct.  Font names are not localized (by us), but they
may be localized by the OS and therefore could be RTL.  This is definitely the
case on Mac.

[Danh Nguyen, 2012-04-11 14:38:25]

On 2012/04/10 23:35:32, csilv wrote:
>
http://codereview.chromium.org/9838055/diff/1/chrome/browser/resources/option...
> File chrome/browser/resources/options2/font_settings.js (right):
> 
>
http://codereview.chromium.org/9838055/diff/1/chrome/browser/resources/option...
> chrome/browser/resources/options2/font_settings.js:176: option.dir = 'ltr';
> On 2012/03/23 16:28:21, Danh Nguyen wrote:
> > On 2012/03/23 15:56:57, Sheridan Rawlins wrote:
> > > How do we know it's ltr?  Is it ever bidi or rtl?
> > 
> > Font names are not translated.  So I think they should be displayed as is,
> i.e.
> > 'ltr'.  Their characters should not be reversed on a page with RTL language.
> 
> I'm not sure if this is correct.  Font names are not localized (by us), but
they
> may be localized by the OS and therefore could be RTL.  This is definitely the
> case on Mac.

Yes, I was incorrect on this one.  Fonts are loaded from the host machine, so
they must be localized.  I'm going to look for a different approach to solve
this issue.  Thanks, Chris.

Danh

[Danh Nguyen, 2012-04-23 20:38:59]

Hi Chris,

This is my renewed attempt on fixing this issue.  This change looks into the
font string and sets its directionality accordingly.

I've tested this change in both RTL and LTR settings(US English and Hebrew), and
seen the fonts translated and displayed as expected.

Could you take another look whenever you have a chance?

Thanks,
Danh

[csilv, 2012-04-25 17:35:38]

lgtm

[danhn, 2012-04-25 17:54:15]

Thanks Chris.

Danh

On Wed, Apr 25, 2012 at 10:35 AM, <csilv@chromium.org> wrote:

> lgtm
>
>
>
>
http://codereview.chromium.**org/9838055/<http://codereview.chromium.org/9838...
>

[commit-bot: I haz the power, 2012-04-25 19:50:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/danhn@chromium.org/9838055/7001

[commit-bot: I haz the power, 2012-04-25 21:26:17]

Change committed as 133983