Bug 110140

Summary: Add list view for new calendar picker
Product: WebKit Reporter: Keishi Hattori <keishi>
Component: FormsAssignee: Keishi Hattori <keishi>
Status: RESOLVED FIXED    
Severity: Normal CC: dglazkov, tkent, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 110137    
Bug Blocks: 109439    
Attachments:
Description Flags
Patch
none
Patch none

Description Keishi Hattori 2013-02-18 10:07:29 PST
Adding list view for new calendar picker
Comment 1 Keishi Hattori 2013-02-18 10:29:26 PST
Created attachment 188916 [details]
Patch
Comment 2 WebKit Review Bot 2013-02-21 05:58:51 PST
Comment on attachment 188916 [details]
Patch

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

New failing tests:
platform/chromium/fast/forms/suggestion-picker/datetimelocal-suggestion-picker-appearance-rtl.html
platform/chromium/fast/forms/suggestion-picker/date-suggestion-picker-step-attribute.html
platform/chromium/fast/forms/page-popup/page-popup-adjust-rect.html
platform/chromium/fast/forms/calendar-picker/month-picker-key-operations.html
inspector/audits/audits-panel-functional.html
platform/chromium/fast/forms/calendar-picker/calendar-picker-datetimelocal.html
platform/chromium/fast/forms/calendar-picker/calendar-picker-with-step.html
platform/chromium/fast/forms/suggestion-picker/date-suggestion-picker-appearance.html
fast/loader/text-document-wrapping.html
fast/text/international/hindi-spacing.html
platform/chromium/fast/forms/suggestion-picker/datetimelocal-suggestion-picker-key-operations.html
platform/chromium/fast/forms/suggestion-picker/date-suggestion-picker-appearance-with-scroll-bar.html
platform/chromium/fast/forms/suggestion-picker/datetimelocal-suggestion-picker-appearance-with-scroll-bar.html
platform/chromium/fast/forms/calendar-picker/date-picker-events.html
platform/chromium/fast/forms/calendar-picker/calendar-picker-pre-100-year.html
platform/chromium/fast/forms/calendar-picker/week-picker-mouse-operations.html
platform/chromium/fast/forms/suggestion-picker/datetimelocal-suggestion-picker-appearance.html
platform/chromium/fast/forms/calendar-picker/month-picker-with-step.html
platform/chromium/fast/forms/suggestion-picker/date-suggestion-picker-key-operations.html
platform/chromium/fast/forms/suggestion-picker/date-suggestion-picker-min-max-attribute.html
platform/chromium/fast/forms/suggestion-picker/datetimelocal-suggestion-picker-appearance-locale-hebrew.html
platform/chromium/fast/forms/suggestion-picker/date-suggestion-picker-appearance-rtl.html
platform/chromium/fast/forms/calendar-picker/calendar-picker-key-operations.html
platform/chromium/fast/forms/suggestion-picker/date-suggestion-picker-reset-value-after-reload.html
platform/chromium/fast/forms/calendar-picker/calendar-picker-type-change-onchange.html
platform/chromium/fast/forms/calendar-picker/month-picker-mouse-operations.html
platform/chromium/fast/forms/calendar-picker/datetimelocal-picker-events.html
platform/chromium/fast/forms/calendar-picker/calendar-picker-f4-key.html
platform/chromium/fast/forms/calendar-picker/week-picker-key-operations.html
fast/loader/javascript-url-in-object.html
Comment 3 Kent Tamura 2013-02-21 06:32:09 PST
Comment on attachment 188916 [details]
Patch

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

> Source/WebCore/Resources/pagepopups/calendarPicker.js:1097
> +    this.row = null;
> +    this._width = 0;
> +    this._position = 0;

Data members should have JsDoc comments in the constructor.

> Source/WebCore/Resources/pagepopups/calendarPicker.js:1105
> + * @return {!Array}

Array of what?

