Bug 85039 – [Mac] Add LocalizedDateMac

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Kent Tamura]

It will be used for Chromium-Mac. Of couse we can use it for Apple Mac.

[Kent Tamura]

Created attachment 139153 [details]
Patch

[Kentaro Hara]

Comment on attachment 139153 [details]
Patch

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.

[Kent Tamura]

Comment on attachment 139153 [details]
Patch

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

>> Source/WebCore/platform/text/mac/LocalizedDateMac.mm:94
>> +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?

Because the format used in OS X and the format used in Windows are different.

Windows:
http://msdn.microsoft.com/en-us/library/dd317787(v=vs.85).aspx

OS X:
Using http://unicode.org/reports/tr35/tr35-6.html#Date_Format_Patterns according to the NSDateFormatter reference.

>> Source/WebCore/platform/text/mac/LocalizedDateMac.mm:123
>> +            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.

done

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

done

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

done

>> Source/WebCore/platform/text/mac/LocalizedDateMac.mm:174
>> +    labels.append("December");
> 
> Let's implement WTF::monthFullName[]. You are using in Win too.

done

[Kent Tamura]

Created attachment 139386 [details]
Patch 2

[Kentaro Hara]

Comment on attachment 139386 [details]
Patch 2

Looks OK to me. r+ it for now. If keishi-san would have other thoughts, please fix it in a follow-up patch.

[WebKit Review Bot]

Comment on attachment 139386 [details]
Patch 2

Clearing flags on attachment: 139386

Committed r115600: <http://trac.webkit.org/changeset/115600>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Maciej Stachowiak]

Looks like this change broke the chromium-mac build. Also why was this done Chromium-specific? And finally, the claim that this is covered by <fast/forms/date/date-appearance.html> seems dubious, since that doesn't test the effect of different preference settings.

[Kent Tamura]

Thank you for the comment.

(In reply to comment #8)
> Looks like this change broke the chromium-mac build.

Fixed.

> Also why was this done Chromium-specific?

Because non-Chromium ports have no code paths to use this file for now.  I'm going to add <input type=date> related files to each of build files later.

> And finally, the claim that this is covered by <fast/forms/date/date-appearance.html> seems dubious, since that doesn't test the effect of different preference settings.

I have no good idea to test this with various settings.