636344 - Android Geolocation provider does not provide geocoded civic addresses

Status: RESOLVED FIXED

(Core :: DOM: Geolocation, defect)

Comment 1

[Doug Turner (:dougt)]

Attachment: patch v.1

Comment 2

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 520660 [details] [diff] [review]
patch v.1

>     public static final int ACTIVITY_PAUSING = 9;
>     public static final int ACTIVITY_SHUTDOWN = 10;
>     public static final int LOAD_URI = 11;
>-
>     public static final int SURFACE_CREATED = 12;
>     public static final int SURFACE_DESTROYED = 13;
>     public static final int GECKO_EVENT_SYNC = 14;
nit, don't remove this space



> 
>+    private class GeocoderTask extends AsyncTask<Location, Void, Void> {
>+        protected Void doInBackground(Location... location) {
>+            try {
>+                List<Address> addresses = mGeocoder.getFromLocation(location[0].getLatitude(), location[0].getLongitude(), 1);
>+                mLastGeoAddress = addresses.get(0);
check that addresses.size() is at least 1 before accessing it.

Also, are these addresses sorted in anyway? How do we know that the first one is the best one to return?

>+            } catch (Exception e) {
>+                Log.w("GeckoSurfaceView", "GeocoderTask "+e);
>+            }
Log.w("GeckoSurfaceView", "GeocoderTask", e);

Also, what exceptions are you expecting from this code? Finally should this be logged as an error (Log.e)?


>+            return null;
>+        }
>+    }
>+
>     // geolocation
>     public void onLocationChanged(Location location)
>     {
>-        GeckoAppShell.sendEventToGecko(new GeckoEvent(location));
>+        try {
>+            if (mGeocoder == null)
>+                mGeocoder = new Geocoder(getContext());
>+
>+            float[] results = new float[1];
>+
>+            if (mLastGeoAddress == null) {
>+                new GeocoderTask().execute(location);
>+            }
>+            else {
>+                Location.distanceBetween(location.getLatitude(),
>+                                         location.getLongitude(),
>+                                         mLastGeoAddress.getLatitude(),
>+                                         mLastGeoAddress.getLongitude(),
>+                                         results);
>+                Log.w("GeckoSurfaceView", "onLocationChanged distance between " + results[0]);
this sounds like info rather than a warning. 

>+
>+                if (results[0] > 100) 
magic number? Can we define a constant and comment on where it came from?

>+        } catch (Exception e) {
>+            Log.w("GeckoSurfaceView", "onLocationChanged "+e);
>+        }
Does this code throw any specific errors? If so please catch them explicitly. Otherwise, loose the try-catch



>+    // Currently unsupported by the Android Address object
>+    nsString streetNumber;
>+    nsString premises;
>+    nsString region;

as discussed on irc, the android address object has a field for premises (http://developer.android.com/reference/android/location/Address.html#getPremises%28%29) and getSubThoroughfare() should provide street number. I also it looks like getAdminArea() would map to region.

>+    nsJNIString street(static_cast<jstring>(jenv->CallObjectMethod(jobj, jGetAddressLineMethod, 0)), jenv);
Also discussed on irc, getThoroughfare() would be better mapped to street than the address line

r+, but please test with the other field mappings. Please request re-review if the changes aren't trivial

Comment 3

[Doug Turner (:dougt)]

(In reply to comment #2)
> Comment on attachment 520660 [details] [diff] [review]
> patch v.1
> 
> >     public static final int ACTIVITY_PAUSING = 9;
> >     public static final int ACTIVITY_SHUTDOWN = 10;
> >     public static final int LOAD_URI = 11;
> >-
> >     public static final int SURFACE_CREATED = 12;
> >     public static final int SURFACE_DESTROYED = 13;
> >     public static final int GECKO_EVENT_SYNC = 14;
> nit, don't remove this space


The are all part of the same enumeration. Why is the space there?



> >+                List<Address> addresses = mGeocoder.getFromLocation(location[0].getLatitude(), location[0].getLongitude(), 1);
> >+                mLastGeoAddress = addresses.get(0);
> check that addresses.size() is at least 1 before accessing it.

The try will catch this.

> Also, are these addresses sorted in anyway? How do we know that the first one
> is the best one to return?

No, we don't.  in testing, there is only one address.  Comment added.


> >+            } catch (Exception e) {
> >+                Log.w("GeckoSurfaceView", "GeocoderTask "+e);
> >+            }
> Log.w("GeckoSurfaceView", "GeocoderTask", e);
> 
> Also, what exceptions are you expecting from this code? Finally should this be
> logged as an error (Log.e)?

warning is fine, since |address| is really not a required piece of geolocation.


> >+                                         results);
> >+                Log.w("GeckoSurfaceView", "onLocationChanged distance between " + results[0]);
> this sounds like info rather than a warning. 
> 

removed.


> >+
> >+                if (results[0] > 100) 
> magic number? Can we define a constant and comment on where it came from?

commented.

> 
> >+        } catch (Exception e) {
> >+            Log.w("GeckoSurfaceView", "onLocationChanged "+e);
> >+        }
> Does this code throw any specific errors? If so please catch them explicitly.
> Otherwise, loose the try-catch

done.

> 
> 
> 
> >+    // Currently unsupported by the Android Address object
> >+    nsString streetNumber;
> >+    nsString premises;
> >+    nsString region;
> 
> as discussed on irc, the android address object has a field for premises
> (http://developer.android.com/reference/android/location/Address.html#getPremises%28%29)
> and getSubThoroughfare() should provide street number. I also it looks like
> getAdminArea() would map to region.
> 
> >+    nsJNIString street(static_cast<jstring>(jenv->CallObjectMethod(jobj, jGetAddressLineMethod, 0)), jenv);
> Also discussed on irc, getThoroughfare() would be better mapped to street than
> the address line

Thanks.. works and matches gls geolocation service.

Comment 4

[Doug Turner (:dougt)]

http://hg.mozilla.org/mozilla-central/rev/4dcf1c5c0c3f