[gwt-contrib] Re: Resubmitting r9970 again (again) - updating Animation to use the native requestAnimationFrame, w... (issue1446812)
Committed as r10257
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 14:04:34, tbroyer wrote:
On 2011/05/27 13:47:12, jlabanca wrote:
On 2011/05/27 13:17:02, tbroyer wrote:
On 2011/05/27 13:07:01, jlabanca wrote:
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.
How about my proposal for protected AnimationScheduler
getAnimationScheduler()
{ return AnimationScheduler.get(); } in Animation?
(which you'd override in TestAnimation in the AnimationTest)
Or maybe a protected constructor that takes an AnimationScheduler
instead?
Works for me.
Done.
http://gwt-code-reviews.appspot.com/1446812/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Resubmitting r9970 again (again) - updating Animation to use the native requestAnimationFrame, w... (issue1446812)
I'll let fabbott continue the review, unless you have remarks on my
comments.
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;
Should everything *Impl be moved in to an impl subpackage?
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).
unbalanced parenthesis
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 {
Rename to AnimationHandleImpl?
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;
return !!$wnd.mozRequestAnimationFrame ?
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;
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)
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;
You don't need that. Using 'callback' in the 'wrapper' function will
just work.
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 {
Rename to AnimationHandleImpl
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);
How about using a Duration *object* instead, isn't it meant for these
use cases?
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 {
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.
http://gwt-code-reviews.appspot.com/1446812/diff/3002/user/test/com/google/gwt/animation/AnimationTest.gwt.xml
File
[gwt-contrib] Re: Resubmitting r9970 again (again) - updating Animation to use the native requestAnimationFrame, w... (issue1446812)
http://gwt-code-reviews.appspot.com/1446812/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Resubmitting r9970 again (again) - updating Animation to use the native requestAnimationFrame, w... (issue1446812)
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
[gwt-contrib] Re: Resubmitting r9970 again (again) - updating Animation to use the native requestAnimationFrame, w... (issue1446812)
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#newcode75
user/src/com/google/gwt/animation/client/AnimationSchedulerImplMozilla.java:75:
handle.canceled = false;
On 2011/05/27 13:07:01, jlabanca wrote:
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.
I was actually suggesting using something like:
class AnimationHandleImpl implements AnimationHandle {
private boolean canceled;
@Override
public void cancel() { this.canceled = true; }
}
and then in the 'wrapper' function:
if (!handle.@...AnimationHandleImpl::canceled) { ...
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 13:07:01, jlabanca wrote:
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.
I for one find the instanceof check clean, if you ask me; and I believe
it'll be optimized out.
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 13:07:01, jlabanca wrote:
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.
How about my proposal for protected AnimationScheduler
getAnimationScheduler() { return AnimationScheduler.get(); } in
Animation?
(which you'd override in TestAnimation in the AnimationTest)
http://gwt-code-reviews.appspot.com/1446812/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Resubmitting r9970 again (again) - updating Animation to use the native requestAnimationFrame, w... (issue1446812)
http://gwt-code-reviews.appspot.com/1446812/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Resubmitting r9970 again (again) - updating Animation to use the native requestAnimationFrame, w... (issue1446812)
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#newcode75
user/src/com/google/gwt/animation/client/AnimationSchedulerImplMozilla.java:75:
handle.canceled = false;
On 2011/05/27 13:17:02, tbroyer wrote:
On 2011/05/27 13:07:01, jlabanca wrote:
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.
I was actually suggesting using something like:
class AnimationHandleImpl implements AnimationHandle {
private boolean canceled;
@Override
public void cancel() { this.canceled = true; }
}
and then in the 'wrapper' function:
if (!handle.@...AnimationHandleImpl::canceled) { ...
Done. That's makes it cleaner.
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 13:17:02, tbroyer wrote:
On 2011/05/27 13:07:01, jlabanca wrote:
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.
How about my proposal for protected AnimationScheduler
getAnimationScheduler()
{ return AnimationScheduler.get(); } in Animation?
(which you'd override in TestAnimation in the AnimationTest)
Or maybe a protected constructor that takes an AnimationScheduler
instead? The getter suggests that the AnimationScheduler can be swapped
out at any time. And maybe that's would work, but I'm not sure we want
to support that use case.
http://gwt-code-reviews.appspot.com/1446812/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Resubmitting r9970 again (again) - updating Animation to use the native requestAnimationFrame, w... (issue1446812)
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 13:47:12, jlabanca wrote:
On 2011/05/27 13:17:02, tbroyer wrote:
On 2011/05/27 13:07:01, jlabanca wrote:
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.
How about my proposal for protected AnimationScheduler
getAnimationScheduler()
{ return AnimationScheduler.get(); } in Animation?
(which you'd override in TestAnimation in the AnimationTest)
Or maybe a protected constructor that takes an AnimationScheduler
instead?
Works for me.
http://gwt-code-reviews.appspot.com/1446812/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Resubmitting r9970 again (again) - updating Animation to use the native requestAnimationFrame, w... (issue1446812)
http://gwt-code-reviews.appspot.com/1446812/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Resubmitting r9970 again (again) - updating Animation to use the native requestAnimationFrame, w... (issue1446812)
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
[gwt-contrib] Re: Resubmitting r9970 again (again) - updating Animation to use the native requestAnimationFrame, w... (issue1446812)
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
[gwt-contrib] Re: Resubmitting r9970 again (again) - updating Animation to use the native requestAnimationFrame, w... (issue1446812)
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 news
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
[gwt-contrib] Re: Resubmitting r9970 again (again) - updating Animation to use the native requestAnimationFrame, w... (issue1446812)
http://gwt-code-reviews.appspot.com/1446812/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Resubmitting r9970 again (again) - updating Animation to use the native requestAnimationFrame, w... (issue1446812)
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 news 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
[gwt-contrib] Re: Resubmitting r9970 again (again) - updating Animation to use the native requestAnimationFrame, w... (issue1446812)
I'm sending this issue for review. tbroyer's original issue is: http://gwt-code-reviews.appspot.com/1355805/ @fabbott - You can hold off on the review until we get feedback, at least from tbroyer. http://gwt-code-reviews.appspot.com/1446812/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
