Fixed 3 tests that failed in hebrew enabled vista...

Fixed 3 tests that failed in hebrew enabled vista
BrowserTest.NoTitle
BrowserTest.Title
AutomationProxyVisibleTest.WindowGetViewBounds

Some notes : 
There has been a small regression in the UI tests which makes them
get stuck even more on my very slow machine (maybe race conditions)
however, the fixes i post here do not make this state any worse
and fix these 3 tests above which always failed

Test=1. run ui_tests on a RTL windows (or use --extra-chrome-flags=--lang=he)
2. the 3 tests listed above will always fail, as they expect LTR behavior
3. this fixes it

BUG=12636
http://code.google.com/p/chromium/issues/detail?id=12636

[Mohamed Mansour, 2009-06-09 20:21:02]

lgtm, hbono can clarify.

[jeremy, 2009-06-09 20:56:46]

Sky: can you review the automation part of this CL?

A few general comments:
* Which of these routines is only used in testing code?
* Could you add comments for the various routines explaining what they do in a
concise manner.  Few programmers on the Chrome team understand rtl issues and it
should be clear from the comments what the function does and what case it's
needed for.
* The CL description should have a BUG= and TEST= line.

http://codereview.chromium.org/118441/diff/1/6
File chrome/browser/browser_uitest.cc (right):

http://codereview.chromium.org/118441/diff/1/6#newcode75
Line 75: const wchar_t POP_DIRECTIONAL_FORMATTING = L'\u202c';
I'm pretty sure there are already constants for this in il10nutils

http://codereview.chromium.org/118441/diff/1/6#newcode82
Line 82: return page_title;
Perhaps add a TODO constant to implement this on other platforms.

http://codereview.chromium.org/118441/diff/1/2
File chrome/test/automation/automation_messages_internal.h (right):

