Bug 90248 – [Platform-ChromiumMac] Introduce LocaleMac class

• Component: Platform
• Status: RESOLVED FIXED

For unit testing, we would like to have LocaleMac class as of LocaleICU and LocaleWin to use specific locale rather than system default locale.

Created attachment 151212 [details]
Patch 1

Created attachment 151214 [details]
Patch 2

Comment on attachment 151214 [details]
Patch 2

Could you review this patch?
Thanks in advance.

[Kent Tamura]

Comment on attachment 151214 [details]
Patch 2

View in context: https://bugs.webkit.org/attachment.cgi?id=151214&action=review

> Source/WebCore/platform/text/mac/LocaleMac.mm:83
> +        return numeric_limits<double>::quiet_NaN();

We should prepend std:: to numeric_limits<>.

> Source/WebCore/platform/text/mac/LocaleMac.mm:181
> -        // weekdayName starts with Monday.
> -        labels.append(WTF::weekdayName[(i + 6) % 7]);
> +        m_weekDayShortLabels.append(WTF::weekdayName[(i + 6) % 7]);

Do not remove the comment.

> Source/WebKit/chromium/tests/LocaleMacTest.cpp:18
> + *     * Redistributions of source code must retain the above copyright
> + * notice, this list of conditions and the following disclaimer.
> + *     * Redistributions in binary form must reproduce the above
> + * copyright notice, this list of conditions and the following disclaimer
> + * in the documentation and/or other materials provided with the
> + * distribution.
> + *     * Neither the name of Google Inc. nor the names of its
> + * contributors may be used to endorse or promote products derived from
> + * this software without specific prior written permission.
> + *
> + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS

Please use the correct license header.

> Source/WebKit/chromium/tests/LocaleMacTest.cpp:147
> +    UChar frDecember[] = { 'd', 0xE9, 'c', 'e', 'm', 'b', 'r', 'e', 0 };
> +    EXPECT_STREQ(String(frDecember).utf8().data(), monthLabel("fr_FR", December).utf8().data());

The conversions for UChar -> String- > UTF8 CString is redundant.  Why don't you specify EXPECT_STREQ("d\xC3\xA9cembre", ...?

> Source/WebKit/chromium/tests/LocaleMacTest.cpp:154
> +    UChar jaJanuary[] = { '1', 0x6708, 0 };
> +    EXPECT_STREQ(String(jaJanuary).utf8().data(), monthLabel("ja_JP", January).utf8().data());
> +    UChar jaJune[] = { '6', 0x6708, 0 };
> +    EXPECT_STREQ(String(jaJune).utf8().data(), monthLabel("ja_JP", June).utf8().data());
> +    UChar jaDecember[] = { '1', '2', 0x6708, 0 };
> +    EXPECT_STREQ(String(jaDecember).utf8().data(), monthLabel("ja_JP", December).utf8().data());

ditto.

> Source/WebKit/chromium/tests/LocaleMacTest.cpp:172
> +    UChar jaSunday[] = { 0x65E5, 0 };
> +    EXPECT_STREQ(String(jaSunday).utf8().data(), weekDayShortLabel("ja_JP", Sunday).utf8().data());
> +    UChar jaWednesday[] = { 0x6C34, 0 };
> +    EXPECT_STREQ(String(jaWednesday).utf8().data(), weekDayShortLabel("ja_JP", Wednesday).utf8().data());
> +    UChar jaSaturday[] = { 0x571F, 0 };
> +    EXPECT_STREQ(String(jaSaturday).utf8().data(), weekDayShortLabel("ja_JP", Saturday).utf8().data());

ditto.

Created attachment 151384 [details]
Patch 3

Comment on attachment 151384 [details]
Patch 3

Could you review this patch?
Thanks in advance.

== Changes since the last review ==
* Add "std::" to numeric_limits
* Restore a removed comment in weekDayShortLabels()
* Update copyright text in LocaleMacTest.cpp
* Use UTF-8 string literal in LocaleMacTest.cpp

[Kent Tamura]

Comment on attachment 151384 [details]
Patch 3

Looks ok

Comment on attachment 151384 [details]
Patch 3

Clearing flags on attachment: 151384

Committed r122184: <http://trac.webkit.org/changeset/122184>

All reviewed patches have been landed.  Closing bug.

[Kent Tamura]

Comment on attachment 151384 [details]
Patch 3

View in context: https://bugs.webkit.org/attachment.cgi?id=151384&action=review

> Source/WebCore/platform/text/mac/LocaleMac.mm:63
> +    static LocaleMac* currentLocale = LocaleMac::create([NSLocale currentLocale]).leakPtr();

The return value of [NSLocale currentLocale] is id, not a string.