Bug 85039

Created attachment 139153 [details] Patch

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 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

Created attachment 139386 [details] Patch 2

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 on attachment 139386 [details] Patch 2 Clearing flags on attachment: 139386 Committed r115600: <http://trac.webkit.org/changeset/115600>

All reviewed patches have been landed. Closing bug.

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.

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.