http://codereview.chromium.org/118441/diff/1/2#newcode709
Line 709: IPC_SYNC_MESSAGE_ROUTED1_2(\
IMHO this space shouldn't be removed.

[jeremy, 2009-06-09 20:57:44]

Pressed submit too fast :|  Strike my comment about BUG=, but  TEST= would still
be nice.

[nakro, 2009-06-09 21:02:42]

Hbono (and jeremy if you read this)

i have some comments about this solution
- i tried to keep the same code path as before for Mac/Linux, even though they
might need this same solution, but i have no way to debug this
- i have added the locale text just for future use, my code does not need it,
but i fetched it anyways
- there should be a check that the IPC reaches the main UI thread, as your code
suggests that |g_browser_process| is non reentrant
and should only be accessed from the main thread, but i did not find example
code to DCHECK it, but i think it must be added
- i opted to use a bool for the RTL/LTR issue as writing a template
specialization for 'TextDirection' seemed an over-kill and i thought you would
not like dragging 'l10n_util.h' into 'ipc_message_utils.h'

[sky, 2009-06-09 21:15:47]

just nits, LGTM otherwise.

http://codereview.chromium.org/118441/diff/1/8
File chrome/browser/automation/automation_provider.cc (right):

http://codereview.chromium.org/118441/diff/1/8#newcode1340
Line 1340: void AutomationProvider::GetChromeLocale(bool *is_rtl, std::wstring*
locale) {
nit: "bool *is_rtl" -> "bool* is_rtl"

http://codereview.chromium.org/118441/diff/1/8#newcode1345
Line 1345: DCHECK(direction == l10n_util::RIGHT_TO_LEFT ||
I don't think this DCHECK is very useful. No where else do we have it.

http://codereview.chromium.org/118441/diff/1/8#newcode1347
Line 1347: *is_rtl = direction == l10n_util::RIGHT_TO_LEFT ? true : false;
nit: *is_rtl = (drection == ...);

http://codereview.chromium.org/118441/diff/1/7
File chrome/browser/automation/automation_provider.h (right):

http://codereview.chromium.org/118441/diff/1/7#newcode152
Line 152: void GetChromeLocale(bool *is_rtl, std::wstring* locale);
nit: "bool *is_rtl" -> "bool* is_rtl"

http://codereview.chromium.org/118441/diff/1/2
File chrome/test/automation/automation_messages_internal.h (right):

http://codereview.chromium.org/118441/diff/1/2#newcode943
Line 943: // This message requests some info about the UI locale of the chrome
browser,
nit: , -> .

http://codereview.chromium.org/118441/diff/1/5
File chrome/test/automation/automation_proxy.h (right):

http://codereview.chromium.org/118441/diff/1/5#newcode104
Line 104: bool GetChromeLocale(bool *is_rtl, std::wstring *locale);
nit: "bool *" -> "bool* ". "wstring *" -> "wstring*"

[jeremy, 2009-06-09 22:24:09]

http://codereview.chromium.org/118441/diff/1/5
File chrome/test/automation/automation_proxy.h (right):

http://codereview.chromium.org/118441/diff/1/5#newcode104
Line 104: bool GetChromeLocale(bool *is_rtl, std::wstring *locale);
I think there's a function in i18nutils which given a locale will tell you if
it's rtl or not, so I don't think you need the is_rtl info here.

[nakro, 2009-06-09 22:31:04]

http://codereview.chromium.org/118441/diff/1/6
File chrome/browser/browser_uitest.cc (right):

http://codereview.chromium.org/118441/diff/1/6#newcode75
Line 75: const wchar_t POP_DIRECTIONAL_FORMATTING = L'\u202c';
the only file who defines it is 'src\third_party\icu38\source\common\ubidiimp.h'

which does not even have a namespace, and is not included by any other file (and
it declares these chars as enum)

since this file is not in the "normal" include path won't i add some hard-coded
dependencies ?

[nakro, 2009-06-09 22:56:14]

http://codereview.chromium.org/118441/diff/1/6
File chrome/browser/browser_uitest.cc (right):

http://codereview.chromium.org/118441/diff/1/6#newcode75
Line 75: const wchar_t POP_DIRECTIONAL_FORMATTING = L'\u202c';
ok, i just saw a wealth of unicode funcs in the l10n ... i will remove my const
wchars, and post a fix if it all works good

http://codereview.chromium.org/118441/diff/1/5
File chrome/test/automation/automation_proxy.h (right):

http://codereview.chromium.org/118441/diff/1/5#newcode104
Line 104: bool GetChromeLocale(bool *is_rtl, std::wstring *locale);
There is indeed one, i saw it and forgot it....
GetTextDirectionForLocale (char *)

i will switch to it, and remove the |is_rtl| thing

[nakro, 2009-06-10 01:37:19]

fixed the issues

[Hironori Bono, 2009-06-10 02:15:15]

LGTM++ but a few nits.
Thank you so much for your great change!

http://codereview.chromium.org/118441/diff/1014/1019
File chrome/browser/browser_uitest.cc (right):

http://codereview.chromium.org/118441/diff/1014/1019#newcode75
Line 75: const std::wstring &expected_title) {
nit: 'const std::wstring &expected_title' -> 'const std::wstring&
expected_title'.

http://codereview.chromium.org/118441/diff/1014/1019#newcode82
Line 82: const std::string &locale_utf8 = UTF16ToUTF8(browser_locale);
nit: 'const std::string &locale_utf8' -> 'const std::string& locale_utf8'.

http://codereview.chromium.org/118441/diff/1014/1017
File chrome/test/automation/automation_proxy.cc (right):

http://codereview.chromium.org/118441/diff/1014/1017#newcode416
Line 416: bool AutomationProxy::GetChromeLocale(string16* locale) {
nit: Add DCHECK(locale != NULL);

[nakro, 2009-06-10 02:41:38]

fixed new issues

[jeremy, 2009-06-10 07:13:33]

Nice work, nearly there...

http://codereview.chromium.org/118441/diff/1025/1032
File chrome/browser/automation/automation_provider.cc (right):

http://codereview.chromium.org/118441/diff/1025/1032#newcode959
Line 959: IPC_MESSAGE_HANDLER(AutomationMsg_GetChromeLocale, GetChromeLocale)
Does this get the locale of the browser process?  If so perhaps call this
GetBrowserLocale?

http://codereview.chromium.org/118441/diff/1025/1030
File chrome/browser/browser_uitest.cc (right):

http://codereview.chromium.org/118441/diff/1025/1030#newcode73
Line 73: // [5]   = 0        the good old End of string mark
Perhaps we could do with a shorter comment here:
In RTL locales wrap the page title with RTL embedding characters so that it
matches the value returned by GetWindowTitle().

(Why are we wrapping the page title with embedding characters?
Is there a function in i18nutils that already conditionally wraps a string in
embedding chars?

http://codereview.chromium.org/118441/diff/1025/1030#newcode94
Line 94: // the code path followed by windows fits him as well.
perhaps modify comment to:

Do we need to use the above code on POSIX as well?

http://codereview.chromium.org/118441/diff/1025/1026
File chrome/test/automation/automation_messages_internal.h (right):

http://codereview.chromium.org/118441/diff/1025/1026#newcode946
Line 946: IPC_SYNC_MESSAGE_ROUTED0_1(AutomationMsg_GetChromeLocale,
Perhaps change coment to:

This message retrieves the locale of the browser process.  On success
|chrome_locale| will contain the locale as reported by ICU.  On failure
|chrome_locale| is the empty string.

[nakro, 2009-06-10 09:46:42]

http://codereview.chromium.org/118441/diff/1025/1032
File chrome/browser/automation/automation_provider.cc (right):

http://codereview.chromium.org/118441/diff/1025/1032#newcode959
Line 959: IPC_MESSAGE_HANDLER(AutomationMsg_GetChromeLocale, GetChromeLocale)
I found the terminology confusing a bit, just by looking at your code it would
seem you call every new (Ctrl+N) a browser, but you seem to share the same
resource bundle, so while you could have 2 browsers, it seems that they must
share the same locale, so to avoid confusion i called it 'chrome' to make it
clear there is only one answer 
(besides even if there is a way to have several diff langs i did not use the
browsers handle, so i would anyways return the locals of the main one)

http://codereview.chromium.org/118441/diff/1025/1030
File chrome/browser/browser_uitest.cc (right):

http://codereview.chromium.org/118441/diff/1025/1030#newcode73
Line 73: // [5]   = 0        the good old End of string mark
A good question, i can only guess why you do it and i thought you do it to make
string building 100% cool (a smart move)
this way you can embed a substr in a string without worrying about the current
direction
but in truth i was surprised that other tests did not fail as well, as what do
you return when the user Cut & pastes ?
i only took care of what failed
As for a function, i will have another look...

http://codereview.chromium.org/118441/diff/1025/1026
File chrome/test/automation/automation_messages_internal.h (right):

http://codereview.chromium.org/118441/diff/1025/1026#newcode946
Line 946: IPC_SYNC_MESSAGE_ROUTED0_1(AutomationMsg_GetChromeLocale,
of course, i will do it if this is your wish (oh noble born :) but again, unless
i missed something, i found the terminology confusing as there could be several
browsers, but only one locale.... 
if i am right, new comers will also be confused by this but ok, done!

[nakro, 2009-06-10 12:27:01]

fixed all issues raised by jeremy

just one note about security : if the chrome exe contains these automation IPC
messages, then i think that to make chrome more secure all these messages should
only be treated when a flag is given to the chrome exe, that way, an attack from
the sandbox cannot exploit some code who is needed for testing, but i don't wish
to argue over this, it is just a FYI thing....

hope this is the last patch

[jeremy, 2009-06-10 16:19:46]

LGTM

http://codereview.chromium.org/118441/diff/1025/1032
File chrome/browser/automation/automation_provider.cc (right):

http://codereview.chromium.org/118441/diff/1025/1032#newcode959
Line 959: IPC_MESSAGE_HANDLER(AutomationMsg_GetChromeLocale, GetChromeLocale)
I think the reason you're confused is that you're looking at the UI tests?

A browser process is what's launched when you launch Chrome and shows all UI. 
Renderer processes are then spawned per tab.

All Chrome processes should share the same locale AFAIK.

[nakro, 2009-06-10 16:34:42]

do not commit it yet, there is a problem what i can see that on windows
std::wstring is the same as string16, while on some unixes it could be 4 bytes
per wchar....

the problem appears on 'automation_proxy_uittest.cc' 
i will fix it and send a patch

[nakro, 2009-06-10 16:43:49]

fixed

[jeremy, 2009-06-10 17:10:40]

LGTM

[Hironori Bono, 2009-06-15 03:58:34]

Sorry for my slow update.
This change has been committed as r18370.
Thank you again for your awesome change!