Bug 85039 - [Mac] Add LocalizedDateMac
Summary: [Mac] Add LocalizedDateMac
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P1 Normal
Assignee: Kent Tamura
URL:
Keywords:
Depends on:
Blocks: 84388
  Show dependency treegraph
 
Reported: 2012-04-27 02:14 PDT by Kent Tamura
Modified: 2012-04-30 20:06 PDT (History)
5 users (show)

See Also:


Attachments
Patch (11.01 KB, patch)
2012-04-27 02:24 PDT, Kent Tamura
no flags Details | Formatted Diff | Diff
Patch 2 (13.56 KB, patch)
2012-04-28 19:11 PDT, Kent Tamura
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kent Tamura 2012-04-27 02:14:36 PDT
It will be used for Chromium-Mac. Of couse we can use it for Apple Mac.
Comment 1 Kent Tamura 2012-04-27 02:24:00 PDT
Created attachment 139153 [details]
Patch
Comment 2 Kentaro Hara 2012-04-27 11:17:43 PDT
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.
Comment 3 Kent Tamura 2012-04-28 19:04:58 PDT
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
Comment 4 Kent Tamura 2012-04-28 19:11:50 PDT
Created attachment 139386 [details]
Patch 2
Comment 5 Kentaro Hara 2012-04-28 19:38:23 PDT
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.
Comment 6 WebKit Review Bot 2012-04-29 17:25:14 PDT
Comment on attachment 139386 [details]
Patch 2

Clearing flags on attachment: 139386

Committed r115600: <http://trac.webkit.org/changeset/115600>
Comment 7 WebKit Review Bot 2012-04-29 17:25:18 PDT
All reviewed patches have been landed.  Closing bug.
Comment 8 Maciej Stachowiak 2012-04-29 18:45:01 PDT
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.
Comment 9 Kent Tamura 2012-04-30 20:06:07 PDT
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.