Change in gwt[master]: Stop using prefixed API's in AnimationScheduler by default. ...

[Brian Slesinsky]

Brian Slesinsky has uploaded a new change for review.

https://gwt-review.googlesource.com/1780


Change subject: Stop using prefixed API's in AnimationScheduler by default.
Firefox and Safari will use the Timer-based implementation. For Chrome we
can use requestAnimationFrame because it's unprefixed starting in Chrome 24.
......................................................................

Stop using prefixed API's in AnimationScheduler by default.
Firefox and Safari will use the Timer-based implementation. For Chrome
we can use requestAnimationFrame because it's unprefixed starting in
Chrome 24.

Since Chrome and Safari are in the same permutation, a future version of
Safari
that adds the unprefixed API will automatically pick up
requestAnimationFrame
once it's unprefixed. I attempted to avoid depending on any details of the
JavaScript API (it's mostly based on the Mozilla implementation) but can't
predict what the final standard will be, so it still seems risky. Perhaps we
should test specifically for Chrome?

Keep in mind that some apps built with GWT 2.5.1 will be around for a long
time.

(Not tested yet.)

Change-Id: I3011dceab489871a5864eed1ece47ec850d82425
---
M user/src/com/google/gwt/animation/Animation.gwt.xml
M
user/src/com/google/gwt/animation/client/AnimationSchedulerImplMozilla.java
A user/src/com/google/gwt/animation/client/AnimationSchedulerImplNative.java
M user/src/com/google/gwt/animation/client/AnimationSchedulerImplWebkit.java
4 files changed, 97 insertions(+), 2 deletions(-)



diff --git a/user/src/com/google/gwt/animation/Animation.gwt.xml
b/user/src/com/google/gwt/animation/Animation.gwt.xml
index bc2a47c..8e1f752 100644
--- a/user/src/com/google/gwt/animation/Animation.gwt.xml
+++ b/user/src/com/google/gwt/animation/Animation.gwt.xml
@@ -29,14 +29,31 @@
</replace-with>

<!-- Implementation based on mozRequestAnimationFrame -->
+ <!-- Disabled by default because it uses a prefixed API. -->
+<!--
<replace-with
class="com.google.gwt.animation.client.AnimationSchedulerImplMozilla">
<when-type-is
class="com.google.gwt.animation.client.AnimationScheduler"/>
<when-property-is name="user.agent" value="gecko1_8"/>
</replace-with>
+-->

<!-- Implementation based on webkitRequestAnimationFrame -->
+ <!-- Disabled by default because it uses a prefixed API. -->
+<!--
<replace-with
class="com.google.gwt.animation.client.AnimationSchedulerImplWebkit">
<when-type-is
class="com.google.gwt.animation.client.AnimationScheduler"/>
<when-property-is name="user.agent" value="safari"/>
</replace-with>
+-->
+
+ <!--
+ Implementation based on requestAnimationFrame. Note: as of January
2013 this is
+ only available for Chrome 24 and above. Safari will fall back to the
Timer-based
+ implementation, but this may change in a future Safari release.
+ -->
+ <replace-with
class="com.google.gwt.animation.client.AnimationSchedulerImplNative">
+ <when-type-is
class="com.google.gwt.animation.client.AnimationScheduler"/>
+ <when-property-is name="user.agent" value="gecko1_8"/>
+ <when-property-is name="user.agent" value="safari"/>
+ </replace-with>
</module>
diff --git
a/user/src/com/google/gwt/animation/client/AnimationSchedulerImplMozilla.java
b/user/src/com/google/gwt/animation/client/AnimationSchedulerImplMozilla.java
index 157a53e..2f7d0c9 100644
---
a/user/src/com/google/gwt/animation/client/AnimationSchedulerImplMozilla.java
+++
b/user/src/com/google/gwt/animation/client/AnimationSchedulerImplMozilla.java
@@ -18,7 +18,7 @@
import com.google.gwt.dom.client.Element;

/**
- * Implementation using <code>mozRequestAnimationFrame</code>.
+ * Implementation using <code>mozRequestAnimationFrame</code>. Not
currently used by default.
*
* @see <a
*
href="https://developer.mozilla.org/en/DOM/window.mozRequestAnimationFrame">
diff --git
a/user/src/com/google/gwt/animation/client/AnimationSchedulerImplNative.java
b/user/src/com/google/gwt/animation/client/AnimationSchedulerImplNative.java
new file mode 100644
index 0000000..d0c4f8f
--- /dev/null
+++
b/user/src/com/google/gwt/animation/client/AnimationSchedulerImplNative.java
@@ -0,0 +1,78 @@
+/*
+ * Copyright 2011 Google Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License"); you may
not
+ * use this file except in compliance with the License. You may obtain a
copy of
+ * the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+ * License for the specific language governing permissions and limitations
under
+ * the License.
+ */
+package com.google.gwt.animation.client;
+
+import com.google.gwt.dom.client.Element;
+
+/**
+ * An implementation using the unprefixed
<code>requestAnimationFrame</code>.
+ * Since browser support is in flux, assumes as little as possible about
the
+ * JavaScript API. In particular, we only pass in the callback and don't
look
+ * at the return value. Also, the callback doesn't look at any parameters.
+ *
+ * <p>(This API was unprefixed in Chrome 24, despite not being
standardized yet.)
+ *
+ * @see <a
+ * href="http://www.w3.org/TR/animation-timing/">Timing control for
script-based animations</a>
+ */
+class AnimationSchedulerImplNative extends AnimationSchedulerImpl {
+
+ /**
+ * A handle that remembers whether it was cancelled..
+ */
+ private class AnimationHandleImpl extends AnimationHandle {
+ @SuppressWarnings("unused")
+ private boolean cancelled;
+
+ @Override
+ public void cancel() {
+ cancelled = true;
+ }
+ }
+
+ @Override
+ public AnimationHandle requestAnimationFrame(AnimationCallback callback,
Element element) {
+ AnimationHandleImpl handle = new AnimationHandleImpl();
+ requestAnimationFrameImpl(callback, handle);
+ return handle;
+ }
+
+ @Override
+ protected native boolean isNativelySupported() /*-{
+ return !!($wnd.requestAnimationFrame);
+ }-*/;
+
+ /**
+ * Request an animation frame. To avoid depending on a request ID, we
+ * create a JavaScriptObject and add an expando named "cancelled" to
indicate
+ * that the request was cancelled. The callback wrapper checks the
expando before
+ * executing the user callback.
+ *
+ * @param callback the user callback to execute
+ * @param handle the handle object
+ */
+ private native void requestAnimationFrameImpl(AnimationCallback callback,
+ AnimationHandleImpl
handle) /*-{
+ var wrapper = $entry(function() {
+ if
(!handle.@com.google.gwt.animation.client.AnimationSchedulerImplNative.AnimationHandleImpl::cancelled)
{
+ // Ignore any time parameter that we were called with.
+ var now =
@com.google.gwt.core.client.Duration::currentTimeMillis()();
+
callback.@com.google.gwt.animation.client.AnimationScheduler.AnimationCallback::execute(D)(now);
+ }
+ });
+ $wnd.requestAnimationFrame(wrapper);
+ }-*/;
+}
diff --git
a/user/src/com/google/gwt/animation/client/AnimationSchedulerImplWebkit.java
b/user/src/com/google/gwt/animation/client/AnimationSchedulerImplWebkit.java
index ed02124..7b60173 100644
---
a/user/src/com/google/gwt/animation/client/AnimationSchedulerImplWebkit.java
+++
b/user/src/com/google/gwt/animation/client/AnimationSchedulerImplWebkit.java
@@ -19,7 +19,7 @@

/**
* Implementation using <code>webkitRequestAnimationFrame</code> and
- * <code>webkitCancelRequestAnimationFrame</code>.
+ * <code>webkitCancelRequestAnimationFrame</code>. Not currently used by
default.
*
* @see <a
*
href="http://www.chromium.org/developers/web-platform-status#TOC-requestAnimationFrame">

--
To view, visit https://gwt-review.googlesource.com/1780
To unsubscribe, visit https://gwt-review.googlesource.com/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: I3011dceab489871a5864eed1ece47ec850d82425
Gerrit-PatchSet: 1
Gerrit-Project: gwt
Gerrit-Branch: master
Gerrit-Owner: Brian Slesinsky <skyb...@google.com>

[Goktug Gokdogan]

Goktug Gokdogan has posted comments on this change.

Change subject: Stop using prefixed API's in AnimationScheduler by default.
Firefox and Safari will use the Timer-based implementation. For Chrome we
can use requestAnimationFrame because it's unprefixed starting in Chrome 24.
......................................................................

Patch Set 1:

(3 comments)

I think this is a good idea.
Some comments are inlined.

....................................................
Commit Message
Line 14: once it's unprefixed. I attempted to avoid depending on any
details of the
also Firefox shares the same deferred binding in the xml, that means it
will also pickup once it is unprefixed.


....................................................
File user/src/com/google/gwt/animation/Animation.gwt.xml
Line 27: <replace-with
class="com.google.gwt.animation.client.AnimationSchedulerImplTimer">
why is native not the default? If browser supports unprefixed
requestAnimationFrame, I think we can safely use it; especially as we are
not relying on any parameters or return values.


Line 41: <!-- Disabled by default because it uses a prefixed API. -->
Perhaps we can introduce a "use-prefixed" property (false by default) to be
used in this kind of situations?

--
To view, visit https://gwt-review.googlesource.com/1780
To unsubscribe, visit https://gwt-review.googlesource.com/settings

Gerrit-MessageType: comment

Gerrit-Change-Id: I3011dceab489871a5864eed1ece47ec850d82425
Gerrit-PatchSet: 1
Gerrit-Project: gwt
Gerrit-Branch: master
Gerrit-Owner: Brian Slesinsky <skyb...@google.com>

Gerrit-Reviewer: Goktug Gokdogan <gok...@google.com>
Gerrit-HasComments: Yes

[Thomas Broyer]

Thomas Broyer has posted comments on this change.

Change subject: Stop using prefixed API's in AnimationScheduler by default.
Firefox and Safari will use the Timer-based implementation. For Chrome we
can use requestAnimationFrame because it's unprefixed starting in Chrome 24.
......................................................................

Patch Set 1:

(3 comments)

FYI, Mozilla is discussing what to do wrt high-res timers: change mozRAF
(would break GWT 2.4 users like the Chrome change broke them already) or
only rAF.

See https://bugzilla.mozilla.org/show_bug.cgi?id=753453

....................................................
File user/src/com/google/gwt/animation/Animation.gwt.xml
Line 27: <replace-with
class="com.google.gwt.animation.client.AnimationSchedulerImplTimer">

According to "Can I Use" and the MSDN, it seems like IE10 has it unprefixed
already (with the same behavior as Chrome 24, which is the one from the
latest Editor's Draft).


Line 41: <!-- Disabled by default because it uses a prefixed API. -->
+1

I proposed it on the G+ Community too:
https://plus.google.com/114156500057804356924/posts/45D9ZfZkF28

The default value is controversial though. I'd lean toward setting it to
true by default and documenting it prominently in the developer guide (and
possibly even emitting a warning at compile-time). Let's discuss it in
GWT-Contrib though.

Ideally, there should probably be a property per module, whose default
value is controlled by a global property declared in c.g.g.core.Core.
Something like:

<define-property name="animation.useExperimentalApis"
values="true,false" />
<set-property name="animation.useExperimentalApis" value="true">
<when-property-is name="useExperimentalApis" value="true" />
</set-property>
<set-property name="animation.useExperimentalApis" value="false">
<when-property-is name="useExperimentalApis" value="false" />
</set-property>


....................................................
File
user/src/com/google/gwt/animation/client/AnimationSchedulerImplNative.java
Line 60: * create a JavaScriptObject and add an expando
named "cancelled" to indicate
The Javadoc doesn't match the implementation. We should fix *ImplMozilla
too.

--
To view, visit https://gwt-review.googlesource.com/1780
To unsubscribe, visit https://gwt-review.googlesource.com/settings

Gerrit-MessageType: comment

Gerrit-Change-Id: I3011dceab489871a5864eed1ece47ec850d82425
Gerrit-PatchSet: 1
Gerrit-Project: gwt
Gerrit-Branch: master
Gerrit-Owner: Brian Slesinsky <skyb...@google.com>

Gerrit-Reviewer: Goktug Gokdogan <gok...@google.com>
Gerrit-Reviewer: Thomas Broyer <t.br...@gmail.com>
Gerrit-HasComments: Yes

[Ray Cromwell]

Not using the most efficient implementation by default seems overly
harsh and likely to bias people against using GWT, so I tend to side
with making the efficient one the default. Javascript programmers
would usually react differently, by doing capability tests, and
selecting the appropriate implementation at runtime. We are somewhat
culturally constrained in the GWT world to think in terms of deferred
bindings, but writing JSNI code that has multiple browser fallbacks
into it is not necessarily a bad solution to bridge the timeframe in
which we wait for browser upgrades.

If you consider something like iOS Safari, they upgrade the browser
like once per year, so a policy that works well with Chrome I think
harshly punishes Safari.

-Ray

> --
> http://groups.google.com/group/Google-Web-Toolkit-Contributors

[John A. Tamplin]

On Sun, Jan 20, 2013 at 8:25 PM, Ray Cromwell <cromw...@google.com> wrote:

Not using the most efficient implementation by default seems overly
harsh and likely to bias people against using GWT, so I tend to side
with making the efficient one the default. Javascript programmers
would usually react differently, by doing capability tests, and
selecting the appropriate implementation at runtime. We are somewhat
culturally constrained in the GWT world to think in terms of deferred
bindings, but writing JSNI code that has multiple browser fallbacks
into it is not necessarily a bad solution to bridge the timeframe in
which we wait for browser upgrades.

If you consider something like iOS Safari, they upgrade the browser
like once per year, so a policy that works well with Chrome I think
harshly punishes Safari.

I agree, plus I think if we are going to do this there should be a more global deferred binding property like "useExperimentalApis" in core. 

--
John A. Tamplin

[Brian Slesinsky]

On Sun, Jan 20, 2013 at 7:35 PM, John A. Tamplin <j...@jaet.org> wrote:

I agree, plus I think if we are going to do this there should be a more global deferred binding property like "useExperimentalApis" in core.

What do you think about flags to turn on each prefixed symbol separately:

  useMozRequestAnimationFrame={true,false}

  useWebkitRequestAnimationFrame={true,false}

I'm not sure about the terminology but I'm thinking of using define-configuration-property and set-configuration-property. (That is, essentially a flag, not a permutation.) 

This would make it easy for people with the resources to do browser testing to turn on the features that seem to be working, and quickly disable them if they run into problems. (Much in the same way server-side flags are a way to leave decisions to sysadmins in the server-side world.) We can't know what the state of browser breakage will be in a year. when Safari 7 comes out, anyone using a non-trunk version of GWT needs to make their own decisions.

On Sun, Jan 20, 2013 at 8:25 PM, Ray Cromwell <cromw...@google.com> wrote:

Not using the most efficient implementation by default seems overly
harsh and likely to bias people against using GWT, so I tend to side

What do you mean by "too harsh"? I haven't tested yet, but it seems unlikely that any users would care if we dropped a few frames in a dialog animation in an enterprise app. (Yes, of course gamers would care, but that's a separate audience.)

- Brian

[Daniel Kurka]

Am 21.01.2013 um 06:17 schrieb Brian Slesinsky <skyb...@google.com>:

On Sun, Jan 20, 2013 at 7:35 PM, John A. Tamplin <j...@jaet.org> wrote:

I agree, plus I think if we are going to do this there should be a more global deferred binding property like "useExperimentalApis" in core.

What do you think about flags to turn on each prefixed symbol separately:

  useMozRequestAnimationFrame={true,false}

  useWebkitRequestAnimationFrame={true,false}

I'm not sure about the terminology but I'm thinking of using define-configuration-property and set-configuration-property. (That is, essentially a flag, not a permutation.) 

This would make it easy for people with the resources to do browser testing to turn on the features that seem to be working, and quickly disable them if they run into problems. (Much in the same way server-side flags are a way to leave decisions to sysadmins in the server-side world.) We can't know what the state of browser breakage will be in a year. when Safari 7 comes out, anyone using a non-trunk version of GWT needs to make their own decisions.

We have seen that most of the community is always on the latest version of GWT. So for most users doing a simple recompile once we fix a bug would be fine.

Since upgrading GWT versions is normally quite straight forward, I don`t really understand why we should take the hit and compromise performance for everyone.

To me this would be a different argument, if we would have seen lots of breakages (did we?), but right now my feeling is that we are doing very good in those terms.

On Sun, Jan 20, 2013 at 8:25 PM, Ray Cromwell <cromw...@google.com> wrote:

Not using the most efficient implementation by default seems overly
harsh and likely to bias people against using GWT, so I tend to side

What do you mean by "too harsh"? I haven't tested yet, but it seems unlikely that any users would care if we dropped a few frames in a dialog animation in an enterprise app. (Yes, of course gamers would care, but that's a separate audience.)

On mobile we care deeply and work very hard to be as fast as possible. Falling back back to setTimeout would burden mgwt with a huge difference since there is no way to properly sync setTimeout with the screen refresh rate.

- Brian

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[Jens]

What do you mean by "too harsh"? I haven't tested yet, but it seems unlikely that any users would care if we dropped a few frames in a dialog animation in an enterprise app. (Yes, of course gamers would care, but that's a separate audience.)

Sorry but if you choose to animate things then it should be smooth. Otherwise you better not use animations at all. Its just bad user experience if the animation feels sluggish especially on mobile devices (and its not just about dialogs..). It would really annoy me if I had to re-enable all browser specific performance optimizations that use experimental apis. I would expect it to be enabled by default. 

In that specific case of Chrome breaking animations you could have cherry picked the patch into 2.4.0 and release it as 2.4.1 or am I wrong? Maybe its just better to leave everything as is and instead define a rule that bugfixes for features that are based around experimental apis will be back ported/cherry picked into the last two (choose your number here) major versions of GWT. 

Also in case of animations a method on widgets that enables/disables animations would also work if animations are disabled by default and the method's JavaDoc clearly states that enabled animations will use experimental apis for better performance and that you should test your app against beta browsers, report any issues and be prepared to update GWT. If you then call setAnimationEnabled(true) you know what to expect.

-- J.

[Brian Slesinsky]

Brian Slesinsky has uploaded a new patch set (#2).

Change subject: Make AnimationScheduler configurable and change defaults.
......................................................................

Make AnimationScheduler configurable and change defaults.

The new properties are:
requestAnimationFrameMozilla
requestAnimationFrameWebkit

Each property has the following settings:
fallback - try unprefixed API, then prefixed (default)
prefixed - just try prefixed
unprefixed - just try unprefixed
none - don't try to use requestAnimationFrame, just use a timer.

Also, change the implementation to avoid any dependency on
requestAnimationFrame's return value.

Change-Id: I3011dceab489871a5864eed1ece47ec850d82425
Review-Link: https://gwt-review.googlesource.com/#/c/1780/
---
M user/src/com/google/gwt/animation/Animation.gwt.xml
M user/src/com/google/gwt/animation/client/AnimationSchedulerImpl.java

M
user/src/com/google/gwt/animation/client/AnimationSchedulerImplMozilla.java
A user/src/com/google/gwt/animation/client/AnimationSchedulerImplNative.java
M user/src/com/google/gwt/animation/client/AnimationSchedulerImplWebkit.java

A user/src/com/google/gwt/animation/client/Config.java
A user/src/com/google/gwt/animation/client/ConfigFallback.java
A user/src/com/google/gwt/animation/client/ConfigMozilla.java
A user/src/com/google/gwt/animation/client/ConfigPrefixed.java
A user/src/com/google/gwt/animation/client/ConfigUnprefixed.java
A user/src/com/google/gwt/animation/client/ConfigWebkit.java
11 files changed, 376 insertions(+), 111 deletions(-)

--
To view, visit https://gwt-review.googlesource.com/1780
To unsubscribe, visit https://gwt-review.googlesource.com/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I3011dceab489871a5864eed1ece47ec850d82425
Gerrit-PatchSet: 2

Gerrit-Project: gwt
Gerrit-Branch: master
Gerrit-Owner: Brian Slesinsky <skyb...@google.com>

[Brian Slesinsky]

Brian Slesinsky has uploaded a new patch set (#3).

Change subject: Make AnimationScheduler configurable and change defaults.
......................................................................

Make AnimationScheduler configurable and change defaults.

The new properties are:
requestAnimationFrameMozilla
requestAnimationFrameWebkit

Each property has the following settings:
fallback - try unprefixed API, then prefixed (default)
prefixed - just try prefixed
unprefixed - just try unprefixed
none - don't try to use requestAnimationFrame, just use a timer.

Also, change the implementation to avoid any dependency on
requestAnimationFrame's return value.

Change-Id: I3011dceab489871a5864eed1ece47ec850d82425
Review-Link: https://gwt-review.googlesource.com/#/c/1780/
---
M user/src/com/google/gwt/animation/Animation.gwt.xml
M user/src/com/google/gwt/animation/client/AnimationSchedulerImpl.java

M
user/src/com/google/gwt/animation/client/AnimationSchedulerImplMozilla.java
A user/src/com/google/gwt/animation/client/AnimationSchedulerImplNative.java
M user/src/com/google/gwt/animation/client/AnimationSchedulerImplWebkit.java

A user/src/com/google/gwt/animation/client/Config.java
A user/src/com/google/gwt/animation/client/ConfigFallback.java
A user/src/com/google/gwt/animation/client/ConfigMozilla.java
A user/src/com/google/gwt/animation/client/ConfigPrefixed.java
A user/src/com/google/gwt/animation/client/ConfigUnprefixed.java
A user/src/com/google/gwt/animation/client/ConfigWebkit.java

11 files changed, 374 insertions(+), 111 deletions(-)

--
To view, visit https://gwt-review.googlesource.com/1780
To unsubscribe, visit https://gwt-review.googlesource.com/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I3011dceab489871a5864eed1ece47ec850d82425
Gerrit-PatchSet: 3

Gerrit-Project: gwt
Gerrit-Branch: master
Gerrit-Owner: Brian Slesinsky <skyb...@google.com>

[Brian Slesinsky]

Brian Slesinsky has posted comments on this change.

Change subject: Make AnimationScheduler configurable and change defaults.
......................................................................

Patch Set 1:

(4 comments)

....................................................
File user/src/com/google/gwt/animation/Animation.gwt.xml
Line 27: <replace-with
class="com.google.gwt.animation.client.AnimationSchedulerImplTimer">

I have Windows 7 so it's not so easy to test. Can someone try it on Windows
8? (And it seems like it's another patch.)

Line 27: <replace-with
class="com.google.gwt.animation.client.AnimationSchedulerImplTimer">

I think we shouldn't use it without testing it?


Line 41: <!-- Disabled by default because it uses a prefixed API. -->
See the new patch. Not sure about the defaults yet.


....................................................
File
user/src/com/google/gwt/animation/client/AnimationSchedulerImplNative.java
Line 60: * create a JavaScriptObject and add an expando
named "cancelled" to indicate
Done

--
To view, visit https://gwt-review.googlesource.com/1780
To unsubscribe, visit https://gwt-review.googlesource.com/settings

Gerrit-MessageType: comment

Gerrit-Change-Id: I3011dceab489871a5864eed1ece47ec850d82425
Gerrit-PatchSet: 1
Gerrit-Project: gwt
Gerrit-Branch: master
Gerrit-Owner: Brian Slesinsky <skyb...@google.com>

Gerrit-Reviewer: Brian Slesinsky <skyb...@google.com>

Gerrit-Reviewer: Goktug Gokdogan <gok...@google.com>
Gerrit-Reviewer: Thomas Broyer <t.br...@gmail.com>

Gerrit-HasComments: Yes

[Ray Cromwell]

Ray Cromwell has posted comments on this change.

Change subject: Make AnimationScheduler configurable and change defaults.
......................................................................

Patch Set 3:

// TODO: register a Disposable when unloadModule patch lands

--
To view, visit https://gwt-review.googlesource.com/1780
To unsubscribe, visit https://gwt-review.googlesource.com/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I3011dceab489871a5864eed1ece47ec850d82425
Gerrit-PatchSet: 3

Gerrit-Project: gwt
Gerrit-Branch: master
Gerrit-Owner: Brian Slesinsky <skyb...@google.com>

Gerrit-Reviewer: Brian Slesinsky <skyb...@google.com>
Gerrit-Reviewer: Goktug Gokdogan <gok...@google.com>

Gerrit-Reviewer: Ray Cromwell <cromw...@google.com>
Gerrit-Reviewer: Thomas Broyer <t.br...@gmail.com>
Gerrit-HasComments: No

[Thomas Broyer]

Thomas Broyer has posted comments on this change.

Change subject: Make AnimationScheduler configurable and change defaults.
......................................................................


Patch Set 3:

(3 comments)

Proposal:

Let's remove the ConfigMozilla and ConfigWebkit marker interfaces and use
<when-property-is name="user.agent"> conditions: e.g. we'll use
ConfigFallback if requestAnimationFrameMozilla=fallback and
user.agent=gecko1_8, or requestAnimationFrameWebkit=fallback and
user.agent=safari.

Let's make AnimationSchedulerImplNative concrete and add a new set of
classes to get the prefixed method: the default implementation returns
null, the Mozilla variant returns $wnd.mozRequestAnimationFrame, and the
Webkit variant returns $wnd.webkitRequestAnimationFrame.

---

I otherwise agree that IE10 support can wait (we should do it in one shot
for all of gwt-user).

I'd love to see a wider requestAnimationFrame switch (using conditionals to
initialize the Mozilla and Webkit variants to the same value) and a global
experimentalApis (not sure about the name: the unprefixed versions
shouldn't be seen as experimental, and definitely will no longer be in a
few years from now) switch that would initialize requestAnimationFrame
(using conditionals); but that can wait.

....................................................
File user/src/com/google/gwt/animation/Animation.gwt.xml
Line 68: <any>
What's this <any> for?


Line 69: <when-type-is
class="com.google.gwt.animation.client.ConfigMozilla"/>
Couldn't we use

<when-property-is name="user.agent" value="gecko1_8" />

instead of introducing those specific ConfigMozilla and ConfigWebkit
interfaces?


....................................................
File
user/src/com/google/gwt/animation/client/AnimationSchedulerImplNative.java
Line 84: protected abstract JavaScriptObject getPrefixedFunction();
How about a GWT.create()d class with Webkit and Mozilla variants, similar
to Config, instead of subclasses of AnimationSchedulerImplNative?

--
To view, visit https://gwt-review.googlesource.com/1780
To unsubscribe, visit https://gwt-review.googlesource.com/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I3011dceab489871a5864eed1ece47ec850d82425
Gerrit-PatchSet: 3

Gerrit-Project: gwt
Gerrit-Branch: master
Gerrit-Owner: Brian Slesinsky <skyb...@google.com>

Gerrit-Reviewer: Brian Slesinsky <skyb...@google.com>
Gerrit-Reviewer: Goktug Gokdogan <gok...@google.com>
Gerrit-Reviewer: Ray Cromwell <cromw...@google.com>
Gerrit-Reviewer: Thomas Broyer <t.br...@gmail.com>

Gerrit-HasComments: Yes

[Goktug Gokdogan]

Goktug Gokdogan has posted comments on this change.

Change subject: Make AnimationScheduler configurable and change defaults.
......................................................................


Patch Set 3:

I think this much configuration doesn't really add much value.

With current implementation (i.e. not relying on any params and falling
back from raf -> prefixedRaf -> timer);

- We should not be broken by Chrome and IE as they are already switched to
non-prefixed version and our code works with the prefixed versions out
there.
- It is unlikely to be broken by Safari as with the next release they will
be following Chrome by using a newer version of webkit.
- It is unlikely to be broken by Firefox given that API much more mature
than before and they will not make a big enough change that will break our
code which doesn't rely much on params & return value.

So our configuration is basically targeting a scenario where firefox or
safari will cause a very unlikely big change and the developer of the GWT
app is not willing to upgrade to a newer version of GWT.
I think in the event of this scenario, it is a fair solution to just
disable the RAF with a flag (i.e. "disableRequestAnimationFrame"). If they
don't like doing it, then they can either upgrade or apply a path to their
version of GWT.

--
To view, visit https://gwt-review.googlesource.com/1780
To unsubscribe, visit https://gwt-review.googlesource.com/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I3011dceab489871a5864eed1ece47ec850d82425
Gerrit-PatchSet: 3

Gerrit-Project: gwt
Gerrit-Branch: master
Gerrit-Owner: Brian Slesinsky <skyb...@google.com>

Gerrit-Reviewer: Brian Slesinsky <skyb...@google.com>
Gerrit-Reviewer: Goktug Gokdogan <gok...@google.com>
Gerrit-Reviewer: Ray Cromwell <cromw...@google.com>
Gerrit-Reviewer: Thomas Broyer <t.br...@gmail.com>

Gerrit-HasComments: No

[Brian Slesinsky]

Brian Slesinsky has uploaded a new patch set (#4).

Change subject: Make AnimationScheduler configurable and change defaults.
......................................................................

Make AnimationScheduler configurable and change defaults.

The new properties are:
requestAnimationFrameMozilla
requestAnimationFrameWebkit

Each property has the following settings:
fallback - try unprefixed API, then prefixed (default)
prefixed - just try prefixed
unprefixed - just try unprefixed
none - don't try to use requestAnimationFrame, just use a timer.

Also, change the implementation to avoid any dependency on
requestAnimationFrame's return value.

Change-Id: I3011dceab489871a5864eed1ece47ec850d82425
Review-Link: https://gwt-review.googlesource.com/#/c/1780/
---
M user/src/com/google/gwt/animation/Animation.gwt.xml
M user/src/com/google/gwt/animation/client/AnimationSchedulerImpl.java

D
user/src/com/google/gwt/animation/client/AnimationSchedulerImplMozilla.java
A user/src/com/google/gwt/animation/client/AnimationSchedulerImplNative.java
D user/src/com/google/gwt/animation/client/AnimationSchedulerImplWebkit.java
A user/src/com/google/gwt/animation/client/BrowserFunctions.java
A user/src/com/google/gwt/animation/client/BrowserFunctionsMozilla.java
A user/src/com/google/gwt/animation/client/BrowserFunctionsWebkit.java
A user/src/com/google/gwt/animation/client/Config.java
A user/src/com/google/gwt/animation/client/ConfigFallback.java
A user/src/com/google/gwt/animation/client/ConfigPrefixed.java
A user/src/com/google/gwt/animation/client/ConfigUnprefixed.java
12 files changed, 399 insertions(+), 158 deletions(-)

--
To view, visit https://gwt-review.googlesource.com/1780
To unsubscribe, visit https://gwt-review.googlesource.com/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I3011dceab489871a5864eed1ece47ec850d82425
Gerrit-PatchSet: 4

Gerrit-Project: gwt
Gerrit-Branch: master
Gerrit-Owner: Brian Slesinsky <skyb...@google.com>

[Brian Slesinsky]

Brian Slesinsky has posted comments on this change.

Change subject: Make AnimationScheduler configurable and change defaults.
......................................................................

Patch Set 3:

(2 comments)

....................................................
File user/src/com/google/gwt/animation/Animation.gwt.xml

Line 69: <when-type-is
class="com.google.gwt.animation.client.ConfigMozilla"/>

Done

....................................................
File
user/src/com/google/gwt/animation/client/AnimationSchedulerImplNative.java
Line 84: protected abstract JavaScriptObject getPrefixedFunction();

Done

--
To view, visit https://gwt-review.googlesource.com/1780
To unsubscribe, visit https://gwt-review.googlesource.com/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I3011dceab489871a5864eed1ece47ec850d82425
Gerrit-PatchSet: 3

Gerrit-Project: gwt
Gerrit-Branch: master
Gerrit-Owner: Brian Slesinsky <skyb...@google.com>

Gerrit-Reviewer: Brian Slesinsky <skyb...@google.com>
Gerrit-Reviewer: Goktug Gokdogan <gok...@google.com>
Gerrit-Reviewer: Ray Cromwell <cromw...@google.com>
Gerrit-Reviewer: Thomas Broyer <t.br...@gmail.com>

Gerrit-HasComments: Yes

[Brian Slesinsky]

Brian Slesinsky has posted comments on this change.

Change subject: Make AnimationScheduler configurable and change defaults.
......................................................................

Patch Set 4:

I think Gotug has a good point; since we're unlikely to see another bad bug
in requestAnimationFrame, this is an exercise in closing the barn door
after the horses are gone. So I'm okay with releasing GWT 2.5.1 without
this patch.

About the general problem:

For someone releasing a GWT app that they won't be able to upgrade quickly
(it may be software they're shipping so it's out of their hands), setting a
single "conservative mode" flag seems like the best option.

But I was actually thinking about a different case, where someone is
reacting to a new browser bug by setting a flag, recompiling, and
redeploying. It would be a shame to have to disable a feature in all
browsers because of a browser bug in one of them, or to degrade all
features because of a bug in one of them. So that's why I was thinking of
having feature and browser-specific flags.

But actually, for a sysadmin, having to recompile a GWT app and do a
release to make a change isn't a great workflow either. It would make more
sense to have flags that can be set from the server at runtime so that they
can be hooked into the administrator's dashboard.

So maybe we should have runtime options that come from the HTML page or a
separate server-side file?

--
To view, visit https://gwt-review.googlesource.com/1780
To unsubscribe, visit https://gwt-review.googlesource.com/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I3011dceab489871a5864eed1ece47ec850d82425
Gerrit-PatchSet: 4

Gerrit-Project: gwt
Gerrit-Branch: master
Gerrit-Owner: Brian Slesinsky <skyb...@google.com>

Gerrit-Reviewer: Brian Slesinsky <skyb...@google.com>
Gerrit-Reviewer: Goktug Gokdogan <gok...@google.com>
Gerrit-Reviewer: Ray Cromwell <cromw...@google.com>
Gerrit-Reviewer: Thomas Broyer <t.br...@gmail.com>

Gerrit-HasComments: No

[John A. Tamplin]

John A. Tamplin has posted comments on this change.

Change subject: Make AnimationScheduler configurable and change defaults.
......................................................................


Patch Set 4:

No, just make it a regular deferred binding property. It can be set to a
default value so it doesn't change the number of permutations for most
people (and we can make it conservative or not), and we already support
having the server set properties that can be picked up by the property
provider (ie, locale), and can even be used with server-side script
selection. No need to invent new mechanisms.

--
To view, visit https://gwt-review.googlesource.com/1780
To unsubscribe, visit https://gwt-review.googlesource.com/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I3011dceab489871a5864eed1ece47ec850d82425
Gerrit-PatchSet: 4

Gerrit-Project: gwt
Gerrit-Branch: master
Gerrit-Owner: Brian Slesinsky <skyb...@google.com>

Gerrit-Reviewer: Brian Slesinsky <skyb...@google.com>
Gerrit-Reviewer: Goktug Gokdogan <gok...@google.com>

Gerrit-Reviewer: John A. Tamplin <j...@jaet.org>

[Brian Slesinsky]

Brian Slesinsky has posted comments on this change.

Change subject: Make AnimationScheduler configurable and change defaults.
......................................................................


Patch Set 4:

But that seems unusable for anyone using lots of locales? (Due to the N*M
problem.) There are too many permutations already - I don't want to add any
more.

--
To view, visit https://gwt-review.googlesource.com/1780
To unsubscribe, visit https://gwt-review.googlesource.com/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I3011dceab489871a5864eed1ece47ec850d82425
Gerrit-PatchSet: 4

Gerrit-Project: gwt
Gerrit-Branch: master
Gerrit-Owner: Brian Slesinsky <skyb...@google.com>

[John A. Tamplin]

John A. Tamplin has posted comments on this change.

Change subject: Make AnimationScheduler configurable and change defaults.
......................................................................


Patch Set 4:

Your suggestion would mean having both sets of code in the binary and
making the decision at runtime. For people who care about the number of
permutations but still want both options, we already have the solution in
place -- soft permutations, which puts both of them in the app and does a
runtime switch.

Personally, I still don't think it is a problem to have hundreds of
permutations -- if you have that big an app, you have a build farm and can
do them in parallel, and you only do it for a production deploy.

--
To view, visit https://gwt-review.googlesource.com/1780
To unsubscribe, visit https://gwt-review.googlesource.com/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I3011dceab489871a5864eed1ece47ec850d82425
Gerrit-PatchSet: 4

Gerrit-Project: gwt
Gerrit-Branch: master
Gerrit-Owner: Brian Slesinsky <skyb...@google.com>

[Brian Slesinsky]

Brian Slesinsky has posted comments on this change.

Change subject: Make AnimationScheduler configurable and change defaults.
......................................................................


Patch Set 4:

Hmm. It looks like we tried and failed to use soft permutations before:
https://code.google.com/p/google-web-toolkit/source/list?path=/trunk/user/src/com/google/gwt/animation/client/Animation.java

--
To view, visit https://gwt-review.googlesource.com/1780
To unsubscribe, visit https://gwt-review.googlesource.com/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I3011dceab489871a5864eed1ece47ec850d82425
Gerrit-PatchSet: 4

Gerrit-Project: gwt
Gerrit-Branch: master
Gerrit-Owner: Brian Slesinsky <skyb...@google.com>

[Brian Slesinsky]

Brian Slesinsky has abandoned this change.

Change subject: Make AnimationScheduler configurable and change defaults.
......................................................................

Abandoned

We went with the simpler fix for 2.5.1 and I'm unlikely to get back to this.

--
To view, visit https://gwt-review.googlesource.com/1780
To unsubscribe, visit https://gwt-review.googlesource.com/settings

Gerrit-MessageType: abandon
Gerrit-Change-Id: I3011dceab489871a5864eed1ece47ec850d82425
Gerrit-PatchSet: 4

Gerrit-Project: gwt
Gerrit-Branch: master
Gerrit-Owner: Brian Slesinsky <skyb...@google.com>