[webkit-reviews] review denied: [Bug 98240] Implement DataList UI for input type time on chromium : [Attachment 166825] Patch

[bugzilla-daemon at webkit.org]

Kent Tamura <tkent at chromium.org> has denied Keishi Hattori
<keishi at webkit.org>'s request for review:
Bug 98240: Implement DataList UI for input type time on chromium
https://bugs.webkit.org/show_bug.cgi?id=98240

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

------- Additional Comments from Kent Tamura <tkent at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=166825&action=review


> Source/WebCore/css/html.css:573
> +    display: -webkit-box;
> +    -webkit-box-align: center;

nit:
Can you use the standard flex box instead of the legacy one?

  display: -webkit-flex;
  -webkit-align-content: center;

I thinks this works.

> Source/WebCore/html/BaseMultipleFieldsDateAndTimeInputType.cpp:96
>      , m_dateTimeEditElement(0)
> +    , m_pickerIndicatorIsVisible(false)

should initialize m_pickerIndicatorElement.

> Source/WebCore/html/BaseMultipleFieldsDateAndTimeInputType.cpp:118
> +{	

trailing whitespace

> Source/WebCore/html/BaseMultipleFieldsDateAndTimeInputType.cpp:119
> +    DEFINE_STATIC_LOCAL(AtomicString, dateAndTimeInputContainerPseudoId,
("-webkit-date-and-time-container"));

should be ("-webkit-date-and-time-container",
AtomicString::ConstructFromLiteral)

> Source/WebCore/html/BaseMultipleFieldsDateAndTimeInputType.cpp:275
> +    HTMLDataListElement* dataList = element()->dataList();
> +    if (dataList) {

nit:
if (HTMLDataListElement* dataList = element()->dataList()) {

> Source/WebCore/html/BaseMultipleFieldsDateAndTimeInputType.cpp:292
> +    m_pickerIndicatorElement->setInlineStyleProperty(CSSPropertyDisplay,
CSSValueNone);

nice to have ASSERT(m_pickerIndicatorElement) ?

> Source/WebCore/html/BaseMultipleFieldsDateAndTimeInputType.cpp:300
> +    m_pickerIndicatorElement->removeInlineStyleProperty(CSSPropertyDisplay);


ditto.

> LayoutTests/ChangeLog:32
> +	   *
platform/chromium/fast/forms/time/time-suggestion-picker-appearance-expected.tx
t: Added.
> +	   *
platform/chromium/fast/forms/time/time-suggestion-picker-appearance-rtl-expecte
d.txt: Added.
> +	   *
platform/chromium/fast/forms/time/time-suggestion-picker-appearance-rtl.html:
Added.
> +	   *
platform/chromium/fast/forms/time/time-suggestion-picker-appearance-with-scroll
-bar-expected.txt: Added.
> +	   *
platform/chromium/fast/forms/time/time-suggestion-picker-appearance-with-scroll
-bar.html: Added.
> +	   *
platform/chromium/fast/forms/time/time-suggestion-picker-appearance.html:
Added.
> +	   *
platform/chromium/fast/forms/time/time-suggestion-picker-key-operations-expecte
d.txt: Added.
> +	   *
platform/chromium/fast/forms/time/time-suggestion-picker-key-operations.html:
Added.
> +	   *
platform/chromium/fast/forms/time/time-suggestion-picker-mouse-operations-expec
ted.txt: Added.
> +	   *
platform/chromium/fast/forms/time/time-suggestion-picker-mouse-operations.html:
Added.

should update chromium/TestExpectations for new tests on non-Mac.

>
LayoutTests/fast/forms/time-multiple-fields/time-multiple-fields-spinbutton-cli
ck-in-iframe.html:5
> +<script src="../resources/common.js"></script>

Why did you add common.js to this?

>
LayoutTests/platform/chromium/fast/forms/time/time-suggestion-picker-appearance
-rtl.html:31
> +function sendKey(input, keyName, ctrlKey, altKey) {
> +    var event = document.createEvent('KeyboardEvent');
> +    event.initKeyboardEvent('keydown', true, true, document.defaultView,
keyName, 0, ctrlKey, altKey);
> +    input.dispatchEvent(event);
> +}

suggestion-picker-common.js has it.

>
LayoutTests/platform/chromium/fast/forms/time/time-suggestion-picker-appearance
-with-scroll-bar.html:32
> +function sendKey(input, keyName, ctrlKey, altKey) {
> +    var event = document.createEvent('KeyboardEvent');
> +    event.initKeyboardEvent('keydown', true, true, document.defaultView,
keyName, 0, ctrlKey, altKey);
> +    input.dispatchEvent(event);
> +}

ditto.

>
LayoutTests/platform/chromium/fast/forms/time/time-suggestion-picker-appearance
.html:9
> +    <option>01:02</option>

We should have some option values with seconds, milliseconds, afternoon times.

>
LayoutTests/platform/chromium/fast/forms/time/time-suggestion-picker-appearance
.html:34
> +function sendKey(input, keyName, ctrlKey, altKey) {
> +    var event = document.createEvent('KeyboardEvent');
> +    event.initKeyboardEvent('keydown', true, true, document.defaultView,
keyName, 0, ctrlKey, altKey);
> +    input.dispatchEvent(event);
> +}

suggestion-picker-common.js has it.