735863 - Implement navigator.geolocation.getAddress()

Status: RESOLVED WONTFIX

(Core :: DOM: Geolocation, defect)

Description

[Josh Dhaliwal (:Arreth)]

Refer to blog post regarding changes in this bug's URL field.

Comment 1

[Josh Dhaliwal (:Arreth)]

Attachment: drafted-implementation

First draft of implementation. Only stubs for actual geocoding. Thoughts?

Comment 2

[Josh Dhaliwal (:Arreth)]

Attachment: drafted-implementation

added changes to not rely on permission prompt for geocoding, and did some cleanup

Comment 3

[Doug Turner (:dougt)]

Comment on attachment 607748 [details] [diff] [review]
drafted-implementation

Review of attachment 607748 [details] [diff] [review]:
-----------------------------------------------------------------

There will be other changes required.  Specifically to define these types in the javascript namespace.  Take a look at /dom/base/nsDOMClassInfo.cpp for "GeoPositionCoords".  Be careful here because the ordering of the types in that file needs to match up with the header (dom/base/nsDOMClassInfoClasses.h). 

You will also need to add back the geo address (it was removed when v2 was removed).  Follow what GeoPositionCoords does.

At this point, you should have a test that you are working against.  Please post that too so that we can see what it looks like.

Good so far.  Keep driving.

::: dom/interfaces/geolocation/nsIDOMGeoGeolocation.idl
@@ +54,5 @@
>  
> +  void getAddress(in nsIDOMGeoPositionCoords coords,
> +				  in nsIDOMGeoAddressCallback successCallback,
> +				  [optional] in nsIDOMGeoAddressErrorCallback errorCallback,
> +				  [optional] in nsIDOMGeoAddressOptions options);

Move this to the bottom of the class description.

::: dom/src/geolocation/nsGeolocation.cpp
@@ +171,5 @@
>  private:
>    nsCOMPtr<nsIDOMGeoPosition>    mPosition;
> +  nsRefPtr<nsGeolocationPositionRequest> mRequest;
> +
> +  nsRefPtr<nsGeolocation>        mLocator;

I am worried about leaking here.  Why do you need this object to have a strong reference back to the nsGeolocation?

@@ +182,5 @@
> +  // event should remove the request from it.  If we ever
> +  // have to do more, then we can change this around.
> +  RequestSendAddressEvent(nsIDOMGeoAddress* anAddress,
> +						  nsGeolocationAddressRequest* aRequest,
> +						  nsGeolocation* aLocator)

white space off

@@ +191,5 @@
> +  }
> +
> +  NS_IMETHOD Run() {
> +    mRequest->SendAddress(mAddress);
> +    if (mLocator)

how can mLocator be null?

::: dom/src/geolocation/nsGeolocation.h
@@ +119,5 @@
> +  nsGeolocationPositionRequest(nsGeolocation* locator,
> +							   nsIDOMGeoPositionCallback* callback,
> +							   nsIDOMGeoPositionErrorCallback* errorCallback,
> +							   nsIDOMGeoPositionOptions* options,
> +							   bool watchPositionRequest = false);

The whitespace is off here.

::: mobile/android/base/GeckoAppShell.java
@@ +579,5 @@
>                  }
>              });
>      }
>  
> +    private static boolean mLocationHighAccuracy = false;

file a separate bug for this.

Comment 4

[Josh Dhaliwal (:Arreth)]

Attachment: drafted-implementation

added nsGeoAddress class and class info to nsDOMClassInfo. Also added newly created files to Makefile.

Comment 5

[Josh Dhaliwal (:Arreth)]

I also had one other question. I noticed in nsGeoPosition, you had the following:

