We should be able to use dom::GeoPositionOptions instead.

Hi, I am willing to work on this bug. I request you to assign it to me. Thanks in Advance, Regards, A. Anup

Comment on attachment 797459 [details] [diff] [review] changes made according to the bug description. Review of attachment 797459 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/interfaces/geolocation/nsIDOMGeoGeolocation.idl @@ +13,2 @@ > > dictionary GeoPositionOptions Please delete this; this would have caught the places below where we change idl::GeoPositionOptions to GeoPositionOptions instead of PositionOptions.

This doesn't have review+ yet. It can't land until it does. Also, please make sure that all obsolete patches are marked as such.

> This doesn't have review+ yet. It can't land until it does. Also, please sorry I didn't see that > make sure that all obsolete patches are marked as such. Ok I will do it

Comment on attachment 798264 [details] [diff] [review] bug_909768 Review of attachment 798264 [details] [diff] [review]: ----------------------------------------------------------------- Nearly there! A couple small changes and this will be ready. ::: dom/interfaces/geolocation/nsIDOMGeoGeolocation.idl @@ +10,5 @@ > %{C++ > #include "DictionaryHelpers.h" > %} > > +[ptr] native NamespacedGeoPositionOptions(mozilla::PositionOptions); This should be mozilla::dom::PositionOptions. ::: dom/src/geolocation/nsGeolocation.h @@ +176,5 @@ > > ~Geolocation(); > > + nsresult GetCurrentPosition(GeoPositionCallback& aCallback, GeoPositionErrorCallback& aErrorCallback, mozilla::PositionOptions* aOptions); > + nsresult WatchPosition(GeoPositionCallback& aCallback, GeoPositionErrorCallback& aErrorCallback, mozilla::PositionOptions* aOptions, int32_t* aRv); These can drop the mozilla:: prefix.

Comment on attachment 798402 [details] [diff] [review] bug_909768 Review of attachment 798402 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/interfaces/geolocation/nsIDOMGeoGeolocation.idl @@ +10,5 @@ > %{C++ > #include "DictionaryHelpers.h" > %} > > +[ptr] native NamespacedGeoPositionOptions(mozilla::dom::GeoPositionOptions); Please be more careful when making changes, this should be mozilla::dom::PositionOptions.

> Please be more careful when making changes, this should be > mozilla::dom::PositionOptions. I am sorry just forgot in a overlook

Sorry for that, I think I need to be even more careful while submitting patches and stuff

Comment on attachment 798535 [details] [diff] [review] ?dl=2935634 Review of attachment 798535 [details] [diff] [review]: ----------------------------------------------------------------- There are still references to GeoPositionOptions in this patch. Can you delete http://mxr.mozilla.org/mozilla-central/source/js/xpconnect/src/dictionary_helper_gen.conf#7 and try rebuilding?

Checked and verified.

Comment on attachment 798716 [details] [diff] [review] ?dl=2935634 Sorry to keep asking for new revisions. There are three changes I would like to see: 1) please include the deletion in I asked for in comment 17 2) PositionOptionsFromPositionOptions is redundant. Please remove the function and all of its callers; just use the argument that used to be passed instead. 3) In the commit message, remove "in nsGeolocation.cpp" and add the dom:: prefix to the PositionOptions.

Verified and patched

This updated patch fixes the build errors, but it doesn't remove the GeoPositionOptionsFromPositionOptions function because I'm not sure how best to do that. The problem is that the function takes a const PositionOptions& argument but returns a non-const PositionOptions*. So it can't be replaced by just using the argument, because that results in an invalid conversion from const to non-const.

If you look at the way the argument is used, we should be able to just pass |new PositionOptions(aOptions)| in place of calling PositionOptionsFromGeoPositionOptions.

WebIDL dictionaries have deleted copy constructors.

So what would be the best thing to do here, do you think?

I think what you have right now is probably fine, actually, though I'd rename it to CreatePositionOptionsCopy or something.

Made the change suggested by bz, so I think this is now ready to be reviewed.

Comment on attachment 800944 [details] [diff] [review] updated patch Review of attachment 800944 [details] [diff] [review]: ----------------------------------------------------------------- Great. Thank you!