Review Request: display tooltip in the directionality of its element's

This CL fixes issue 5996: Tooltip gets direction from Chrome langauge, not element (or even page)

The related webkit bug is
https://bugs.webkit.org/show_bug.cgi?id=24187

After webkit passes both the title string and its directionality to its client, Chrome will force the title to be displayed in correct directionality using Unicode Control characters.

BUG=http://crbug.com/5996
TEST=In both English Chrome and Hebrew Chrome
1. open the test case in http://crbug.com/5996 comment #1 
2. hover over the red span, tooltip should be displayed as RTL in both English Chrome and Hebrew Chrome.
3. hover over the blue span, tooltip should be displayed as LTR in both English Chrome and Hebrew Chrome.


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

[xji, 2009-07-14 19:35:00]

This CL depends on webkit patch for issue 24187:
https://bugs.webkit.org/show_bug.cgi?id=24187

try test wont work until Chrome picks latest webkit containing  patch for issue
24187, and the CL will be checked in only after that.

[xji, 2009-07-18 00:45:17]

this is a kindly reminder.... (The webkit changes was pulled in)

Thanks,
Xiaomei

[idana, 2009-07-20 20:50:10]

http://codereview.chromium.org/147240/diff/1/3
File webkit/glue/chrome_client_impl.cc (right):

http://codereview.chromium.org/147240/diff/1/3#newcode516
Line 516: tooltip_text_as_wstring.push_back(WebCore::popDirectionalFormatting);
Are you sure that it is safe to insert these characters when the directionality
is LTR?

I know that, originally, I was very careful not to insert directionality
characters when the Chrome UI language was not RTL because it Windows doesn't
know how to handle these characters if the right to left fonts are not
installed. You can test that on an XP box by making sure the fonts are
un-installed before you run Chrome.

[jungshik at Google, 2009-07-20 21:18:33]

http://codereview.chromium.org/147240/diff/1/3
File webkit/glue/chrome_client_impl.cc (right):

http://codereview.chromium.org/147240/diff/1/3#newcode516
Line 516: tooltip_text_as_wstring.push_back(WebCore::popDirectionalFormatting);
On 2009/07/20 20:50:10, idana wrote:
> Are you sure that it is safe to insert these characters when the
directionality
> is LTR?
> 
> I know that, originally, I was very careful not to insert directionality
> characters when the Chrome UI language was not RTL because it Windows doesn't
> know how to handle these characters if the right to left fonts are not
> installed. You can test that on an XP box by making sure the fonts are
> un-installed before you run Chrome.

It's whether or not 'install files for complex script and rtl languages
(including Thai)' is checked  in Control panel - regional and language settings
- language tab. 

If it's indeed a problem without that checked on XP, we have to figure out how
to tell whether that's checked or not and do things differently depending on the
result. 

BTW, is this code cross-platform? If so, we have to check on Mac and Linux, too
although it's likely to be ok there.

[xji, 2009-07-20 23:51:04]

http://codereview.chromium.org/147240/diff/1/3
File webkit/glue/chrome_client_impl.cc (right):

http://codereview.chromium.org/147240/diff/1/3#newcode516
Line 516: tooltip_text_as_wstring.push_back(WebCore::popDirectionalFormatting);
On 2009/07/20 21:18:33, Jungshik Shin wrote:
> On 2009/07/20 20:50:10, idana wrote:
> > Are you sure that it is safe to insert these characters when the
> directionality
> > is LTR?
> > 
> > I know that, originally, I was very careful not to insert directionality
> > characters when the Chrome UI language was not RTL because it Windows
doesn't
> > know how to handle these characters if the right to left fonts are not
> > installed. You can test that on an XP box by making sure the fonts are
> > un-installed before you run Chrome.
> 
> It's whether or not 'install files for complex script and rtl languages
> (including Thai)' is checked  in Control panel - regional and language
settings
> - language tab. 
> 
> If it's indeed a problem without that checked on XP, we have to figure out how
> to tell whether that's checked or not and do things differently depending on
the
> result. 

I unchecked "install files for complex script and rtl languages (including
thai)" and reboot my XP,
the above change works fine for both Hebrew and English Chrome. Unicode marks
did not cause any problem, and the tooltip are displayed with expected
directionality.

> 
> BTW, is this code cross-platform? If so, we have to check on Mac and Linux,
too
> although it's likely to be ok there. 
> 
> 
> 
> 
>

[idana, 2009-07-21 00:45:26]

LGTM (with one more nit)

Thanks for testing that the change works well when the right-to-left system
files are not present.

http://codereview.chromium.org/147240/diff/1/2
File webkit/glue/chrome_client_impl.h (right):

http://codereview.chromium.org/147240/diff/1/2#newcode115
Line 115: virtual void setToolTip(const WebCore::String& tooltip_text,
WebCore::TextDirection dir);
Can you put the second parameter on its own line (like the function above) to
avoid exceeding the 80 characters per line limit?

[jungshik at Google, 2009-07-21 05:57:32]

lgtm

On 2009/07/20 23:51:04, xji wrote:

> > BTW, is this code cross-platform? If so, we have to check on Mac and Linux,
> too
> > although it's likely to be ok there. 

I've tried a slightly more challenging case than described in comment #1 of
issue 5996 ( http://i18nl10n.com/chrome/tooltip_dir.html )
and it works fine on Linux with your CL applied.

[xji, 2009-07-22 05:54:42]

Thanks for testing it in Linux.
I tried in Mac and the patch works as expected.
I will check it in.

On 2009/07/21 05:57:32, Jungshik Shin wrote:
> lgtm
> 
> On 2009/07/20 23:51:04, xji wrote:
> 
> > > BTW, is this code cross-platform? If so, we have to check on Mac and
Linux,
> > too
> > > although it's likely to be ok there. 
> 
> I've tried a slightly more challenging case than described in comment #1 of
> issue 5996 ( http://i18nl10n.com/chrome/tooltip_dir.html )
> and it works fine on Linux with your CL applied.