[webkit-reviews] review denied: [Bug 110132] Add animation class for new calendar picker : [Attachment 188910] Patch

[bugzilla-daemon at webkit.org]

Kent Tamura (ooo until Mar 15) <tkent at chromium.org> has denied Keishi Hattori
<keishi at webkit.org>'s request for review:
Bug 110132: Add animation class for new calendar picker
https://bugs.webkit.org/show_bug.cgi?id=110132

Attachment 188910: Patch
https://bugs.webkit.org/attachment.cgi?id=188910&action=review

------- Additional Comments from Kent Tamura (ooo until Mar 15)
<tkent at chromium.org>
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;