I only gave it a glance but here are a few comments: - while I like the public requestAnimationFrame API, I don't like the static methods - I don't quite like the ImplMozilla/ImplWebkit extends ImplTimer pattern See details inline.
http://gwt-code-reviews.appspot.com/1446812/diff/1/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/1446812/diff/1/user/src/com/google/gwt/animation/Animation.gwt.xml#newcode22 user/src/com/google/gwt/animation/Animation.gwt.xml:22: <inherits name="com.google.gwt.user.UserAgent"/> Those are implicitly inherited from the c.g.g.user.User module. + inheriting UserAgent without User cause all sorts warnings, see http://code.google.com/p/google-web-toolkit/issues/detail?id=2815 http://gwt-code-reviews.appspot.com/1446812/diff/1/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/1446812/diff/1/user/src/com/google/gwt/animation/client/Animation.java#newcode56 user/src/com/google/gwt/animation/client/Animation.java:56: public static void cancelAnimationFrame(AnimationRequestId requestId) { How about a cancel() method on AnimationRequestId instead? http://gwt-code-reviews.appspot.com/1446812/diff/1/user/src/com/google/gwt/animation/client/Animation.java#newcode82 user/src/com/google/gwt/animation/client/Animation.java:82: public static AnimationRequestId requestAnimationFrame(AnimationCallback callback) { The issue with static methods is that you cannot mock them. When doing the previous patch, my idea rather was to extend Scheduler with a "scheduleAnimationFrame" or something like that (but without the "cancel" bit then, as there's no such notion of cancellation in Scheduler). I didn't go as far as implementing it as I though it'd be better done in a distinct CL. http://gwt-code-reviews.appspot.com/1446812/diff/1/user/src/com/google/gwt/animation/client/Animation.java#newcode105 user/src/com/google/gwt/animation/client/Animation.java:105: private static AnimationImpl impl() { Is there anything in Animation that wouldn't use the impl and justify lazy-initializing it? or is to allow injecting a mock AnimationImpl using reflection in unit tests? (looks bad; I like the Scheduler.get() / SchedulerImpl.INSTANCE pattern much better) http://gwt-code-reviews.appspot.com/1446812/diff/1/user/src/com/google/gwt/animation/client/AnimationImplMozilla.java File user/src/com/google/gwt/animation/client/AnimationImplMozilla.java (right): http://gwt-code-reviews.appspot.com/1446812/diff/1/user/src/com/google/gwt/animation/client/AnimationImplMozilla.java#newcode57 user/src/com/google/gwt/animation/client/AnimationImplMozilla.java:57: if (!isSupported) { Can't there be a better pattern that would use either AnimationImplMozilla (mozRequestAnimationFrame) or AnimationImplTimer so that "isSupported" is only evaluated once at startup/first use? Maybe something like a hasNativeSupport() method that Animation#impl() would call, and if it returns false it switches to an explicit "new AnimationImplTimer()" ? (and those ifs in ImplMozilla and ImplWebkit would simply turn into asserts: if the ImplMozilla/ImplWebkit is used, it must be that there is native support for requestAnimationFrame) http://gwt-code-reviews.appspot.com/1446812/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
