Bug 101024

Created attachment 172002 [details] Patch

Comment on attachment 172002 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=172002&action=review Nice refactoring. I have some comments. > Source/WebCore/Resources/pagepopups/calendarPicker.js:146 > + Month.prototype.toLocaleString = function() { Need to update the jsdoc annotation above. > Source/WebCore/Resources/pagepopups/calendarPicker.js:214 > +function Month(valueOrMonthOrYear, month) { Please add jsdoc annotation. > Source/WebCore/Resources/pagepopups/calendarPicker.js:223 > + this.year = Math.floor(arguments[0] / 12) + 1970; > + this.month = arguments[0] % 12; nit: using arguments[0] is inconsistent. valueOrMonthOrYear is better for consistency. > Source/WebCore/Resources/pagepopups/calendarPicker.js:242 > +Month.parse = function(str) { Please add jsdoc annotation. > Source/WebCore/Resources/pagepopups/calendarPicker.js:251 > +Month.containing = function(date) { nit: The name "containing" looks not good. createFromDate? > Source/WebCore/Resources/pagepopups/calendarPicker.js:267 > +Month.prototype.minimum = function() { nit: not good name. createMInimumDate? > Source/WebCore/Resources/pagepopups/calendarPicker.js:271 > +Month.prototype.maximum = function() { Ditto. > Source/WebCore/Resources/pagepopups/calendarPicker.js:629 > + * @return {?month} nit: month -> Month > Source/WebCore/Resources/pagepopups/calendarPicker.js:639 > * @param {!number} year > * @param {!number} month > */ > -YearMonthController.prototype.setYearMonth = function(year, month) { > - this._currentYear = year; > - this._currentMonth = month; > +YearMonthController.prototype.setMonth = function(month) { Update the jsdoc annotation please. > Source/WebCore/Resources/pagepopups/calendarPicker.js:990 > * @param {!number} year > * @param {!number} month > */ > -DaysTable.prototype._navigateToMonth = function(year, month) { > - this.picker.yearMonthController.setYearMonth(year, month); > - this._renderMonth(year, month); > +DaysTable.prototype._navigateToMonth = function(month) { Need to update the jsdoc annotation. > Source/WebCore/Resources/pagepopups/calendarPicker.js:999 > * @param {!number} year > * @param {!number} month > */ > -DaysTable.prototype._navigateToMonthWithAnimation = function(year, month) { > - if (this._currentYear >= 0 && this._currentMonth >= 0) { > - if (year == this._currentYear && month == this._currentMonth) > +DaysTable.prototype._navigateToMonthWithAnimation = function(month) { Ditto. > Source/WebCore/Resources/pagepopups/calendarPicker.js:1028 > * @param {!number} year > * @param {!number} month > */ > -DaysTable.prototype.navigateToMonthAndKeepSelectionPosition = function(year, month) { > - if (year == this._currentYear && month == this._currentMonth) > +DaysTable.prototype.navigateToMonthAndKeepSelectionPosition = function(month) { > + if (this._currentMonth.equals(month)) Ditto.

Created attachment 172022 [details] Patch

> > Source/WebCore/Resources/pagepopups/calendarPicker.js:267 > > +Month.prototype.minimum = function() { > > nit: not good name. createMInimumDate? How about start/end?

(In reply to comment #4) > > > Source/WebCore/Resources/pagepopups/calendarPicker.js:267 > > > +Month.prototype.minimum = function() { > > > > nit: not good name. createMInimumDate? > How about start/end? Just "start" or "end" sounds bad. The problem is that one can't imagine the meaning of code like "month.start()" without reading the function definition. It's bad code readability. startDate, or createStartDate is better.

Comment on attachment 172022 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=172022&action=review > Source/WebCore/Resources/pagepopups/calendarPicker.js:144 > + Month.prototype.toLocaleString = function() { unnecessary leading space

Created attachment 172026 [details] Patch

(In reply to comment #5) > (In reply to comment #4) > > > > Source/WebCore/Resources/pagepopups/calendarPicker.js:267 > > > > +Month.prototype.minimum = function() { > > > > > > nit: not good name. createMInimumDate? > > How about start/end? > > Just "start" or "end" sounds bad. The problem is that one can't imagine the meaning of code like "month.start()" without reading the function definition. It's bad code readability. > > startDate, or createStartDate is better. Going with startDate/endDate

Comment on attachment 172026 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=172026&action=review ok > Source/WebCore/ChangeLog:23 > + (Month.prototype.start): Returns a datetime that is the start of this month. The value is inclusive. > + (Month.prototype.end): Returns a datetime that is the end of this month. The value is exclusive. Need to update > Source/WebCore/Resources/pagepopups/calendarPicker.js:1118 > + var currentMonthStart = currentMonth.startDate(); > + if (this.picker.minimumDate >= currentMonthStart) nit: You can remove the local variable 'currentMonthStart'.

Created attachment 172056 [details] Patch

Created attachment 172057 [details] Patch

Comment on attachment 172057 [details] Patch Rejecting attachment 172057 [details] from commit-queue. New failing tests: platform/chromium/fast/forms/calendar-picker/calendar-picker-key-operations.html Full output: http://queues.webkit.org/results/14678919

Created attachment 172271 [details] Patch

Comment on attachment 172271 [details] Patch Clearing flags on attachment: 172271 Committed r133434: <http://trac.webkit.org/changeset/133434>

All reviewed patches have been landed. Closing bug.