Review request: fix 7324 and 7326: missing icon and wrong path in "download location"

This CL fix the following 2 bugs:
1.7324 -- RTL: Path contains Hebrew is wrong on "Download location" text box
(http://crbug.com/7324)

2. 7326 -- RTL: Folder icon missing on "Download location" text box
(http://crbug.com/7326)

The fix are:
1. force path to have LTR directionality.
2. draw icon in mirrored position in RTL locales.

Steps for test:
1. Create a folder with a Hebrew file name on Hebrew XP
2. Change the download location to the folder created in step 1
3. Observe the path displayed on "Google Chrome Options" --> "Minor Tweaks" --> "Download location" text box

Without the fix: 
The path contains Hebrew folder name is wrong. It displayed as "cCBA\:" while the path is "c:\CBA" where "CBA" is a Hebrew folder name.
And there is no folder icon in the "download location" text box.

With the fix:
The path displayed correctly as "c:\CBA" or "c:\CBA\FED" where "FED" is subfolder of "CBA".
And the folder icon showed at the very right.



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

[jeremy, 2009-02-04 07:14:55]

http://codereview.chromium.org/20038/diff/11/1001
File chrome/browser/views/options/content_page_view.cc (right):

http://codereview.chromium.org/20038/diff/11/1001#newcode162
Line 162: // check whether the locale is RTL locale.
How about:
// We'd prefer to use UILayoutIsRightToLeft() to perform this // check but it's
nonstatic.

http://codereview.chromium.org/20038/diff/11/1001#newcode163
Line 163: bool rtl = l10n_util::GetTextDirection() == l10n_util::RIGHT_TO_LEFT;
IMHO ui_is_rtl is a bit clearer as a variable name.

http://codereview.chromium.org/20038/diff/11/1003
File chrome/common/l10n_util.cc (right):

http://codereview.chromium.org/20038/diff/11/1003#newcode597
Line 597: void WrapPathWithLTRFormatting(std::wstring* path) {
Can you use the file_utils::PathComponents() function to split the path?

http://codereview.chromium.org/20038/diff/11/1003#newcode602
Line 602: #endif
I think FilePath::kSeparators[0] will do this for you.

http://codereview.chromium.org/20038/diff/11/1002
File chrome/common/l10n_util.h (right):

http://codereview.chromium.org/20038/diff/11/1002#newcode156
Line 156: // properly in an RTL context.
How about:
// Wraps individual file path components to get them to
// display correctly in an LTR UI.
// All filepaths should be passed through this function
// before display in UI for RTL locales.

http://codereview.chromium.org/20038/diff/11/1002#newcode157
Line 157: void WrapPathWithLTRFormatting(std::wstring* path);
I know the surrounding code still uses wstrings but we're transitioning to
FilePath objects to represent files.

Please change this prototype to take a FilePath as input and return an
std::string16.

You can use the To/FromWStringHack() functions to perform the conversion.

[jungshik at Google, 2009-02-04 18:21:40]

In addition to addressing Jeremy's comments, why don't you add a unit test for a
new function?

[xji, 2009-02-06 08:15:12]

On 2009/02/04 07:14:55, jeremy wrote:
> http://codereview.chromium.org/20038/diff/11/1001
> File chrome/browser/views/options/content_page_view.cc (right):
> 
> http://codereview.chromium.org/20038/diff/11/1001#newcode162
> Line 162: // check whether the locale is RTL locale.
> How about:
> // We'd prefer to use UILayoutIsRightToLeft() to perform this // check but
it's
> nonstatic.

done

> 
> http://codereview.chromium.org/20038/diff/11/1001#newcode163
> Line 163: bool rtl = l10n_util::GetTextDirection() ==
l10n_util::RIGHT_TO_LEFT;
> IMHO ui_is_rtl is a bit clearer as a variable name.

done

> 
> http://codereview.chromium.org/20038/diff/11/1003
> File chrome/common/l10n_util.cc (right):
> 
> http://codereview.chromium.org/20038/diff/11/1003#newcode597
> Line 597: void WrapPathWithLTRFormatting(std::wstring* path) {
> Can you use the file_utils::PathComponents() function to split the path?

done

> 
> http://codereview.chromium.org/20038/diff/11/1003#newcode602
> Line 602: #endif
> I think FilePath::kSeparators[0] will do this for you.

done

> 
> http://codereview.chromium.org/20038/diff/11/1002
> File chrome/common/l10n_util.h (right):
> 
> http://codereview.chromium.org/20038/diff/11/1002#newcode156
> Line 156: // properly in an RTL context.
> How about:
> // Wraps individual file path components to get them to
> // display correctly in an LTR UI.
> // All filepaths should be passed through this function
> // before display in UI for RTL locales.

done

> 
> http://codereview.chromium.org/20038/diff/11/1002#newcode157
> Line 157: void WrapPathWithLTRFormatting(std::wstring* path);
> I know the surrounding code still uses wstrings but we're transitioning to
> FilePath objects to represent files.
> 
> Please change this prototype to take a FilePath as input and return an
> std::string16.
> 
> You can use the To/FromWStringHack() functions to perform the conversion.

I changed the signature to  take a FilePath as input and return
FilePath::StringType.

I did not use string16 since from comments in file_path.h, the path in Mac OS
are UTF-8 encoded, they are std::string.

[xji, 2009-02-06 08:15:29]

On 2009/02/04 18:21:40, Jungshik Shin wrote:
> In addition to addressing Jeremy's comments, why don't you add a unit test for
a
> new function?

Done.

[jeremy, 2009-02-06 17:02:03]

http://codereview.chromium.org/20038/diff/1010/1013
File chrome/common/l10n_util.cc (right):

http://codereview.chromium.org/20038/diff/1010/1013#newcode618
Line 618: localized_path->append(path_components[start]);
May be a little cleaner if you use a for loop and something like:
bool first_time = (i == 0);
bool last_time  = (i == size - 1);

if (first_time)...
if (last_time)...

in the loop body.

http://codereview.chromium.org/20038/diff/1010/1012
File chrome/common/l10n_util.h (right):

http://codereview.chromium.org/20038/diff/1010/1012#newcode162
Line 162: FilePath::StringType* localized_path);
Conceptually this class turns a FilePath into a string suitable for display in
the UI.

I'm pretty sure you want to output an std::string16 here, even if it means doing
conversions internally in some cases.

http://codereview.chromium.org/20038/diff/1010/1014
File chrome/common/l10n_util_unittest.cc (right):

http://codereview.chromium.org/20038/diff/1010/1014#newcode262
Line 262: const std::wstring kSeparator = L"/";
Can you use FilePath::kSeparators[0] for both cases?

http://codereview.chromium.org/20038/diff/1010/1014#newcode267
Line 267: } test_data[] = {
Could you add tests for:
* empty string
* paths ending with /

[xji, 2009-02-06 22:19:53]

Hi Jeremy,

Thanks for the review.
Please see my reply inline.


On 2009/02/06 17:02:03, jeremy wrote:
> http://codereview.chromium.org/20038/diff/1010/1013
> File chrome/common/l10n_util.cc (right):
> 
> http://codereview.chromium.org/20038/diff/1010/1013#newcode618
> Line 618: localized_path->append(path_components[start]);
> May be a little cleaner if you use a for loop and something like:
> bool first_time = (i == 0);
> bool last_time  = (i == size - 1);
> 
> if (first_time)...
> if (last_time)...
> 
> in the loop body.

done

> 
> http://codereview.chromium.org/20038/diff/1010/1012
> File chrome/common/l10n_util.h (right):
> 
> http://codereview.chromium.org/20038/diff/1010/1012#newcode162
> Line 162: FilePath::StringType* localized_path);
> Conceptually this class turns a FilePath into a string suitable for display in
> the UI.
> 
> I'm pretty sure you want to output an std::string16 here, even if it means
doing
> conversions internally in some cases.

Change to string16.

> 
> http://codereview.chromium.org/20038/diff/1010/1014
> File chrome/common/l10n_util_unittest.cc (right):
> 
> http://codereview.chromium.org/20038/diff/1010/1014#newcode262
> Line 262: const std::wstring kSeparator = L"/";
> Can you use FilePath::kSeparators[0] for both cases?

done

> 
> http://codereview.chromium.org/20038/diff/1010/1014#newcode267
> Line 267: } test_data[] = {
> Could you add tests for:
> * empty string
> * paths ending with /

added test for empty string.
path ending with separator was included before.

Thanks,
Xiaomei

[jeremy, 2009-02-06 22:55:02]

http://codereview.chromium.org/20038/diff/26/27
File chrome/browser/views/options/content_page_view.cc (right):

http://codereview.chromium.org/20038/diff/26/27#newcode97
Line 97: void FileDisplayArea::SetFile(const std::wstring& file_path) {
Could you change the prototype of this function to accept a FilePath as well as
long as you're here?

Looks like there's only one caller,so this shouldn't lead to too many code
changes.

There's really no need to follow this all the way up the chain, only this one
function.

http://codereview.chromium.org/20038/diff/26/29
File chrome/common/l10n_util.cc (right):

http://codereview.chromium.org/20038/diff/26/29#newcode610
Line 610: localized_path->append(WideToUTF16(kLeftToRightEmbeddingMark));
Could you make kLeftToRightEmbeddingMark a char16, then you won't need a bunch
of these ifdefs.

http://codereview.chromium.org/20038/diff/26/29#newcode627
Line 627: FilePath::IsSeparator(path_components[index][0]))) {
could you create a temp component_is_separator bool, I think that would make
this if a little mor readable.

http://codereview.chromium.org/20038/diff/26/30
File chrome/common/l10n_util_unittest.cc (right):

http://codereview.chromium.org/20038/diff/26/30#newcode325
Line 325: for (int i = 0; i < sizeof(test_data)/sizeof(test_data[0]); ++i) {
you can use ARRAY_SIZE(test_data).

http://codereview.chromium.org/20038/diff/26/30#newcode326
Line 326: FilePath::StringType localized_file_path_string;
this should be an std::string16

[jungshik at Google, 2009-02-06 23:22:55]

http://codereview.chromium.org/20038/diff/26/29
File chrome/common/l10n_util.cc (right):

http://codereview.chromium.org/20038/diff/26/29#newcode610
Line 610: localized_path->append(WideToUTF16(kLeftToRightEmbeddingMark));
I agree with Jeremy. All kFoo constants you introduced in this CL can be char16.


BTW, I don't thin 'localized_path' is such a good name. How about
'rtl_safe_path'?

http://codereview.chromium.org/20038/diff/26/29#newcode616
Line 616: localized_path->append(UTF8ToUTF16(path_components[index]));
On Linux, this is not correct unless we decide to support only UTF-8 locales.
(yes, there are other files that do this !)  Why don't you replace it with
SysMBToWide and WideToUTF16 on Linux? Or, at least add a TODO comment.

http://codereview.chromium.org/20038/diff/26/29#newcode629
Line 629: localized_path->append(UTF8ToUTF16(FilePath::kSeparators), 1);
can you just use push_back(static_cast<char16>(FilePath::kSeparators[0]))?

http://codereview.chromium.org/20038/diff/26/29#newcode646
Line 646: localized_path->append(WideToUTF16(kPopDirectionalFormatting));
same here as other kFoo constants.

http://codereview.chromium.org/20038/diff/1021/38
File chrome/common/l10n_util.h (right):

http://codereview.chromium.org/20038/diff/1021/38#newcode164
Line 164: string16* localized_path);
How about FormatPathForDisplay or FormatPathForUI? I wonder FilePath is a better
place for this function.

http://codereview.chromium.org/20038/diff/1021/40
File chrome/common/l10n_util_unittest.cc (right):

http://codereview.chromium.org/20038/diff/1021/40#newcode328
Line 328: l10n_util::WrapPathWithLTRFormatting(path,
&localized_file_path_string);
Does this even compile on Linux/Mac? out param of WrapPathWithLTRFormatting is
string16, but FilePath::StringType is std::string on Linux/Mac.

[xji, 2009-02-11 00:23:28]

On 2009/02/06 22:55:02, jeremy wrote:
> http://codereview.chromium.org/20038/diff/26/27
> File chrome/browser/views/options/content_page_view.cc (right):
> 
> http://codereview.chromium.org/20038/diff/26/27#newcode97
> Line 97: void FileDisplayArea::SetFile(const std::wstring& file_path) {
> Could you change the prototype of this function to accept a FilePath as well
as
> long as you're here?
> 
> Looks like there's only one caller,so this shouldn't lead to too many code
> changes.
> 
> There's really no need to follow this all the way up the chain, only this one
> function.

Change the signature of FileDisplayArea::SetFile().

> 
> http://codereview.chromium.org/20038/diff/26/29
> File chrome/common/l10n_util.cc (right):
> 
> http://codereview.chromium.org/20038/diff/26/29#newcode610
> Line 610: localized_path->append(WideToUTF16(kLeftToRightEmbeddingMark));
> Could you make kLeftToRightEmbeddingMark a char16, then you won't need a bunch
> of these ifdefs.

done.

> 
> http://codereview.chromium.org/20038/diff/26/29#newcode627
> Line 627: FilePath::IsSeparator(path_components[index][0]))) {
> could you create a temp component_is_separator bool, I think that would make
> this if a little mor readable.
> 

Since I would still like to use short circuit to check FilePath::IsSeparator()
as the last step, I moved the 3 logic AND out of the "if".


> http://codereview.chromium.org/20038/diff/26/30
> File chrome/common/l10n_util_unittest.cc (right):
> 
> http://codereview.chromium.org/20038/diff/26/30#newcode325
> Line 325: for (int i = 0; i < sizeof(test_data)/sizeof(test_data[0]); ++i) {
> you can use ARRAY_SIZE(test_data).


Done.

> 
> http://codereview.chromium.org/20038/diff/26/30#newcode326
> Line 326: FilePath::StringType localized_file_path_string;
> this should be an std::string16

Done.

[xji, 2009-02-11 00:31:18]

On 2009/02/06 23:22:55, Jungshik Shin wrote:
> http://codereview.chromium.org/20038/diff/26/29
> File chrome/common/l10n_util.cc (right):
> 
> http://codereview.chromium.org/20038/diff/26/29#newcode610
> Line 610: localized_path->append(WideToUTF16(kLeftToRightEmbeddingMark));
> I agree with Jeremy. All kFoo constants you introduced in this CL can be
char16.
> 

Done.

> 
> BTW, I don't thin 'localized_path' is such a good name. How about
> 'rtl_safe_path'?

Changed.


> 
> http://codereview.chromium.org/20038/diff/26/29#newcode616
> Line 616: localized_path->append(UTF8ToUTF16(path_components[index]));
> On Linux, this is not correct unless we decide to support only UTF-8 locales.
> (yes, there are other files that do this !)  Why don't you replace it with
> SysMBToWide and WideToUTF16 on Linux? Or, at least add a TODO comment.

Changed to 3 separate cases for Linux, MACOS, and Windows.

> 
> http://codereview.chromium.org/20038/diff/26/29#newcode629
> Line 629: localized_path->append(UTF8ToUTF16(FilePath::kSeparators), 1);
> can you just use push_back(static_cast<char16>(FilePath::kSeparators[0]))?

Done.

> 
> http://codereview.chromium.org/20038/diff/26/29#newcode646
> Line 646: localized_path->append(WideToUTF16(kPopDirectionalFormatting));
> same here as other kFoo constants.

Done.

> 
> http://codereview.chromium.org/20038/diff/1021/38
> File chrome/common/l10n_util.h (right):
> 
> http://codereview.chromium.org/20038/diff/1021/38#newcode164
> Line 164: string16* localized_path);
> How about FormatPathForDisplay or FormatPathForUI? I wonder FilePath is a
better
> place for this function.

I did not change the function name here to be consistent with
WrapStringWithLTRFormatting.

I am not sure where should put this function.
It is related to file path, but it is not purely dealing with path components,
it is more to adjust path to display in UI.



> 
> http://codereview.chromium.org/20038/diff/1021/40
> File chrome/common/l10n_util_unittest.cc (right):
> 
> http://codereview.chromium.org/20038/diff/1021/40#newcode328
> Line 328: l10n_util::WrapPathWithLTRFormatting(path,
> &localized_file_path_string);
> Does this even compile on Linux/Mac? out param of WrapPathWithLTRFormatting is
> string16, but FilePath::StringType is std::string on Linux/Mac.

Changed.

[jeremy, 2009-02-11 05:32:34]

http://codereview.chromium.org/20038/diff/1054/79
File chrome/browser/views/options/content_page_view.cc (right):

http://codereview.chromium.org/20038/diff/1054/79#newcode103
Line 103: text_field_->SetText(UTF16ToWide(localized_file_path));
Are you sure you need this #ifdef?  Shouldn't the POSIX case work for win32 as
well?

http://codereview.chromium.org/20038/diff/1054/79#newcode160
Line 160: void FileDisplayArea::InitClass() {
The convention used in other places is to place a // static comment before
static method bodies.

http://codereview.chromium.org/20038/diff/1054/81
File chrome/common/l10n_util.cc (right):

http://codereview.chromium.org/20038/diff/1054/81#newcode613
Line 613: #if defined(OS_MACOSX)
What about LINUX?

Maybe
#if defined(OS_WIN)
#else
#endif  // !OS_WIN

http://codereview.chromium.org/20038/diff/1054/81#newcode632
Line 632:
rtl_safe_path->push_back(static_cast<char16>(FilePath::kSeparators[0]));
Maybe a little more readable to declare a 
char16 path_separator
outside the loop and use that here.

http://codereview.chromium.org/20038/diff/1054/82
File chrome/common/l10n_util_unittest.cc (right):

http://codereview.chromium.org/20038/diff/1054/82#newcode24
Line 24: #define ARRAY_SIZE(array) (sizeof array  / sizeof array[0])
I think this is already defined in base somewhere.

http://codereview.chromium.org/20038/diff/1054/82#newcode333
Line 333: std::wstring wrapped_path = UTF16ToWide(localized_file_path_string);
Can you use this case for all platforms?

[xji, 2009-02-11 18:51:59]

On 2009/02/11 05:32:34, jeremy wrote:
> http://codereview.chromium.org/20038/diff/1054/79
> File chrome/browser/views/options/content_page_view.cc (right):
> 
> http://codereview.chromium.org/20038/diff/1054/79#newcode103
> Line 103: text_field_->SetText(UTF16ToWide(localized_file_path));
> Are you sure you need this #ifdef?  Shouldn't the POSIX case work for win32 as
> well?
> 
Yes, you are right. I do not need to use the #ifdef here.
removed them.


> http://codereview.chromium.org/20038/diff/1054/79#newcode160
> Line 160: void FileDisplayArea::InitClass() {
> The convention used in other places is to place a // static comment before
> static method bodies.
> 

added "// static".

> http://codereview.chromium.org/20038/diff/1054/81
> File chrome/common/l10n_util.cc (right):
> 
> http://codereview.chromium.org/20038/diff/1054/81#newcode613
> Line 613: #if defined(OS_MACOSX)
> What about LINUX?
> 
> Maybe
> #if defined(OS_WIN)
> #else
> #endif  // !OS_WIN


the Linux is in 3rd branch, it uses SysNativeMBToWide() for conversion.

The structure was:
#if defined (OS_MACOS)
#else
#if defined (OS_WIN)
#else
#endif
#endif

I changed it to:
#if defined (OS_MACOS)
  UTF8ToWide()
#elif defined (OS_WIN)
  direct assign
#else
  SysNativeMBToWide()
#endif


> 
> http://codereview.chromium.org/20038/diff/1054/81#newcode632
> Line 632:
> rtl_safe_path->push_back(static_cast<char16>(FilePath::kSeparators[0]));
> Maybe a little more readable to declare a 
> char16 path_separator
> outside the loop and use that here.

done

> 
> http://codereview.chromium.org/20038/diff/1054/82
> File chrome/common/l10n_util_unittest.cc (right):
> 
> http://codereview.chromium.org/20038/diff/1054/82#newcode24
> Line 24: #define ARRAY_SIZE(array) (sizeof array  / sizeof array[0])
> I think this is already defined in base somewhere.

I did not find it in public header.

> 
> http://codereview.chromium.org/20038/diff/1054/82#newcode333
> Line 333: std::wstring wrapped_path = UTF16ToWide(localized_file_path_string);
> Can you use this case for all platforms?

Done.

[jeremy, 2009-02-11 19:19:13]

A few comments, but LGTM

http://codereview.chromium.org/20038/diff/87/1062
File chrome/common/l10n_util.cc (right):

http://codereview.chromium.org/20038/diff/87/1062#newcode617
Line 617: #else
Could you add a // defined(OS_LINUX) comment after the #else just so it's a
little clearer that this is the case.

http://codereview.chromium.org/20038/diff/87/1062#newcode630
Line 630: char16 path_separator = static_cast<char16>(FilePath::kSeparators[0]);
I think you can hoist this outside the loop.

http://codereview.chromium.org/20038/diff/87/1063
File chrome/common/l10n_util_unittest.cc (right):

http://codereview.chromium.org/20038/diff/87/1063#newcode24
Line 24: #define ARRAY_SIZE(array) (sizeof array  / sizeof array[0])
yep, it's a little hard to find, the definition is in base/basictypes.h and it's
called arraysize() , there's also an ARRAYSIZE_UNSAFE() macro which you need to
use sometimes.

[jungshik at Google, 2009-02-11 19:48:07]

>> http://codereview.chromium.org/20038/diff/1054/82#newcode24
>> Line 24: #define ARRAY_SIZE(array) (sizeof array  / sizeof array[0])
>> I think this is already defined in base somewhere.

>I did not find it in public header.

  arraysize (not ARRAY_SIZE) is defined in base/basictypes.h

http://codereview.chromium.org/20038/diff/87/1062
File chrome/common/l10n_util.cc (right):

http://codereview.chromium.org/20038/diff/87/1062#newcode621
Line 621: #endif
This hurts eyes :-). Maybe, you'd better write an inline helper for #if ...
#endif.

[xji, 2009-02-11 20:25:05]

On 2009/02/11 19:19:13, jeremy wrote:
> A few comments, but LGTM
> 
> http://codereview.chromium.org/20038/diff/87/1062
> File chrome/common/l10n_util.cc (right):
> 
> http://codereview.chromium.org/20038/diff/87/1062#newcode617
> Line 617: #else
> Could you add a // defined(OS_LINUX) comment after the #else just so it's a
> little clearer that this is the case.

done.

> 
> http://codereview.chromium.org/20038/diff/87/1062#newcode630
> Line 630: char16 path_separator =
static_cast<char16>(FilePath::kSeparators[0]);
> I think you can hoist this outside the loop.

Yes, you are right.
Done.

> 
> http://codereview.chromium.org/20038/diff/87/1063
> File chrome/common/l10n_util_unittest.cc (right):
> 
> http://codereview.chromium.org/20038/diff/87/1063#newcode24
> Line 24: #define ARRAY_SIZE(array) (sizeof array  / sizeof array[0])
> yep, it's a little hard to find, the definition is in base/basictypes.h and
it's
> called arraysize() , there's also an ARRAYSIZE_UNSAFE() macro which you need
to
> use sometimes.

done.
And moved the definition of the structure out of the function in order to use
arraysize().

[xji, 2009-02-11 20:27:34]

On 2009/02/11 19:48:07, Jungshik Shin wrote:
> >> http://codereview.chromium.org/20038/diff/1054/82#newcode24
> >> Line 24: #define ARRAY_SIZE(array) (sizeof array  / sizeof array[0])
> >> I think this is already defined in base somewhere.
> 
> >I did not find it in public header.
> 
>   arraysize (not ARRAY_SIZE) is defined in base/basictypes.h

done

> 
> http://codereview.chromium.org/20038/diff/87/1062
> File chrome/common/l10n_util.cc (right):
> 
> http://codereview.chromium.org/20038/diff/87/1062#newcode621
> Line 621: #endif
> This hurts eyes :-). Maybe, you'd better write an inline helper for #if ...
> #endif.

do you think the following will be better?

#if defined(OS_MACOSX)
    string16 one_component = UTF8ToUTF16(path_components[index]);
#elif defined(OS_WIN)
    string16 one_component = path_components[index];
#else  // defined(OS_LINUX)
    string16 one_component =
        WideToUTF16(base::SysNativeMBToWide(path_components[index]));
#endif
    rtl_safe_path->append(one_component);

[jungshik at Google, 2009-02-13 23:55:40]

lgtm 
On 2009/02/11 20:27:34, xji wrote:
> On 2009/02/11 19:48:07, Jungshik Shin wrote:
> > >> http://codereview.chromium.org/20038/diff/1054/82#newcode24
> > >> Line 24: #define ARRAY_SIZE(array) (sizeof array  / sizeof array[0])
> > >> I think this is already defined in base somewhere.
> > 
> > >I did not find it in public header.
> > 
> >   arraysize (not ARRAY_SIZE) is defined in base/basictypes.h
> 
> done
> 
> > 
> > http://codereview.chromium.org/20038/diff/87/1062
> > File chrome/common/l10n_util.cc (right):
> > 
> > http://codereview.chromium.org/20038/diff/87/1062#newcode621
> > Line 621: #endif
> > This hurts eyes :-). Maybe, you'd better write an inline helper for #if ...
> > #endif.
> 
> do you think the following will be better?
> 
> #if defined(OS_MACOSX)
>     string16 one_component = UTF8ToUTF16(path_components[index]);
> #elif defined(OS_WIN)
>     string16 one_component = path_components[index];
> #else  // defined(OS_LINUX)
>     string16 one_component =
>         WideToUTF16(base::SysNativeMBToWide(path_components[index]));
> #endif
>     rtl_safe_path->append(one_component);

[idana, 2009-02-20 07:49:16]

LGTM

Just a few nits.

http://codereview.chromium.org/20038/diff/95/1068
File chrome/browser/views/options/content_page_view.cc (right):

http://codereview.chromium.org/20038/diff/95/1068#newcode162
Line 162: // locale is RTL locale.
"locale is RTL locale." -> "locale is RTL."

http://codereview.chromium.org/20038/diff/95/1070
File chrome/common/l10n_util.cc (right):

http://codereview.chromium.org/20038/diff/95/1070#newcode609
Line 609: // any) at the end of the path will not be displayed at correct
position.
"at correct position" -> "at the correct position"

http://codereview.chromium.org/20038/diff/95/1069
File chrome/common/l10n_util.h (right):

http://codereview.chromium.org/20038/diff/95/1069#newcode161
Line 161: // LTR UI. All filepaths should be passed through this function before
display
"LTR UI": did you mean RTL?