Closed Bug 850442 Opened 7 years ago Closed 7 years ago

Convert Geolocation to WebIDL

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla23

People

(Reporter: ggp, Assigned: ggp)

References

(Blocks 1 open bug)

Details

Attachments

(6 files, 11 obsolete files)

6.00 KB, patch
ggp
: review+
Details | Diff | Splinter Review
19.43 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
9.71 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
22.79 KB, text/plain
Details
4.20 KB, patch
ggp
: review+
Details | Diff | Splinter Review
35.98 KB, patch
ggp
: review+
Details | Diff | Splinter Review
No description provided.
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.
Attached patch Part 2 - Convert PositionError (obsolete) — Splinter Review
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.
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.
Attachment #729043 - Attachment is patch: true
Attached patch Part 2 - Convert PositionError (obsolete) — Splinter Review
Attachment #729042 - Attachment is obsolete: true
Attachment #729043 - Attachment is obsolete: true
Attachment #729044 - Attachment is obsolete: true
Attachment #729866 - Flags: review?(bzbarsky)
Attachment #729867 - Flags: review?(bzbarsky)
Attachment #729868 - Flags: review?(bzbarsky)
Attachment #729872 - Flags: review?(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?
Attachment #729866 - Flags: review?(bzbarsky) → review-
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....
(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 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.
Attachment #729867 - Flags: review?(bzbarsky) → review+
Comment on attachment 729867 [details] [diff] [review]
Part 2 - Convert PositionError

Oh, and r=me with the nits.
> ... 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 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 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;
  };
Attachment #729868 - Flags: review?(bzbarsky) → review-
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.
Attachment #729872 - Flags: review?(bzbarsky) → review+
(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.
(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?
> 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.
Attachment #729866 - Attachment is obsolete: true
Attachment #732551 - Flags: review?(bzbarsky)
Just addressed the comments for the previous patch. Keeping r+ from bz.
Attachment #729867 - Attachment is obsolete: true
Attachment #732552 - Flags: review+
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.
Attachment #729868 - Attachment is obsolete: true
Attachment #729872 - Attachment is obsolete: true
Attachment #732554 - Flags: review?(bzbarsky)
Attachment #732555 - Flags: review?(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.  ;)
Attachment #732551 - Flags: review?(doug.turner)
Attachment #732551 - Flags: review?(bzbarsky)
Attachment #732551 - Flags: review-
Comment on attachment 732555 [details] [diff] [review]
Part 4 - Make nsIDOMGeoGeolocation non-scriptable

r=me
Attachment #732555 - Flags: review?(bzbarsky) → review+
Comment on attachment 732556 [details] [diff] [review]
Part 5 - Rename nsGeolocation to Geolocation

Lovely, thanks!
Attachment #732556 - Flags: review?(bzbarsky) → review+
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 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.
Attachment #732551 - Flags: review?(doug.turner) → review-
(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?
(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.
I talked to Doug in person and he's ok with the changes explained in the previous comment. Fixed other comments.
Attachment #732551 - Attachment is obsolete: true
Attachment #733029 - Flags: review?(bzbarsky)
Attached file Interdiff for part 3
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.
Attachment #733029 - Flags: review?(bzbarsky) → review+
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!
Attachment #732554 - Flags: review?(bzbarsky) → review+
Blocks: 858178
I missed one test. Replacing nsIDOMGeoGeolocation with nsISupports in test_geolocation.xul and keeping r+ from bz.
Attachment #732555 - Attachment is obsolete: true
Attachment #734049 - Flags: review+
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+.
Attachment #732554 - Attachment is obsolete: true
Attachment #736468 - Flags: review+
(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..."
Follow to remove classinfo too, please.
You need to log in before you can comment on or make changes to this bug.