Closed Bug 735863 Opened 10 years ago Closed 5 years ago

Implement navigator.geolocation.getAddress()

Categories

(Core :: DOM: Geolocation, defect)

ARM
Android
defect
Not set
normal

Tracking

()

RESOLVED WONTFIX

People

(Reporter: Arreth, Assigned: Arreth)

References

()

Details

(Keywords: privacy-review-needed, Whiteboard: [sec-assigned:curtisk:actionitems])

Attachments

(3 files, 14 obsolete files)

2.19 KB, text/html
Details
23.74 KB, patch
dougt
: review-
Details | Diff | Splinter Review
87.01 KB, patch
dougt
: review-
Details | Diff | Splinter Review
Refer to blog post regarding changes in this bug's URL field.
Assignee: nobody → jdhaliwal
Attached patch drafted-implementation (obsolete) — Splinter Review
First draft of implementation. Only stubs for actual geocoding. Thoughts?
Attachment #607650 - Flags: feedback?(doug.turner)
Attached patch drafted-implementation (obsolete) — Splinter Review
added changes to not rely on permission prompt for geocoding, and did some cleanup
Attachment #607650 - Attachment is obsolete: true
Attachment #607650 - Flags: feedback?(doug.turner)
Attachment #607748 - Flags: feedback?(doug.turner)
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.
Attachment #607748 - Flags: feedback?(doug.turner) → feedback+
Attached patch drafted-implementation (obsolete) — Splinter Review
added nsGeoAddress class and class info to nsDOMClassInfo. Also added newly created files to Makefile.
Attachment #607748 - Attachment is obsolete: true
Attachment #608393 - Flags: feedback?(doug.turner)
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?
Attached file getAddress-test (obsolete) —
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.
Attachment #608535 - Flags: feedback?(doug.turner)
Cleaned up the patch. Hopefully no more whitespace abnormalities or unrelated files included.
Attachment #608393 - Attachment is obsolete: true
Attachment #608393 - Flags: feedback?(doug.turner)
Attachment #608553 - Flags: feedback?(doug.turner)
Attachment #608535 - Flags: feedback?(doug.turner) → feedback+
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.
Attachment #608553 - Flags: feedback?(doug.turner) → feedback-
(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...><
Added the missing files, and made the suggested changes from the previous comments.
Attachment #608553 - Attachment is obsolete: true
Attachment #608936 - Flags: feedback?(doug.turner)
>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.
@jdm: Thanks! I'll check it out :D
marking bug for security review
Sec Review Notes: https://wiki.mozilla.org/Securtity/Reviews/GeolocationWebAPI

will initiate privacy review per the action items from the review
Whiteboard: [secr:curtisk]
Whiteboard: [secr:curtisk] → [secr:curtisk:actionitems]
Attachment #608936 - Attachment is obsolete: true
Attachment #608936 - Flags: feedback?(doug.turner)
Attachment #611060 - Flags: review?(doug.turner)
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
Attachment #611060 - Attachment is obsolete: true
Attachment #611060 - Flags: review?(doug.turner)
Attachment #616259 - Flags: review?(doug.turner)
Attached file getAddress-test (obsolete) —
Attachment #608535 - Attachment is obsolete: true
Attached file getAddress-test
quick fix to test for readability.
Attachment #616261 - Attachment is obsolete: true
quick one line modification to get rid of unnecessary comment/operation.
Attachment #616259 - Attachment is obsolete: true
Attachment #616259 - Flags: review?(doug.turner)
Attachment #616276 - Flags: review?(doug.turner)
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.
Attachment #616276 - Flags: review?(doug.turner) → review-
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.
Attachment #616276 - Attachment is obsolete: true
Attachment #616820 - Flags: review?(doug.turner)
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.
Attachment #616820 - Attachment is obsolete: true
Attachment #616820 - Flags: review?(doug.turner)
Attachment #616906 - Flags: review?(doug.turner)
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
Attachment #616906 - Flags: review?(doug.turner) → review-
I've got the patch updated to the version of moz-central now, so it should need one last check up.
Attachment #616906 - Attachment is obsolete: true
Attachment #617289 - Flags: review?(doug.turner)
just added contributor section to license headers for shameless plugs to look back on ;)
Attachment #617289 - Attachment is obsolete: true
Attachment #617289 - Flags: review?(doug.turner)
Attachment #617292 - Flags: review?(doug.turner)
This is a patch with all the necessary changes in order to implement some mochi tests for the getAddress feature.
Attachment #617929 - Flags: review?(doug.turner)
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.
Attachment #617292 - Attachment is obsolete: true
Attachment #617292 - Flags: review?(doug.turner)
Attachment #618567 - Flags: review?(doug.turner)
Whiteboard: [secr:curtisk:actionitems] → [sec-assigned:curtisk:actionitems]
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.
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 :)
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
Flags: sec-review?(curtisk)
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.
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 on attachment 617929 [details] [diff] [review]
getAddress-mochi-test

doesn't apply
Attachment #617929 - Flags: review?(doug.turner) → review-
Attachment #618567 - Flags: review?(doug.turner) → review-
Flags: sec-review?(curtisk) → sec-review+
blocking-basecamp: --- → ?
I suspect this was a mistaken basecamp nom so I'm clearing the flag.
blocking-basecamp: ? → ---
We are not implementing civic addresses.
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.