Bug 110132 - Add animation class for new calendar picker
Summary: Add animation class for new 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: 110131
Blocks: 109439 110137
  Show dependency tree / graph
 
Reported: 2013-02-18 09:17 PST by Keishi Hattori
Modified: 2013-02-21 06:59 PST (History)
2 users (show)

See Also:

Attachments
Patch (6.64 KB, patch)
2013-02-18 09:43 PST, Keishi Hattori 		no flags Details | Formatted Diff | Diff
Patch (7.08 KB, patch)
2013-02-21 02:46 PST, Keishi Hattori 		no flags Details | Formatted Diff | Diff
Patch (7.12 KB, patch)
2013-02-21 06:05 PST, Keishi Hattori 		no flags Details | Formatted Diff | Diff
Show Obsolete (2) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
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.