Getting closer!
http://gwt-code-reviews.appspot.com/1451811/diff/2013/user/src/com/google/gwt/geolocation/client/Geolocation.java File user/src/com/google/gwt/geolocation/client/Geolocation.java (right): http://gwt-code-reviews.appspot.com/1451811/diff/2013/user/src/com/google/gwt/geolocation/client/Geolocation.java#newcode28 user/src/com/google/gwt/geolocation/client/Geolocation.java:28: * <code>GWT.create(Geolocation.class)</code>. You can remove this comment since the user can now call createIfSupported(). http://gwt-code-reviews.appspot.com/1451811/diff/2013/user/src/com/google/gwt/geolocation/client/Geolocation.java#newcode68 user/src/com/google/gwt/geolocation/client/Geolocation.java:68: private static boolean supported = detectSupport(); This should be non-static to ensure we don't end up with clinits(). I'm not 100% sure that it matters, but its better to be safe. http://gwt-code-reviews.appspot.com/1451811/diff/2013/user/src/com/google/gwt/geolocation/client/Geolocation.java#newcode93 user/src/com/google/gwt/geolocation/client/Geolocation.java:93: public class PositionOptions { This could be a JSO. Make its constructor private, then add a static method Geolocation.createPositionOptions(), which in turn calls JavaScriptObject.createObject() and casts it to a PositionOptions. Then, you can get rid of the fields and just update the object directly. You also won't need to convert back and forth from a JSO in the impl class. http://gwt-code-reviews.appspot.com/1451811/diff/2013/user/src/com/google/gwt/geolocation/client/Geolocation.java#newcode112 user/src/com/google/gwt/geolocation/client/Geolocation.java:112: public PositionOptions setEnableHighAccuracy(boolean enable) { setEnableHighAccuracy/setHighAccuracyEnabled http://gwt-code-reviews.appspot.com/1451811/diff/2013/user/src/com/google/gwt/geolocation/client/Geolocation.java#newcode121 user/src/com/google/gwt/geolocation/client/Geolocation.java:121: * locate the user and cache and return the position. Add a comment that if the maximum age is 0, then there is no max (if that is true). http://gwt-code-reviews.appspot.com/1451811/diff/2013/user/src/com/google/gwt/geolocation/client/Geolocation.java#newcode131 user/src/com/google/gwt/geolocation/client/Geolocation.java:131: * takes more than this amount of time, an error will result. Add a comment that setting the timeout to -1 results in no timeout. http://gwt-code-reviews.appspot.com/1451811/diff/2013/user/src/com/google/gwt/geolocation/client/Geolocation.java#newcode138 user/src/com/google/gwt/geolocation/client/Geolocation.java:138: Add a static method isSupported() that returns a boolean. http://gwt-code-reviews.appspot.com/1451811/diff/2013/user/src/com/google/gwt/geolocation/client/Geolocation.java#newcode139 user/src/com/google/gwt/geolocation/client/Geolocation.java:139: public static Geolocation createIfSupported() { Since there is only on Geolocation object, you should rename this to getGeolocationIfSupported() (see Storage.java). It looks like you always return the same implementation anyway. http://gwt-code-reviews.appspot.com/1451811/diff/2013/user/src/com/google/gwt/geolocation/client/Geolocation.java#newcode194 user/src/com/google/gwt/geolocation/client/Geolocation.java:194: public boolean isSupported() { Remove the isSupported() instance method and add a static one. If you have a Geolocation instance, then its already supported. http://gwt-code-reviews.appspot.com/1451811/diff/2013/user/src/com/google/gwt/geolocation/client/GeolocationImpl.java File user/src/com/google/gwt/geolocation/client/GeolocationImpl.java (right): http://gwt-code-reviews.appspot.com/1451811/diff/2013/user/src/com/google/gwt/geolocation/client/GeolocationImpl.java#newcode26 user/src/com/google/gwt/geolocation/client/GeolocationImpl.java:26: class GeolocationImpl extends Geolocation { Can you roll GeolocationImpl into Geolocation now that you only create a Geolocation if supported? http://gwt-code-reviews.appspot.com/1451811/diff/2013/user/src/com/google/gwt/geolocation/client/GeolocationImpl.java#newcode28 user/src/com/google/gwt/geolocation/client/GeolocationImpl.java:28: private static native boolean detectSupport() /*-{ Remove - this was moved to the Detector http://gwt-code-reviews.appspot.com/1451811/diff/2013/user/src/com/google/gwt/geolocation/client/GeolocationImpl.java#newcode36 user/src/com/google/gwt/geolocation/client/GeolocationImpl.java:36: private static void handleSuccess(Callback<Position, PositionError> callback, JavaScriptObject obj) { I think you can change the JavaScriptObject parameter to a PositionImpl, and GWT will cast it for you http://gwt-code-reviews.appspot.com/1451811/diff/2013/user/src/com/google/gwt/geolocation/client/GeolocationImpl.java#newcode41 user/src/com/google/gwt/geolocation/client/GeolocationImpl.java:41: private static native JavaScriptObject toJso(PositionOptions options) /*-{ Is PositionOptions is a JSO, you can eliminate this method. http://gwt-code-reviews.appspot.com/1451811/diff/2013/user/src/com/google/gwt/geolocation/client/GeolocationImpl.java#newcode67 user/src/com/google/gwt/geolocation/client/GeolocationImpl.java:67: @com.google.gwt.geolocation.client.GeolocationImpl::handleSuccess(*)(pos); handleSuccess also needs to take the callback. http://gwt-code-reviews.appspot.com/1451811/diff/2013/user/src/com/google/gwt/geolocation/client/GeolocationImpl.java#newcode71 user/src/com/google/gwt/geolocation/client/GeolocationImpl.java:71: @com.google.gwt.geolocation.client.GeolocationImpl::handleFailure(*)(err.code, err.message); handleFailure needs to take the callback. http://gwt-code-reviews.appspot.com/1451811/diff/2013/user/src/com/google/gwt/geolocation/client/GeolocationImpl.java#newcode80 user/src/com/google/gwt/geolocation/client/GeolocationImpl.java:80: public boolean isSupported() { remove http://gwt-code-reviews.appspot.com/1451811/diff/2013/user/src/com/google/gwt/geolocation/client/GeolocationImpl.java#newcode92 user/src/com/google/gwt/geolocation/client/GeolocationImpl.java:92: @com.google.gwt.geolocation.client.GeolocationImpl::handleSuccess(*)(pos); need to pass the callback? http://gwt-code-reviews.appspot.com/1451811/diff/2013/user/src/com/google/gwt/geolocation/client/GeolocationImpl.java#newcode96 user/src/com/google/gwt/geolocation/client/GeolocationImpl.java:96: @com.google.gwt.geolocation.client.GeolocationImpl::handleFailure(*)(err.code, err.message); need to pass the callback? http://gwt-code-reviews.appspot.com/1451811/diff/2013/user/test/com/google/gwt/geolocation/client/GeolocationTest.java File user/test/com/google/gwt/geolocation/client/GeolocationTest.java (right): http://gwt-code-reviews.appspot.com/1451811/diff/2013/user/test/com/google/gwt/geolocation/client/GeolocationTest.java#newcode48 user/test/com/google/gwt/geolocation/client/GeolocationTest.java:48: geolocation.getCurrentPosition(new Callback<Position, PositionError>() { You should set a timeout to 2000ms in case the test machine asks for permission, and the user isn't there to respond. http://gwt-code-reviews.appspot.com/1451811/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
