Review request: fix issue 6099 -- bad display of multi-line English message in pop ups

This CL fixes issue 6099 -- bad display of multi-line English message in pop ups.
(http://crbug.com/6099)

Currently, in RTL locales, a pure *multi-line* English message is displayed as right aligned and has RTL directionality (such as the ending punctuation appears at the very left).

Single-line message works fine.

I do not know why, but simply putting LRE-PDF around a multi-line English text which has the following flags 
DT_WORDBREAK | DT_RTLREADING 
wont render the English message as LTR.
We have to remove the DT_RTLREADING to render multi-line English message correctly in LTR direction.


The fix is that in RTL locales, for JavaScript message, if the message is pure English, the alignment is set to be left-aligned, and the directionality is set to be left-to-right. If the message is mixed BiDi text, the alignment and the directionality is determined by the directionality of the first character with strong directionality in the text. 

JavaScript message is a MessageBoxView, and the message is a view::Label. Both MessageBoxView and Label are used by Chrome's UI  as well.
If the message is one of Chrome UI's, the alignment and directionality of the message should be taken from that of the UI's.

In order to distinguish where the message comes from, a new flag kFlagWebMessage is introduced in MessageBoxView, and a new argument is introduced in Label::SetHorizontalAlignment() to control whether the alignment need to be flipped or not for RTL locales.

Consequently, quite a few files which calls Label::SetHorizontalAlignment() are changed.

The main changes are in 5 areas. Other files are changed due to the signature change of Label::SetHorizontalAlignment().

1. jsmessage_box_handler.cc: pass in extra flag kFlagWebPage when creating MessageBoxView to indicate the message box is one from a web page, not from Chrome UI.

2. l10n_util.h/.cc
added function GetFirstStrongDirection()

3. label.h/.cc
a new argument is introduced in Label::SetHorizontalAlignment() to control whether the alignment need to be flipped or not for RTL locales.

4. message_box_view.cc
when init message box view, if the flag is kFlagWebPage, get the text directionality from the text itself (not Chrome UI's) and calls Label::SetHorizontalAlignment() to not resetting the flag for RTL locales.

5. chrome_canvas_win.cc
5.1 ComputeFormatFlags()
only set flag DT_RTLREADING for RTL locales if the text contains strong RTL characters and the alignment is RIGHT aligned. All labels of Chrome's UI and other Chrome UI components in RTL locales have been set as (or flipped to) RIGHT aligned. 

5.2 DoDrawText()
Only adjust string for locale is the reading direction is DT_RTLREADING.




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

[jeremy, 2009-01-28 20:47:02]

Very nice!

http://codereview.chromium.org/18863/diff/105/214
File chrome/browser/jsmessage_box_handler.cc (right):

http://codereview.chromium.org/18863/diff/105/214#newcode199
Line 199: message_text, default_prompt_text)) {
message_text should align under dialog_flags since this is an argument to new
MessageBoxView.

http://codereview.chromium.org/18863/diff/105/241
File chrome/common/gfx/chrome_canvas_win.cc (right):

http://codereview.chromium.org/18863/diff/105/241#newcode25
Line 25: // the string has RTL directionality.
maybe:
// Only adjust string directionality if both of the following are true:
//1. The current locale is RTL.
//2. The string itself has RTL directionality.

http://codereview.chromium.org/18863/diff/105/241#newcode27
Line 27: l10n_util::AdjustStringForLocaleDirection(text, &localized_text)) {
Could you move this out of the if and into a separate statement. Short circuit
logic can be confusing.

http://codereview.chromium.org/18863/diff/105/241#newcode90
Line 90: // locales.
How about:
"Caveat: If the string is purely LTR, don't set DTL_RTLREADING since when the
flag is set, LRE-PDF don't have the desired effect of rendering multiline
English-only text as LTR"

http://codereview.chromium.org/18863/diff/105/241#newcode99
Line 99: // DT_RIGHT.
How about something along the lines of:
"We make a distinction between Chrome UI strings and text coming from a web
page.

For text coming from a web page we determine the alignment based on the first
character with strong directionality.

This heuristic doesn't work for Chrome UI strings since even in RTL locales,
some of those might start with English text but we know they're localized so we
always want them to be RTL aligned.

We want UI strings in RTL locales to always be right aligned since they're
localized.

http://codereview.chromium.org/18863/diff/105/241#newcode100
Line 100: if (l10n_util::GetTextDirection() == l10n_util::RIGHT_TO_LEFT &&
How about nesting another if statement rather than the multiple &&s.

Short circuit logic makes things harder to understand.

http://codereview.chromium.org/18863/diff/105/240
File chrome/common/l10n_util.cc (right):

http://codereview.chromium.org/18863/diff/105/240#newcode507
Line 507: int position = 0;
any reason not to use a size_t rather than int?

http://codereview.chromium.org/18863/diff/105/240#newcode516
Line 516: if ((property == U_RIGHT_TO_LEFT) || (property ==
U_RIGHT_TO_LEFT_ARABIC) ||
I'd stick one condition on each line.

http://codereview.chromium.org/18863/diff/105/240#newcode524
Line 524: position = next_position;
Could you add a newline above this to separate it from the return.

Also, my preference is for curly braces around the body of ifs, IMHO it would
help readability in this case.

http://codereview.chromium.org/18863/diff/105/242
File chrome/common/l10n_util.h (right):

http://codereview.chromium.org/18863/diff/105/242#newcode112
Line 112: TextDirection GetFirstStrongDirection(const std::wstring& text);
Maybe we could name this something a tad more descriptive:
GetFirstStrongCharacterDirection() ?

Could you add some unit tests for this function?

http://codereview.chromium.org/18863/diff/105/244
File chrome/views/label.cc (right):

http://codereview.chromium.org/18863/diff/105/244#newcode235
Line 235: // directionality.
Could you update the comment adding info about the new flag.

http://codereview.chromium.org/18863/diff/105/249
File chrome/views/label.h (right):

http://codereview.chromium.org/18863/diff/105/249#newcode85
Line 85: // is flipped if |flip_alignment_for_rtl| is true.
Could you redo this comment to give a deeper explanation of why this is needed
and in what situations.

Also, the bool argument is confusing to me, maybe you could make this method
private and have two small inline methods with more descriptive names that call
through to this with true/false (better yet, an enum).

The main reason I say this is because looking at the callsites for this
function, it's very hard to tell why true/false need to be passed in.  If there
where a more descriptive function name without the extra argument it'd be a
little clearer.

http://codereview.chromium.org/18863/diff/105/250
File chrome/views/message_box_view.cc (right):

http://codereview.chromium.org/18863/diff/105/250#newcode105
Line 105: // by the directionality of the overall web page document
directionality.
Great comment!

Like in the header, maybe file a bug and point at it from here.

http://codereview.chromium.org/18863/diff/105/250#newcode106
Line 106: if (l10n_util::GetFirstStrongDirection(message_label_->GetText()) ==
IMHO reading the text direction into a temp variable and then using that in the
if would be a little clearer.

Also, how about computing the arguments to SetHorizontalAlignment() and then
making a single call with those parameters, rather than 4 separate calls.

http://codereview.chromium.org/18863/diff/105/248
File chrome/views/message_box_view.h (right):

http://codereview.chromium.org/18863/diff/105/248#newcode33
Line 33: // from Chrome's UI, it should inherit directionality from Chrome UI's.
Following -> The following.

This doesn't reflect the current behavior where the directionality for web
messages is picked based on a heuristic and not the web page's language.  Maybe
file a [low priority bug] about this and point to it from the comment.

Also, if you could add a brief summary of why we need to make this distinction.

[jungshik at Google, 2009-01-29 19:04:04]

Very nice, indeed !
I just looked at label.{h,cc}, msgbox_view.cc and l10util*. After you address my
comments and Jeremy's, I'll look at other files.

http://codereview.chromium.org/18863/diff/652/446
File chrome/common/l10n_util.cc (right):

http://codereview.chromium.org/18863/diff/652/446#newcode505
Line 505: const wchar_t* string = text.c_str();
Let's not introduce another port issue in this file. (I realize that this file
has several functions like that which needs to be changed. I'll clean that up
soon). wchar_t* on Linux/Mac is not UTF-16 but UTF-32. So, the above should
read:

#if defined(WCHAR_T_IS_UTF32)
string16 text_utf16 = WideToUTF16(text);
const wchar_t* string = text_utf16.c_str();
#else
const wchar_t* string = text.c_str();
#endif

Eventually, we have to replace most of std::wstring arguments with string16.

http://codereview.chromium.org/18863/diff/652/457
File chrome/common/l10n_util_unittest.cc (right):

http://codereview.chromium.org/18863/diff/652/457#newcode194
Line 194: bidi_rtl_first_with_weak_char));
Although not so likely to happen in practice, the default case (no character has
a strong directionarlity) is not tested here. An non-IDN URL may be an example.
 
How about different classes of RTL characters (RTL, RTL_ARABIC, RTL_EMBEDDING,
RTL_OVERRIDE)? 

BTW, I don't have a strong opinion, but it seems a bit redundant to have
separate variables for bidi_rtl_first_with_weak_char and other cases rather than
just using literal strings directly in EXPECT_EQ. 

One more edge case (that needs to be tested for the completeness) is RTL
characters in non-BMP (e.g. Phoenician letters :
http://demo.icu-project.org/icu-bin/ubrowse?scr=151&b=10910 )

To make that test pass on Linux and Mac, you need to do:

#if defined(WCHAR_T_IS_UTF32) 
std::wstring non_bmp_bidi_rtl(L" ! \x10910" L"abc 123");
std::wstring non_bmp_bidi_ltr(L" ! \x10401" L"abc 123");
#elif defined(WCHAR_T_IS_UTF16)
std::wstring non_bmp_bidi_rtl(L" ! \xHHHH\xLLLL" L"abc 
123");
.....
#else 
#error "wchar_t should be either UTF-16 or UTF-32"
#endif

http://codereview.chromium.org/18863/diff/652/450
File chrome/views/label.cc (right):

http://codereview.chromium.org/18863/diff/652/450#newcode239
Line 239: if (horiz_alignment_ != a) {
It's been like this for a while, but I wonder if this test should be done AFTER
recalculating 'a' using your logic below.

http://codereview.chromium.org/18863/diff/652/455
File chrome/views/label.h (right):

http://codereview.chromium.org/18863/diff/652/455#newcode89
Line 89: void SetHorizontalAlignment(Alignment a);
I was very confused by this comment until I saw actual callers in other files.
'The text's alignment is determined ..... the Chrome UI is' part is not
necessary for this function. This function does not do anything about that, but
just sets the alignment to |a|, isn't it?

http://codereview.chromium.org/18863/diff/652/455#newcode93
Line 93: // strings, in RTL locales, the stirng's alignment need to be flipped
around.
Flip |a| in a RTL locale and set the alignment to |a|. In a LTR locale, just set
the alignment to |a| (the same as SetHorizontalAlignment). For labels in Chrome
UI, this function should be used instead of SetHorizontalAlignment.

http://codereview.chromium.org/18863/diff/652/455#newcode176
Line 176: void SetAlignment(Alignment a, FlipAlignmentForRTL flip);
Why don't you move this function declaration next to SetHorizontalAlignment?
And, I'm not sure if it's wise to have separate helper functions
|SetHorizontalAlignment| and |SetHorizontalAlignmentWithFlipForRTL|. When I just
read this header file, I was really confused about differences among three
similar functions. With changes in comment I suggested, it may be clearer, but
still names are similar and a bit confusing.

You added them in patch set 2, but can you just do what you did in the patch set
1. If you think just using a boolean as the 2nd argument is not clear/readable
(at call sites), how about keeping enum (you added later) and use that as the
2nd argument?

[jeremy, 2009-01-29 19:34:04]

http://codereview.chromium.org/18863/diff/652/427
File chrome/browser/views/options/languages_page_view.cc (right):

http://codereview.chromium.org/18863/diff/652/427#newcode604
Line 604:
ui_language_label_->SetHorizontalAlignmentWithFlipForRTL(views::Label::ALIGN_LEFT);
>80 chars

http://codereview.chromium.org/18863/diff/652/446
File chrome/common/l10n_util.cc (right):

http://codereview.chromium.org/18863/diff/652/446#newcode507
Line 507: int position = 0;
Could you change this to use size_t, I realize the same code is in
StringContainsStrongRTLChars but I think that should be fixed as well.

[xji, 2009-01-30 00:39:40]

On 2009/01/29 19:04:04, Jungshik Shin wrote:
> Very nice, indeed !
> I just looked at label.{h,cc}, msgbox_view.cc and l10util*. After you address
my
> comments and Jeremy's, I'll look at other files.
> 
> http://codereview.chromium.org/18863/diff/652/446
> File chrome/common/l10n_util.cc (right):
> 
> http://codereview.chromium.org/18863/diff/652/446#newcode505
> Line 505: const wchar_t* string = text.c_str();
> Let's not introduce another port issue in this file. (I realize that this file
> has several functions like that which needs to be changed. I'll clean that up
> soon). wchar_t* on Linux/Mac is not UTF-16 but UTF-32. So, the above should
> read:
> 
> #if defined(WCHAR_T_IS_UTF32)
> string16 text_utf16 = WideToUTF16(text);
> const wchar_t* string = text_utf16.c_str();
> #else
> const wchar_t* string = text.c_str();
> #endif
> 
> Eventually, we have to replace most of std::wstring arguments with string16.

Changed for GetFirstStrongCharacterDirection() and
StringContainsStrongRTLChars()

> 
> http://codereview.chromium.org/18863/diff/652/457
> File chrome/common/l10n_util_unittest.cc (right):
> 
> http://codereview.chromium.org/18863/diff/652/457#newcode194
> Line 194: bidi_rtl_first_with_weak_char));
> Although not so likely to happen in practice, the default case (no character
has
> a strong directionarlity) is not tested here. An non-IDN URL may be an
example.
>  
> How about different classes of RTL characters (RTL, RTL_ARABIC, RTL_EMBEDDING,
> RTL_OVERRIDE)? 
> 
> BTW, I don't have a strong opinion, but it seems a bit redundant to have
> separate variables for bidi_rtl_first_with_weak_char and other cases rather
than
> just using literal strings directly in EXPECT_EQ. 
> 
> One more edge case (that needs to be tested for the completeness) is RTL
> characters in non-BMP (e.g. Phoenician letters :
> http://demo.icu-project.org/icu-bin/ubrowse?scr=151&b=10910 )
> 
> To make that test pass on Linux and Mac, you need to do:
> 
> #if defined(WCHAR_T_IS_UTF32) 
> std::wstring non_bmp_bidi_rtl(L" ! \x10910" L"abc 123");
> std::wstring non_bmp_bidi_ltr(L" ! \x10401" L"abc 123");
> #elif defined(WCHAR_T_IS_UTF16)
> std::wstring non_bmp_bidi_rtl(L" ! \xHHHH\xLLLL" L"abc 
> 123");
> .....
> #else 
> #error "wchar_t should be either UTF-16 or UTF-32"
> #endif

added test cases for:
all weak character string; empty string, string begins with LRE, LRO, AL, RLE,
RLO, and non-BMP.

> 
> http://codereview.chromium.org/18863/diff/652/450
> File chrome/views/label.cc (right):
> 
> http://codereview.chromium.org/18863/diff/652/450#newcode239
> Line 239: if (horiz_alignment_ != a) {
> It's been like this for a while, but I wonder if this test should be done
AFTER
> recalculating 'a' using your logic below.

You are right. Changed.

> 
> http://codereview.chromium.org/18863/diff/652/455
> File chrome/views/label.h (right):
> 
> http://codereview.chromium.org/18863/diff/652/455#newcode89
> Line 89: void SetHorizontalAlignment(Alignment a);
> I was very confused by this comment until I saw actual callers in other files.
> 'The text's alignment is determined ..... the Chrome UI is' part is not
> necessary for this function. This function does not do anything about that,
but
> just sets the alignment to |a|, isn't it?
> 
> http://codereview.chromium.org/18863/diff/652/455#newcode93
> Line 93: // strings, in RTL locales, the stirng's alignment need to be flipped
> around.
> Flip |a| in a RTL locale and set the alignment to |a|. In a LTR locale, just
set
> the alignment to |a| (the same as SetHorizontalAlignment). For labels in
Chrome
> UI, this function should be used instead of SetHorizontalAlignment.
> 
> http://codereview.chromium.org/18863/diff/652/455#newcode176
> Line 176: void SetAlignment(Alignment a, FlipAlignmentForRTL flip);
> Why don't you move this function declaration next to SetHorizontalAlignment?
> And, I'm not sure if it's wise to have separate helper functions
> |SetHorizontalAlignment| and |SetHorizontalAlignmentWithFlipForRTL|. When I
just
> read this header file, I was really confused about differences among three
> similar functions. With changes in comment I suggested, it may be clearer, but
> still names are similar and a bit confusing.
> 
> You added them in patch set 2, but can you just do what you did in the patch
set
> 1. If you think just using a boolean as the 2nd argument is not clear/readable
> (at call sites), how about keeping enum (you added later) and use that as the
> 2nd argument?

Keep SetHorizontalAlignment(Alignment, FlipAlignmentForRTL) as public.
And removed the 2 helper functions.

[xji, 2009-01-30 00:40:41]

On 2009/01/29 19:34:04, jeremy wrote:
> http://codereview.chromium.org/18863/diff/652/427
> File chrome/browser/views/options/languages_page_view.cc (right):
> 
> http://codereview.chromium.org/18863/diff/652/427#newcode604
> Line 604:
>
ui_language_label_->SetHorizontalAlignmentWithFlipForRTL(views::Label::ALIGN_LEFT);
> >80 chars
> 

Fixed.

> http://codereview.chromium.org/18863/diff/652/446
> File chrome/common/l10n_util.cc (right):
> 
> http://codereview.chromium.org/18863/diff/652/446#newcode507
> Line 507: int position = 0;
> Could you change this to use size_t, I realize the same code is in
> StringContainsStrongRTLChars but I think that should be fixed as well.

Changed for both StringContainsStrongRTLChars and
GetFirstStrongCharacterDirection

[jeremy, 2009-01-30 05:44:04]

lgtm

[jungshik at Google, 2009-01-30 20:45:27]

lgtm with the following addressed.

http://codereview.chromium.org/18863/diff/670/698
File chrome/common/l10n_util.cc (right):

http://codereview.chromium.org/18863/diff/670/698#newcode507
Line 507: const wchar_t* string = text_utf16.c_str();
Sorry if I told you otherwise. This should be 

const UChar* string = text_utf16.c_str();

c_str() returns uint16_t* (not wchar_t*)

http://codereview.chromium.org/18863/diff/670/698#newcode509
Line 509: const wchar_t* string = text.c_str();
the same here although on Windows, wchar_t and UChar are identical.

http://codereview.chromium.org/18863/diff/670/698#newcode558
Line 558: const wchar_t* string = text_utf16.c_str();
same here

http://codereview.chromium.org/18863/diff/670/700
File chrome/common/l10n_util.h (right):

http://codereview.chromium.org/18863/diff/670/700#newcode108
Line 108: // in the text has strong directionality, return LEFT_TO_RIGHT as
default. Bidi
nit: If no character in the ext has strong directionality

[xji, 2009-01-31 06:17:30]

On 2009/01/30 20:45:27, Jungshik Shin wrote:
> lgtm with the following addressed.
> 
> http://codereview.chromium.org/18863/diff/670/698
> File chrome/common/l10n_util.cc (right):
> 
> http://codereview.chromium.org/18863/diff/670/698#newcode507
> Line 507: const wchar_t* string = text_utf16.c_str();
> Sorry if I told you otherwise. This should be 
> 
> const UChar* string = text_utf16.c_str();
> 
> c_str() returns uint16_t* (not wchar_t*)

done

> 
> http://codereview.chromium.org/18863/diff/670/698#newcode509
> Line 509: const wchar_t* string = text.c_str();
> the same here although on Windows, wchar_t and UChar are identical.

done.
And changed the variable "character" inside the loop from UChar32 to UChar.


> 
> http://codereview.chromium.org/18863/diff/670/698#newcode558
> Line 558: const wchar_t* string = text_utf16.c_str();
> same here

done

> 
> http://codereview.chromium.org/18863/diff/670/700
> File chrome/common/l10n_util.h (right):
> 
> http://codereview.chromium.org/18863/diff/670/700#newcode108
> Line 108: // in the text has strong directionality, return LEFT_TO_RIGHT as
> default. Bidi
> nit: If no character in the ext has strong directionality

done

[idana, 2009-02-02 01:41:52]

http://codereview.chromium.org/18863/diff/493/728
File chrome/browser/jsmessage_box_handler.cc (right):

http://codereview.chromium.org/18863/diff/493/728#newcode197
Line 197: message_box_view_(new MessageBoxView(
Alignment of "message_box_view_(..." is incorrect.

http://codereview.chromium.org/18863/diff/493/755
File chrome/common/gfx/chrome_canvas_win.cc (right):

http://codereview.chromium.org/18863/diff/493/755#newcode102
Line 102: // localized so we always want them to be RTL aligned. Chrome UI
strings in
"be RTL aligned." -> "be right aligned."

The following sentence ("Chrome UI strings in...") should be removed because it
is redundant. You should keep the Caveat paragraph at the end, though.

http://codereview.chromium.org/18863/diff/493/754
File chrome/common/l10n_util.cc (right):

http://codereview.chromium.org/18863/diff/493/754#newcode563
Line 563: size_t position = 0;
I can not recall why I originally wrote this code using int and static_cast...

Can you let someone else look at this code snippet to make sure there are no
hidden bugs (maybe brettw can take a look?).

http://codereview.chromium.org/18863/diff/493/756
File chrome/common/l10n_util.h (right):

http://codereview.chromium.org/18863/diff/493/756#newcode107
Line 107: // character with strong directionality in the |text|. If no character
in the
"in the |text|" -> "in the string"

http://codereview.chromium.org/18863/diff/493/756#newcode108
Line 108: // text has strong directionality, return LEFT_TO_RIGHT as default.
Bidi
"return LEFT_TO_RIGHT as default" -> "LEFT_TO_RIGHT is returned"

http://codereview.chromium.org/18863/diff/493/756#newcode110
Line 110: // directionality. Please refer to http://unicode.org/reports/tr9/ for
more
"strong directionality." -> "strong directionality characters."

http://codereview.chromium.org/18863/diff/493/763
File chrome/views/label.h (right):

http://codereview.chromium.org/18863/diff/493/763#newcode30
Line 30: // control whether the input alignment need to be flipped around in RTL
"need" -> "needs"

http://codereview.chromium.org/18863/diff/493/763#newcode31
Line 31: // locales. NOT_FLIP need to used if the label text is from a web page.
FLIP
"need to used" -> "should be used"

http://codereview.chromium.org/18863/diff/493/763#newcode32
Line 32: // need to be used if the label text is from Chrome UI. Please refer to
the
"need to be used" -> "should be used"
"from Chrome UI" -> "from the Chrome UI"

http://codereview.chromium.org/18863/diff/493/763#newcode35
Line 35: NOT_FLIP };
I think the naming here is a little ambiguous (when one looks at a call site and
sees "FLIP" or "NO_FLIP", it is hard to infer what they mean). Perhaps this will
be more clear:

enum RTLAlignmentSetting { 
  FLIP_ALIGNMENT_FOR_RTL_LOCALES = 0,
  DO_NOT_FLIP_ALIGNMENT_FOR_RTL_LOCALES
};

http://codereview.chromium.org/18863/diff/493/763#newcode92
Line 92: // Set horizontal alignment. If the locale is RTL locale, and the
|flip| is
"If the locale is RTL locale" -> "If the locale is RTL"

http://codereview.chromium.org/18863/diff/493/763#newcode95
Line 95: // |flip| in introduced to distinguish whether the label text is from a
web
"in introduced to distinguish" -> "indicates"

http://codereview.chromium.org/18863/diff/493/763#newcode96
Line 96: // page or from Chrome UI.
"or from Chrome UI" -> "or from the Chrome UI"

http://codereview.chromium.org/18863/diff/493/763#newcode98
Line 98: // If the label text comes from Chrome UI, |flip| should be set as
FLIP, so
"from Chrome UI" -> "from the Chrome UI"
"should be set as" -> "should be set to"

http://codereview.chromium.org/18863/diff/493/763#newcode101
Line 101: // If the text comes from a web page. The text's alignment is
determined
"a web page. The" -> "a web page, the"

http://codereview.chromium.org/18863/diff/493/763#newcode103
Line 103: // directionality the Chrome UI is. So, the |flip| should be set as
NO_FLIP,
"So, the |flip| should" -> "Thus, |flip| should"

http://codereview.chromium.org/18863/diff/493/763#newcode106
Line 106: void SetHorizontalAlignment(Alignment a, FlipAlignmentForRTL flip);
I wonder if it is going to better to keep the signature of this function as is
(without the new enum parameter) and add a member variable which indicates the
flip settings. This member should be initialized to FLIP by default and it can
be set by a public setter routine. This way, most of the files you changed in
this patch won't need to change because they'll get the same behavior. The only
change will be in the js message box because it needs to specifically change the
default to NO_FLIP.

http://codereview.chromium.org/18863/diff/493/759
File chrome/views/label_unittest.cc (right):

http://codereview.chromium.org/18863/diff/493/759#newcode63
Line 63: EXPECT_EQ(Label::ALIGN_CENTER, label.GetHorizontalAlignment());
You should add test cases for the newly added flag.

http://codereview.chromium.org/18863/diff/493/764
File chrome/views/message_box_view.cc (right):

http://codereview.chromium.org/18863/diff/493/764#newcode106
Line 106: // Please refer to http://crbug.com/7166 for more information.
These two comment paragraphs are very similar to the ones in the .h file so
please remove them. Instead of these two paragraph, it is sufficient to include
a comment saying: "If the message originates from a web page, we should
determine the directionality based on the first character with strong
directionality."

http://codereview.chromium.org/18863/diff/493/762
File chrome/views/message_box_view.h (right):

http://codereview.chromium.org/18863/diff/493/762#newcode29
Line 29: // box is a string of Chrome UI's or a message of a web page (such as
the
"differentiate whether the message in message box is a string of Chrome UI's or
a message of a web page" -> "differentiate between text originating from the
Chrome UI and text originating from a web page"

http://codereview.chromium.org/18863/diff/493/762#newcode31
Line 31: // message's directionality. If the message is from a web page, its
"Such differentiation is used to determine message's directionality" -> "The
origin of the text is used to determine its directionality"

http://codereview.chromium.org/18863/diff/493/762#newcode33
Line 33: // But Chrome UI strings are localized strings, they should inherit
"But Chrome UI strings are localized string, they should inherit directionality
from Chrome UI's." -> "Chrome UI strings are localized string and therefore they
should have the same directionality as that of the Chrome UI."

http://codereview.chromium.org/18863/diff/493/762#newcode34
Line 34: // directionality from Chrome UI's. For example, in RTL locales, even
some
"even some" -> "even though some"

http://codereview.chromium.org/18863/diff/493/762#newcode39
Line 39: // directionality from the web page it is from. Please refer to
"it should inherit directionality from the web page it is from" -> "then the
message directionality should be determined based on the directionality of the
web page"

[jungshik at Google, 2009-02-02 19:17:57]

http://codereview.chromium.org/18863/diff/493/754
File chrome/common/l10n_util.cc (right):

http://codereview.chromium.org/18863/diff/493/754#newcode514
Line 514: UChar character;
argh.. This should remain  UChar32 ;-). 
What U16_NEXT does is to iterate over a UChar* array (a string in UTF-16) and
store the Unicode codepoint (UChar32) in |character| taking care of surrogate
pairs.

http://codereview.chromium.org/18863/diff/493/754#newcode565
Line 565: UChar character;
again, this should be UChar32 as it was before.

[xji, 2009-02-02 23:53:06]

On 2009/02/02 01:41:52, idana wrote:
> http://codereview.chromium.org/18863/diff/493/728
> File chrome/browser/jsmessage_box_handler.cc (right):
> 
> http://codereview.chromium.org/18863/diff/493/728#newcode197
> Line 197: message_box_view_(new MessageBoxView(
> Alignment of "message_box_view_(..." is incorrect.

done.

> 
> http://codereview.chromium.org/18863/diff/493/755
> File chrome/common/gfx/chrome_canvas_win.cc (right):
> 
> http://codereview.chromium.org/18863/diff/493/755#newcode102
> Line 102: // localized so we always want them to be RTL aligned. Chrome UI
> strings in
> "be RTL aligned." -> "be right aligned."

done

> 
> The following sentence ("Chrome UI strings in...") should be removed because
it
> is redundant. You should keep the Caveat paragraph at the end, though.

done

> 
> http://codereview.chromium.org/18863/diff/493/754
> File chrome/common/l10n_util.cc (right):
> 
> http://codereview.chromium.org/18863/diff/493/754#newcode563
> Line 563: size_t position = 0;
> I can not recall why I originally wrote this code using int and static_cast...
> 
> Can you let someone else look at this code snippet to make sure there are no
> hidden bugs (maybe brettw can take a look?).

will ask brettw.

> 
> http://codereview.chromium.org/18863/diff/493/756
> File chrome/common/l10n_util.h (right):
> 
> http://codereview.chromium.org/18863/diff/493/756#newcode107
> Line 107: // character with strong directionality in the |text|. If no
character
> in the
> "in the |text|" -> "in the string"

done

> 
> http://codereview.chromium.org/18863/diff/493/756#newcode108
> Line 108: // text has strong directionality, return LEFT_TO_RIGHT as default.
> Bidi
> "return LEFT_TO_RIGHT as default" -> "LEFT_TO_RIGHT is returned"

done

> 
> http://codereview.chromium.org/18863/diff/493/756#newcode110
> Line 110: // directionality. Please refer to http://unicode.org/reports/tr9/
for
> more
> "strong directionality." -> "strong directionality characters."

done

> 
> http://codereview.chromium.org/18863/diff/493/763
> File chrome/views/label.h (right):
> 
> http://codereview.chromium.org/18863/diff/493/763#newcode30
> Line 30: // control whether the input alignment need to be flipped around in
RTL
> "need" -> "needs"

done

> 
> http://codereview.chromium.org/18863/diff/493/763#newcode31
> Line 31: // locales. NOT_FLIP need to used if the label text is from a web
page.
> FLIP
> "need to used" -> "should be used"

done


> 
> http://codereview.chromium.org/18863/diff/493/763#newcode32
> Line 32: // need to be used if the label text is from Chrome UI. Please refer
to
> the
> "need to be used" -> "should be used"
> "from Chrome UI" -> "from the Chrome UI"

done

> 
> http://codereview.chromium.org/18863/diff/493/763#newcode35
> Line 35: NOT_FLIP };
> I think the naming here is a little ambiguous (when one looks at a call site
and
> sees "FLIP" or "NO_FLIP", it is hard to infer what they mean). Perhaps this
will
> be more clear:
> 
> enum RTLAlignmentSetting { 
>   FLIP_ALIGNMENT_FOR_RTL_LOCALES = 0,
>   DO_NOT_FLIP_ALIGNMENT_FOR_RTL_LOCALES
> };

done


> 
> http://codereview.chromium.org/18863/diff/493/763#newcode92
> Line 92: // Set horizontal alignment. If the locale is RTL locale, and the
> |flip| is
> "If the locale is RTL locale" -> "If the locale is RTL"

done

> 
> http://codereview.chromium.org/18863/diff/493/763#newcode95
> Line 95: // |flip| in introduced to distinguish whether the label text is from
a
> web
> "in introduced to distinguish" -> "indicates"

done

> 
> http://codereview.chromium.org/18863/diff/493/763#newcode96
> Line 96: // page or from Chrome UI.
> "or from Chrome UI" -> "or from the Chrome UI"

done


> 
> http://codereview.chromium.org/18863/diff/493/763#newcode98
> Line 98: // If the label text comes from Chrome UI, |flip| should be set as
> FLIP, so
> "from Chrome UI" -> "from the Chrome UI"
> "should be set as" -> "should be set to"

done


> 
> http://codereview.chromium.org/18863/diff/493/763#newcode101
> Line 101: // If the text comes from a web page. The text's alignment is
> determined
> "a web page. The" -> "a web page, the"

done

> 
> http://codereview.chromium.org/18863/diff/493/763#newcode103
> Line 103: // directionality the Chrome UI is. So, the |flip| should be set as
> NO_FLIP,
> "So, the |flip| should" -> "Thus, |flip| should"

done

> 
> http://codereview.chromium.org/18863/diff/493/763#newcode106
> Line 106: void SetHorizontalAlignment(Alignment a, FlipAlignmentForRTL flip);
> I wonder if it is going to better to keep the signature of this function as is
> (without the new enum parameter) and add a member variable which indicates the
> flip settings. This member should be initialized to FLIP by default and it can
> be set by a public setter routine. This way, most of the files you changed in
> this patch won't need to change because they'll get the same behavior. The
only
> change will be in the js message box because it needs to specifically change
the
> default to NO_FLIP.

I think both have pro and con.
Keeping the old signature and introducing a member variable and a setter makes
users have to call SetRTLAlignmentSetting() and SetHorizontalAlignment() to set
alignment of label originates from a web page. While
SetHorizontalAlignment(Alignment, enum) makes the operation atomic.

But in the other hand, since the majority of the operations are done to  Chrome
UI, it is a good idea to keep them untouched, by default, and as easy as
possible.

I changed it per your suggestion.

> 
> http://codereview.chromium.org/18863/diff/493/759
> File chrome/views/label_unittest.cc (right):
> 
> http://codereview.chromium.org/18863/diff/493/759#newcode63
> Line 63: EXPECT_EQ(Label::ALIGN_CENTER, label.GetHorizontalAlignment());
> You should add test cases for the newly added flag.

added new test cases for the new functionality introduced.

> 
> http://codereview.chromium.org/18863/diff/493/764
> File chrome/views/message_box_view.cc (right):
> 
> http://codereview.chromium.org/18863/diff/493/764#newcode106
> Line 106: // Please refer to http://crbug.com/7166 for more information.
> These two comment paragraphs are very similar to the ones in the .h file so
> please remove them. Instead of these two paragraph, it is sufficient to
include
> a comment saying: "If the message originates from a web page, we should
> determine the directionality based on the first character with strong
> directionality."

done.

> 
> http://codereview.chromium.org/18863/diff/493/762
> File chrome/views/message_box_view.h (right):
> 
> http://codereview.chromium.org/18863/diff/493/762#newcode29
> Line 29: // box is a string of Chrome UI's or a message of a web page (such as
> the
> "differentiate whether the message in message box is a string of Chrome UI's
or
> a message of a web page" -> "differentiate between text originating from the
> Chrome UI and text originating from a web page"

done

> 
> http://codereview.chromium.org/18863/diff/493/762#newcode31
> Line 31: // message's directionality. If the message is from a web page, its
> "Such differentiation is used to determine message's directionality" -> "The
> origin of the text is used to determine its directionality"

done

> 
> http://codereview.chromium.org/18863/diff/493/762#newcode33
> Line 33: // But Chrome UI strings are localized strings, they should inherit
> "But Chrome UI strings are localized string, they should inherit
directionality
> from Chrome UI's." -> "Chrome UI strings are localized string and therefore
they
> should have the same directionality as that of the Chrome UI."

done

> 
> http://codereview.chromium.org/18863/diff/493/762#newcode34
> Line 34: // directionality from Chrome UI's. For example, in RTL locales, even
> some
> "even some" -> "even though some"

done

> 
> http://codereview.chromium.org/18863/diff/493/762#newcode39
> Line 39: // directionality from the web page it is from. Please refer to
> "it should inherit directionality from the web page it is from" -> "then the
> message directionality should be determined based on the directionality of the
> web page"

done

[xji, 2009-02-02 23:54:03]

On 2009/02/02 19:17:57, Jungshik Shin wrote:
> http://codereview.chromium.org/18863/diff/493/754
> File chrome/common/l10n_util.cc (right):
> 
> http://codereview.chromium.org/18863/diff/493/754#newcode514
> Line 514: UChar character;
> argh.. This should remain  UChar32 ;-). 
> What U16_NEXT does is to iterate over a UChar* array (a string in UTF-16) and
> store the Unicode codepoint (UChar32) in |character| taking care of surrogate
> pairs.

Done. I should be more careful :(

> 
> http://codereview.chromium.org/18863/diff/493/754#newcode565
> Line 565: UChar character;
> again, this should be UChar32 as it was before.

done

[xji, 2009-02-02 23:59:30]

Hi Brett,

I made some variable type change in l10n_util::StringContainsStrongRTLChars().

Previously, the "length" and "position" variables are declared as "int" or
statically casted to "int".

I changed them to size_t.

I checked with Idan, and he can not recall why he originally wrote this code
using int and static_cast. 

He suggested me to check with you  to make sure there
are no hidden bugs.

Could you please take a look at the above code snippet?

Thanks,
Xiaomei

[brettw, 2009-02-06 19:05:00]

Mostly looks good, I have a few naming suggestions.

http://codereview.chromium.org/18863/diff/1447/2448
File chrome/views/label.h (right):

http://codereview.chromium.org/18863/diff/1447/2448#newcode41
Line 41: DO_NOT_FLIP_ALIGNMENT_FOR_RTL_LOCALES
It seems like this second mode is really more like "autodetect alignment",
rather than "don't flip". Does this sound right? If so, can you change the name
to reflect that?

I also wonder whether FLIP_ALIGNMENT_FOR_RTL_LOCALES might be more clear as
something like USE_UI_DIRECTIONALITY or USE_UI_ALIGNMENT.

http://codereview.chromium.org/18863/diff/1447/2447
File chrome/views/message_box_view.h (right):

http://codereview.chromium.org/18863/diff/1447/2447#newcode41
Line 41: static const int kFlagWebMessage = 0x10;
I wonder if calling this kAutoDetectDirecionality or ...Alighnment would be more
clear. Then it describes what you want to happen rather than where the message
came from, which I think is a bit more obvious. Of course, your comment should
still keep the description about when you would want to use it or not.

[xji, 2009-02-06 22:33:58]

Hi Brett,

Thanks for your review!

As to the naming convention, those names/flags are introduced based on the
reason that we need to differentiate whether a message originates from a webpage
or from the Chrome UI.

If the message is from a web page, it inherits its directionality from  that of
the web page, that is why I introduced kFlagWebMessage. And the directionality
wont be changed based on Chrome UI directionality, that is why Idan and I chosed
the name DO_NOT_FLIP_ALIGNMENT_FOR_RTL_LOCALES.

I prefer to keep the current name other than using name "autodetect", due to the
above 2 reasons.

As to USE_UI_ALIGNMENT, it is not exactly the same as
FLIP_ALIGNMENT_FOR_RTL_LOCALES.
Flip is more like a toggle operation. If a label is right aligned in LTR UI, it
will be changed to left aligned in RTL UI, which is nto the same as the
UI_ALIGNMENT.

Thanks,
Xiaomei



On 2009/02/06 19:05:00, brettw wrote:
> Mostly looks good, I have a few naming suggestions.
> 
> http://codereview.chromium.org/18863/diff/1447/2448
> File chrome/views/label.h (right):
> 
> http://codereview.chromium.org/18863/diff/1447/2448#newcode41
> Line 41: DO_NOT_FLIP_ALIGNMENT_FOR_RTL_LOCALES
> It seems like this second mode is really more like "autodetect alignment",
> rather than "don't flip". Does this sound right? If so, can you change the
name
> to reflect that?
> 
> I also wonder whether FLIP_ALIGNMENT_FOR_RTL_LOCALES might be more clear as
> something like USE_UI_DIRECTIONALITY or USE_UI_ALIGNMENT.
> 
> http://codereview.chromium.org/18863/diff/1447/2447
> File chrome/views/message_box_view.h (right):
> 
> http://codereview.chromium.org/18863/diff/1447/2447#newcode41
> Line 41: static const int kFlagWebMessage = 0x10;
> I wonder if calling this kAutoDetectDirecionality or ...Alighnment would be
more
> clear. Then it describes what you want to happen rather than where the message
> came from, which I think is a bit more obvious. Of course, your comment should
> still keep the description about when you would want to use it or not.

[brettw, 2009-02-06 23:08:11]

On 2009/02/06 22:33:58, xji wrote:
> As to the naming convention, those names/flags are introduced based on the
> reason that we need to differentiate whether a message originates from a
webpage
> or from the Chrome UI.
> 
> If the message is from a web page, it inherits its directionality from  that
of
> the web page, that is why I introduced kFlagWebMessage. And the directionality
> wont be changed based on Chrome UI directionality, that is why Idan and I
chosed
> the name DO_NOT_FLIP_ALIGNMENT_FOR_RTL_LOCALES.
> 
> I prefer to keep the current name other than using name "autodetect", due to
the
> above 2 reasons.

Yes, I understand why you picked these names. But the views layer is the user
interface layer, sort of line Windows. It seems to me that it should not be
responsible for making these policy decisions about web content, for the same
reason that such a flag would not be added to the Win32 API. Rather, that should
be the responsibility of the code calling it to enable or disable the
appropriate features in the presentation layer to achieve the appropriate
effect.

I'm CCing Ben who may have some comments.

[idana, 2009-02-08 05:40:10]

A few more nit comments.

http://codereview.chromium.org/18863/diff/2469/2472
File chrome/common/l10n_util.h (right):

http://codereview.chromium.org/18863/diff/2469/2472#newcode115
Line 115: // type L, LRE, LRO, R, AL, RLE, and RLO are considered as strong
"Bidi character type L, LRE" -> "The Bidi character types L, LRE"

http://codereview.chromium.org/18863/diff/2469/2473
File chrome/views/label.cc (right):

http://codereview.chromium.org/18863/diff/2469/2473#newcode37
Line 37: : rtl_alignment_setting_(FLIP_ALIGNMENT_FOR_RTL_LOCALES) {
It is probably better to set rtl_alignment_setting_ inside Label::Init() instead
of doing it three times for the three constructors.

http://codereview.chromium.org/18863/diff/2469/2475
File chrome/views/label.h (right):

http://codereview.chromium.org/18863/diff/2469/2475#newcode103
Line 103: // Caveat: for labels originate from a web page, its RTL alignment
setting
"originate" -> "originating"
"its RTL alignment setting" -> "the RTL alignment setting"

http://codereview.chromium.org/18863/diff/2469/2475#newcode104
Line 104: // should be reset to DO_NOT_FLIP_ALIGNMENT_FOR_RTL_LOCALES before its
"its" -> "the"

http://codereview.chromium.org/18863/diff/2469/2475#newcode105
Line 105: // horizontal alignment is set. Otherwise, its input alignment will be
"Otherwise, its input alignment will be flipped" -> "Otherwise, the label's
alignment specified as a parameter will be flipped"

http://codereview.chromium.org/18863/diff/2469/2475#newcode112
Line 112: // Set RTL alignment setting. RTL alignment setting is initialized to
"Set RTL alignment setting. RTL alignment setting" -> "Set the RTL alignment
setting. The RTL alignment setting"

http://codereview.chromium.org/18863/diff/2469/2475#newcode113
Line 113: // FLIP_ALIGNMENT_FOR_RTL_LOCALES when label is constructed.
"when label is constructed" -> "when the label is constructed"

http://codereview.chromium.org/18863/diff/2469/2475#newcode115
Line 115: // the Chrome UI. But if a label originates from a web page, its
alignment
"But if a label" -> "However, if the label"

http://codereview.chromium.org/18863/diff/2469/2475#newcode116
Line 116: // should not be flipped around for RTL locales. For such label, we
need to
"For such label," -> "For such labels,"

http://codereview.chromium.org/18863/diff/2469/2475#newcode117
Line 117: // set RTL alignment setting to DO_NOT_FLIP_ALIGNMENT_FOR_RTL_LOCALES
so that
"set RTL alignment" -> "set the RTL alignment"

http://codereview.chromium.org/18863/diff/2469/2475#newcode118
Line 118: // subsequent SetHorizontalAlignment() call will not flip the input
alignment
"call" -> "calls"
"flip the input alignment" -> "flip the label's alignment"

http://codereview.chromium.org/18863/diff/2469/2478
File chrome/views/label_unittest.cc (right):

http://codereview.chromium.org/18863/diff/2469/2478#newcode65
Line 65: // Alignment is not flipped if RTL alignment setting is
"Alignment is not flipped if RTL alignment setting is" -> "The label's alignment
should not be flipped if the RTL alignment setting is"

http://codereview.chromium.org/18863/diff/2469/2476
File chrome/views/message_box_view.cc (right):

http://codereview.chromium.org/18863/diff/2469/2476#newcode108
Line 108: // And we should set the RTL alignment setting as
"And we should" -> "In addition, we should"

[xji, 2009-02-11 01:57:22]

Hi Brett, 

Idan discussed those naming with me.
He thinks your point about kFlagWebMessage is pretty valid.
And I will change kFlagWebMessage to kAutoDetectAlignment.

As to the DO_NOT_FLIP_ALIGNMENT_FOR_RTL_LOCALES and 
FLIP_ALIGNMENT_FOR_RTL_LOCALES, we think we should leave the names the way they
are because we are literally flipping the alignment.

Or you feel strongly that we should change them as well?

Thanks,
Xiaomei


On 2009/02/06 19:05:00, brettw wrote:
> Mostly looks good, I have a few naming suggestions.
> 
> http://codereview.chromium.org/18863/diff/1447/2448
> File chrome/views/label.h (right):
> 
> http://codereview.chromium.org/18863/diff/1447/2448#newcode41
> Line 41: DO_NOT_FLIP_ALIGNMENT_FOR_RTL_LOCALES
> It seems like this second mode is really more like "autodetect alignment",
> rather than "don't flip". Does this sound right? If so, can you change the
name
> to reflect that?
> 
> I also wonder whether FLIP_ALIGNMENT_FOR_RTL_LOCALES might be more clear as
> something like USE_UI_DIRECTIONALITY or USE_UI_ALIGNMENT.
> 
> http://codereview.chromium.org/18863/diff/1447/2447
> File chrome/views/message_box_view.h (right):
> 
> http://codereview.chromium.org/18863/diff/1447/2447#newcode41
> Line 41: static const int kFlagWebMessage = 0x10;
> I wonder if calling this kAutoDetectDirecionality or ...Alighnment would be
more
> clear. Then it describes what you want to happen rather than where the message
> came from, which I think is a bit more obvious. Of course, your comment should
> still keep the description about when you would want to use it or not.

[xji, 2009-02-11 02:17:32]

All changed per suggestion.

Also, changed the kFlagWebMessage to kAutoDetectAlignment, and slightly changed
the related comments.

On 2009/02/08 05:40:10, idana wrote:
> A few more nit comments.
> 
> http://codereview.chromium.org/18863/diff/2469/2472
> File chrome/common/l10n_util.h (right):
> 
> http://codereview.chromium.org/18863/diff/2469/2472#newcode115
> Line 115: // type L, LRE, LRO, R, AL, RLE, and RLO are considered as strong
> "Bidi character type L, LRE" -> "The Bidi character types L, LRE"
> 
> http://codereview.chromium.org/18863/diff/2469/2473
> File chrome/views/label.cc (right):
> 
> http://codereview.chromium.org/18863/diff/2469/2473#newcode37
> Line 37: : rtl_alignment_setting_(FLIP_ALIGNMENT_FOR_RTL_LOCALES) {
> It is probably better to set rtl_alignment_setting_ inside Label::Init()
instead
> of doing it three times for the three constructors.
> 
> http://codereview.chromium.org/18863/diff/2469/2475
> File chrome/views/label.h (right):
> 
> http://codereview.chromium.org/18863/diff/2469/2475#newcode103
> Line 103: // Caveat: for labels originate from a web page, its RTL alignment
> setting
> "originate" -> "originating"
> "its RTL alignment setting" -> "the RTL alignment setting"
> 
> http://codereview.chromium.org/18863/diff/2469/2475#newcode104
> Line 104: // should be reset to DO_NOT_FLIP_ALIGNMENT_FOR_RTL_LOCALES before
its
> "its" -> "the"
> 
> http://codereview.chromium.org/18863/diff/2469/2475#newcode105
> Line 105: // horizontal alignment is set. Otherwise, its input alignment will
be
> "Otherwise, its input alignment will be flipped" -> "Otherwise, the label's
> alignment specified as a parameter will be flipped"
> 
> http://codereview.chromium.org/18863/diff/2469/2475#newcode112
> Line 112: // Set RTL alignment setting. RTL alignment setting is initialized
to
> "Set RTL alignment setting. RTL alignment setting" -> "Set the RTL alignment
> setting. The RTL alignment setting"
> 
> http://codereview.chromium.org/18863/diff/2469/2475#newcode113
> Line 113: // FLIP_ALIGNMENT_FOR_RTL_LOCALES when label is constructed.
> "when label is constructed" -> "when the label is constructed"
> 
> http://codereview.chromium.org/18863/diff/2469/2475#newcode115
> Line 115: // the Chrome UI. But if a label originates from a web page, its
> alignment
> "But if a label" -> "However, if the label"
> 
> http://codereview.chromium.org/18863/diff/2469/2475#newcode116
> Line 116: // should not be flipped around for RTL locales. For such label, we
> need to
> "For such label," -> "For such labels,"
> 
> http://codereview.chromium.org/18863/diff/2469/2475#newcode117
> Line 117: // set RTL alignment setting to
DO_NOT_FLIP_ALIGNMENT_FOR_RTL_LOCALES
> so that
> "set RTL alignment" -> "set the RTL alignment"
> 
> http://codereview.chromium.org/18863/diff/2469/2475#newcode118
> Line 118: // subsequent SetHorizontalAlignment() call will not flip the input
> alignment
> "call" -> "calls"
> "flip the input alignment" -> "flip the label's alignment"
> 
> http://codereview.chromium.org/18863/diff/2469/2478
> File chrome/views/label_unittest.cc (right):
> 
> http://codereview.chromium.org/18863/diff/2469/2478#newcode65
> Line 65: // Alignment is not flipped if RTL alignment setting is
> "Alignment is not flipped if RTL alignment setting is" -> "The label's
alignment
> should not be flipped if the RTL alignment setting is"
> 
> http://codereview.chromium.org/18863/diff/2469/2476
> File chrome/views/message_box_view.cc (right):
> 
> http://codereview.chromium.org/18863/diff/2469/2476#newcode108
> Line 108: // And we should set the RTL alignment setting as
> "And we should" -> "In addition, we should"

[idana, 2009-02-20 08:05:51]

We are almost done, Xiaomei.

The only remaining thing to change is what Ben suggested about using 'mode'
instead of 'setting' in the various names (see my comment), because I do agree
with Ben that it is strange to name a function SetRTLAlignmentSetting (I should
have thought about that when I suggested that name initially :-)).

http://codereview.chromium.org/18863/diff/1462/1468
File chrome/views/label.h (right):

http://codereview.chromium.org/18863/diff/1462/1468#newcode39
Line 39: enum RTLAlignmentSetting {
I think Ben made a good point about the name of this enumeration and the
corresponding Get/Set method and the class member. We should change things in
the following manner:

"RTLAlignmentSetting" -> "RTLAlignmentMode"
"GetRTLAlignmentSetting" -> "GetRTLAlignmentMode"
"SetRTLAlignmentSetting" -> "SetRTLAlignmentMode"
"rtl_alignment_setting_" -> "rtl_alignment_mode_"

You should also change the comments, but that should be easy (replace 'setting'
with 'mode' pretty much...).

[brettw, 2009-02-20 21:36:44]

On 2009/02/11 01:57:22, xji wrote:
> As to the DO_NOT_FLIP_ALIGNMENT_FOR_RTL_LOCALES and 
> FLIP_ALIGNMENT_FOR_RTL_LOCALES, we think we should leave the names the way
they
> are because we are literally flipping the alignment.
> 
> Or you feel strongly that we should change them as well?

I'm thinking as a future developer using this API. I have a string I want to
display here, and I need to decide which flags to use. It seems like your names
FLIP_ALIGNMENT_FOR_RTL_LOCALES and DO_NOT_FLIP_ALIGNMENT_FOR_RTL_LOCALES are
hard for me to think about.

What I want to say is "I know my string is in the UI language" and "I have no
idea what this string contains". This is why I suggested the alternate names.

[xji, 2009-02-21 00:09:44]

Done the naming changes.

On 2009/02/20 08:05:51, idana wrote:
> We are almost done, Xiaomei.
> 
> The only remaining thing to change is what Ben suggested about using 'mode'
> instead of 'setting' in the various names (see my comment), because I do agree
> with Ben that it is strange to name a function SetRTLAlignmentSetting (I
should
> have thought about that when I suggested that name initially :-)).
> 
> http://codereview.chromium.org/18863/diff/1462/1468
> File chrome/views/label.h (right):
> 
> http://codereview.chromium.org/18863/diff/1462/1468#newcode39
> Line 39: enum RTLAlignmentSetting {
> I think Ben made a good point about the name of this enumeration and the
> corresponding Get/Set method and the class member. We should change things in
> the following manner:
> 
> "RTLAlignmentSetting" -> "RTLAlignmentMode"
> "GetRTLAlignmentSetting" -> "GetRTLAlignmentMode"
> "SetRTLAlignmentSetting" -> "SetRTLAlignmentMode"
> "rtl_alignment_setting_" -> "rtl_alignment_mode_"
> 
> You should also change the comments, but that should be easy (replace
'setting'
> with 'mode' pretty much...).

[xji, 2009-02-21 00:10:25]

Changed the enum names to:
USE_UI_ALIGNMENT and AUTO_DETECT_ALIGNMENT

Thanks,
Xiaomei


On 2009/02/20 21:36:44, brettw wrote:
> On 2009/02/11 01:57:22, xji wrote:
> > As to the DO_NOT_FLIP_ALIGNMENT_FOR_RTL_LOCALES and 
> > FLIP_ALIGNMENT_FOR_RTL_LOCALES, we think we should leave the names the way
> they
> > are because we are literally flipping the alignment.
> > 
> > Or you feel strongly that we should change them as well?
> 
> I'm thinking as a future developer using this API. I have a string I want to
> display here, and I need to decide which flags to use. It seems like your
names
> FLIP_ALIGNMENT_FOR_RTL_LOCALES and DO_NOT_FLIP_ALIGNMENT_FOR_RTL_LOCALES are
> hard for me to think about.
> 
> What I want to say is "I know my string is in the UI language" and "I have no
> idea what this string contains". This is why I suggested the alternate names.

[brettw, 2009-02-21 02:21:35]

I don't have any other high-level comments other than this one formatting fix. I
assume Idan is checking all the I18N details, so LGTM from my side.

http://codereview.chromium.org/18863/diff/3402/4404
File chrome/views/label.cc (right):

http://codereview.chromium.org/18863/diff/3402/4404#newcode246
Line 246: rtl_alignment_mode_ == USE_UI_ALIGNMENT) {
This probably fits on the previous line now.

[xji, 2009-02-21 07:15:59]

Done


On 2009/02/21 02:21:35, brettw wrote:
> I don't have any other high-level comments other than this one formatting fix.
I
> assume Idan is checking all the I18N details, so LGTM from my side.
> 
> http://codereview.chromium.org/18863/diff/3402/4404
> File chrome/views/label.cc (right):
> 
> http://codereview.chromium.org/18863/diff/3402/4404#newcode246
> Line 246: rtl_alignment_mode_ == USE_UI_ALIGNMENT) {
> This probably fits on the previous line now.

[idana, 2009-02-22 07:48:35]

LGTM