http://crbug.com/177405 When hour format is 1-24, hour field does not accept input "24".
Created attachment 189503 [details] Patch
Hattori-san, could you please give an informal review for this patch?
Comment on attachment 189503 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=189503&action=review > Source/WebCore/html/HTMLInputElement.cpp:1571 > + if (m_inputType->isDateField() || m_inputType->isTimeField() || m_inputType->isDateTimeField() || m_inputType->isDateTimeLocalField()) I think we decided not to branch depending on input types in HTMLInputElement.cpp. setDateTimeFormat should be added to InputType.h as a virtual method.
Comment on attachment 189503 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=189503&action=review I have another idea. How about adding pattern attribute to DateTimeEditElement? In this case, we don't need to add an Internals method, and use getElementByPseudoId(internals.youngestShadowRoot(input), "-webkit-datetime-edit").setAttribute("pattern", "KK:mm a") (then doing something invoking updateInnerTextValue()). Also, we don't need to add extra data member for date/time format. >> Source/WebCore/html/HTMLInputElement.cpp:1571 >> + if (m_inputType->isDateField() || m_inputType->isTimeField() || m_inputType->isDateTimeField() || m_inputType->isDateTimeLocalField()) > > I think we decided not to branch depending on input types in HTMLInputElement.cpp. setDateTimeFormat should be added to InputType.h as a virtual method. Keishi is right.
Created attachment 189686 [details] Patch 2
Please take another look. (In reply to comment #4) > How about adding pattern attribute to DateTimeEditElement? That's a good idea! Done.
Comment on attachment 189686 [details] Patch 2 View in context: https://bugs.webkit.org/attachment.cgi?id=189686&action=review > Source/WebCore/html/BaseMultipleFieldsDateAndTimeInputType.cpp:376 > + const AtomicString pattern = m_dateTimeEditElement->getAttribute(HTMLNames::patternAttr); getAttribute should be fastGetAttribute because it is not a "style" attribute. You can remove HTMLNames::. > Source/WebCore/html/BaseMultipleFieldsDateAndTimeInputType.cpp:377 > + if (!pattern.isNull() && !pattern.isEmpty()) isNull() check is unnecessary. isEmpty means "null or 0-length"
Created attachment 189693 [details] Patch 3
Comment on attachment 189686 [details] Patch 2 Thank you for reviewing during your absence on leave. View in context: https://bugs.webkit.org/attachment.cgi?id=189686&action=review >> Source/WebCore/html/BaseMultipleFieldsDateAndTimeInputType.cpp:376 >> + const AtomicString pattern = m_dateTimeEditElement->getAttribute(HTMLNames::patternAttr); > > getAttribute should be fastGetAttribute because it is not a "style" attribute. > You can remove HTMLNames::. Changed to fastGetAttribute. HTMLNames:: is necessary here - or should I add "using namespace HTMLNames" at the top? >> Source/WebCore/html/BaseMultipleFieldsDateAndTimeInputType.cpp:377 >> + if (!pattern.isNull() && !pattern.isEmpty()) > > isNull() check is unnecessary. isEmpty means "null or 0-length" Done.
Comment on attachment 189686 [details] Patch 2 View in context: https://bugs.webkit.org/attachment.cgi?id=189686&action=review >>> Source/WebCore/html/BaseMultipleFieldsDateAndTimeInputType.cpp:376 >>> + const AtomicString pattern = m_dateTimeEditElement->getAttribute(HTMLNames::patternAttr); >> >> getAttribute should be fastGetAttribute because it is not a "style" attribute. >> You can remove HTMLNames::. > > Changed to fastGetAttribute. > HTMLNames:: is necessary here - or should I add "using namespace HTMLNames" at the top? We usually declare "using namespace foo" in an implementation file, and omit the namespace prefix, except std::. This is not mandatory.
Comment on attachment 189693 [details] Patch 3 Rejecting attachment 189693 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=gce-cq-03', 'land-attachment', '--force-clean', '--non-interactive', '--parent-command=commit-queue', 189693, '--port=chromium-xvfb']" exit_code: 2 cwd: /mnt/git/webkit-commit-queue Last 500 characters of output: fatal: read error: Connection reset by peer Died at Tools/Scripts/update-webkit line 151. Failed to run "['Tools/Scripts/update-webkit', '--chromium', '--force-update']" exit_code: 2 Updating OpenSource fatal: read error: Connection reset by peer Died at Tools/Scripts/update-webkit line 151. Failed to run "['Tools/Scripts/update-webkit', '--chromium', '--force-update']" exit_code: 2 Updating OpenSource fatal: read error: Connection reset by peer Died at Tools/Scripts/update-webkit line 151. Full output: http://queues.webkit.org/results/16694798
Comment on attachment 189693 [details] Patch 3 Clearing flags on attachment: 189693 Committed r143770: <http://trac.webkit.org/changeset/143770>
All reviewed patches have been landed. Closing bug.