Bug 101889

Created attachment 173568 [details] Patch

Comment on attachment 173568 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=173568&action=review > Source/WebCore/html/shadow/DateTimeEditElement.cpp:487 > +void DateTimeEditElement::setValueFromCalendarPicker(const DateComponents& date) nit: It's not a good name because it doesn't represent what the function does. My proposal is setOnlyYearMonthDay. > Source/WebCore/html/shadow/DateTimeEditElement.cpp:498 > + setValueAsDateTimeFieldsState(dateTimeFieldsState); Can you avoid to change the setValueAsDateTimeFieldsState signature? I think passing a DateComponents for the old value as dateForReadOnlyField argument works well though I'm not 100% sure. > Source/WebCore/html/shadow/PickerIndicatorElement.h:53 > + class PickerIndicatorOwner { > + public: > + virtual ~PickerIndicatorOwner() { } > + virtual void pickerIndicatorChooseValue(const String&) = 0; > + }; Introducing PickerIndicatorOwner interface is nice. However it's not great that it has only one function. If you'd like to introduce PickerIndicatorOwner, you should remove HTMLInputElement (hostInput()) dependency from PickerIndicatorElement, and PickerIndicatorOwner should replace the role of HTMLInputElement in PickerIndicatorElement. Such change should be done *before* this bug. > Source/WebCore/rendering/RenderThemeChromiumCommon.cpp:56 > return type == InputTypeNames::date() > || type == InputTypeNames::month() > - || type == InputTypeNames::week(); > + || type == InputTypeNames::week() > + || type == InputTypeNames::datetime() > + || type == InputTypeNames::datetimelocal(); nit: please sort in alphabetical order

Comment on attachment 173568 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=173568&action=review >> Source/WebCore/html/shadow/DateTimeEditElement.cpp:498 >> + setValueAsDateTimeFieldsState(dateTimeFieldsState); > > Can you avoid to change the setValueAsDateTimeFieldsState signature? > I think passing a DateComponents for the old value as dateForReadOnlyField argument works well though I'm not 100% sure. Hmm, It won't work if there is a read-only field and fields has empty values.

Comment on attachment 173568 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=173568&action=review >>> Source/WebCore/html/shadow/DateTimeEditElement.cpp:498 >>> + setValueAsDateTimeFieldsState(dateTimeFieldsState); >> >> Can you avoid to change the setValueAsDateTimeFieldsState signature? >> I think passing a DateComponents for the old value as dateForReadOnlyField argument works well though I'm not 100% sure. > > Hmm, It won't work if there is a read-only field and fields has empty values. The logic same as BaseMultipleFieldsDateAndTimeInputType::updateInnerTextValue should work. i.e. Use the previous value for dateForReadOnlyField if it's not empty, otherwise use StepRange::minimum().

Comment on attachment 173568 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=173568&action=review >> Source/WebCore/html/shadow/PickerIndicatorElement.h:53 >> + }; > > Introducing PickerIndicatorOwner interface is nice. However it's not great that it has only one function. > If you'd like to introduce PickerIndicatorOwner, you should remove HTMLInputElement (hostInput()) dependency from PickerIndicatorElement, and PickerIndicatorOwner should replace the role of HTMLInputElement in PickerIndicatorElement. Such change should be done *before* this bug. OK, I will do that first. I've filed another bug for this: https://bugs.webkit.org/show_bug.cgi?id=101913

Created attachment 174064 [details] Patch 2

Comment on attachment 173568 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=173568&action=review >> Source/WebCore/html/shadow/DateTimeEditElement.cpp:487 >> +void DateTimeEditElement::setValueFromCalendarPicker(const DateComponents& date) > > nit: It's not a good name because it doesn't represent what the function does. My proposal is setOnlyYearMonthDay. Renamed it to setOnlyYearMonthDay. >>>> Source/WebCore/html/shadow/DateTimeEditElement.cpp:498 >>>> + setValueAsDateTimeFieldsState(dateTimeFieldsState); >>> >>> Can you avoid to change the setValueAsDateTimeFieldsState signature? >>> I think passing a DateComponents for the old value as dateForReadOnlyField argument works well though I'm not 100% sure. >> >> Hmm, It won't work if there is a read-only field and fields has empty values. > > The logic same as BaseMultipleFieldsDateAndTimeInputType::updateInnerTextValue should work. > i.e. Use the previous value for dateForReadOnlyField if it's not empty, otherwise use StepRange::minimum(). Done. Thanks to your refactoring, now setValueAsDateTimeFieldsState does not require dateForReadOnlyField. >> Source/WebCore/rendering/RenderThemeChromiumCommon.cpp:56 >> + || type == InputTypeNames::datetimelocal(); > > nit: please sort in alphabetical order Done.

Comment on attachment 174064 [details] Patch 2 View in context: https://bugs.webkit.org/attachment.cgi?id=174064&action=review > LayoutTests/ChangeLog:12 > + * platform/chromium/fast/forms/calendar-picker/calendar-picker-datetime-local-expected.txt: Added. > + * platform/chromium/fast/forms/calendar-picker/calendar-picker-datetime-local.html: Added. nit: Please rename calendar-picker-datetime-local.html to calendar-picker-datetimelocal.html. We omit '-' of datetime-local for test names.

Created attachment 174071 [details] Patch 3

Comment on attachment 174064 [details] Patch 2 View in context: https://bugs.webkit.org/attachment.cgi?id=174064&action=review >> LayoutTests/ChangeLog:12 >> + * platform/chromium/fast/forms/calendar-picker/calendar-picker-datetime-local.html: Added. > > nit: Please rename calendar-picker-datetime-local.html to calendar-picker-datetimelocal.html. We omit '-' of datetime-local for test names. Done.

Comment on attachment 174071 [details] Patch 3 ok

Comment on attachment 174071 [details] Patch 3 Clearing flags on attachment: 174071 Committed r134546: <http://trac.webkit.org/changeset/134546>

All reviewed patches have been landed. Closing bug.