909768 - Remove idl::GeoPositionOptions

Status: RESOLVED FIXED

(Core :: DOM: Geolocation, defect)

Description

[Boris Zbarsky [:bzbarsky]]

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

Comment 1

[Josh Matthews [:jdm]]

http://mxr.mozilla.org/mozilla-central/search?string=idl%3A%3Ageoposi

Comment 2

[Anup Kumar [:reznord]]

Hi, 

I am willing to work on this bug. I request you to assign it to me.

Thanks in Advance,

Regards,
A. Anup

Comment 3

[Anup Kumar [:reznord]]

Attachment: Changes made according to the bug

Comment 4

[Anup Kumar [:reznord]]

Attachment: Changes made according to the bug

Comment 5

[Anup Kumar [:reznord]]

Attachment: changes made according to the bug description.

Comment 6

[Josh Matthews [:jdm]]

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.

Comment 7

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 8

[Anup Kumar [:reznord]]

> 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 9

[Anup Kumar [:reznord]]

Attachment: changes made

Comment 10

[Anup Kumar [:reznord]]

Attachment: bug_909768

Comment 11

[Anup Kumar [:reznord]]

Attachment: bug_909768

Comment 12

[Josh Matthews [:jdm]]

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 13

[Anup Kumar [:reznord]]

Attachment: bug_909768

Comment 14

[Josh Matthews [:jdm]]

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.

Comment 15

[Anup Kumar [:reznord]]

 
> Please be more careful when making changes, this should be
> mozilla::dom::PositionOptions.


I am sorry just forgot in a overlook

Comment 16

[Anup Kumar [:reznord]]

Attachment: ?dl=2935634

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

Comment 17

[Josh Matthews [:jdm]]

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?

Comment 18

[Anup Kumar [:reznord]]

Attachment: ?dl=2935634

Checked and verified.

Comment 19

[Josh Matthews [:jdm]]

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.

Comment 20

[Anup Kumar [:reznord]]

Attachment: ?dl=2935634

Verified and patched

Comment 21

[mjh563]

Attachment: updated patch

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.

Comment 22

[Josh Matthews [:jdm]]

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.

Comment 23

[Boris Zbarsky [:bzbarsky]]

WebIDL dictionaries have deleted copy constructors.

Comment 24

[mjh563]

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

Comment 25

[Boris Zbarsky [:bzbarsky]]

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

Comment 26

[mjh563]

Attachment: updated patch

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

Comment 27

[Josh Matthews [:jdm]]

Comment on attachment 800944 [details] [diff] [review]
updated patch

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

Great. Thank you!

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/fc3a3227fc82

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/fc3a3227fc82