[webkit-reviews] review denied: [Bug 90248] [Platform-ChromiumMac] Introduce LocaleMac class : [Attachment 151214] Patch 2

[bugzilla-daemon at webkit.org]

Kent Tamura <tkent at chromium.org> has denied yosin at chromium.org's request for
review:
Bug 90248: [Platform-ChromiumMac] Introduce LocaleMac class
https://bugs.webkit.org/show_bug.cgi?id=90248

Attachment 151214: Patch 2
https://bugs.webkit.org/attachment.cgi?id=151214&action=review

------- Additional Comments from Kent Tamura <tkent at chromium.org>
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.