Thanks for the detailed review! @fabbott -
This change is ready for your final review. 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; On 2011/05/27 12:11:44, tbroyer wrote:
Should everything *Impl be moved in to an "impl" subpackage?
We stopped using impl packages because we have to make the impl classes public if they are in a separate package. Instead, we make the impl classes package protected and put them in the same package. This class was public to allow StubAnimationSchedulerImpl to extend it, but I've followed your other suggestions, so this impl class is now package protected. 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). On 2011/05/27 12:11:44, tbroyer wrote:
unbalanced parenthesis
Done. 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 { On 2011/05/27 12:11:44, tbroyer wrote:
Rename to AnimationHandleImpl?
Done. 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; On 2011/05/27 12:11:44, tbroyer wrote:
"return !!$wnd.mozRequestAnimationFrame" ?
Done. 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; On 2011/05/27 12:11:44, tbroyer wrote:
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)
If we pass AnimationHandleImpl as an argument, then we need another JSNI block to create the JavaScriptObject that it contains. Or, we need a setter to set the JavaScriptObject. Personally, I like the way its implemented now where requestAnimationFrameImpl() returns a JSO handle, and the Java code wraps it in a logical handle. 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; On 2011/05/27 12:11:44, tbroyer wrote:
You don't need that. Using 'callback' in the 'wrapper' function will
just work. Done. 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 { On 2011/05/27 12:11:44, tbroyer wrote:
Rename to AnimationHandleImpl
Done. 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); On 2011/05/27 12:11:44, tbroyer wrote:
How about using a Duration *object* instead, isn't it meant for these
use cases? Done. 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 { On 2011/05/27 12:11:44, tbroyer wrote:
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.
Done. I couldn't find a clean way to implement the static initializer in AnimationSchedulerImpl if GWT.create() returns an AnimationScheduler, because we need to check if the impl class natively supports requestAnimationFrame. However, an instanceof check works, and its only called once. 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"> On 2011/05/27 12:11:44, tbroyer wrote:
Would probably be better to somehow "inject" a StubAnimationScheduler
into
Animation, rather than "hack" AnimationScheduler.
Agreed if we had an injection framework like gin built in to GWT. Deferred bindings are the closest thing we have for now. http://gwt-code-reviews.appspot.com/1446812/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
