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>. On 2011/06/08 18:42:47, jlabanca wrote:
You can remove this comment since the user can now call
createIfSupported(). Done. 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(); On 2011/06/08 18:42:47, jlabanca wrote:
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.
Done. 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 { On 2011/06/08 18:42:47, jlabanca wrote:
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. If this is a JSO, JUnit tests can't use it, right? I agree the POJO-to-JSO conversion is kind of gross, but it was the best I could come up with that was JRE-compatible. 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) { On 2011/06/08 18:42:47, jlabanca wrote:
setEnableHighAccuracy/setHighAccuracyEnabled
I'm not sure I understand the comment, but I changed it to match what I think you meant... 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. On 2011/06/08 18:42:47, jlabanca wrote:
Add a comment that if the maximum age is 0, then there is no max (if
that is
true).
Done. 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. On 2011/06/08 18:42:47, jlabanca wrote:
Add a comment that setting the timeout to -1 results in no timeout.
Done. 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: On 2011/06/08 18:42:47, jlabanca wrote:
Add a static method isSupported() that returns a boolean.
Done. 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() { On 2011/06/08 18:42:47, jlabanca wrote:
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.
Done. 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() { On 2011/06/08 18:42:47, jlabanca wrote:
Remove the isSupported() instance method and add a static one. If you
have a
Geolocation instance, then its already supported.
Done. 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#newcode28 user/src/com/google/gwt/geolocation/client/GeolocationImpl.java:28: private static native boolean detectSupport() /*-{ On 2011/06/08 18:42:47, jlabanca wrote:
Remove - this was moved to the Detector
Done. 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) { On 2011/06/08 18:42:47, jlabanca wrote:
I think you can change the JavaScriptObject parameter to a
PositionImpl, and GWT
will cast it for you
Done. 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); On 2011/06/08 18:42:47, jlabanca wrote:
handleSuccess also needs to take the callback.
Done. 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); On 2011/06/08 18:42:47, jlabanca wrote:
handleFailure needs to take the callback.
Done. 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() { On 2011/06/08 18:42:47, jlabanca wrote:
remove
Done. 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); On 2011/06/08 18:42:47, jlabanca wrote:
need to pass the callback?
Done. 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); On 2011/06/08 18:42:47, jlabanca wrote:
need to pass the callback?
Done. 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>() { On 2011/06/08 18:42:47, jlabanca wrote:
You should set a timeout to 2000ms in case the test machine asks for
permission,
and the user isn't there to respond.
Ah, yeah, good point. This would explain why running this test hangs forever :) So actually, the timeout doesn't actually start ticking until the user grants permission. I'm not sure how to get around this without levels of GWT-fu beyond my own... http://gwt-code-reviews.appspot.com/1451811/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
