Bug 101194

Created attachment 172459 [details] Patch

Comment on attachment 172459 [details] Patch 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.

Created attachment 172480 [details] Patch

Comment on attachment 172459 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=172459&action=review >> Source/WebCore/Resources/pagepopups/calendarPicker.js:242 >> + 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. Done. >> Source/WebCore/Resources/pagepopups/calendarPicker.js:268 >> + * @return {!Day} > > !Day -> !Date Done. >> 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. Done. >> Source/WebCore/Resources/pagepopups/calendarPicker.js:620 >> +CalendarPicker.prototype.shouldShowMonth = function(month) { > > Please add jsdoc annotation. Done. >> Source/WebCore/Resources/pagepopups/calendarPicker.js:624 >> +CalendarPicker.prototype.showMonth = function(month, animate, keepSelectionPosition) { > > ditto. Done. >> Source/WebCore/Resources/pagepopups/calendarPicker.js:636 >> +CalendarPicker.prototype.currentMonth = function() { > > ditto. Done. >> Source/WebCore/Resources/pagepopups/calendarPicker.js:771 >> + 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. I didn't realize. Reverting the behavior. >> Source/WebCore/Resources/pagepopups/calendarPicker.js:1047 >> + var value = range.valueOf(); > > Please update the annotation. Done. >> Source/WebCore/Resources/pagepopups/calendarPicker.js:1056 >> + var dayIterator = month.startDate(); > > Ditto. Done. >> Source/WebCore/Resources/pagepopups/calendarPicker.js:1091 >> +DaysTable.prototype.navigateToMonth = function(month, animate, keepSelectionPosition) { > > ditto. Done. >> Source/WebCore/Resources/pagepopups/calendarPicker.js:1174 >> + this.picker.showMonth(previousMonth); > > The original code is "WithAnimation" but the new code has no "animation" flag. Done. >> Source/WebCore/Resources/pagepopups/calendarPicker.js:1185 >> + this.picker.showMonth(nextMonth); > > Ditto. Done.

Comment on attachment 172480 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=172480&action=review > Source/WebCore/Resources/pagepopups/calendarPicker.js:632 > + * @param {!bool} animate > + * @param {!bool} keepSelectionPosition Some callsites omit these bool arguments. They should be {bool=}.

Created attachment 172482 [details] Patch

Comment on attachment 172482 [details] Patch Clearing flags on attachment: 172482 Committed r133565: <http://trac.webkit.org/changeset/133565>

All reviewed patches have been landed. Closing bug.