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"/>
On 2011/05/26 13:07:58, tbroyer wrote:
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
Done.
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) {
That works for me. Should we rename AnimationRequestId to
AnimationHandle?
On 2011/05/26 13:07:58, tbroyer wrote:
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) {
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)?
On 2011/05/26 13:07:58, tbroyer wrote:
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() {
Agreed. See AnimationScheduler suggestion above.
On 2011/05/26 13:07:58, tbroyer wrote:
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) {
On 2011/05/26 13:07:58, tbroyer wrote:
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)
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.
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.
http://gwt-code-reviews.appspot.com/1446812/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