class nsGeoPosition : public nsIDOMGeoPosition
{
public:
  NS_DECL_ISUPPORTS
  NS_DECL_NSIDOMGEOPOSITION

I was just wondering what these macros did? I didn't see any declarations for the getter methods for the various class properties (timestamp, coords, lat, lng, etc.), so I was just wondering where those were? I figured that these macros might have something to do with that? And perhaps if that's true, then I would need to do something similar for nsGeoAddress?

Comment 6

[Josh Dhaliwal (:Arreth)]

Attachment: getAddress-test

I uploaded a test to here as well for testing the getAddress function.

The test for this is also up at 
http://people.mozilla.com/~jdhaliwal/testGetAddress.html

Hopefully it's a decent test for what should be produced from the results of this bug. Right now it causes a global failure, but that's only because getAddress isn't implemented yet haha.

Comment 7

[Josh Dhaliwal (:Arreth)]

Attachment: getAddress-drafted-implementation

Cleaned up the patch. Hopefully no more whitespace abnormalities or unrelated files included.

Comment 8

[Doug Turner (:dougt)]

Comment on attachment 608553 [details] [diff] [review]
getAddress-drafted-implementation

Review of attachment 608553 [details] [diff] [review]:
-----------------------------------------------------------------

getting pretty close.  Lets add the missing files and fix up the nits.

I'd also like to see the patch where we tie this into android's backend.  You probably just want to follow what the geolocation providers do.  keep that stuff as a separate patch.

::: dom/interfaces/geolocation/Makefile.in
@@ +57,5 @@
> +            nsIDOMGeoAddress.idl               \
> +            nsIDOMGeoAddressCallback.idl       \
> +            nsIDOMGeoAddressError.idl          \
> +            nsIDOMGeoAddressErrorCallback.idl  \
> +            nsIDOMGeoAddressOptions.idl        \

this patch is missing most of those files.  be sure to hg add them before diffing.

::: dom/src/geolocation/Makefile.in
@@ +51,5 @@
>  
> +CPPSRCS	= \
> +        nsGeolocation.cpp \
> +        nsGeoPosition.cpp \
> +        nsGeoAddress.cpp \

This patch is also missing that file too.

::: dom/src/geolocation/nsGeolocation.cpp
@@ +171,5 @@
>  private:
>    nsCOMPtr<nsIDOMGeoPosition>    mPosition;
> +  nsRefPtr<nsGeolocationPositionRequest> mRequest;
> +
> +  nsRefPtr<nsGeolocation>        mLocator;

no extra space need here above mLocator

@@ +619,5 @@
> +														 nsIDOMGeoAddressOptions* aOptions)
> +  : mAllowed(false),
> +    mCleared(false),
> +	mCoords(coords),
> +    mCallback(aCallback),

align mCoords with the others.

@@ +652,5 @@
> +  // remove ourselves from the locator's callback lists.
> +  mLocator->RemoveRequest(this);
> +  NotifyError(nsIDOMGeoAddressError::TIMEOUT);
> +
> +  mTimeoutTimer = nsnull;

you want mTimeoutTimer->Cancel before setting the timer to nsnull

@@ +667,5 @@
> +  addressError->NotifyCallback(mErrorCallback);
> +}
> +
> +NS_IMETHODIMP
> +nsGeolocationAddressRequest::Allow()

For now, just jut dispatch as if mAllowed was set to true by the permission prompt. We can discuss the privacy/security model at the review.  (you scheduled that with Curtis, right?)

@@ +1188,1 @@
>  								    callback,

align parameters.

