http://gwt-code-reviews.appspot.com/1370801/diff/1016/user/src/com/google/gwt/touch/Touch.gwt.xml File user/src/com/google/gwt/touch/Touch.gwt.xml (right):
http://gwt-code-reviews.appspot.com/1370801/diff/1016/user/src/com/google/gwt/touch/Touch.gwt.xml#newcode25 user/src/com/google/gwt/touch/Touch.gwt.xml:25: <when-property-is name="user.agent" value="safari" /> FF4 and IE9 may support touch events for tablets/touch devices. Could you check for the other browsers as well (FF3.5, etc.)? It may be best to set the default to "maybe", and set the support of IE6 and other known-not-to-support browsers to "no" so that a user who has defined a permutation for "mobile-webkit" will get "maybe" as expected. http://gwt-code-reviews.appspot.com/1370801/diff/1016/user/src/com/google/gwt/touch/client/DefaultMomentum.java File user/src/com/google/gwt/touch/client/DefaultMomentum.java (right): http://gwt-code-reviews.appspot.com/1370801/diff/1016/user/src/com/google/gwt/touch/client/DefaultMomentum.java#newcode39 user/src/com/google/gwt/touch/client/DefaultMomentum.java:39: public Snapshot updateSnapshot(Snapshot snapshot) { This design feels a little odd. Why not have: public boolean update(Snapshot snapshot); That returns false if there is no more momentum, and true if snapshot is successfully updated? http://gwt-code-reviews.appspot.com/1370801/diff/1016/user/src/com/google/gwt/touch/client/Momentum.java File user/src/com/google/gwt/touch/client/Momentum.java (right): http://gwt-code-reviews.appspot.com/1370801/diff/1016/user/src/com/google/gwt/touch/client/Momentum.java#newcode26 user/src/com/google/gwt/touch/client/Momentum.java:26: * A snapshot of the current state. I think users will find this name confusing. Could State work? Or MomentumState? http://gwt-code-reviews.appspot.com/1370801/diff/1016/user/src/com/google/gwt/touch/client/Momentum.java#newcode153 user/src/com/google/gwt/touch/client/Momentum.java:153: * This method can modify the existing snapshot by called called -> calling http://gwt-code-reviews.appspot.com/1370801/diff/1016/user/src/com/google/gwt/touch/client/Point.java File user/src/com/google/gwt/touch/client/Point.java (right): http://gwt-code-reviews.appspot.com/1370801/diff/1016/user/src/com/google/gwt/touch/client/Point.java#newcode19 user/src/com/google/gwt/touch/client/Point.java:19: * A simple point class. This is a generally useful class. Could it be renamed Vector or Vec2 and moved somewhere more general? A mul(double c) method would be useful for cleaning up the DefaultMomentum code. http://gwt-code-reviews.appspot.com/1370801/diff/1016/user/src/com/google/gwt/touch/client/Point.java#newcode24 user/src/com/google/gwt/touch/client/Point.java:24: private final double y; Is it faster to set these as final and create lots of intermediate Point objects, or to set these as non-final and have methods like void point.sub(Point p). ? http://gwt-code-reviews.appspot.com/1370801/diff/1016/user/src/com/google/gwt/touch/client/TouchScroller.java File user/src/com/google/gwt/touch/client/TouchScroller.java (right): http://gwt-code-reviews.appspot.com/1370801/diff/1016/user/src/com/google/gwt/touch/client/TouchScroller.java#newcode44 user/src/com/google/gwt/touch/client/TouchScroller.java:44: * Adds touch based scrolling to a scroll panel. On the Android browser, dragging feels "choppier" than coasting on momentum. Can you repro this? http://gwt-code-reviews.appspot.com/1370801/diff/1016/user/src/com/google/gwt/touch/client/TouchScroller.java#newcode90 user/src/com/google/gwt/touch/client/TouchScroller.java:90: schedule((int) MS_PER_FRAME); May be cleaner to use schedulerepeating((int) MS_PER_FRAME) once, then call cancel() if snapshot == null to stop the timer. http://gwt-code-reviews.appspot.com/1370801/diff/1016/user/src/com/google/gwt/touch/client/TouchScroller.java#newcode181 user/src/com/google/gwt/touch/client/TouchScroller.java:181: * Runtime check for whether the canvas element is supported in this browser. canvas -> scrollable widget http://gwt-code-reviews.appspot.com/1370801/diff/1016/user/src/com/google/gwt/touch/client/TouchScroller.java#newcode183 user/src/com/google/gwt/touch/client/TouchScroller.java:183: * @return whether the canvas element is supported canvas -> scrollable widget http://gwt-code-reviews.appspot.com/1370801/diff/1016/user/src/com/google/gwt/touch/client/TouchScroller.java#newcode254 user/src/com/google/gwt/touch/client/TouchScroller.java:254: * If the gesture takes too long, we update the recentTuochPosition to the recentTuochPosition -> recentTouchPosition http://gwt-code-reviews.appspot.com/1370801/diff/1016/user/src/com/google/gwt/touch/client/TouchScroller.java#newcode256 user/src/com/google/gwt/touch/client/TouchScroller.java:256: * do this so that we don't based the velocity on two touch events that based -> base http://gwt-code-reviews.appspot.com/1370801/diff/1016/user/src/com/google/gwt/touch/client/TouchScroller.java#newcode537 user/src/com/google/gwt/touch/client/TouchScroller.java:537: } else if (trackingTime > MAX_TRACKING_TIME / 2) { Is there a reason for this being specifically 1/2? If not, maybe use a constant for it. http://gwt-code-reviews.appspot.com/1370801/diff/1016/user/src/com/google/gwt/touch/client/TouchScroller.java#newcode558 user/src/com/google/gwt/touch/client/TouchScroller.java:558: */ Why is this additional handler for click events necessary? Could it be replaced with a check in onTouchStart() checking if the velocity is not zero? http://gwt-code-reviews.appspot.com/1370801/diff/1016/user/src/com/google/gwt/user/User.gwt.xml File user/src/com/google/gwt/user/User.gwt.xml (right): http://gwt-code-reviews.appspot.com/1370801/diff/1016/user/src/com/google/gwt/user/User.gwt.xml#newcode31 user/src/com/google/gwt/user/User.gwt.xml:31: <inherits name="com.google.gwt.touch.Touch"/> Nit: not alphabetical order. http://gwt-code-reviews.appspot.com/1370801/diff/1016/user/src/com/google/gwt/user/client/ui/ScrollPanel.java File user/src/com/google/gwt/user/client/ui/ScrollPanel.java (right): http://gwt-code-reviews.appspot.com/1370801/diff/1016/user/src/com/google/gwt/user/client/ui/ScrollPanel.java#newcode82 user/src/com/google/gwt/user/client/ui/ScrollPanel.java:82: private native boolean isRtl(Element scrollable) /*-{ Is it possible to safely cache the result of this call to prevent recomputing it so much? http://gwt-code-reviews.appspot.com/1370801/diff/1016/user/test/com/google/gwt/touch/client/TouchScrollTest.java File user/test/com/google/gwt/touch/client/TouchScrollTest.java (right): http://gwt-code-reviews.appspot.com/1370801/diff/1016/user/test/com/google/gwt/touch/client/TouchScrollTest.java#newcode31 user/test/com/google/gwt/touch/client/TouchScrollTest.java:31: Can you add a test or two for actions during the coasting/momentum scroll? For instance, that touching during a coast stops the scrolling. Can you add a test that things work fine when a resize occurs during the coasting/momentum scroll? For instance, changing the orientation of a mobile device could change the panel's dimensions. Will everything still stop where it should? http://gwt-code-reviews.appspot.com/1370801/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
