[webkit-reviews] review denied: [Bug 101194] Introduce Day class to calendar picker : [Attachment 172459] 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 101194: Introduce Day class to calendar picker
https://bugs.webkit.org/show_bug.cgi?id=101194

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

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


> Source/WebCore/Resources/pagepopups/calendarPicker.js:242
> +Day.createFromToday = function() {
> +    return Day.createFromDate(new Date());

I think this doesn't work as expected during 00:00 - 09:00 in Japan Standard
Time.
For example, "new Date()" has 2012-11-06T07:00 in the local time, but
getUTCDate() returns 5.

> Source/WebCore/Resources/pagepopups/calendarPicker.js:268
> + * @return {!Day}

!Day -> !Date

> Source/WebCore/Resources/pagepopups/calendarPicker.js:507
> +    this._minimumValue = (typeof this._config.min !== "undefined") ?
parseDateString(this._config.min).valueOf() : Day.Minimum;

Type mismatch. parseDateString().valueOf() is a number and Day.Minimum is a
Date.

> Source/WebCore/Resources/pagepopups/calendarPicker.js:620
> +CalendarPicker.prototype.shouldShowMonth = function(month) {

Please add jsdoc annotation.

> Source/WebCore/Resources/pagepopups/calendarPicker.js:624
> +CalendarPicker.prototype.showMonth = function(month, animate,
keepSelectionPosition) {

ditto.

> Source/WebCore/Resources/pagepopups/calendarPicker.js:636
> +CalendarPicker.prototype.currentMonth = function() {

ditto.

> Source/WebCore/Resources/pagepopups/calendarPicker.js:771
> -	   this._left3.disabled = current - 13 < min;
> -    this._left2.disabled = current - 2 < min;
> -    this._left1.disabled = current - 1 < min;
> -    this._right1.disabled = current + 1 > max;
> -    this._right2.disabled = current + 2 > max;
> +	   this._left3.disabled = !this.picker.shouldShowMonth(new
Month(monthValue - 121));
> +    this._left2.disabled = !this.picker.shouldShowMonth(new Month(monthValue
- 12));
> +    this._left1.disabled = !this.picker.shouldShowMonth(new Month(monthValue
- 1));
> +    this._right1.disabled = !this.picker.shouldShowMonth(new
Month(monthValue + 1));
> +    this._right2.disabled = !this.picker.shouldShowMonth(new
Month(monthValue + 12));
>      if (this._right3)
> -	   this._right3.disabled = current + 13 > max;
> -    this._month.innerText = this._currentMonth.toLocaleString();
> +	   this._left3.disabled = !this.picker.shouldShowMonth(new
Month(monthValue + 121));

This changes behavior.	Is it intentional?
The current code intention is that _left2 button moves to min(minimum-month,
current-month - 12) and should be disabled if _left2 would have same behavior
with _left1.

> Source/WebCore/Resources/pagepopups/calendarPicker.js:1047
>   * @param {!number} time date in millisecond.
>   * @return {!boolean}
>   */
> -CalendarPicker.prototype.isValidDate = function(time) {
> -    return !this.outOfRange(time) && !this.stepMismatch(time);
> +CalendarPicker.prototype.isValidDate = function(range) {
> +    var value = range.valueOf();

Please update the annotation.

> Source/WebCore/Resources/pagepopups/calendarPicker.js:1056
>   * @param {!number} month
>   */
>  DaysTable.prototype._renderMonth = function(month) {
> -    this._currentMonth = new Month(month);
> -    var dayIterator = this._currentMonth.startDate();
> +    var dayIterator = month.startDate();

Ditto.

> Source/WebCore/Resources/pagepopups/calendarPicker.js:1091
> +DaysTable.prototype.navigateToMonth = function(month, animate,
keepSelectionPosition) {

ditto.

> Source/WebCore/Resources/pagepopups/calendarPicker.js:1174
> -    this._navigateToMonthWithAnimation(currentMonth.previous());
> +    this.picker.showMonth(previousMonth);

The original code is "WithAnimation" but the new code has no "animation" flag.

> Source/WebCore/Resources/pagepopups/calendarPicker.js:1185
> -    this._navigateToMonthWithAnimation(currentMonth.next());
> +    this.picker.showMonth(nextMonth);

Ditto.