[gwt-contrib] Re: Adds {moz,webkit}RequestAnimationFrame support to animations. (issue1355805)

[jlabanca]

The implementation looks really good, and should hopefully smooth out
animations. Take a look at my comments about the deferred binding.


http://gwt-code-reviews.appspot.com/1355805/diff/3003/user/src/com/google/gwt/animation/Animation.gwt.xml
File user/src/com/google/gwt/animation/Animation.gwt.xml (right):

http://gwt-code-reviews.appspot.com/1355805/diff/3003/user/src/com/google/gwt/animation/Animation.gwt.xml#newcode25
user/src/com/google/gwt/animation/Animation.gwt.xml:25:
<property-provider name="animationTimingSupport"><![CDATA[
This property provider is going to add 4 additional permutations:
gecko1_8:moz
gecko1_8:webkit // Compiler doesn't know this is invalid
safari:webkit
safari:moz // Compiler doesn't know this is invalid

We've been using runtime checks for features that are supported only in
newer
versions (see Canvas).  AnimationImplMoz/WebkitAnimTiming should do a
runtime check upon instantiation to check for support.  If not
supported, they should delegate
to AnimationImplTimer.

Unfortunately, that causes both the timer and native versions to be
compiled into webkit and mozilla.  However, thats better than adding
four permutations.

We're looking into an alternative to user.agent that is feature based,
but doesn't have the permutation explosion.

http://gwt-code-reviews.appspot.com/1355805/diff/3003/user/src/com/google/gwt/animation/Animation.gwt.xml#newcode48
user/src/com/google/gwt/animation/Animation.gwt.xml:48:
<when-property-is name="animationTimingSupport" value="moz" />
Replace with:
<when-property-is name="user.agent" value="gecko1_8" />

http://gwt-code-reviews.appspot.com/1355805/diff/3003/user/src/com/google/gwt/animation/Animation.gwt.xml#newcode53
user/src/com/google/gwt/animation/Animation.gwt.xml:53:
<when-property-is name="animationTimingSupport" value="webkit" />
Replace with:
<when-property-is name="user.agent" value="safari" />

http://gwt-code-reviews.appspot.com/1355805/diff/3003/user/src/com/google/gwt/animation/client/Animation.java
File user/src/com/google/gwt/animation/client/Animation.java (right):

http://gwt-code-reviews.appspot.com/1355805/diff/3003/user/src/com/google/gwt/animation/client/Animation.java#newcode98
user/src/com/google/gwt/animation/client/Animation.java:98: * passed,
the animation will be synchronize as if it started at the specified
will be synchronize => will run synchronously

http://gwt-code-reviews.appspot.com/1355805/diff/3003/user/src/com/google/gwt/animation/client/Animation.java#newcode113
user/src/com/google/gwt/animation/client/Animation.java:113: * passed,
the animation will be synchronize as if it started at the specified
will be synchronize => will run synchronously

http://gwt-code-reviews.appspot.com/1355805/diff/3003/user/src/com/google/gwt/animation/client/AnimationImplMozAnimTiming.java
File
user/src/com/google/gwt/animation/client/AnimationImplMozAnimTiming.java
(right):

http://gwt-code-reviews.appspot.com/1355805/diff/3003/user/src/com/google/gwt/animation/client/AnimationImplMozAnimTiming.java#newcode32
user/src/com/google/gwt/animation/client/AnimationImplMozAnimTiming.java:32:
cancelled = true;
cancelled is never used.  It doesn't look like the animation can be
cancelled.

http://gwt-code-reviews.appspot.com/1355805/diff/3003/user/src/com/google/gwt/animation/client/AnimationImplMozAnimTiming.java#newcode45
user/src/com/google/gwt/animation/client/AnimationImplMozAnimTiming.java:45:
if (!complete) {
If you check !cancelled here, it could result in a bad state if the
Animation is restarted synchronously.

http://gwt-code-reviews.appspot.com/1355805/diff/3003/user/src/com/google/gwt/animation/client/AnimationImplTimer.java
File user/src/com/google/gwt/animation/client/AnimationImplTimer.java
(right):

http://gwt-code-reviews.appspot.com/1355805/diff/3003/user/src/com/google/gwt/animation/client/AnimationImplTimer.java#newcode81
user/src/com/google/gwt/animation/client/AnimationImplTimer.java:81: //
Restart the timer if there is the only animation
if there is => if this is

http://gwt-code-reviews.appspot.com/1355805/diff/3003/user/src/com/google/gwt/animation/client/AnimationImplTimer.java#newcode83
user/src/com/google/gwt/animation/client/AnimationImplTimer.java:83:
Scheduler.get().scheduleFixedDelay(animationCommand,
DEFAULT_FRAME_DELAY);
If the animation runs slowly, we may end up animating with only 1ms
intervals in between, which would make the UI unresponsive.  Can you use
a ScheduledCommand instead of a RepeatingCommand, and reschedule
manually with a delay?

Also, note that in the current implementation, this animationCommand
will never stop executing in the background.

http://gwt-code-reviews.appspot.com/1355805/

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors