Created attachment 172557 [details] Patch

Comment on attachment 172557 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=172557&action=review > Source/WebCore/Resources/pagepopups/calendarPicker.js:241 > -}; > +} nit: ";" should not be removed because this is a sentence. > Source/WebCore/Resources/pagepopups/calendarPicker.js:529 > + // We assume this._config.min/max are valid dates. dates -> dates or months > Source/WebCore/Resources/pagepopups/calendarPicker.js:1203 > + return Day.parse(this._days[5][6].dataset.submitValue).endDate(); please write as _days[DaysTable._Weeks - 1][7 - 1] for readability. > Source/WebCore/Resources/pagepopups/calendarPicker.js:1385 > + * @param {!boolean} showEntireRange > + */ > +MonthPickerDaysTable.prototype.selectRange = function(month, showEntireRange) { Boolean argument is not good. Why don't you introduce another function like selectRangeAndShowEntireRange? We can remove ugly comment /*, showEntireRange*/ of DaysTable.prototype.selectRange. > Source/WebCore/Resources/pagepopups/calendarPicker.js:1424 > + var firstNodeInSelectedRange = this._firstNodeInSelectedRange(); > + if (!firstNodeInSelectedRange > + && (key == "Right" || key == "Left" || key == "Up" || key == "Down" || key == "PageUp" || key == "PageDown")) { > + this.picker.showMonth(currentMonth); In what case does this happen? this.picker.showMonth(currentMonth) looks no-op. Why is this necessary?

Created attachment 172692 [details] Patch

Comment on attachment 172557 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=172557&action=review >> Source/WebCore/Resources/pagepopups/calendarPicker.js:241 >> +} > > nit: ";" should not be removed because this is a sentence. Done. >> Source/WebCore/Resources/pagepopups/calendarPicker.js:529 >> + // We assume this._config.min/max are valid dates. > > dates -> dates or months Done. >> Source/WebCore/Resources/pagepopups/calendarPicker.js:1203 >> + return Day.parse(this._days[5][6].dataset.submitValue).endDate(); > > please write as _days[DaysTable._Weeks - 1][7 - 1] for readability. Done. >> Source/WebCore/Resources/pagepopups/calendarPicker.js:1385 >> +MonthPickerDaysTable.prototype.selectRange = function(month, showEntireRange) { > > Boolean argument is not good. > Why don't you introduce another function like selectRangeAndShowEntireRange? We can remove ugly comment /*, showEntireRange*/ of DaysTable.prototype.selectRange. Done. >> Source/WebCore/Resources/pagepopups/calendarPicker.js:1424 >> + this.picker.showMonth(currentMonth); > > In what case does this happen? > this.picker.showMonth(currentMonth) looks no-op. Why is this necessary? This was a mistake. We should be selecting the current month when there is no selection and an arrow key is pressed.

Comment on attachment 172692 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=172692&action=review > Source/WebCore/Resources/pagepopups/calendarPicker.js:1422 > +MonthPickerDaysTable.prototype.selectRangeAndShowEntireRange = function(month, showEntireRange) { showEntireRange argument is unnecessary.

Created attachment 172714 [details] Patch

Comment on attachment 172714 [details] Patch Clearing flags on attachment: 172714 Committed r133722: <http://trac.webkit.org/changeset/133722>

All reviewed patches have been landed. Closing bug.