Bug 93929

Created attachment 158204 [details] Patch 1

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

Comment on attachment 158204 [details] Patch 1 Attachment 158204 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13492397 New failing tests: fast/forms/time/time-stepup-stepdown-from-renderer.html fast/forms/time/time-input-visible-string.html

Created attachment 158211 [details] Archive of layout-test-results from gce-cr-linux-06 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: gce-cr-linux-06 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

Comment on attachment 158204 [details] Patch 1 View in context: https://bugs.webkit.org/attachment.cgi?id=158204&action=review > Source/WebCore/ChangeLog:16 > + * bindings/generic/RuntimeEnabledFeatures.cpp: > + (WebCore): Incorrect file entry > Source/WebCore/html/HTMLInputElement.cpp:728 > +#if ENABLE(INPUT_TYPE_TIME_MULTIPLE_FIELDS) > + if (attribute.name() == maxAttr || attribute.name() == minAttr || attribute.name() == stepAttr) > + m_inputType->stepRangeChanged(); > +#endif No need to introduce InputType::stepRangeChanged(). We should override InputType::minOrMaxAttributeChanged() and InputType::stepAttributeChanged(). > Source/WebCore/html/TimeInputType.cpp:146 > + ChromeClient* chromeClient = document->page() ? document->page()->chrome()->client() : 0; > + bool shouldAddDecorations = chromeClient && chromeClient->willAddTextFieldDecorationsTo(element()); > + > + if (shouldAddDecorations) > + chromeClient->addTextFieldDecorationsTo(element()); Remove this code. The time type is no longer a text field. So we don't care about TextFieldDecorations. > Source/WebCore/html/TimeInputType.cpp:154 > + DateComponents date; > + if (parseToDateComponents(element()->value(), &date)) > + m_dateTimeEditElement->setValueAsDate(date); > + else { > + setMillisecondToDateComponents(stepRange.minimum().toDouble(), &date); > + m_dateTimeEditElement->setEmptyValue(date); > + } should call updateInnerTextValue(). > Source/WebCore/html/TimeInputType.cpp:160 > + m_dateTimeEditElement->removeEditControlOwner(); > + m_dateTimeEditElement.clear(); We need to detach m_dateTimeEditElement from the oldest ShadowRoot. > Source/WebCore/html/TimeInputType.h:47 > +#if ENABLE(INPUT_TYPE_TIME_MULTIPLE_FIELDS) > +#include "DateTimeEditElement.h" > +#define TIME_INPUT_TYPE_BASE_CLASS_LIST public BaseDateAndTimeInputType, private DateTimeEditElement::EditControlOwner > +#else > +#define TIME_INPUT_TYPE_BASE_CLASS_LIST public BaseDateAndTimeInputType > +#endif > + > namespace WebCore { > > -class TimeInputType : public BaseDateAndTimeInputType { > +class TimeInputType : TIME_INPUT_TYPE_BASE_CLASS_LIST { This looks ugly. How about introducing a small proxy class implementing EditControlOwner? > Source/WebCore/html/TimeInputType.h:89 > + RefPtr<DateTimeEditElement> m_dateTimeEditElement; This should be a raw pointer instead of RefPtr<>.

Created attachment 158233 [details] Patch 2

Created attachment 158244 [details] Patch 3

Comment on attachment 158244 [details] Patch 3 Could you review this patch? Thanks in advance. = Changes since the last review = * Add test expectations for Chromium * Remove InputType::stepRangeChanged() ** No changes for HTMLInputElement.cpp and InputType.{cpp,h} * Introduce DateTimeEditControlOwner class for avoiding ugly #ifdef on base class list.

Comment on attachment 158244 [details] Patch 3 View in context: https://bugs.webkit.org/attachment.cgi?id=158244&action=review > Source/WebCore/html/TimeInputType.cpp:45 > +#include "Chrome.h" Do we need Chrome.h? > Source/WebCore/html/TimeInputType.cpp:135 > + Document* document = element()->document(); The local variable 'document' is used only once. We don't need it. > Source/WebCore/html/TimeInputType.h:52 > +#if ENABLE(INPUT_TYPE_TIME_MULTIPLE_FIELDS) > +class DateTimeEditElementOwner : protected DateTimeEditElement::EditControlOwner { > +}; > +#else > +class DateTimeEditElementOwner { > +}; > +#endif > + > +class TimeInputType : public BaseDateAndTimeInputType, private DateTimeEditElementOwner { Oh, I didn't mean it though this patch is better than "Patch 1" I meant we didn't change the base class list, but TimeInputType had an data member of DateTimeEditElementOwnerImpl class.

Created attachment 158276 [details] Patch 4

Comment on attachment 158276 [details] Patch 4 Could you review this patch? Thanks in advance. = Changes since the last review = * Introduce TimeInputType::DateTimeEditOwnerImpl class and TimeInputType::m_dateTimeEditOwnerImpl for keep base class list of TimeInputType class.

Comment on attachment 158276 [details] Patch 4 View in context: https://bugs.webkit.org/attachment.cgi?id=158276&action=review > Source/WebCore/html/TimeInputType.cpp:283 > +#endif // ENABLE(INPUT_TYPE_TIME_MULTIPLE_FIELDS) The comment is confusing. I guess some people recognize the code just above the comment is enabled if ENABLE(INPUT_TYPE_TIME_MULTIPLE_FILEDS). We had better remove the comment because we don't have a common comment notation for the end of #else block.

Created attachment 158478 [details] Patch 5

Comment on attachment 158478 [details] Patch 5 Clearing flags on attachment: 158478 Committed r125634: <http://trac.webkit.org/changeset/125634>

All reviewed patches have been landed. Closing bug.

This change broke Chromium-android, and I landed a build fix: <http://trac.webkit.org/changeset/125649>. Please ping me in case this fix is not right.