Bug 101024 - Introduce Month class to calendar picker
Summary: Introduce Month class to calendar picker
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: 100938
  Show dependency treegraph
 
Reported: 2012-11-02 00:32 PDT by Keishi Hattori
Modified: 2012-11-05 19:14 PST (History)
2 users (show)

See Also:


Attachments
Patch (30.11 KB, patch)
2012-11-02 01:12 PDT, Keishi Hattori
no flags Details | Formatted Diff | Diff
Patch (30.62 KB, patch)
2012-11-02 03:12 PDT, Keishi Hattori
no flags Details | Formatted Diff | Diff
Patch (30.65 KB, patch)
2012-11-02 03:34 PDT, Keishi Hattori
no flags Details | Formatted Diff | Diff
Patch (30.83 KB, patch)
2012-11-02 07:20 PDT, Keishi Hattori
no flags Details | Formatted Diff | Diff
Patch (30.76 KB, patch)
2012-11-02 07:26 PDT, Keishi Hattori
no flags Details | Formatted Diff | Diff
Patch (33.25 KB, patch)
2012-11-04 22:52 PST, 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-11-02 00:32:18 PDT
Introduce Month class to calendar picker
Comment 1 Keishi Hattori 2012-11-02 01:12:55 PDT
Created attachment 172002 [details]
Patch
Comment 2 Kent Tamura 2012-11-02 01:33:00 PDT
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.
Comment 3 Keishi Hattori 2012-11-02 03:12:44 PDT
Created attachment 172022 [details]
Patch
Comment 4 Keishi Hattori 2012-11-02 03:16:12 PDT
> > Source/WebCore/Resources/pagepopups/calendarPicker.js:267
> > +Month.prototype.minimum = function() {
> 
> nit: not good name.  createMInimumDate?
How about start/end?
Comment 5 Kent Tamura 2012-11-02 03:20:50 PDT
(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.
Comment 6 Kent Tamura 2012-11-02 03:22:13 PDT
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
Comment 7 Keishi Hattori 2012-11-02 03:34:26 PDT
Created attachment 172026 [details]
Patch
Comment 8 Keishi Hattori 2012-11-02 03:37:44 PDT
(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
Comment 9 Kent Tamura 2012-11-02 03:41:14 PDT
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'.
Comment 10 Keishi Hattori 2012-11-02 07:20:06 PDT
Created attachment 172056 [details]
Patch
Comment 11 Keishi Hattori 2012-11-02 07:26:50 PDT
Created attachment 172057 [details]
Patch
Comment 12 WebKit Review Bot 2012-11-02 10:57:21 PDT
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
Comment 13 Keishi Hattori 2012-11-04 22:52:22 PST
Created attachment 172271 [details]
Patch
Comment 14 WebKit Review Bot 2012-11-04 23:13:53 PST
Comment on attachment 172271 [details]
Patch

Clearing flags on attachment: 172271

Committed r133434: <http://trac.webkit.org/changeset/133434>
Comment 15 WebKit Review Bot 2012-11-04 23:13:56 PST
All reviewed patches have been landed.  Closing bug.