Bug 101449

Created attachment 172767 [details] Patch

Comment on attachment 172767 [details] Patch Attachment 172767 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/14760349 New failing tests: platform/chromium/fast/forms/calendar-picker/calendar-picker-mouse-operations.html

Comment on attachment 172767 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=172767&action=review > Source/WebCore/ChangeLog:45 > + (DaysTable.prototype._handleKey): Refactored to share the same structure as subclasses. > + (MonthPickerDaysTable.prototype._handleKey): Would you make another patch for the refactoring of existing _handleKey please? > Source/WebCore/Resources/pagepopups/calendarPicker.css:236 > +.week-mode .available.day-selected.monday { Inconsistent space before '.' > Source/WebCore/Resources/pagepopups/calendarPicker.css:262 > +.week-mode .week-column.unavailable.day-selected { two spaces before '.' > Source/WebCore/Resources/pagepopups/calendarPicker.js:200 > + var week = Week.parse(dateString); > + if (week) > + return week; Need to update the jsdoc annotation. > Source/WebCore/Resources/pagepopups/calendarPicker.js:333 > + var week = Week.createFromDate(new Date(arguments[0])); arguments[0] is inconsistent. it should be valueOrWeekOrYear. > Source/WebCore/Resources/pagepopups/calendarPicker.js:407 > + var MillisecondsPerWeek = 7 * 24 * 60 * 60 * 1000; > + return Math.floor((createUTCDate(year + 1, 0, 4).getTime() - Week.weekOneStartDateForYear(year)) / MillisecondsPerWeek); This calculation appears in Week.createFromDate too. Please consider introducing another function. > Source/WebCore/Resources/pagepopups/calendarPicker.js:1428 > + * @param {!boolean} keepSelectionPosition > * @return {!boolean} > */ > -DaysTable.prototype._maybeSetPreviousMonth = function() { > +DaysTable.prototype._maybeSetPreviousMonth = function(keepSelectionPosition) { Boolean argument is not good for readability. Please provide a wrapper function, or an enum. Also, I think keepSelectionPosition argument should be optional in order to avoid existing callsites.

> Also, I think keepSelectionPosition argument should be optional in order to avoid existing callsites. avoid updating existing callsites.

Created attachment 172924 [details] Patch

Comment on attachment 172767 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=172767&action=review >> Source/WebCore/Resources/pagepopups/calendarPicker.css:236 >> +.week-mode .available.day-selected.monday { > > Inconsistent space before '.' I'm not sure what you mean.

Comment on attachment 172767 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=172767&action=review >>> Source/WebCore/Resources/pagepopups/calendarPicker.css:236 >>> +.week-mode .available.day-selected.monday { >> >> Inconsistent space before '.' > > I'm not sure what you mean. Please ignore this comment. I though you meant .week-mode.available.day-selected.monday or .week-mode .available .day-selected .monday But your code is correct.

(In reply to comment #3) > > Source/WebCore/ChangeLog:45 > > + (DaysTable.prototype._handleKey): Refactored to share the same structure as subclasses. > > + (MonthPickerDaysTable.prototype._handleKey): > > Would you make another patch for the refactoring of existing _handleKey please? Don't you want to separate the patch?

Created attachment 172927 [details] Patch

Created attachment 172929 [details] Patch

(In reply to comment #8) > (In reply to comment #3) > > > Source/WebCore/ChangeLog:45 > > > + (DaysTable.prototype._handleKey): Refactored to share the same structure as subclasses. > > > + (MonthPickerDaysTable.prototype._handleKey): > > > > Would you make another patch for the refactoring of existing _handleKey please? > > Don't you want to separate the patch? OK. I removed all changes to _handleKey.

Comment on attachment 172929 [details] Patch ok. Please don't forget smaller patches are better. Time to review a patch is superlinear. In this case, you can split the DaysTable.prototype.navigateToMonth argument change to another patch, and it would make this focus on the week support.

Comment on attachment 172929 [details] Patch Clearing flags on attachment: 172929 Committed r133850: <http://trac.webkit.org/changeset/133850>

All reviewed patches have been landed. Closing bug.