Added a flag to the Canvas class which allows forcing an RTL directionality...

Added a flag to the Canvas class which allows forcing an RTL directionality
on text drawn using Canvas::DrawStringInt(). Using this flag, it is possible to
render RTL text correctly on platforms where LRE/PDF chars are not supported (e.g.
Windows XP with no RTL fonts installed).

BUG=23425
TEST=Run chrome and enter Hebrew text into the omnibox (see bug description). Make sure suggestions are formatted correctly. Verify in both RTL and LTR UIs.


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

[idana, 2009-11-10 21:12:28]

Reviewers: Peter Kasting,

Description:
Added a flag to the Canvas class which allows forcing an RTL directionality
on text drawn using Canvas::DrawStringInt(). Using this flag, it is  
possible to
render RTL text correctly on platforms where LRE/PDF chars are not supported
(e.g.
Windows XP with no RTL fonts installed).

BUG=23425
TEST=Run chrome and enter Hebrew text into the omnibox (see bug  
description).
Make sure suggestions are formatted correctly. Verify in both RTL and LTR  
UIs.


Please review this at http://codereview.chromium.org/384018

SVN Base: svn://chrome-svn/chrome/trunk/src/

Affected files:
   M     app/gfx/canvas.h
   M     app/gfx/canvas_win.cc
   M     chrome/browser/autocomplete/autocomplete.h
   M      
chrome/browser/views/autocomplete/autocomplete_popup_contents_view.cc

[Peter Kasting, 2009-11-10 21:26:50]

Basically looks good, I think a few spots could be written a bit more simply (to
avoid as many conditionals).

http://codereview.chromium.org/384018/diff/1/3
File app/gfx/canvas_win.cc (right):

