850442 - Convert Geolocation to WebIDL

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Comment 1

[Guilherme Gonçalves [:ggp]]

Attachment: Wrap nsGeoPosition and nsGeoPositionCoords into Position and Coordinates

nsGeoPosition and nsGeoPositionCoords need to be threadsafe, so I couldn't
set them up with cycle collection. My solution was to wrap them into the Position
and Coordinates objects, which use the WebIDL bindings. If there's a better way
to do this, I'd love to hear about it.

Comment 2

[Guilherme Gonçalves [:ggp]]

Attachment: Part 2 - Convert PositionError

Although nsDOMGeoPositionError had threadsafe refcounting, it didn't seem
necessary. I converted it the usual way and left an assertion that it's only
instantiated in the main thread.

Comment 3

[Guilherme Gonçalves [:ggp]]

Attachment: Part 3 - Convert callbacks, Geolocation and Position

Comment 4

[Guilherme Gonçalves [:ggp]]

Attachment: Part 4 - Fix test_optional_api_params

The new bindings throw TypeError for missing parameters, which wasn't expected by the test. With this patch, all mochitest plain and xpcshell tests pass.

Comment 5

[Guilherme Gonçalves [:ggp]]

Attachment: Part 1 - Wrap nsGeoPosition and nsGeoPositionCoords into Position and Coordinates

Comment 6

[Guilherme Gonçalves [:ggp]]

Attachment: Part 2 - Convert PositionError

Comment 7

[Guilherme Gonçalves [:ggp]]

Attachment: Part 3 - Convert callbacks, Geolocation and Position

Comment 8

[Guilherme Gonçalves [:ggp]]

Attachment: Part 4 - Fix test_optional_api_params

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 729866 [details] [diff] [review]
Part 1 - Wrap nsGeoPosition and nsGeoPositionCoords into Position and Coordinates

Is it really expected that .coords will return a new object each time?  Seems odd, even if the spec doesn't say it's always the same object...  r- for this part, since I bet it's wrong.

Are we guaranteed that mGeoPosition->GetCoords never returns null?  Code inspection doesn't make that clear.

> +#define GENERATE_COORDS_WRAPPED_GETTER(name) \

Probably better to line up the backslashes here (and I'm a bit surprised your editor didn't automatically do that).

>+#define GENERATE_COORDS_WRAPPED_GETTER_NULLABLE(name) \

Is there a followup bug on the fact that we're not returning null when we probably should be?

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 729866 [details] [diff] [review]
Part 1 - Wrap nsGeoPosition and nsGeoPositionCoords into Position and Coordinates

Oh, and also, why are these [NoInterfaceObject]?  That will keep showing the XPCOM "GeoPosition" on the global, say, which looks very wrong.

If the spec really says there should be no window.GeoPosition, then we should fix that to be the case, but I can see that being a followup....

Comment 11

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

(In reply to Boris Zbarsky (:bz) from comment #10)
> Comment on attachment 729866 [details] [diff] [review]
> Part 1 - Wrap nsGeoPosition and nsGeoPositionCoords into Position and
> Coordinates
> 
> Oh, and also, why are these [NoInterfaceObject]?  That will keep showing the
> XPCOM "GeoPosition" on the global, say, which looks very wrong.
> 
> If the spec really says there should be no window.GeoPosition, then we
> should

... fix the spec ;)

Comment 12

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 729867 [details] [diff] [review]
Part 2 - Convert PositionError

As long as you're changing just about every line of this class, why not just rename it to mozilla::dom::PositionError and drop the nativeType bit?

>+  void GetMessage(nsString& aRetVal) const {
>+    aRetVal = NS_LITERAL_STRING("");
>+  }

  aRetVal.Truncate();

or just do nothing, if desired, since caller will pass in an empty string anyway.  But the Truncate() is clearer.

Again, we should probably change things so that window.GeoPositionError does not exist, but at least the interface name is different from the WebIDL one.  Though this one being NoInterfaceObject is even more daft, given the named constants... ah, well.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 729867 [details] [diff] [review]
Part 2 - Convert PositionError

Oh, and r=me with the nits.

Comment 14

[Boris Zbarsky [:bzbarsky]]

> ... fix the spec ;)

