Bug 110132

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

Description Keishi Hattori 2013-02-18 09:17:57 PST
Add animation related classes for the new calendar picker
Comment 1 Keishi Hattori 2013-02-18 09:43:26 PST
Created attachment 188910 [details]
Patch
Comment 2 Kent Tamura 2013-02-20 04:21:51 PST
Comment on attachment 188910 [details]
Patch

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

> Source/WebCore/Resources/pagepopups/calendarPicker.js:782
> +    this.dispatchEvent("animationFrameWillFinish");

If the name "animationFrameWillFinish" is used by other classes, this class should provide a constant symbol for it.

> Source/WebCore/Resources/pagepopups/calendarPicker.js:797
> + */

need @override

> Source/WebCore/Resources/pagepopups/calendarPicker.js:807
> + */

Ditto.

> Source/WebCore/Resources/pagepopups/calendarPicker.js:831
> +    this.id = Animator.lastId++;
> +    this.from = 0;
> +    this.to = 0;
> +    this._delta = 0;
> +    this.duration = 100;
> +    this.step = null;
> +    this.lastStepTime = null;
> +    this.progress = 0.0;
> +    this.timingFunction = AnimationTimingFunction.Linear;

Should these members be public?  If they should not be updated by other objects, they should be private and should provide accessor functions.

> Source/WebCore/Resources/pagepopups/calendarPicker.js:832
> +};

; is not needed.

> Source/WebCore/Resources/pagepopups/calendarPicker.js:860
> +    this.dispatchEvent("didAnimationStart");

If the name "didAnimationStart" is used by other classes, this class should provide a constant symbol for it.

> Source/WebCore/Resources/pagepopups/calendarPicker.js:870
> +    this.dispatchEvent("didAnimationStop", this);

If the name "didAnimationStop" is used by other classes, this class should provide a constant symbol for it.

> Source/WebCore/Resources/pagepopups/calendarPicker.js:880
> +        return this.stop();

confusing code. should be
  this.stop();
  return;
Comment 3 Keishi Hattori 2013-02-21 02:46:39 PST
Created attachment 189491 [details]
Patch
Comment 4 Kent Tamura 2013-02-21 05:28:25 PST
Comment on attachment 189491 [details]
Patch

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

> Source/WebCore/ChangeLog:14
> +        (AnimationTimingFunction.Linear): Parameter t should be a number between 0 and 1.

Such comment should be in the code, or the code should have console.assert.

> Source/WebCore/Resources/pagepopups/calendarPicker.js:841
> +Animator.lastId = 0;

I guess this should be private.
Comment 5 Keishi Hattori 2013-02-21 06:05:00 PST
Created attachment 189516 [details]
Patch
Comment 6 WebKit Review Bot 2013-02-21 06:59:25 PST
Comment on attachment 189516 [details]
Patch

Clearing flags on attachment: 189516

Committed r143598: <http://trac.webkit.org/changeset/143598>
Comment 7 WebKit Review Bot 2013-02-21 06:59:28 PST
All reviewed patches have been landed.  Closing bug.