Bug 100460 - Refactor calendar picker to remove _x/_y from DaysTable
Summary: Refactor calendar picker to remove _x/_y from DaysTable
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Forms (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Keishi Hattori
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-10-25 22:31 PDT by Keishi Hattori
Modified: 2012-10-26 02:56 PDT (History)
2 users (show)

See Also:


Attachments
Patch (7.02 KB, patch)
2012-10-25 22:48 PDT, Keishi Hattori
no flags Details | Formatted Diff | Diff
Patch (7.75 KB, patch)
2012-10-25 23:50 PDT, Keishi Hattori
no flags Details | Formatted Diff | Diff
Patch (7.71 KB, patch)
2012-10-26 00:06 PDT, Keishi Hattori
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Keishi Hattori 2012-10-25 22:31:19 PDT
Refactor calendar picker to remove _x/_y from DaysTable to prepare to add week and month picking.
Comment 1 Keishi Hattori 2012-10-25 22:48:46 PDT
Created attachment 170820 [details]
Patch
Comment 2 Kent Tamura 2012-10-25 23:10:36 PDT
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 3 Keishi Hattori 2012-10-25 23:28:41 PDT
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().
Comment 4 Keishi Hattori 2012-10-25 23:50:47 PDT
Created attachment 170830 [details]
Patch
Comment 5 Kent Tamura 2012-10-25 23:55:21 PDT
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.
Comment 6 Keishi Hattori 2012-10-26 00:06:48 PDT
Created attachment 170834 [details]
Patch
Comment 7 Kent Tamura 2012-10-26 00:10:54 PDT
Comment on attachment 170834 [details]
Patch

ok
Comment 8 WebKit Review Bot 2012-10-26 02:56:37 PDT
Comment on attachment 170834 [details]
Patch

Clearing flags on attachment: 170834

Committed r132596: <http://trac.webkit.org/changeset/132596>
Comment 9 WebKit Review Bot 2012-10-26 02:56:40 PDT
All reviewed patches have been landed.  Closing bug.