Good luck with that, given that it's a PR and people don't seem to have bothered writing an actual comprehensive test suite.  :(

Comment 15

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 729867 [details] [diff] [review]
Part 2 - Convert PositionError

Oh, and please make sure to not check in the diffstat gunk in the commit message.  That applies to all of these patches.

Comment 16

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 729868 [details] [diff] [review]
Part 3 - Convert callbacks, Geolocation and Position

> +class nsGeolocation;

Can we rename the class to just mozilla::dom::Geolocation (and perhaps rename the header file accordingly)?  Followup patch on top of these is fine.

>+[ptr] native NamespacedGeoPositionOptions(mozilla::idl::GeoPositionOptions);

I have to ask.  Why is this needed?  Why can't you just use the WebIDL dictionary?

>+[scriptable, builtinclass, uuid(1bc7d103-c7ae-4467-881c-21a8dfa17938)]

Why scriptable?  We don't allow either calling it or implementing it from script....

>+GeoPositionOptionsFromPositionOptions(const PositionOptions& aOptions)

The callers of this function leak the returned struct.  Yet more reasons to not have the two separate structs.

>+  if (aOptions.mEnableHighAccuracy.WasPassed()) {

This shouldn't be needed if the IDL is fixed to not be buggy.  More on this below; we should raise spec issues...

>+  geoOptions->maximumAge = std::numeric_limits<int32_t>::max();

That's not what the spec says to do, as far as I can tell.  Why the difference?

>+  geoOptions->timeout = 0;

Likewise.  I think you just got these backwards, in fact.

>+nsDOMGeoPositionError::NotifyCallback(GeoPositionErrorCallback& aCallback)

Is there a reason that's not a const reference?

> nsGeolocationRequest::nsGeolocationRequest(nsGeolocation* aLocator,
>+                                           GeoPositionCallback& aCallback,
>+                                           GeoPositionErrorCallback& aErrorCallback,

Likewise.

>@@ -541,7 +560,19 @@ nsGeolocationRequest::SendLocation(nsIDOMGeoPosition* aPosition)
>+    nsRefPtr<Position> pos = new Position(mLocator, aPosition);

This always calls the callback with a new object.  Per spec, getCurrentPosition should call it with an existing Position in some cases but not others (which is somewhat daft, so maybe this is a spec bug)...

>+nsGeolocation::GetCurrentPosition(OwningNonNull<PositionCallback> aCallback,

This silently swallows exceptions from the nsresult version of GetCurrentPosition.  That seems wrong, on its face.  Do we not have tests for those cases (e.g. for the MAX_GEO_REQUESTS_PER_WINDOW handling)?  :(

Also, the argument here should just be a |PositionCallback& aCallback|.

>+nsGeolocation::WatchPosition(OwningNonNull<PositionCallback> aCallback,

Again, |PositionCallback& aCallback|.  And again, this silently eats exceptions.

>+  NS_DECL_CYCLE_COLLECTION_SCRIPT_HOLDER_CLASS_AMBIGUOUS(nsGeolocation, nsIDOMGeoGeolocation)

This one isn't ambiguous; drop the ambiguous bits, please.

>+  virtual JSObject* WrapObject(JSContext *aCtx, JSObject *aScope);

MOZ_OVERRIDE, please.  Same for the other patches if I missed it there.

>+dictionary PositionOptions {
>+  boolean enableHighAccuracy;
>+  long timeout;
>+  long maximumAge;
>+};

That should probably be:

  dictionary PositionOptions {
    boolean enableHighAccuracy = false;
    long timeout = 0xffffffff;
    long maximumAge = 0;
  };

Comment 17

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 729872 [details] [diff] [review]
Part 4 - Fix test_optional_api_params

r=me, but seems like this should be folded into part 3 to avoid having broken changesets in the tree.

Comment 18

[Guilherme Gonçalves [:ggp]]

(In reply to Boris Zbarsky (:bz) from comment #9)
> Comment on attachment 729866 [details] [diff] [review]
> Part 1 - Wrap nsGeoPosition and nsGeoPositionCoords into Position and
> Coordinates
> 
> Are we guaranteed that mGeoPosition->GetCoords never returns null?  Code
> inspection doesn't make that clear.

It does look possible for GetCoords() to return null, if for some reason we get an nsGeoPosition with null mCoords for the first update. I'll add a null check for coordinates along with position to nsGeolocationRequest::SendLocation, and an assertion to mGeoPosition->GetCoords() on the next version of the patch.

Comment 19

[Guilherme Gonçalves [:ggp]]

(In reply to Boris Zbarsky (:bz) from comment #16)
> Comment on attachment 729868 [details] [diff] [review]
> Part 3 - Convert callbacks, Geolocation and Position
> 
> >+[scriptable, builtinclass, uuid(1bc7d103-c7ae-4467-881c-21a8dfa17938)]
> 
> Why scriptable?  We don't allow either calling it or implementing it from
> script....
> 

This seems necessary for the xpcshell tests under dom/tests/unit/

> >+nsGeolocation::GetCurrentPosition(OwningNonNull<PositionCallback> aCallback,
> 
> This silently swallows exceptions from the nsresult version of
> GetCurrentPosition.  That seems wrong, on its face.  Do we not have tests
> for those cases (e.g. for the MAX_GEO_REQUESTS_PER_WINDOW handling)?  :(

We don't seem to have tests for that.

Since the spec doesn't seem to mention exceptions, should I just add [Throws] to the webidl files and throw the same exceptions that were previously there?

Comment 20

[Boris Zbarsky [:bzbarsky]]

> This seems necessary for the xpcshell tests under dom/tests/unit/

Oh, just to get it to show up under Ci.whatever?  Blah.  :(

Can you at least mark the (stealth-noscript) methods noscript, please?

> should I just add [Throws] to the webidl files and throw the same exceptions that were
> previously there?

That seems like the simplest thing, yes: just try to not change the behavior.

Comment 21

[Guilherme Gonçalves [:ggp]]

Attachment: Part 1 - Wrap nsGeoPosition and nsGeoPositionCoords into Position and Coordinates

Comment 22

[Guilherme Gonçalves [:ggp]]

Attachment: Part 2 - Convert PositionError

Just addressed the comments for the previous patch. Keeping r+ from bz.

Comment 23

[Guilherme Gonçalves [:ggp]]

Attachment: Part 3 - Convert callbacks, Geolocation and Position

Includes fix for test_optional_api_params.

I fixed the problem of creating a new Position wrapper for every invocation of the callbacks by caching the wrapper in the nsGeolocation object, and using it whenever the nsGeoPosition we're about to pass to the callback matches the one inside the wrapper. I also included a test for this in this patch.

Comment 24

[Guilherme Gonçalves [:ggp]]

Attachment: Part 4 - Make nsIDOMGeoGeolocation non-scriptable

Comment 25

[Guilherme Gonçalves [:ggp]]

Attachment: Part 5 - Rename nsGeolocation to Geolocation

Comment 26

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 732551 [details] [diff] [review]
Part 1 - Wrap nsGeoPosition and nsGeoPositionCoords into Position and Coordinates

>+NS_IMPL_CYCLE_COLLECTION_WRAPPERCACHE_1(Position, mParent)

What about mCoordinates?  Need to do that too.

>+already_AddRefed<Coordinates>
>+Position::Coords()

This method looks wrong.  In particular, mCoordinates is never written to.  I suggest adding tests that would have caught that....

What I recommend doing here is making this method return a Coordinates* and giving it this general structure:

  if (!mCoordinates) {
    // Do stuff to set mCoordinates
  }
  return mCoordinates;

>+#define GENERATE_COORDS_WRAPPED_GETTER_NULLABLE(name) \

Again, is there a followup on us not returning null when we should?  If not, please file one.

>+++ b/dom/src/geolocation/nsGeolocation.cpp

I have no idea why this change is being made...  Someone familiar with the geolocation code should review this part.

I'd really appreciate an interdiff in addition to the actual patch with the next patch version, so I don't have to keep worrying about the boilerplate.  ;)

Comment 27

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 732555 [details] [diff] [review]
Part 4 - Make nsIDOMGeoGeolocation non-scriptable

r=me

Comment 28

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 732556 [details] [diff] [review]
Part 5 - Rename nsGeolocation to Geolocation

Lovely, thanks!

Comment 29

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 732554 [details] [diff] [review]
Part 3 - Convert callbacks, Geolocation and Position

Would it be possible to put up an interdiff between the last part 3 I reviewed and this?  Would make review a lot easier....  If not, I can manage, but if it's possible it would be nice.

Comment 30

[Doug Turner (:dougt)]

Comment on attachment 732551 [details] [diff] [review]
Part 1 - Wrap nsGeoPosition and nsGeoPositionCoords into Position and Coordinates

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

::: dom/src/geolocation/nsGeolocation.cpp
@@ +534,5 @@
> +  aPosition->GetCoords(getter_AddRefs(coords));
> +  if (!coords) {
> +    NotifyError(nsIDOMGeoPositionError::POSITION_UNAVAILABLE);
> +    return;
> +  }

ggc, are you making this change because if a position doesn't have coords, we should just report unavailable?

I am suspecting that is the case. A long time ago, we also supported something similar to a civic address.  You could have a coords or one of these civic address or both.

Comment 31

[Guilherme Gonçalves [:ggp]]

(In reply to Doug Turner (:dougt) from comment #30)
> Comment on attachment 732551 [details] [diff] [review]
> Part 1 - Wrap nsGeoPosition and nsGeoPositionCoords into Position and
> Coordinates
> 
> Review of attachment 732551 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/src/geolocation/nsGeolocation.cpp
> @@ +534,5 @@
> > +  aPosition->GetCoords(getter_AddRefs(coords));
> > +  if (!coords) {
> > +    NotifyError(nsIDOMGeoPositionError::POSITION_UNAVAILABLE);
> > +    return;
> > +  }
> 
> ggc, are you making this change because if a position doesn't have coords,
> we should just report unavailable?
> 
> I am suspecting that is the case. A long time ago, we also supported
> something similar to a civic address.  You could have a coords or one of
> these civic address or both.

I was trying to address an issue bz raised in the previous review: that mGeoPosition->GetCoords() might return null, and Position::Coords did nothing about it.

These lines make sure a Position wrapper is only created when the coordinates are non-null. I guess the fact that the code that creates the Position itself after this null-check is hidden in the third patch didn't really help, sorry about that.

I'm not sure what to do with this, then. So is a Position with null Coordinates actually valid?

Comment 32

[Guilherme Gonçalves [:ggp]]

(In reply to Boris Zbarsky (:bz) from comment #29)
> Comment on attachment 732554 [details] [diff] [review]
> Part 3 - Convert callbacks, Geolocation and Position
> 
> Would it be possible to put up an interdiff between the last part 3 I
> reviewed and this?  Would make review a lot easier....  If not, I can
> manage, but if it's possible it would be nice.

Doesn't seem to be possible this time, sorry. I'll do it for the next versions.

Comment 33

[Guilherme Gonçalves [:ggp]]

Attachment: Part 1 - Wrap nsGeoPosition and nsGeoPositionCoords into Position and Coordinates

I talked to Doug in person and he's ok with the changes explained in the previous comment. Fixed other comments.

Comment 34

[Guilherme Gonçalves [:ggp]]

Attachment: Interdiff for part 3

Comment 35

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 733029 [details] [diff] [review]
Part 1 - Wrap nsGeoPosition and nsGeoPositionCoords into Position and Coordinates

r=me

Please do file those followups and tests and whatnot.

Comment 36

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 732554 [details] [diff] [review]
Part 3 - Convert callbacks, Geolocation and Position

r=me.  Thank you very much for the interdiff; it made reviewing this a whole lot more pleasant!  And thank you for catching the 0x7fffffff vs 0xffffffff!

Comment 37

[Guilherme Gonçalves [:ggp]]

Attachment: Part 4 - Make nsIDOMGeoGeolocation non-scriptable

I missed one test. Replacing nsIDOMGeoGeolocation with nsISupports in test_geolocation.xul and keeping r+ from bz.

Comment 38

[Guilherme Gonçalves [:ggp]]

Attachment: Part 3 - Convert callbacks, Geolocation and Position

test_cachedPosition.html assumed no new positions would be cached after calling stop_geolocationProvider(). In practice that could happen and two different Position objects would be assumed to be the cached one, so the test would fail intermittently on try.

This new version of the patch fixes the test watchPosition first to stop the geolocation service, and only after a timeout (which means no new positions are being received) is the cache assumed to be stable, and then the tests are run.

Also, the same test has been disabled on android and b2g -- apparently no other geolocation tests run on these platforms either, and I was getting failures there.

This new version of the patch should now pass all tests. Since it's a test fix, I'm keeping bz's r+.

Comment 39

[Guilherme Gonçalves [:ggp]]

(In reply to Guilherme Gonçalves [:ggp] from comment #38) 
> This new version of the patch fixes the test watchPosition first to stop

Sorry, "...fixes the test so it uses watchPosition..."

Comment 40

[John Schoenick [:johns]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/cbfbde1ca247
https://hg.mozilla.org/integration/mozilla-inbound/rev/394ca10bb287
https://hg.mozilla.org/integration/mozilla-inbound/rev/37b472c847b1
https://hg.mozilla.org/integration/mozilla-inbound/rev/23e907c7c106
https://hg.mozilla.org/integration/mozilla-inbound/rev/f36ce9b95a0c

Comment 41

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/cbfbde1ca247
https://hg.mozilla.org/mozilla-central/rev/394ca10bb287
https://hg.mozilla.org/mozilla-central/rev/37b472c847b1
https://hg.mozilla.org/mozilla-central/rev/23e907c7c106
https://hg.mozilla.org/mozilla-central/rev/f36ce9b95a0c

Comment 42

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Follow to remove classinfo too, please.