Bug 93929 – [Forms] Make input type "time" to use multiple field time input UI

• Component: Forms
• Status: RESOLVED FIXED

This feature is inside ENABLE_INPUT_TIME_MULTIPLE_FILED build flag.

Created attachment 158204 [details]
Patch 1

Comment on attachment 158204 [details]
Patch 1

Could you review this patch?
Thanks in advance.

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Kent Tamura]

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.

[Kent Tamura]

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.

[Kent Tamura]

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.

[Yuta Kitamura]

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.