Bug 101024 – Introduce Month class to calendar picker

• Status: RESOLVED
• Resolution: FIXED
• Component: Forms

[Keishi Hattori]

Introduce Month class to calendar picker

[Keishi Hattori]

Created attachment 172002 [details]
Patch

[Kent Tamura]

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.

[Keishi Hattori]

Created attachment 172022 [details]
Patch

[Keishi Hattori]

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

[Kent Tamura]

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

[Kent Tamura]

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

[Keishi Hattori]

Created attachment 172026 [details]
Patch

[Keishi Hattori]

(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

[Kent Tamura]

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'.

[Keishi Hattori]

Created attachment 172056 [details]
Patch

[Keishi Hattori]

Created attachment 172057 [details]
Patch

[WebKit Review Bot]

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

[Keishi Hattori]

Created attachment 172271 [details]
Patch

[WebKit Review Bot]

Comment on attachment 172271 [details]
Patch

Clearing flags on attachment: 172271

Committed r133434: <http://trac.webkit.org/changeset/133434>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.