[webkit-reviews] review denied: [Bug 109439] Update calendar picker UI : [Attachment 191969] Patch

[bugzilla-daemon at webkit.org]

Kent Tamura (ooo until Mar 15) <tkent at chromium.org> has denied Keishi Hattori
<keishi at webkit.org>'s request for review:
Bug 109439: Update calendar picker UI
https://bugs.webkit.org/show_bug.cgi?id=109439

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

------- Additional Comments from Kent Tamura (ooo until Mar 15)
<tkent at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=191969&action=review


> Source/WebCore/Resources/pagepopups/calendarPicker.css:49
> +    -webkit-transform: translate(0, 0);

Why is this needed?

> Source/WebCore/Resources/pagepopups/calendarPicker.css:70
> +    vertical-align:middle;

nit: need a space after :

> Source/WebCore/Resources/pagepopups/calendarPicker.js:3284
> +    this.setCurrentMonth(Month.createFromDate(new Date()), false);

Is this needed? setCurrentMonth is always called again later in this function.

> Source/WebCore/Resources/pagepopups/calendarPicker.js:3553
> +CalendarPicker.prototype._moveHighlight = function(dateRange) {

should have JsDoc comment

> Source/WebCore/Resources/pagepopups/calendarPicker.js:3642
> +    switch (key) {
> +	   case "U+001B": // Esc key.

We don't add indentation for "case"

> LayoutTests/ChangeLog:7
> +

We should have a test to check the appearance of the month popup.
r- because of this and the EWS failure.

>
LayoutTests/platform/chromium-win/fast/forms/calendar-picker/date-open-picker-w
ith-f4-key-expected.txt:1
> +Tests if pressing F4 opens the calendar picker.

Ideally, *-open-picker-with-f4-key* changes should be done before this patch to
minimize the patch size.