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) {
On 2011/05/26 14:15:47, jlabanca wrote:
That works for me. Should we rename AnimationRequestId to
AnimationHandle?
+1
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) {
On 2011/05/26 14:15:47, jlabanca wrote:
I think the method will be used like a Scheduler as well, but some
people
objected to actually putting animation methods in the Scheduler class.
What
about creating a com.google.gwt.animation.client.AnimationScheduler
class and
move these methods there (as instance methods)?
+1, using the same pattern as Scheduler with a static get() method
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) {
On 2011/05/26 14:15:47, jlabanca wrote:
I don't like mixing GWT.create() with "new", especially if
AnimationImplTimer
uses a Timer that needs to be mocked as well for non-browser testing.
You could GWT.create(AnimationImplTimer.class) if you prefer ;-)
But there's already a similar pattern in FocusImpl where, if the
GWT.create()d class is a FocusImplStandard (instanceof), then it "new"s
a FocusImpl.
I think the idea is not to allow usage with GWTMockUtilities.disarm(),
but rather to use a DI pattern where, outside tests, you fill the value
using Scheduler.get().
(In most cases, code using Scheduler would fail with NPEs if you use it
with GWTMockUtilities.disarm(), and I'd support doing the same for
AnimationScheduler).
Alternatively, AnimationImplMozilla can have an inner class NativeImpl
that
implements AnimationImpl. In the constructor of AnimationImplMozilla,
we check
isSupported(), then set an instance field "impl" = new
AnimationImplTimer() or
to new NativeImpl().
class AnimationImplMozilla implements AnimationImpl }
AnimationImpl impl;
AnimationImplMozilla() {
impl = isSupported() ? new NativeImpl() : new
AnimationImplTimer();
}
requestAnimationFrame() {
return impl.requestAnimationFrame();
}
}
That way, AnimationImplMozilla does not need to extend
AnimationImplTimer, there
is only one check in the constructor, and we do not have any "new"
calls in
Animation.
But that introduces yet another indirection.
Honestly, I have absolutely no problem mixing GWT.create() with new as
in FocusImpl, provided this is done in an "impl" class as well:
public abstract class AnimationScheduler {
public AnimationScheduler get() {
// This is the only use of AnimationSchedulerImpl in
AnimationScheduler, so it's "safe" as long as you don't call get()
outside of a "browser context"; in unit tests, you'd provide a mock
AnimationScheduler and never touch AnimationSchedulerImpl
return AnimationSchedulerImpl.INSTANCE;
}
...
}
class AnimationSchedulerImpl extends AnimationScheduler {
static final AnimationScheduler INSTANCE;
static {
AnimationSchedulerImpl impl =
GWT.create(AnimationSchedulerImpl.class);
// if impl==null, use null (would be because of
GWTMockUtilities.disarm(), we don't want to new an
AnimationSchedulerImpl in this case)
INSTANCE = (impl == null || impl.isSupported()) ? impl : new
AnimationSchedulerImpl();
}
protected abstract boolean isSupported();
}
or something like that.
Of course YMMV, and I'd be OK with whatever the team prefers.
http://gwt-code-reviews.appspot.com/1446812/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
