[webkit-reviews] review denied: [Bug 60786] [Qt] Better complex language support: Switch to ICU library (libicu) : [Attachment 94990] the perfect patch (pun intended)

[bugzilla-daemon at webkit.org]

Benjamin Poulain <benjamin at webkit.org> has denied Siddharth Mathur
<siddharth.mathur at nokia.com>'s request for review:
Bug 60786: [Qt] Better complex language support: Switch to ICU library (libicu)
https://bugs.webkit.org/show_bug.cgi?id=60786

Attachment 94990: the perfect patch (pun intended) 
https://bugs.webkit.org/attachment.cgi?id=94990&action=review

------- Additional Comments from Benjamin Poulain <benjamin at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=94990&action=review

> Source/WebCore/platform/text/qt/TextBreakIteratorInternalICUQt.cpp:30
> +Q_GLOBAL_STATIC_WITH_INITIALIZER(QByteArray, cachedSystemLocale, {*x =
QLocale::system().name().toAscii();})

toAscii() is almost never correct in library context. It is not correct here.
The function return the string encoded with the default encoding, not the ascii
encoding. You should use toLatin1() like the original code.

This caching is a change of behavior since we no longer use the system local
when it changes. So I become curious: how important is the gain?

> Source/WebCore/platform/text/qt/TextBreakIteratorInternalICUQt.cpp:39
> +    return cachedSystemLocale()->data(); 
> +}
> +
> +const char* currentTextBreakLocaleID()
> +{
> +    return cachedSystemLocale()->data(); 

You should use constData, otherwise the implementation have to detach() if
needed.