Bug 101449 – Implement week picking to calendar picker

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

[Keishi Hattori]

Implement week picking to calendar picker

[Keishi Hattori]

Created attachment 172767 [details]
Patch

[WebKit Review Bot]

Comment on attachment 172767 [details]
Patch

Attachment 172767 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14760349

New failing tests:
platform/chromium/fast/forms/calendar-picker/calendar-picker-mouse-operations.html

[Kent Tamura]

Comment on attachment 172767 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=172767&action=review

> Source/WebCore/ChangeLog:45
> +        (DaysTable.prototype._handleKey): Refactored to share the same structure as subclasses.
> +        (MonthPickerDaysTable.prototype._handleKey):

Would you make another patch for the refactoring of existing _handleKey please?

> Source/WebCore/Resources/pagepopups/calendarPicker.css:236
> +.week-mode .available.day-selected.monday {

Inconsistent space before '.'

> Source/WebCore/Resources/pagepopups/calendarPicker.css:262
> +.week-mode  .week-column.unavailable.day-selected {

two spaces before '.'

> Source/WebCore/Resources/pagepopups/calendarPicker.js:200
> +    var week = Week.parse(dateString);
> +    if (week)
> +        return week;

Need to update the jsdoc annotation.

> Source/WebCore/Resources/pagepopups/calendarPicker.js:333
> +        var week = Week.createFromDate(new Date(arguments[0]));

arguments[0] is inconsistent.  it should be valueOrWeekOrYear.

> Source/WebCore/Resources/pagepopups/calendarPicker.js:407
> +    var MillisecondsPerWeek = 7 * 24 * 60 * 60 * 1000;
> +    return Math.floor((createUTCDate(year + 1, 0, 4).getTime() - Week.weekOneStartDateForYear(year)) / MillisecondsPerWeek);

This calculation appears in Week.createFromDate too.  Please consider introducing another function.

> Source/WebCore/Resources/pagepopups/calendarPicker.js:1428
> + * @param {!boolean} keepSelectionPosition
>   * @return {!boolean}
>   */
> -DaysTable.prototype._maybeSetPreviousMonth = function() {
> +DaysTable.prototype._maybeSetPreviousMonth = function(keepSelectionPosition) {

Boolean argument is not good for readability.  Please provide a wrapper function, or an enum.
Also, I think keepSelectionPosition argument should be optional in order to avoid existing callsites.

[Kent Tamura]

> Also, I think keepSelectionPosition argument should be optional in order to avoid existing callsites.

avoid updating existing callsites.

[Keishi Hattori]

Created attachment 172924 [details]
Patch

[Keishi Hattori]

Comment on attachment 172767 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=172767&action=review

>> Source/WebCore/Resources/pagepopups/calendarPicker.css:236
>> +.week-mode .available.day-selected.monday {
> 
> Inconsistent space before '.'

I'm not sure what you mean.

[Kent Tamura]

Comment on attachment 172767 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=172767&action=review

>>> Source/WebCore/Resources/pagepopups/calendarPicker.css:236
>>> +.week-mode .available.day-selected.monday {
>> 
>> Inconsistent space before '.'
> 
> I'm not sure what you mean.

Please ignore this comment.

I though you meant .week-mode.available.day-selected.monday or .week-mode .available .day-selected .monday
But your code is correct.

[Kent Tamura]

(In reply to comment #3)
> > Source/WebCore/ChangeLog:45
> > +        (DaysTable.prototype._handleKey): Refactored to share the same structure as subclasses.
> > +        (MonthPickerDaysTable.prototype._handleKey):
> 
> Would you make another patch for the refactoring of existing _handleKey please?

Don't you want to separate the patch?

[Keishi Hattori]

Created attachment 172927 [details]
Patch

[Keishi Hattori]

Created attachment 172929 [details]
Patch

[Keishi Hattori]

(In reply to comment #8)
> (In reply to comment #3)
> > > Source/WebCore/ChangeLog:45
> > > +        (DaysTable.prototype._handleKey): Refactored to share the same structure as subclasses.
> > > +        (MonthPickerDaysTable.prototype._handleKey):
> > 
> > Would you make another patch for the refactoring of existing _handleKey please?
> 
> Don't you want to separate the patch?

OK. I removed all changes to _handleKey.

[Kent Tamura]

Comment on attachment 172929 [details]
Patch

ok.

Please don't forget smaller patches are better.  Time to review a patch is superlinear. In this case, you can split the DaysTable.prototype.navigateToMonth argument change to another patch, and it would make this focus on the week support.

[WebKit Review Bot]

Comment on attachment 172929 [details]
Patch

Clearing flags on attachment: 172929

Committed r133850: <http://trac.webkit.org/changeset/133850>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.