Bug 101449 - Implement week picking to calendar picker
Summary: Implement week picking 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-07 04:53 PST by Keishi Hattori
Modified: 2012-11-07 22:36 PST (History)
3 users (show)

See Also:


Attachments
Patch (30.81 KB, patch)
2012-11-07 05:29 PST, Keishi Hattori
no flags Details | Formatted Diff | Diff
Patch (38.00 KB, patch)
2012-11-07 20:49 PST, Keishi Hattori
no flags Details | Formatted Diff | Diff
Patch (36.73 KB, patch)
2012-11-07 21:24 PST, Keishi Hattori
no flags Details | Formatted Diff | Diff
Patch (34.51 KB, patch)
2012-11-07 21:49 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-07 04:53:18 PST
Implement week picking to calendar picker
Comment 1 Keishi Hattori 2012-11-07 05:29:07 PST
Created attachment 172767 [details]
Patch
Comment 2 WebKit Review Bot 2012-11-07 06:22:09 PST
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
Comment 3 Kent Tamura 2012-11-07 17:55:22 PST
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.
Comment 4 Kent Tamura 2012-11-07 17:57:56 PST
> Also, I think keepSelectionPosition argument should be optional in order to avoid existing callsites.

avoid updating existing callsites.
Comment 5 Keishi Hattori 2012-11-07 20:49:22 PST
Created attachment 172924 [details]
Patch
Comment 6 Keishi Hattori 2012-11-07 20:51:07 PST
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.
Comment 7 Kent Tamura 2012-11-07 21:04:10 PST
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.
Comment 8 Kent Tamura 2012-11-07 21:13:34 PST
(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?
Comment 9 Keishi Hattori 2012-11-07 21:24:05 PST
Created attachment 172927 [details]
Patch
Comment 10 Keishi Hattori 2012-11-07 21:49:21 PST
Created attachment 172929 [details]
Patch
Comment 11 Keishi Hattori 2012-11-07 21:50:29 PST
(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.
Comment 12 Kent Tamura 2012-11-07 22:14:13 PST
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.
Comment 13 WebKit Review Bot 2012-11-07 22:36:26 PST
Comment on attachment 172929 [details]
Patch

Clearing flags on attachment: 172929

Committed r133850: <http://trac.webkit.org/changeset/133850>
Comment 14 WebKit Review Bot 2012-11-07 22:36:30 PST
All reviewed patches have been landed.  Closing bug.