[webkit-reviews] review denied: [Bug 93929] [Forms] Make input type "time" to use multiple field time input UI : [Attachment 158204] Patch 1

[bugzilla-daemon at webkit.org]

Kent Tamura <tkent at chromium.org> has denied yosin at chromium.org's request for
review:
Bug 93929: [Forms] Make input type "time" to use multiple field time input UI
https://bugs.webkit.org/show_bug.cgi?id=93929

Attachment 158204: Patch 1
https://bugs.webkit.org/attachment.cgi?id=158204&action=review

------- Additional Comments from Kent Tamura <tkent at chromium.org>
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<>.