[gwt-contrib] Re: Adds {moz,webkit}RequestAnimationFrame support to animations. (issue1355805)
http://gwt-code-reviews.appspot.com/1355805/diff/6003/user/src/com/google/gwt/user/client/ui/DeckPanel.java File user/src/com/google/gwt/user/client/ui/DeckPanel.java (right): http://gwt-code-reviews.appspot.com/1355805/diff/6003/user/src/com/google/gwt/user/client/ui/DeckPanel.java#newcode108 user/src/com/google/gwt/user/client/ui/DeckPanel.java:108: // Figure out if the deck panel has a fixed height I'm not sure I'm a good checker, but it smells funny to me that onStart is losing a check that it used to have without (that I saw) routing through the new code in newWidget, nor why newWidget, having decided to animate, doesn't touch onStart. I'm willing to believe there's a good reason for it, but I'd like to know what it is. ;-) http://gwt-code-reviews.appspot.com/1355805/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Adds {moz,webkit}RequestAnimationFrame support to animations. (issue1355805)
http://gwt-code-reviews.appspot.com/1355805/diff/6003/user/src/com/google/gwt/user/client/ui/DeckPanel.java File user/src/com/google/gwt/user/client/ui/DeckPanel.java (right): http://gwt-code-reviews.appspot.com/1355805/diff/6003/user/src/com/google/gwt/user/client/ui/DeckPanel.java#newcode108 user/src/com/google/gwt/user/client/ui/DeckPanel.java:108: // Figure out if the deck panel has a fixed height On 2011/04/11 15:02:02, fabbott wrote: I'm not sure I'm a good checker, but it smells funny to me that onStart is losing a check that it used to have without (that I saw) routing through the new code in newWidget, nor why newWidget, having decided to animate, doesn't touch onStart. I'm willing to believe there's a good reason for it, but I'd like to know what it is. ;-) I wrote this code some times back, but it looks like the goal was to compute fixedHeight *before* calling run() so we could pass the second argument: if the height is fixed, we're sure the page layout won't change, so in case the deck is out of view (hidden, or displayed outside the viewport's bounds) we actually don't need to run our animation code. Passing the deckElem to run() in this case tells the browser it can skip calling us back if the element is out of view (at its discretion; actually, I believe WebKit only skips such calls on background tabs). http://gwt-code-reviews.appspot.com/1355805/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Adds {moz,webkit}RequestAnimationFrame support to animations. (issue1355805)
LGTM On Mon, Apr 11, 2011 at 11:44 AM, t.bro...@gmail.com wrote: http://gwt-code-reviews.appspot.com/1355805/diff/6003/user/src/com/google/gwt/user/client/ui/DeckPanel.java File user/src/com/google/gwt/user/client/ui/DeckPanel.java (right): http://gwt-code-reviews.appspot.com/1355805/diff/6003/user/src/com/google/gwt/user/client/ui/DeckPanel.java#newcode108 user/src/com/google/gwt/user/client/ui/DeckPanel.java:108: // Figure out if the deck panel has a fixed height On 2011/04/11 15:02:02, fabbott wrote: I'm not sure I'm a good checker, but it smells funny to me that onStart is losing a check that it used to have without (that I saw) routing through the new code in newWidget, nor why newWidget, having decided to animate, doesn't touch onStart. I'm willing to believe there's a good reason for it, but I'd like to know what it is. ;-) I wrote this code some times back, but it looks like the goal was to compute fixedHeight *before* calling run() so we could pass the second argument: if the height is fixed, we're sure the page layout won't change, so in case the deck is out of view (hidden, or displayed outside the viewport's bounds) we actually don't need to run our animation code. Passing the deckElem to run() in this case tells the browser it can skip calling us back if the element is out of view (at its discretion; actually, I believe WebKit only skips such calls on background tabs). http://gwt-code-reviews.appspot.com/1355805/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Adds {moz,webkit}RequestAnimationFrame support to animations. (issue1355805)
committed as r9970 Thanks for another patch! http://gwt-code-reviews.appspot.com/1355805/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Adds {moz,webkit}RequestAnimationFrame support to animations. (issue1355805)
It's always a pleasure to work on GWT: the code is quite clean and easy to read! On a related note (to this patch), I was thinking about using CSS3 transitions for animations in widgets, when supported. The only issue is that there would only be an event fired at the end (ontransitionend). Is it really an issue to call child widget's onResize only once at the end of the animation, or should we fake the animation steps with a timer or requestAnimationFrame? (in which case maybe CSS transitions wouldn't be worth adding). -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Adds {moz,webkit}RequestAnimationFrame support to animations. (issue1355805)
LGTM I'll test this out and submit it. http://gwt-code-reviews.appspot.com/1355805/diff/6003/user/src/com/google/gwt/user/client/ui/DeckPanel.java File user/src/com/google/gwt/user/client/ui/DeckPanel.java (right): http://gwt-code-reviews.appspot.com/1355805/diff/6003/user/src/com/google/gwt/user/client/ui/DeckPanel.java#newcode125 user/src/com/google/gwt/user/client/ui/DeckPanel.java:125: // the rest of the page, even if it's not visible to the user, . extra comma http://gwt-code-reviews.appspot.com/1355805/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Adds {moz,webkit}RequestAnimationFrame support to animations. (issue1355805)
I verified that the animations work on IE (timer version), Chrome 10 (verified the time fix is needed and works) and 11, Safari 5 (which does not support animation frames and uses the timer version), firefox 3.6 (timer version) and firefox 4.0 (moz animation frame). I'll submit this change soon. Thanks again for the patch. http://gwt-code-reviews.appspot.com/1355805/diff/6003/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/1355805/diff/6003/user/src/com/google/gwt/animation/Animation.gwt.xml#newcode62 user/src/com/google/gwt/animation/Animation.gwt.xml:62: when-property-is name=user.agent value=webkit/ The user agent value should be safari instead of webkit. http://gwt-code-reviews.appspot.com/1355805/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Adds {moz,webkit}RequestAnimationFrame support to animations. (issue1355805)
I ran the AnimationTest in prod mode in Chrome 9 (stable), 10 (beta) and
11 (dev), as well as Firefox 4b12, IE8, Opera 11 and Safari 5.
I also checked that the moz- (resp. webkit-) -specific implementation
was correctly stripped out of the webkit (resp. gecko1_8) permutations.
http://gwt-code-reviews.appspot.com/1355805/diff/3003/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/1355805/diff/3003/user/src/com/google/gwt/animation/Animation.gwt.xml#newcode25
user/src/com/google/gwt/animation/Animation.gwt.xml:25:
property-provider name=animationTimingSupport![CDATA[
On 2011/03/03 21:32:11, jlabanca wrote:
You're right. I missed the collapse-property in my review.
I moved it just after the define-property to make it more prominent.
http://gwt-code-reviews.appspot.com/1355805/diff/3003/user/src/com/google/gwt/animation/Animation.gwt.xml#newcode37
user/src/com/google/gwt/animation/Animation.gwt.xml:37:
when-property-is name=user.agent value=ie6 /
On 2011/03/03 21:32:11, jlabanca wrote:
Does this need to be wrapped in any?
Done.
http://gwt-code-reviews.appspot.com/1355805/diff/3003/user/src/com/google/gwt/animation/Animation.gwt.xml#newcode48
user/src/com/google/gwt/animation/Animation.gwt.xml:48:
when-property-is name=animationTimingSupport value=moz /
On 2011/03/03 21:32:11, jlabanca wrote:
Adding the line would work, and wrap with an any tag.
Er, no; we want this to be used *only* when animationTimingSupport=moz
(not for any gecko1_8 out there). Given that this can only happen when
user.agent=gecko1_8, the added line would only be an optimization to
compile AnimationImplMozAnimTiming out in the case of webkit.
I.e. in webkit, only the AnimationImplTimer and
AnimationImplWebkitAnimTiming will be compiled in.
I added comments to (try to) make it clearer.
http://gwt-code-reviews.appspot.com/1355805/diff/3003/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/1355805/diff/3003/user/src/com/google/gwt/animation/client/Animation.java#newcode98
user/src/com/google/gwt/animation/client/Animation.java:98: * passed,
the animation will be synchronize as if it started at the specified
On 2011/03/03 19:26:22, jlabanca wrote:
will be synchronize = will run synchronously
Done.
http://gwt-code-reviews.appspot.com/1355805/diff/3003/user/src/com/google/gwt/animation/client/Animation.java#newcode113
user/src/com/google/gwt/animation/client/Animation.java:113: * passed,
the animation will be synchronize as if it started at the specified
On 2011/03/03 19:26:22, jlabanca wrote:
will be synchronize = will run synchronously
Done.
http://gwt-code-reviews.appspot.com/1355805/diff/3003/user/src/com/google/gwt/animation/client/AnimationImplMozAnimTiming.java
File
user/src/com/google/gwt/animation/client/AnimationImplMozAnimTiming.java
(right):
http://gwt-code-reviews.appspot.com/1355805/diff/3003/user/src/com/google/gwt/animation/client/AnimationImplMozAnimTiming.java#newcode45
user/src/com/google/gwt/animation/client/AnimationImplMozAnimTiming.java:45:
if (!complete) {
On 2011/03/03 21:32:11, jlabanca wrote:
On 2011/03/03 20:59:06, tbroyer wrote:
On 2011/03/03 19:26:22, jlabanca wrote:
If you check !cancelled here, it could result in a bad state if
the
Animation
is
restarted synchronously.
What would you propose?
Would it also be an issue if 'cancelled' is checked before update()
is called
(which should have been the case)
We probably need some kind of handle like webkit uses. You could wrap
callback
in an object and assign a static ID. Then, check the ID when the
callback is
invoked.
private static int curId;
public void cancel() {
curId++;
}
public void run(Animation animation, Element element) {
curId++;
nativeRun(animation, curId);
}
private native void nativeRun(Animation animation, double id) /*-{
var self = this;
var handle = id;
var callback = $entry(function(time) {
if (handle !=
se...@com.google.gwt.animation.client.AnimationImplMozAnimTiming::curId)
{
return; // canceled.
}
});
/*-}
Done.
http://gwt-code-reviews.appspot.com/1355805/diff/3003/user/src/com/google/gwt/animation/client/AnimationImplTimer.java
File user/src/com/google/gwt/animation/client/AnimationImplTimer.java
(right):
http://gwt-code-reviews.appspot.com/1355805/diff/3003/user/src/com/google/gwt/animation/client/AnimationImplTimer.java#newcode81
user/src/com/google/gwt/animation/client/AnimationImplTimer.java:81: //
Restart the timer if there is the only animation
On 2011/03/03 19:26:22, jlabanca wrote:
if there is = if this is
Done.
http://gwt-code-reviews.appspot.com/1355805/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Adds {moz,webkit}RequestAnimationFrame support to animations. (issue1355805)
http://gwt-code-reviews.appspot.com/1355805/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Adds {moz,webkit}RequestAnimationFrame support to animations. (issue1355805)
http://gwt-code-reviews.appspot.com/1355805/diff/6003/user/src/com/google/gwt/animation/client/AnimationImplWebkitAnimTiming.java File user/src/com/google/gwt/animation/client/AnimationImplWebkitAnimTiming.java (right): http://gwt-code-reviews.appspot.com/1355805/diff/6003/user/src/com/google/gwt/animation/client/AnimationImplWebkitAnimTiming.java#newcode52 user/src/com/google/gwt/animation/client/AnimationImplWebkitAnimTiming.java:52: time = time || @com.google.gwt.core.client.Duration::currentTimeMillis()(); I actually only tested that this works in Chrome 10, not that the previous code failed (Paul Irish must know what he's talking about re. Chrome ;-) ) http://gwt-code-reviews.appspot.com/1355805/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Adds {moz,webkit}RequestAnimationFrame support to animations. (issue1355805)
The implementation looks really good, and should hopefully smooth out
animations. Take a look at my comments about the deferred binding.
http://gwt-code-reviews.appspot.com/1355805/diff/3003/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/1355805/diff/3003/user/src/com/google/gwt/animation/Animation.gwt.xml#newcode25
user/src/com/google/gwt/animation/Animation.gwt.xml:25:
property-provider name=animationTimingSupport![CDATA[
This property provider is going to add 4 additional permutations:
gecko1_8:moz
gecko1_8:webkit // Compiler doesn't know this is invalid
safari:webkit
safari:moz // Compiler doesn't know this is invalid
We've been using runtime checks for features that are supported only in
newer
versions (see Canvas). AnimationImplMoz/WebkitAnimTiming should do a
runtime check upon instantiation to check for support. If not
supported, they should delegate
to AnimationImplTimer.
Unfortunately, that causes both the timer and native versions to be
compiled into webkit and mozilla. However, thats better than adding
four permutations.
We're looking into an alternative to user.agent that is feature based,
but doesn't have the permutation explosion.
http://gwt-code-reviews.appspot.com/1355805/diff/3003/user/src/com/google/gwt/animation/Animation.gwt.xml#newcode48
user/src/com/google/gwt/animation/Animation.gwt.xml:48:
when-property-is name=animationTimingSupport value=moz /
Replace with:
when-property-is name=user.agent value=gecko1_8 /
http://gwt-code-reviews.appspot.com/1355805/diff/3003/user/src/com/google/gwt/animation/Animation.gwt.xml#newcode53
user/src/com/google/gwt/animation/Animation.gwt.xml:53:
when-property-is name=animationTimingSupport value=webkit /
Replace with:
when-property-is name=user.agent value=safari /
http://gwt-code-reviews.appspot.com/1355805/diff/3003/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/1355805/diff/3003/user/src/com/google/gwt/animation/client/Animation.java#newcode98
user/src/com/google/gwt/animation/client/Animation.java:98: * passed,
the animation will be synchronize as if it started at the specified
will be synchronize = will run synchronously
http://gwt-code-reviews.appspot.com/1355805/diff/3003/user/src/com/google/gwt/animation/client/Animation.java#newcode113
user/src/com/google/gwt/animation/client/Animation.java:113: * passed,
the animation will be synchronize as if it started at the specified
will be synchronize = will run synchronously
http://gwt-code-reviews.appspot.com/1355805/diff/3003/user/src/com/google/gwt/animation/client/AnimationImplMozAnimTiming.java
File
user/src/com/google/gwt/animation/client/AnimationImplMozAnimTiming.java
(right):
http://gwt-code-reviews.appspot.com/1355805/diff/3003/user/src/com/google/gwt/animation/client/AnimationImplMozAnimTiming.java#newcode32
user/src/com/google/gwt/animation/client/AnimationImplMozAnimTiming.java:32:
cancelled = true;
cancelled is never used. It doesn't look like the animation can be
cancelled.
http://gwt-code-reviews.appspot.com/1355805/diff/3003/user/src/com/google/gwt/animation/client/AnimationImplMozAnimTiming.java#newcode45
user/src/com/google/gwt/animation/client/AnimationImplMozAnimTiming.java:45:
if (!complete) {
If you check !cancelled here, it could result in a bad state if the
Animation is restarted synchronously.
http://gwt-code-reviews.appspot.com/1355805/diff/3003/user/src/com/google/gwt/animation/client/AnimationImplTimer.java
File user/src/com/google/gwt/animation/client/AnimationImplTimer.java
(right):
http://gwt-code-reviews.appspot.com/1355805/diff/3003/user/src/com/google/gwt/animation/client/AnimationImplTimer.java#newcode81
user/src/com/google/gwt/animation/client/AnimationImplTimer.java:81: //
Restart the timer if there is the only animation
if there is = if this is
http://gwt-code-reviews.appspot.com/1355805/diff/3003/user/src/com/google/gwt/animation/client/AnimationImplTimer.java#newcode83
user/src/com/google/gwt/animation/client/AnimationImplTimer.java:83:
Scheduler.get().scheduleFixedDelay(animationCommand,
DEFAULT_FRAME_DELAY);
If the animation runs slowly, we may end up animating with only 1ms
intervals in between, which would make the UI unresponsive. Can you use
a ScheduledCommand instead of a RepeatingCommand, and reschedule
manually with a delay?
Also, note that in the current implementation, this animationCommand
will never stop executing in the background.
http://gwt-code-reviews.appspot.com/1355805/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Adds {moz,webkit}RequestAnimationFrame support to animations. (issue1355805)
I stumbled upon this article by Paul Irish
http://paulirish.com/2011/requestanimationframe-for-smart-animating/ a
few hours after sending this patch. There he says that Chrome 10 (beta)
doesn't pass the 'time' argument to the callback, so the
AnimationImplWebkit should probably test that and fallback to
Duration.currentTimeMillis(). I only tested in Chrome 11 at the time, so
I didn't notice it. I haven't had time to work on that yet, but I'll
make sure to test in Chrome 9 (stable) and 10 (beta) in the next patch.
http://gwt-code-reviews.appspot.com/1355805/diff/3003/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/1355805/diff/3003/user/src/com/google/gwt/animation/Animation.gwt.xml#newcode25
user/src/com/google/gwt/animation/Animation.gwt.xml:25:
property-provider name=animationTimingSupport![CDATA[
On 2011/03/03 19:26:22, jlabanca wrote:
This property provider is going to add 4 additional permutations:
gecko1_8:moz
gecko1_8:webkit // Compiler doesn't know this is invalid
safari:webkit
safari:moz // Compiler doesn't know this is invalid
I was hoping soft permutations would mitigate this (the wiki says: It
is possible to collapse all properties to decrease the amount of time
spend compiling a module for testing purposes by adding the following:
collapse-all-properties / which I understood as soft permutations
have a low overhead).
In my understanding, there would also have been the drawback of
compiling all AnimationImpl* into the gecko1_8 and webkit permutations,
but with the advantage of clean separation in the code (and possibility
to compile out some implementations in controlled environments where you
know the one impl that would fit; e.g. Firefox or Chrome extension)
http://gwt-code-reviews.appspot.com/1355805/diff/3003/user/src/com/google/gwt/animation/Animation.gwt.xml#newcode48
user/src/com/google/gwt/animation/Animation.gwt.xml:48:
when-property-is name=animationTimingSupport value=moz /
On 2011/03/03 19:26:22, jlabanca wrote:
Replace with:
when-property-is name=user.agent value=gecko1_8 /
Assuming I'm right above, maybe I should rather *add* that line?
http://gwt-code-reviews.appspot.com/1355805/diff/3003/user/src/com/google/gwt/animation/client/AnimationImplMozAnimTiming.java
File
user/src/com/google/gwt/animation/client/AnimationImplMozAnimTiming.java
(right):
http://gwt-code-reviews.appspot.com/1355805/diff/3003/user/src/com/google/gwt/animation/client/AnimationImplMozAnimTiming.java#newcode45
user/src/com/google/gwt/animation/client/AnimationImplMozAnimTiming.java:45:
if (!complete) {
On 2011/03/03 19:26:22, jlabanca wrote:
If you check !cancelled here, it could result in a bad state if the
Animation is
restarted synchronously.
What would you propose?
Would it also be an issue if 'cancelled' is checked before update() is
called (which should have been the case)
http://gwt-code-reviews.appspot.com/1355805/diff/3003/user/src/com/google/gwt/animation/client/AnimationImplTimer.java
File user/src/com/google/gwt/animation/client/AnimationImplTimer.java
(right):
http://gwt-code-reviews.appspot.com/1355805/diff/3003/user/src/com/google/gwt/animation/client/AnimationImplTimer.java#newcode83
user/src/com/google/gwt/animation/client/AnimationImplTimer.java:83:
Scheduler.get().scheduleFixedDelay(animationCommand,
DEFAULT_FRAME_DELAY);
On 2011/03/03 19:26:22, jlabanca wrote:
If the animation runs slowly, we may end up animating with only 1ms
intervals in
between, which would make the UI unresponsive. Can you use a
ScheduledCommand
instead of a RepeatingCommand, and reschedule manually with a delay?
This is what scheduleFixedDelay does (contrary to scheduleFixedPeriod):
http://google-web-toolkit.googlecode.com/svn/javadoc/2.2/com/google/gwt/core/client/Scheduler.html#scheduleFixedDelay(com.google.gwt.core.client.Scheduler.RepeatingCommand,
int)
You were probably thinking about the old, deprecated
DeferredCommand/IncrementalCommand.
Also, note that in the current implementation, this animationCommand
will never
stop executing in the background.
Er, wouldn't it stop when all animations complete? (animations would
then be empty and the animationCommand would then return false, which
would terminate the loop)
http://gwt-code-reviews.appspot.com/1355805/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Adds {moz,webkit}RequestAnimationFrame support to animations. (issue1355805)
http://gwt-code-reviews.appspot.com/1355805/diff/3003/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/1355805/diff/3003/user/src/com/google/gwt/animation/Animation.gwt.xml#newcode25
user/src/com/google/gwt/animation/Animation.gwt.xml:25:
property-provider name=animationTimingSupport