Bug 97633

Created attachment 165739 [details] Patch 1

Comment on attachment 165739 [details] Patch 1 Could you review this patch? Thanks in advance.

Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

Comment on attachment 165739 [details] Patch 1 View in context: https://bugs.webkit.org/attachment.cgi?id=165739&action=review > Source/WebCore/ChangeLog:16 > + Note: We'll rename ENABLE_INPUT_TYPE_MULTIPLE_FIELDS to generic name > + to support "date", "datetime", "month" and "week" multiple fields input > + UI. Why don't you rename it before this patch? > Source/WebCore/ChangeLog:22 > + (WebCore): Added declarations of dateFormatHelpLetters(), dateFormatHelpText(), monthFormatInLDML() and > + weekFormatInLDML(). Because we have no call sites of these functions yet, none except you can understand what the functions should return. Please explain what they are and why they are needed concretely.

Created attachment 165755 [details] Patch 2

Created attachment 165757 [details] Patch 3

Comment on attachment 165757 [details] Patch 3 Could you review this patch? Thanks in advance. = Changes since the last review = * Update ChangeLog to refer ENABLE_INPUT_MULTIPLE_FIELDS_UI * Add more explanations for new functions. * Fix localized string identifiers in functions.

Comment on attachment 165757 [details] Patch 3 View in context: https://bugs.webkit.org/attachment.cgi?id=165757&action=review > Source/WebKit/chromium/ChangeLog:15 > + (WebCore::dateFormatHelpLetters): Added for DateInputType, DateTimeInputType, and DateTimeLocalInputType to provide threeletters, e.g. "ymd" for English, for indicating order of day, month and year fields to display them intead of "-" when field has empty value. threeletters -> three letters? Is "年" one letter? Should we count a character of which code point is greater than U+FFFF as one letter? How about Unicode composition? > Source/WebKit/chromium/ChangeLog:16 > + (WebCore::dateFormatHelpText): Added for DateInputType, DateTimeInputType, and DateTimeLocalInputType to provide help text of order of day, month and year fields. still unclear. In what situation a user will see this text? What text is expected?

Created attachment 165762 [details] Patch 4

Comment on attachment 165762 [details] Patch 4 Could you review this patch? Thanks in advance. = Changes since the last review = * Add more explanations to new functions in ChangeLog.

Comment on attachment 165762 [details] Patch 4 View in context: https://bugs.webkit.org/attachment.cgi?id=165762&action=review > Source/WebKit/chromium/ChangeLog:15 > + (WebCore::dateFormatHelpLetters): Added for DateInputType, DateTimeInputType, and DateTimeLocalInputType to provide three letters, e.g. "ymd" for English, for indicating order of day, month and year fields to display them intead of "-" when field has empty value. Each letter should have same width to "-" in monospace font-family. This function may return empty string if there are no such letters in locale or order of day, month and year is well-known in locale. This doesn't resolve my previous question. is a sequence of 'A' and U+0300 (COMBINING GRAVE ACCENT) one letter? Why don't you support at most two letters for one field though a day/month/year field has at least two digits? BTW, these explanation should be in LocalizedStrings.h and WebCore/ChangeLog, not WebKit/chromium/ChangeLog. > Source/WebKit/chromium/ChangeLog:16 > + (WebCore::dateFormatHelpText): Added for DateInputType, DateTimeInputType, and DateTimeLocalInputType to provide help text of order of day, month and year fields, e.g. "Day/Month/Year". This string will be displayed bubble when the input element is focused and dateFormatHelpLetters() returns empty string. This function may return empty string if order of day, month, and year is well-known in locale. We already have WebCore::localizedDateFormatText(). Also, a date format depends on a local machine setting, and we need some code to build this text. Such text is not suitable for LocalizedStrings.

Created attachment 165926 [details] Patch 5

Comment on attachment 165926 [details] Patch 5 Could you review this patch? Thanks in advance. = Changes since the last review = * Use placeholderFor{DayOfMonth,Month,Year}() instead of dateFormatHelp{Letters,Text}() for simplicity of implementation and ease of localization.

Did you read my comment #11? > BTW, these explanation should be in LocalizedStrings.h and WebCore/ChangeLog, not WebKit/chromium/ChangeLog.

Created attachment 165930 [details] Patch 6

Comment on attachment 165930 [details] Patch 6 Could you review this patch? Thanks in advance. = Changes since the last review = * Move explanation of new functions into WebCore/platform/LocalizedString.h and WebCore/ChangeLog from WebKit/chromium/ChangeLog

Comment on attachment 165930 [details] Patch 6 ok

Comment on attachment 165930 [details] Patch 6 Clearing flags on attachment: 165930 Committed r129728: <http://trac.webkit.org/changeset/129728>

All reviewed patches have been landed. Closing bug.

See http://crbug.com/152632 for Chromium side changes.