Refactor calendar picker to remove _x/_y from DaysTable to prepare to add week and month picking.
Created attachment 170820 [details] Patch
Comment on attachment 170820 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=170820&action=review > Source/WebCore/ChangeLog:26 > + (DaysTable): > + (DaysTable.prototype._hasSelection): > + (DaysTable.prototype.navigateToMonthAndKeepSelectionPosition): > + (DaysTable.prototype.selectDate): > + (DaysTable.prototype.selectRangeContainingNode): Selects date/week/month containing the given day node. > + (DaysTable.prototype.selectRangeAtPosition): Selects date/week/month at the given position. > + (DaysTable.prototype.firstSelectedNode): Returns first selected day node. > + (DaysTable.prototype.deselect): Deselects all selections. > + (DaysTable.prototype._handleMouseOver): > + (DaysTable.prototype._handleMouseOut): > + (DaysTable.prototype._handleKey): > + (DaysTable.prototype.updateSelection): Please write per-function "why" comments especially to existing functions. > Source/WebCore/Resources/pagepopups/calendarPicker.js:1011 > +DaysTable.prototype.selectRangeContainingNode = function(dayNode) { should be _selectRangeContainingNode Also, you might want to add a FIXME comment about a reason why the function is named "range". > Source/WebCore/Resources/pagepopups/calendarPicker.js:1022 > +DaysTable.prototype.selectRangeAtPosition = function(x, y) { should be _selectRangeAtPosition > Source/WebCore/Resources/pagepopups/calendarPicker.js:1029 > +DaysTable.prototype.firstSelectedNode = function() { I think this should be _firstNodeInSelectedRange > Source/WebCore/Resources/pagepopups/calendarPicker.js:1033 > +DaysTable.prototype.deselect = function() { should be _deselect. > Source/WebCore/Resources/pagepopups/calendarPicker.js:-1161 > - this._days[this._y][this._x].classList.remove(ClassNames.Selected); Why is this correct? Who removes ClassNames.Selected?
Comment on attachment 170820 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=170820&action=review >> Source/WebCore/Resources/pagepopups/calendarPicker.js:1011 >> +DaysTable.prototype.selectRangeContainingNode = function(dayNode) { > > should be _selectRangeContainingNode > Also, you might want to add a FIXME comment about a reason why the function is named "range". Done. >> Source/WebCore/Resources/pagepopups/calendarPicker.js:1022 >> +DaysTable.prototype.selectRangeAtPosition = function(x, y) { > > should be _selectRangeAtPosition Done. >> Source/WebCore/Resources/pagepopups/calendarPicker.js:1029 >> +DaysTable.prototype.firstSelectedNode = function() { > > I think this should be _firstNodeInSelectedRange Done. >> Source/WebCore/Resources/pagepopups/calendarPicker.js:1033 >> +DaysTable.prototype.deselect = function() { > > should be _deselect. Done. >> Source/WebCore/Resources/pagepopups/calendarPicker.js:-1161 >> - this._days[this._y][this._x].classList.remove(ClassNames.Selected); > > Why is this correct? Who removes ClassNames.Selected? This was wrong. I will deselect inside DaysTable.prototype.selectDate().
Created attachment 170830 [details] Patch
Comment on attachment 170830 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=170830&action=review > Source/WebCore/ChangeLog:22 > + (DaysTable.prototype.selectRangeContainingNode): Selects date/week/month containing the given day node. > + (DaysTable.prototype.selectRangeAtPosition): Selects date/week/month at the given position. > + (DaysTable.prototype.firstSelectedNode): Returns first selected day node. > + (DaysTable.prototype.deselect): Deselects all selections. Function names should be updated to "_foo" > Source/WebCore/Resources/pagepopups/calendarPicker.js:1031 > +DaysTable.prototype.firstNodeInSelectedRange = function() { should be _firstNodeInSelectedRange? > Source/WebCore/Resources/pagepopups/calendarPicker.js:1037 > + var selectedNodesLength = selectedNodes.length; This variable is not used.
Created attachment 170834 [details] Patch
Comment on attachment 170834 [details] Patch ok
Comment on attachment 170834 [details] Patch Clearing flags on attachment: 170834 Committed r132596: <http://trac.webkit.org/changeset/132596>
All reviewed patches have been landed. Closing bug.