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:50:36, tbroyer wrote:
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.
Done.
http://gwt-code-reviews.appspot.com/1446812/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
