Bug 100460 – Refactor calendar picker to remove _x/_y from DaysTable

• Component: Forms
• Status: RESOLVED FIXED

[Keishi Hattori]

Refactor calendar picker to remove _x/_y from DaysTable to prepare to add week and month picking.

[Keishi Hattori]

Created attachment 170820 [details]
Patch

[Kent Tamura]

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?

[Keishi Hattori]

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().

[Keishi Hattori]

Created attachment 170830 [details]
Patch

[Kent Tamura]

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.

[Keishi Hattori]

Created attachment 170834 [details]
Patch

[Kent Tamura]

Comment on attachment 170834 [details]
Patch

ok

[WebKit Review Bot]

Comment on attachment 170834 [details]
Patch

Clearing flags on attachment: 170834

Committed r132596: <http://trac.webkit.org/changeset/132596>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.