[webkit-reviews] review denied: [Bug 85039] [Mac] Add LocalizedDateMac : [Attachment 139153] Patch

[bugzilla-daemon at webkit.org]

Kentaro Hara <haraken at chromium.org> has denied Kent Tamura
<tkent at chromium.org>'s request for review:
Bug 85039: [Mac] Add LocalizedDateMac
https://bugs.webkit.org/show_bug.cgi?id=85039

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

------- Additional Comments from Kentaro Hara <haraken at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=139153&action=review


Waiting for keishi-san's informal review. I am not familiar with ObjC.

> Source/WebCore/platform/text/mac/LocalizedDateMac.mm:94
> +static bool isYearSymbol(UChar letter) { return letter == 'y' || letter ==
'Y' || letter == 'u'; }
> +static bool isMonthSymbol(UChar letter) { return letter == 'M' || letter ==
'L'; }

Why do you support 'Y', 'u' and 'L' in Mac but not support in Win?

> Source/WebCore/platform/text/mac/LocalizedDateMac.mm:123
> +	       String text = dateFormatYearText();
> +	       buffer.append(text.isEmpty() ? "Year" : text);

Let's put

    String text = dateFormatYearText();
    String textString = text.isEmpty() ? "Year" : text;

outside of the for loop, and then use textString here.

> Source/WebCore/platform/text/mac/LocalizedDateMac.mm:128
> +	       String text = dateFormatMonthText();
> +	       buffer.append(text.isEmpty() ? "Month" : text);

Ditto.

> Source/WebCore/platform/text/mac/LocalizedDateMac.mm:133
> +	       String text = dateFormatDayInMonthText();
> +	       buffer.append(text.isEmpty() ? "Day" : text);

Ditto.

> Source/WebCore/platform/text/mac/LocalizedDateMac.mm:174
> +    labels.append("January");
> +    labels.append("February");
> +    labels.append("March");
> +    labels.append("April");
> +    labels.append("May");
> +    labels.append("June");
> +    labels.append("July");
> +    labels.append("August");
> +    labels.append("September");
> +    labels.append("October");
> +    labels.append("November");
> +    labels.append("December");

Let's implement WTF::monthFullName[]. You are using in Win too.