Closed Bug 909768 Opened 6 years ago Closed 6 years ago

Remove idl::GeoPositionOptions

Categories

(Core :: DOM: Geolocation, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla26

People

(Reporter: bzbarsky, Assigned: mjh563)

References

Details

(Whiteboard: [mentor=jdm][lang=c++])

Attachments

(1 file, 11 obsolete files)

We should be able to use dom::GeoPositionOptions instead.
Component: DOM → Geolocation
Whiteboard: [mentor=jdm][lang=c++]
Hi, 

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

Thanks in Advance,

Regards,
A. Anup
Assignee: nobody → allamsetty.anup
Attachment #797289 - Flags: review?(josh)
Attachment #797289 - Attachment is obsolete: true
Attachment #797289 - Flags: review?(josh)
Attachment #797294 - Flags: review?(josh)
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.
Attachment #797459 - Flags: review?(josh) → feedback+
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.
Keywords: checkin-needed
> 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
Attached patch changes made (obsolete) — Splinter Review
Attachment #797294 - Attachment is obsolete: true
Attachment #797459 - Attachment is obsolete: true
Attachment #797294 - Flags: review?(josh)
Attachment #797896 - Flags: review?(josh)
Attached patch bug_909768 (obsolete) — Splinter Review
Attachment #797896 - Attachment is obsolete: true
Attachment #797896 - Flags: review?(josh)
Attached patch bug_909768 (obsolete) — Splinter Review
Attachment #798263 - Attachment is obsolete: true
Attachment #798264 - Flags: review?(josh)
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.
Attachment #798264 - Flags: review?(josh) → feedback+
Attached patch bug_909768 (obsolete) — Splinter Review
Attachment #798264 - Attachment is obsolete: true
Attachment #798402 - Flags: review?(josh)
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.
Attachment #798402 - Flags: review?(josh)
 
> Please be more careful when making changes, this should be
> mozilla::dom::PositionOptions.


I am sorry just forgot in a overlook
Attached patch ?dl=2935634 (obsolete) — Splinter Review
Sorry for that, I think I need to be even more careful while submitting patches and stuff
Attachment #798402 - Attachment is obsolete: true
Attachment #798535 - Flags: review?(josh)
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?
Attachment #798535 - Flags: review?(josh)
Attached patch ?dl=2935634 (obsolete) — Splinter Review
Checked and verified.
Attachment #798535 - Attachment is obsolete: true
Attachment #798716 - Flags: review?(josh)
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.
Attachment #798716 - Flags: review?(josh) → feedback+
Attached patch ?dl=2935634 (obsolete) — Splinter Review
Verified and patched
Attachment #798716 - Attachment is obsolete: true
Attachment #798879 - Flags: review?(josh)
Assignee: allamsetty.anup → mjh563
Attached patch updated patch (obsolete) — Splinter Review
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.
Attachment #798879 - Attachment is obsolete: true
Attachment #798879 - Flags: review?(josh)
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?
Flags: needinfo?(bzbarsky)
I think what you have right now is probably fine, actually, though I'd rename it to CreatePositionOptionsCopy or something.
Flags: needinfo?(bzbarsky)
Attached patch updated patchSplinter Review
Made the change suggested by bz, so I think this is now ready to be reviewed.
Attachment #800875 - Attachment is obsolete: true
Attachment #800944 - Flags: review?(josh)
Comment on attachment 800944 [details] [diff] [review]
updated patch

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

Great. Thank you!
Attachment #800944 - Flags: review?(josh) → review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/fc3a3227fc82
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
You need to log in before you can comment on or make changes to this bug.