http://codereview.chromium.org/384018/diff/1/3#newcode109
Line 109: if (l10n_util::GetTextDirection() == l10n_util::RIGHT_TO_LEFT &&
Nit: Can you expand this case and the comments instead of adding another
conditional?  I'm thinking:

if ((flags & gfx::Canvas::FORCE_RTL_DIRECTIONALITY) ||
    ((l10n_util::GetTextDirection() == l10n_util::RIGHT_TO_LEFT) &&
     (f & DT_RIGHT) && l10n_util::StringContainsStrongRTLChars(text)))
  f |= DT_RTLREADING;

http://codereview.chromium.org/384018/diff/1/5
File chrome/browser/views/autocomplete/autocomplete_popup_contents_view.cc
(right):

http://codereview.chromium.org/384018/diff/1/5#newcode520
Line 520: bool force_rtl_directionality = false;
Nit: I'd just do:

bool force_rtl =
    !(classifications[i].style & ACMatchClassification::URL) &&
    (run_direction == UBIDI_RTL) &&
    (l10n_util::GetTextDirection() == l10n_util::LEFT_TO_RIGHT);

http://codereview.chromium.org/384018/diff/1/5#newcode521
Line 521: if (!(classifications[i].style & ACMatchClassification::URL) &&
Nit: Can you note why URLs are excluded?  (I understand but another reader may
not)

http://codereview.chromium.org/384018/diff/1/5#newcode550
Line 550: int flags = 0;
Nit: I'd just do:

int flags = force_rtl_directionality ?
    gfx::Canvas::FORCE_RTL_DIRECTIONALITY : 0;

[Peter Kasting, 2009-11-10 21:27:08]

Basically looks good, I think a few spots could be written a bit more  
simply (to
avoid as many conditionals).


http://codereview.chromium.org/384018/diff/1/3
File app/gfx/canvas_win.cc (right):

http://codereview.chromium.org/384018/diff/1/3#newcode109
Line 109: if (l10n_util::GetTextDirection() == l10n_util::RIGHT_TO_LEFT
&&
Nit: Can you expand this case and the comments instead of adding another
conditional?  I'm thinking:

if ((flags & gfx::Canvas::FORCE_RTL_DIRECTIONALITY) ||
     ((l10n_util::GetTextDirection() == l10n_util::RIGHT_TO_LEFT) &&
      (f & DT_RIGHT) && l10n_util::StringContainsStrongRTLChars(text)))
   f |= DT_RTLREADING;

http://codereview.chromium.org/384018/diff/1/5
File
chrome/browser/views/autocomplete/autocomplete_popup_contents_view.cc
(right):

http://codereview.chromium.org/384018/diff/1/5#newcode520
Line 520: bool force_rtl_directionality = false;
Nit: I'd just do:

bool force_rtl =
     !(classifications[i].style & ACMatchClassification::URL) &&
     (run_direction == UBIDI_RTL) &&
     (l10n_util::GetTextDirection() == l10n_util::LEFT_TO_RIGHT);

http://codereview.chromium.org/384018/diff/1/5#newcode521
Line 521: if (!(classifications[i].style & ACMatchClassification::URL)
&&
Nit: Can you note why URLs are excluded?  (I understand but another
reader may not)

http://codereview.chromium.org/384018/diff/1/5#newcode550
Line 550: int flags = 0;
Nit: I'd just do:

int flags = force_rtl_directionality ?
     gfx::Canvas::FORCE_RTL_DIRECTIONALITY : 0;

http://codereview.chromium.org/384018

[idana, 2009-11-10 23:10:13]

Thanks for the suggestions, Peter. I changed the code based on your feedback and
uploaded a snapshot. Please have another look when you get a chance.

http://codereview.chromium.org/384018/diff/1/3
File app/gfx/canvas_win.cc (right):

http://codereview.chromium.org/384018/diff/1/3#newcode109
Line 109: if (l10n_util::GetTextDirection() == l10n_util::RIGHT_TO_LEFT &&
On 2009/11/10 21:26:50, Peter Kasting wrote:
> Nit: Can you expand this case and the comments instead of adding another
> conditional?  I'm thinking:
> 
> if ((flags & gfx::Canvas::FORCE_RTL_DIRECTIONALITY) ||
>     ((l10n_util::GetTextDirection() == l10n_util::RIGHT_TO_LEFT) &&
>      (f & DT_RIGHT) && l10n_util::StringContainsStrongRTLChars(text)))
>   f |= DT_RTLREADING;

Done.

http://codereview.chromium.org/384018/diff/1/5
File chrome/browser/views/autocomplete/autocomplete_popup_contents_view.cc
(right):

http://codereview.chromium.org/384018/diff/1/5#newcode520
Line 520: bool force_rtl_directionality = false;
On 2009/11/10 21:26:50, Peter Kasting wrote:
> Nit: I'd just do:
> 
> bool force_rtl =
>     !(classifications[i].style & ACMatchClassification::URL) &&
>     (run_direction == UBIDI_RTL) &&
>     (l10n_util::GetTextDirection() == l10n_util::LEFT_TO_RIGHT);
> 

Done.

http://codereview.chromium.org/384018/diff/1/5#newcode521
Line 521: if (!(classifications[i].style & ACMatchClassification::URL) &&
On 2009/11/10 21:26:50, Peter Kasting wrote:
> Nit: Can you note why URLs are excluded?  (I understand but another reader may
> not)

Done.

http://codereview.chromium.org/384018/diff/1/5#newcode550
Line 550: int flags = 0;
On 2009/11/10 21:26:50, Peter Kasting wrote:
> Nit: I'd just do:
> 
> int flags = force_rtl_directionality ?
>     gfx::Canvas::FORCE_RTL_DIRECTIONALITY : 0;

Done.

[idana, 2009-11-10 23:10:35]

Thanks for the suggestions, Peter. I changed the code based on your  
feedback and
uploaded a snapshot. Please have another look when you get a chance.


http://codereview.chromium.org/384018/diff/1/3
File app/gfx/canvas_win.cc (right):

http://codereview.chromium.org/384018/diff/1/3#newcode109
Line 109: if (l10n_util::GetTextDirection() == l10n_util::RIGHT_TO_LEFT
&&
On 2009/11/10 21:26:50, Peter Kasting wrote:
> Nit: Can you expand this case and the comments instead of adding
another
> conditional?  I'm thinking:

> if ((flags & gfx::Canvas::FORCE_RTL_DIRECTIONALITY) ||
>      ((l10n_util::GetTextDirection() == l10n_util::RIGHT_TO_LEFT) &&
>       (f & DT_RIGHT) && l10n_util::StringContainsStrongRTLChars(text)))
>    f |= DT_RTLREADING;

Done.

http://codereview.chromium.org/384018/diff/1/5
File
chrome/browser/views/autocomplete/autocomplete_popup_contents_view.cc
(right):

http://codereview.chromium.org/384018/diff/1/5#newcode520
Line 520: bool force_rtl_directionality = false;
On 2009/11/10 21:26:50, Peter Kasting wrote:
> Nit: I'd just do:

> bool force_rtl =
>      !(classifications[i].style & ACMatchClassification::URL) &&
>      (run_direction == UBIDI_RTL) &&
>      (l10n_util::GetTextDirection() == l10n_util::LEFT_TO_RIGHT);


Done.

http://codereview.chromium.org/384018/diff/1/5#newcode521
Line 521: if (!(classifications[i].style & ACMatchClassification::URL)
&&
On 2009/11/10 21:26:50, Peter Kasting wrote:
> Nit: Can you note why URLs are excluded?  (I understand but another
reader may
> not)

Done.

http://codereview.chromium.org/384018/diff/1/5#newcode550
Line 550: int flags = 0;
On 2009/11/10 21:26:50, Peter Kasting wrote:
> Nit: I'd just do:

> int flags = force_rtl_directionality ?
>      gfx::Canvas::FORCE_RTL_DIRECTIONALITY : 0;

Done.

http://codereview.chromium.org/384018

[Peter Kasting, 2009-11-10 23:31:36]

LGTM, thanks for fixing this!

[Peter Kasting, 2009-11-10 23:31:53]

LGTM, thanks for fixing this!

http://codereview.chromium.org/384018

[basesdedatosargentian2_yahoo.com, 2009-11-10 23:42:41]

--------------------------------------------------
From: <pkasting@chromium.org>
Sent: Tuesday, November 10, 2009 9:31 PM
To: <idana@chromium.org>
Cc: <chromium-reviews@googlegroups.com>; <ben+cc@chromium.org>; 
<cira@chromium.org>; <jshin@chromium.org>
Subject: [chromium-reviews] Re: Added a flag to the Canvas class which 
allows forcing an RTL directionality...

> LGTM, thanks for fixing this!
>
> http://codereview.chromium.org/384018