Bug 31597 – locale for text breakiterator and string search is not set to the UI locale

• Status: RESOLVED
• Resolution: FIXED
• Component: Text

[Jungshik Shin]

Created attachment 43380 [details]
patch (only for Chromium)

All the implementations of two methods in TextBreakIteratorInternalICU return "" and "en-US", respectively. 

There are FIXME comments that they should return the OS UI locale. It works as long as the OS UI locale is the same as the UI locale of a browser. 

In case of Chrome on Windows, the UI locale of Chrome can be different from the OS UI language. That is, English Windows users can run Chrome in Japanese.  And, Chrome's browser process already passes along that information to a renderer process, which is in turn available in WebCore::defaultLanguage().  

So, at least in Chrome, we can return that in two methods in TextBreakIteratorInternalICU.  

I'm tempted to remove two methods and just use WebCore::defaultLanguage() in all the call sites on all ports, but I'm not sure of the reasoning behind having them separate. So, I'm just making changes to Chrome's implementation of TextBreakIteratorInternalICU. If we can agree on the above point, I'll extend the patch to include other ports. 

With this change, for instance,  Swedish Find-in-Page behaves as expected when CHrome is run in Swedish.

[Darin Adler]

Comment on attachment 43380 [details]
patch (only for Chromium)

>  const char* currentSearchLocaleID()
>  {
> -    // FIXME: Should use system locale.
> -    return "";
> +    // Chrome's UI language can be different from the OS UI language on Windows.
> +    // We want to return Chrome's UI language here.
> +    return defaultLanguage().ascii().data();
>  }

This will create a local CString, take a pointer to its contents, then destroy the CString and return a pointer to the destroyed memory. So you definitely can't land this as-is.

The concept is probably OK for Chromium.

For Mac OS X it would not be good to use defaultLanguage() for everything, so it's good that you did not try to change all ports to work this way. Mac OS X has a separate preference for "Order for sorted lists" that should be used for searches but not text breaking.

[Jungshik Shin]

Created attachment 46147 [details]
updated patch per darin's comment

Darin, Thank you for the clarification about other ports and also catching my stupid mistake. Can you take another look?

[WebKit Review Bot]

Attachment 46147 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/platform/text/chromium/TextBreakIteratorInternalICUChromium.cpp:23:  Found other header before a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
WebCore/platform/text/chromium/TextBreakIteratorInternalICUChromium.cpp:28:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 2

[Darin Adler]

Comment on attachment 46147 [details]
updated patch per darin's comment

>  #include "config.h"
> +#include "CString.h"
> +#include "Language.h"
> +#include "PlatformString.h"
>  #include "TextBreakIteratorInternalICU.h"

As the style-bot said, please add the new includes in a new paragraph after a blank line. The file's own header goes first in the same paragraph with config.h.


> +static const char* UILanguage() {

Brace goes on a separate line.

> +    // Chrome's UI language can be different from the OS UI language on Windows.
> +    // We want to return Chrome's UI language here.
> +    static WebCore::CString locale;
> +    locale = WebCore::defaultLanguage().latin1();

This isn't quite right. The locale is a global variable, but you're re-initializing it with new data every time through the function. Merging these two lines of code into one will fix that.

[Jungshik Shin]

Created attachment 46314 [details]
updated patch

Thank you for bearing with me. Here's an updated patch.

[Darin Adler]

Comment on attachment 46314 [details]
updated patch

> +static const char* UILanguage()
> +{
> +    // Chrome's UI language can be different from the OS UI language on Windows.
> +    // We want to return Chrome's UI language here.
> +    static WebCore::CString locale = WebCore::defaultLanguage().latin1();
> +    return locale.data();
> +}

Normally I would put a function like this inside the WebCore namespace rather than outside. Any reason it's outside? You could avoid those two WebCore prefixes that way.

Most places in WebCore we try to avoid global destructors that run on process exit. In fact, for code in the Mac OS X version, it's required. The idiom is to use the DEFINE_STATIC_LOCAL macro.

[Jungshik Shin]

Created attachment 46383 [details]
update to use the macro for static local variable

thanks for the review. 
I've seen that macro but was mistaken for that to be only used with a literal value. 
I updated the patch to use the macro for static local (to leak at the end of a process) and pulled in the helper function into WebCore ns.

I'm carrying along r+ for the updated patch and will plus commit-queue once chrome-bot comes back green (although I locally built and ran some tests)

[Jungshik Shin]

http://crbug.com/31812 and http://crbug.com/31609
are chromium bugs related to this one.

[Darin Adler]

Comment on attachment 46383 [details]
update to use the macro for static local variable

Yes, looks good.

[Jungshik Shin]

Comment on attachment 46383 [details]
update to use the macro for static local variable

with r+, it looks like bots are not supposed to run. it should be ok, though and I'm going ahead with c-q +.

[WebKit Commit Bot]

Comment on attachment 46383 [details]
update to use the macro for static local variable

Clearing flags on attachment: 46383

Committed r53159: <http://trac.webkit.org/changeset/53159>