@@ +1215,5 @@
>  }
>  
>  NS_IMETHODIMP
> +nsGeolocation::GetAddress(nsIDOMGeoPositionCoords coords,
> +						  nsIDOMGeoAddressCallback *callback,

align params

@@ +1225,5 @@
> +  if (!sGeoEnabled)
> +    return NS_ERROR_NOT_AVAILABLE;
> +
> +  nsRefPtr<nsGeolocationAddressRequest> request = new nsGeolocationAddressRequest(this,
> +																				  coords,

again.  align params.

@@ +1233,5 @@
> +  if (!request)
> +    return NS_ERROR_OUT_OF_MEMORY;
> +
> +  if (NS_FAILED(request->Init()))
> +    return NS_ERROR_FAILURE; // this as OKAY.  not sure why we wouldn't throw. xxx dft

This comment doesn't make sense here.  Basically if you ever return anything other than NS_OK from this method, javascript will see an assertion.  This generally sucks because js developers typically aren't expecting DOM apis to throw.

Comment 9

[Josh Dhaliwal (:Arreth)]

(In reply to Doug Turner (:dougt) from comment #8)
> Comment on attachment 608553 [details] [diff] [review]
> 
> @@ +652,5 @@
> > +  // remove ourselves from the locator's callback lists.
> > +  mLocator->RemoveRequest(this);
> > +  NotifyError(nsIDOMGeoAddressError::TIMEOUT);
> > +
> > +  mTimeoutTimer = nsnull;
> 
> you want mTimeoutTimer->Cancel before setting the timer to nsnull

This was actually happening with the nsGeolocationPositionRequest::Notify as well, so I added it there as well.

> @@ +667,5 @@
> > +  addressError->NotifyCallback(mErrorCallback);
> > +}
> > +
> > +NS_IMETHODIMP
> > +nsGeolocationAddressRequest::Allow()
> 
> For now, just jut dispatch as if mAllowed was set to true by the permission
> prompt. We can discuss the privacy/security model at the review.  (you
> scheduled that with Curtis, right?)
 
Okay, we can discuss it then. I haven't contacted him yet. I'm assuming that the review is more of a meeting type deal to discuss the changes and their security, so I'll send him an email and CC you in that.

> @@ +1233,5 @@
> > +  if (!request)
> > +    return NS_ERROR_OUT_OF_MEMORY;
> > +
> > +  if (NS_FAILED(request->Init()))
> > +    return NS_ERROR_FAILURE; // this as OKAY.  not sure why we wouldn't throw. xxx dft
> 
> This comment doesn't make sense here.  Basically if you ever return anything
> other than NS_OK from this method, javascript will see an assertion.  This
> generally sucks because js developers typically aren't expecting DOM apis to
> throw.

That comment was actually copied just from the getCurrentPosition() method, so I'm not exactly sure what it meant. Should we still be throwing here if devs aren't expecting it? Or should we still be throwing for the devs that happen to catch? It seems like a legitimate throw if a failure occurs (even though right now it can't, since Init() just returns NS_OK).

Also, one other thing. Do you know of any "smart tab" type utility for emacs that works well? It seems like I'm creating a lot of formatting annoyances for people, and so far the ones that I've found haven't really been working for me too well. The tab right now for me just does a kind of smart indent thing...><

Comment 10

[Josh Dhaliwal (:Arreth)]

Attachment: getAddress-drafted-implementation

Added the missing files, and made the suggested changes from the previous comments.

Comment 11

[Josh Matthews [:jdm]]

>Also, one other thing. Do you know of any "smart tab" type utility for emacs that 
>works well? It seems like I'm creating a lot of formatting annoyances for people, 
>and so far the ones that I've found haven't really been working for me too well. The 
>tab right now for me just does a kind of smart indent thing...><

https://github.com/emacsmirror/dtrt-indent might work.

Comment 12

[Josh Dhaliwal (:Arreth)]

@jdm: Thanks! I'll check it out :D

Comment 13

[Curtis Koenig [:curtisk-use curtis.koenig+bzATgmail.com]]]

marking bug for security review

Comment 14

[Curtis Koenig [:curtisk-use curtis.koenig+bzATgmail.com]]]

Sec Review Notes: https://wiki.mozilla.org/Securtity/Reviews/GeolocationWebAPI

will initiate privacy review per the action items from the review

Comment 15

[Josh Dhaliwal (:Arreth)]

Attachment: getAddress-drafted-implementation

Comment 16

[Curtis Koenig [:curtisk-use curtis.koenig+bzATgmail.com]]]

Link was wrong for review this is correct https://wiki.mozilla.org/Security/Reviews/GeolocationWebAPI

Comment 17

[Josh Dhaliwal (:Arreth)]

Attachment: getAddress-drafted-implementation

Made a lot of changes, but this did build on my tree and worked correctly using the Android reverse-geocoding platform API. Let me know what you think, and what could be improved ;). Thanks!

Note: it was tested using a test which will be uploaded next

Comment 18

[Josh Dhaliwal (:Arreth)]

Attachment: getAddress-test

Comment 19

[Josh Dhaliwal (:Arreth)]

Attachment: getAddress-test

quick fix to test for readability.

Comment 20

[Josh Dhaliwal (:Arreth)]

Attachment: getAddress-drafted-implementation

quick one line modification to get rid of unnecessary comment/operation.

Comment 21

[Doug Turner (:dougt)]

Comment on attachment 616276 [details] [diff] [review]
getAddress-drafted-implementation

Review of attachment 616276 [details] [diff] [review]:
-----------------------------------------------------------------

This is looking really good.

Mostly nits and naming follow.  Changes to nsIGeocodingProvider.idl and to the options bit need to be fixed up.


Also, we are going to need some mochitests for this.

::: dom/interfaces/geolocation/nsIDOMGeoAddress.idl
@@ +31,5 @@
> + * and other provisions required by the GPL or the LGPL. If you do not delete
> + * the provisions above, a recipient may use your version of this file under
> + * the terms of any one of the MPL, the GPL or the LGPL.
> + *
> + * ***** END LICENSE BLOCK ***** */

There is a smaller header you can use.  http://www.mozilla.org/MPL/headers/.  This applies to all of the new license blocks you added.

::: dom/interfaces/geolocation/nsIDOMGeoAddressOptions.idl
@@ +39,5 @@
> +
> +[scriptable, uuid(439D14E5-94A8-4E96-9D54-98533951EDAB)]
> +interface nsIDOMGeoAddressOptions : nsISupports
> +{
> +  attribute long timeout;

Options are handled a bit different now.  Take a look at http://hg.mozilla.org/mozilla-central/rev/686e5bcf747b

::: dom/interfaces/geolocation/nsIDOMGeoGeolocation.idl
@@ +61,5 @@
> +
> +  void getAddress(in nsIDOMGeoPositionCoords coords,
> +                  in nsIDOMGeoAddressCallback successCallback,
> +                  [optional] in nsIDOMGeoAddressErrorCallback errorCallback,
> +                  [optional] in nsIDOMGeoAddressOptions options);

This signature will be a bit different after you review:

  http://hg.mozilla.org/mozilla-central/rev/686e5bcf747b

::: dom/src/geolocation/nsGeoAddress.cpp
@@ +42,5 @@
> +// nsGeoAddress
> +////////////////////////////////////////////////////
> +
> +nsGeoAddress::nsGeoAddress(const nsAString &aCountry,
> +                           const nsAString &anAdmin,

aAdmin is fine.  English suxs.

@@ +50,5 @@
> +                           const nsAString &aThoroughfare,
> +                           const nsAString &aSubThoroughfare,
> +                           const nsAString &aPremises,
> +                           const nsAString &aPostalCode,
> +                           const nsAString &featureName) :

aFeatureName

@@ +77,5 @@
> +NS_DOM_INTERFACE_MAP_ENTRY_CLASSINFO(GeoAddress)
> +NS_INTERFACE_MAP_END
> +
> +NS_IMPL_THREADSAFE_ADDREF(nsGeoAddress)
> +NS_IMPL_THREADSAFE_RELEASE(nsGeoAddress)

Does this need to be threadsafe?  I don't think it does.

::: mobile/android/base/GeckoEvent.java
@@ +82,5 @@
> +    private static final int ACTIVITY_SHUTDOWN = 12;
> +    private static final int LOAD_URI = 13;
> +    private static final int SURFACE_CREATED = 14;
> +    private static final int SURFACE_DESTROYED = 15;
> +    private static final int GECKO_EVENT_SYNC = 16;

Be pretty careful when changing these.  They're fragile.  I'd just add your ADDRESS_EVENT at the end of this list.

::: widget/android/AndroidJavaWrappers.cpp
@@ +286,4 @@
>  AndroidLocation::InitLocationClass(JNIEnv *jEnv)
>  {
>      initInit();
> +    

kill ws

::: xpcom/system/nsIGeocodingProvider.idl
@@ +55,5 @@
> +  /**
> +   * Return a new address from the reverse geocoding operation.
> +   * This must be called on the main thread
> +   */
> +  void returnAddress(in nsIDOMGeoAddress address);

funky function name.


Also, you might need to pass the associated position, right?

  void returnAddress(in nsIDOMGeoPosition position, in nsIDOMGeoAdress address)

@@ +74,5 @@
> +   * Reverse Geocode
> +   * Starts and runs the reverse-geocoding operation 
> +   */
> +  void reverseGeocode(in double latitude,
> +		      in double longitude);

The more I think about it, the more I dislike this interface.  What do you think about something that takes a callback?

Like:
  http://mxr.mozilla.org/mozilla-central/source/xpcom/threads/nsITimer.idl#173


so,

void geocode(in double latitude, in double longitude, in nsIGeocodeResult callback)


nsIGeocodeResult could just be your nsIGeocodingUpdate interface.

Comment 22

[Josh Dhaliwal (:Arreth)]

Attachment: getAddress-drafted-implementation

Fixed most of the stuff that you mentioned, :dougt. Hopefully the new files' license blocks and the new function names/signatures are okay.

Also, please note that I put in some placeholder code for the nsIDOMGeoPositionOptions stuff that is in Olli's patch (http://hg.mozilla.org/mozilla-central/rev/686e5bcf747b). This will be fixed after I integrate the latest moz-central, so I'll upload a new patch soon once that is finished, but I just wanted a review of the other content (and the basic concept of Olli's patch applied to this feature) in this patch mainly.

Comment 23

[Josh Dhaliwal (:Arreth)]

Attachment: getAddress-drafted-implementation

Almost forgot about getting rid of threadsafe addref/release on nsGeoAddress.cpp ;). Still no integration of Olli's patch yet, but it's coming along I swears.

Comment 24

[Doug Turner (:dougt)]

Comment on attachment 616906 [details] [diff] [review]
getAddress-drafted-implementation

Review of attachment 616906 [details] [diff] [review]:
-----------------------------------------------------------------

Please rebase and add tests.  I want to try it out.

::: dom/interfaces/geolocation/nsIDOMGeoAddressOptions.idl
@@ +1,1 @@
> +/* ***** BEGIN LICENSE BLOCK *****

We don't need this file any more, right?

::: js/xpconnect/src/dictionary_helper_gen.conf
@@ +11,5 @@
>       [ 'IDBObjectStoreParameters', 'nsIIDBDatabase.idl' ],
>       [ 'IDBIndexParameters', 'nsIIDBObjectStore.idl' ],
>       [ 'StorageEventInit', 'nsIDOMStorageEvent.idl' ],
> +     [ 'BlobPropertyBag', 'nsIDOMFile.idl' ],
> +     [ 'GeoAddressOptions', 'nsIDOMGeoGeolocation.idl']

fwiw, when you update - place GeoAddressOptions next to GeoPositionOptions.

::: xpcom/system/nsIGeocodingProvider.idl
@@ +23,5 @@
> +   * This must be called on the main thread
> +   */
> +  void getResultAddress(in nsIDOMGeoAddress address);
> +			
> +

too white space

Comment 25

[Josh Dhaliwal (:Arreth)]

Attachment: getAddress-drafted-implementation

I've got the patch updated to the version of moz-central now, so it should need one last check up.

Comment 26

[Josh Dhaliwal (:Arreth)]

Attachment: getAddress-drafted-implementation

just added contributor section to license headers for shameless plugs to look back on ;)

Comment 27

[Josh Dhaliwal (:Arreth)]

Attachment: getAddress-mochi-test

This is a patch with all the necessary changes in order to implement some mochi tests for the getAddress feature.

Comment 28

[Josh Dhaliwal (:Arreth)]

Attachment: getAddress-drafted-implementation

Added some changes to set a default timeout for geocoding requests, and to provide an alternative route for older phones (where geocoding doesn't function correctly) to retrieve an Address object.

Note: If an exception is thrown when no timeout is set, then the geocoding request can hang. This is why a default timeout is set.

Comment 29

[Tantek Çelik]

The given URL in the header (http://arreth.wordpress.com/2012/03/13/proposed-changes-to-geolocation-api-specification/) appears to propose far more than what an address is defined to be per vCard4, which is what we're using as the basis of the ContactsAPI for B2G;

https://wiki.mozilla.org/WebAPI/ContactsAPI

If anyone would like to extend/grow the definition to add more address properties, please join the VCARDDAV list and propose it there:

https://www.ietf.org/mailman/listinfo/vcarddav

Thanks.

Comment 30

[Josh Dhaliwal (:Arreth)]

Hey Tantek,

The Address data structure from my proposed changes to the GeolocationAPI are based off of an addressing format that is used by the Android platform geocoding API. As per their description of it at (http://developer.android.com/reference/android/location/Address.html):

"The addres format is a simplified version of xAL (eXtensible Address Language) http://www.oasis-open.org/committees/ciq/ciq.html#6"

This format is apparently a standard civic addressing format that works for most locales worldwide. This is why I chose this addressing format.

What you have in your vCard addressing format for the ContactsAPI is a format that enacts a subset of the addresses that are available to be represented by this new addressing format in the GeolocationAPI.

Basically, you should be able to find what you need to fill in your addressing format from this civic addressing format. They are compatible together.

However, I will look into proposing changes to the vCard addressing format when I have some spare time, as I believe it's important to be inclusive of global addressing formats. 

Thanks! Hopefully this helps :)

Comment 31

[Josh Dhaliwal (:Arreth)]

TL;DR: 

A quick walkthrough of the new addressing format:

streetAddress = Thoroughfare (street name) + Sub-Thoroughfare (street number)
locality = Locality
region = Administration Area (state)
postalCode = Postal Code
countryName = Country 

*note: "Country" is in the format of the 2 letter standard country name abbreviation

Comment 32

[Curtis Koenig [:curtisk-use curtis.koenig+bzATgmail.com]]]

We need resolution on the follow-up tasks on the privacy review (https://wiki.mozilla.org/Privacy/Reviews/GeolocationAPI) then we can remove the flag on this bug.

Comment 33

[Doug Turner (:dougt)]

patches needed to be updated.  it is a low priority for me.  So, until there is progress here, we do not need to do anything wrt the privacy review.

Comment 34

[Doug Turner (:dougt)]

Comment on attachment 617929 [details] [diff] [review]
getAddress-mochi-test

doesn't apply

Comment 35

[Andrew Overholt [:overholt]]

I suspect this was a mistaken basecamp nom so I'm clearing the flag.

Comment 36

[Doug Turner (:dougt)]

We are not implementing civic addresses.