I'll let fabbott continue the review, unless you have remarks on my comments.
http://gwt-code-reviews.appspot.com/1446812/diff/3002/user/src/com/google/gwt/animation/client/AnimationSchedulerImpl.java File user/src/com/google/gwt/animation/client/AnimationSchedulerImpl.java (right): http://gwt-code-reviews.appspot.com/1446812/diff/3002/user/src/com/google/gwt/animation/client/AnimationSchedulerImpl.java#newcode16 user/src/com/google/gwt/animation/client/AnimationSchedulerImpl.java:16: package com.google.gwt.animation.client; Should everything *Impl be moved in to an "impl" subpackage? http://gwt-code-reviews.appspot.com/1446812/diff/3002/user/src/com/google/gwt/animation/client/AnimationSchedulerImpl.java#newcode38 user/src/com/google/gwt/animation/client/AnimationSchedulerImpl.java:38: * don't want to create a new AnimationSchedulerImplTimer in this case). unbalanced parenthesis http://gwt-code-reviews.appspot.com/1446812/diff/3002/user/src/com/google/gwt/animation/client/AnimationSchedulerImplMozilla.java File user/src/com/google/gwt/animation/client/AnimationSchedulerImplMozilla.java (right): http://gwt-code-reviews.appspot.com/1446812/diff/3002/user/src/com/google/gwt/animation/client/AnimationSchedulerImplMozilla.java#newcode35 user/src/com/google/gwt/animation/client/AnimationSchedulerImplMozilla.java:35: private class AnimationRequestIdImpl extends AnimationHandle { Rename to AnimationHandleImpl? http://gwt-code-reviews.appspot.com/1446812/diff/3002/user/src/com/google/gwt/animation/client/AnimationSchedulerImplMozilla.java#newcode56 user/src/com/google/gwt/animation/client/AnimationSchedulerImplMozilla.java:56: return $wnd.mozRequestAnimationFrame ? true : false; "return !!$wnd.mozRequestAnimationFrame" ? http://gwt-code-reviews.appspot.com/1446812/diff/3002/user/src/com/google/gwt/animation/client/AnimationSchedulerImplMozilla.java#newcode75 user/src/com/google/gwt/animation/client/AnimationSchedulerImplMozilla.java:75: handle.canceled = false; Hox about constructing an AnimationHandleImpl object in requestAnimationFrame and passing it as an argument here, given that it's always constructed? (or construct it here in the JSNI if you prefer) http://gwt-code-reviews.appspot.com/1446812/diff/3002/user/src/com/google/gwt/animation/client/AnimationSchedulerImplMozilla.java#newcode77 user/src/com/google/gwt/animation/client/AnimationSchedulerImplMozilla.java:77: var _callback = callback; You don't need that. Using 'callback' in the 'wrapper' function will just work. http://gwt-code-reviews.appspot.com/1446812/diff/3002/user/src/com/google/gwt/animation/client/AnimationSchedulerImplTimer.java File user/src/com/google/gwt/animation/client/AnimationSchedulerImplTimer.java (right): http://gwt-code-reviews.appspot.com/1446812/diff/3002/user/src/com/google/gwt/animation/client/AnimationSchedulerImplTimer.java#newcode34 user/src/com/google/gwt/animation/client/AnimationSchedulerImplTimer.java:34: private class AnimationRequestIdImpl extends AnimationHandle { Rename to AnimationHandleImpl http://gwt-code-reviews.appspot.com/1446812/diff/3002/user/src/com/google/gwt/animation/client/AnimationSchedulerImplTimer.java#newcode133 user/src/com/google/gwt/animation/client/AnimationSchedulerImplTimer.java:133: int execTime = (int) (Duration.currentTimeMillis() - curTime); How about using a Duration *object* instead, isn't it meant for these use cases? http://gwt-code-reviews.appspot.com/1446812/diff/3002/user/src/com/google/gwt/animation/client/testing/StubAnimationSchedulerImpl.java File user/src/com/google/gwt/animation/client/testing/StubAnimationSchedulerImpl.java (right): http://gwt-code-reviews.appspot.com/1446812/diff/3002/user/src/com/google/gwt/animation/client/testing/StubAnimationSchedulerImpl.java#newcode29 user/src/com/google/gwt/animation/client/testing/StubAnimationSchedulerImpl.java:29: public class StubAnimationSchedulerImpl extends AnimationSchedulerImpl { Should IMO extend AnimationScheduler to bypass the AnimationSchedulerImpl clinit and be usable in a "plain JVM" without requiring GWTMockUtilities.disarm(). AnimationSchedulerImpl could then be tweaked a bit to allow the GWT.create() to return something that doesn't extend AnimationSchedulerImpl (I'd then also change it to GWT.create(AnimationScheduler.class) rather than AnimationSchedulerImpl.class), so that StubAnimationSchedulerImpl could still be used with deferred-binding as in the unit tests here. But even better would be, IMO, to allow somehow "injecting" the AnimationScheduler actual implementation into the Animation class, so that AnimationTest could run in a plain-JVM instead of being a GWTTestCase. It could be as simple as adding a protected getAnimationScheduler to the Animation class that defaults to returning AnimationScheduler.get(), and overriding it in the TestAnimation to return the StubAnimationScheduler. http://gwt-code-reviews.appspot.com/1446812/diff/3002/user/test/com/google/gwt/animation/AnimationTest.gwt.xml File user/test/com/google/gwt/animation/AnimationTest.gwt.xml (right): http://gwt-code-reviews.appspot.com/1446812/diff/3002/user/test/com/google/gwt/animation/AnimationTest.gwt.xml#newcode25 user/test/com/google/gwt/animation/AnimationTest.gwt.xml:25: <replace-with class="com.google.gwt.animation.client.testing.StubAnimationSchedulerImpl"> Would probably be better to somehow "inject" a StubAnimationScheduler into Animation, rather than "hack" AnimationScheduler. http://gwt-code-reviews.appspot.com/1446812/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