> Source/WebCore/Resources/pagepopups/calendarPicker.js:1116
> +    if (this._recycleBin().length < limit)
> +        this._recycleBin().push(this);

this._recycleBin() should be stored to a local variable.  Calling it twice makes unnecessary constraint.

> Source/WebCore/Resources/pagepopups/calendarPicker.js:1128
> + * @return {!number}

should mention it's in pixel unit.

> Source/WebCore/Resources/pagepopups/calendarPicker.js:1135
> + * @param {!number} width

ditto.

> Source/WebCore/Resources/pagepopups/calendarPicker.js:1145
> + * @return {!number}

ditto.

> Source/WebCore/Resources/pagepopups/calendarPicker.js:1152
> + * @param {!number} y

ditto.

> Source/WebCore/Resources/pagepopups/calendarPicker.js:1187
> +    this._width = 0;
> +    this._cells = {};
> +    
> +    this.selectedRow = ListView.NoSelection;
> +
> +    this.scrollView = new ScrollView();

Data members should have JsDoc comments in the constructor.

> Source/WebCore/Resources/pagepopups/calendarPicker.js:1196
> +    this._needsUpdateCells = false;

ditto.

> Source/WebCore/Resources/pagepopups/calendarPicker.js:1201
> +ListView.NoSelection = -1;

Should this be public?

> Source/WebCore/Resources/pagepopups/calendarPicker.js:1218
> +        AnimationManager.shared.on("animationFrameWillFinish", this.onAnimationFrameWillFinish);
> +    else
> +        AnimationManager.shared.removeListener("animationFrameWillFinish", this.onAnimationFrameWillFinish);

Use AnimationManager.EventTypeAnimationFrameWillFinish

> Source/WebCore/Resources/pagepopups/calendarPicker.js:1357
> + * @return {!number}

mention it's in pixel unit.

> Source/WebCore/Resources/pagepopups/calendarPicker.js:1364
> + * @param {!number} height

ditto.
Comment 4 Keishi Hattori 2013-02-24 23:46:50 PST
> > Source/WebCore/Resources/pagepopups/calendarPicker.js:1201
> > +ListView.NoSelection = -1;
> 
> Should this be public?
It could be private, but I think it should be public in case we want to check if a selection exists.
Comment 5 Keishi Hattori 2013-02-24 23:58:17 PST
Created attachment 190004 [details]
Patch
Comment 6 Kent Tamura 2013-02-25 00:22:20 PST
Comment on attachment 190004 [details]
Patch

ok
Comment 7 WebKit Review Bot 2013-02-25 02:44:12 PST
Comment on attachment 190004 [details]
Patch

Rejecting attachment 190004 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=gce-cq-02', 'land-attachment', '--force-clean', '--non-interactive', '--parent-command=commit-queue', 190004, '--port=chromium-xvfb']" exit_code: 2 cwd: /mnt/git/webkit-commit-queue

Last 500 characters of output:

fatal: read error: Connection reset by peer
Died at Tools/Scripts/update-webkit line 151.

Failed to run "['Tools/Scripts/update-webkit', '--chromium', '--force-update']" exit_code: 2

Updating OpenSource
fatal: read error: Connection reset by peer
Died at Tools/Scripts/update-webkit line 151.

Failed to run "['Tools/Scripts/update-webkit', '--chromium', '--force-update']" exit_code: 2
Updating OpenSource
fatal: read error: Connection reset by peer
Died at Tools/Scripts/update-webkit line 151.

Full output: http://queues.webkit.org/results/16746147
Comment 8 WebKit Review Bot 2013-02-25 03:32:59 PST
Comment on attachment 190004 [details]
Patch

Clearing flags on attachment: 190004

Committed r143901: <http://trac.webkit.org/changeset/143901>
Comment 9 WebKit Review Bot 2013-02-25 03:33:03 PST
All reviewed patches have been landed.  Closing bug.