Allow Addons the use of the DOM Geolocation API

RESOLVED FIXED

Status

()

Core
Geolocation
RESOLVED FIXED
9 years ago
8 years ago

People

(Reporter: dougt, Assigned: dougt)

Tracking

({dev-doc-complete})

unspecified
dev-doc-complete
Points:
---

Firefox Tracking Flags

(status1.9.2 beta3-fixed, fennec1.0-)

Details

Attachments

(2 attachments, 3 obsolete attachments)

(Assignee)

Description

9 years ago
Right now, addon developers have to use a different API than those developers building websites.  It would be great if these two APIs were the same.

Webdevelopers use:
https://developer.mozilla.org/En/Using_geolocation

Addons use:

function foo(position) {}

var geoprovider = Cc["@mozilla.org/geolocation/provider;1"].getService(Ci.nsIGeolocationProvider);
geoprovider.startup();
geoprovider.watch(foo);
As long as chrome code doesn't need to access the geolocation object via the navigator object.
(Assignee)

Comment 2

9 years ago
right, there should be a separate way to get the geolocation object.
(Assignee)

Updated

9 years ago
Assignee: nobody → doug.turner
tracking-fennec: --- → ?
OS: Windows XP → All
Hardware: x86 → All
(Assignee)

Comment 3

9 years ago
Created attachment 398164 [details] [diff] [review]
patch v.1 (WIP)
(Assignee)

Comment 4

9 years ago
Created attachment 398165 [details]
example component

Sample Component

Comment 5

9 years ago
This patch works well for me (tested on Fennec).

Updated

9 years ago
tracking-fennec: ? → 1.0-
(Assignee)

Comment 6

9 years ago
Comment on attachment 398165 [details]
example component

createInstance should be getService.
(Assignee)

Updated

9 years ago
Attachment #398164 - Flags: review?(Olli.Pettay)

Comment 7

9 years ago
Comment on attachment 398164 [details] [diff] [review]
patch v.1 (WIP)

> PRBool
> nsGeolocation::OwnerStillExists()
> {
>   nsCOMPtr<nsPIDOMWindow> window = do_QueryReferent(mOwner);
> 
>-  if (!window)
>-    return PR_FALSE;
>-
>   if (window)
>   {
>     PRBool closed = PR_FALSE;
>     window->GetClosed(&closed);
>     if (closed)
>       return PR_FALSE;
>+
>+    nsPIDOMWindow* outer = window->GetOuterWindow();
>+    if (!outer || outer->GetCurrentInnerWindow() != window)
>+      return PR_FALSE;
>   }
> 
>-  nsPIDOMWindow* outer = window->GetOuterWindow();
>-  if (!outer || outer->GetCurrentInnerWindow() != window)
>-    return PR_FALSE;
>-
>   return PR_TRUE;

So if the content window has disappeared, you return true. Does look quite right.
Should you add
NS_ENSURE_TRUE(!mOwner || window, PR_FALSE);
Attachment #398164 - Flags: review?(Olli.Pettay) → review-
(Assignee)

Comment 8

9 years ago
well, if there is no window, then this object is being used by an addon/chrome.

Comment 9

9 years ago
 (In reply to comment #0)
> Right now, addon developers have to use a different API than those developers
> building websites.  It would be great if these two APIs were the same.
> 
> Webdevelopers use:
> https://developer.mozilla.org/En/Using_geolocation
> 
> Addons use:
> 
> function foo(position) {}
> 
> var geoprovider =
> Cc["@mozilla.org/geolocation/provider;1"].getService(Ci.nsIGeolocationProvider);
> geoprovider.startup();
> geoprovider.watch(foo);

FWIW, this was a glimmer of hope for me because I've been banging my head trying to get geolocation working with my addon.  Unfortunately, I don't think the original workaround of calling the nsIGeoLocationProvider directly actually works for addons that are using the wifi service.  If you look at the NetworkGeoLocation provider the watch method doesn't add callbacks to any list; it simply starts up the service if it's not running:

194     watch: function(c) {
195         LOG("watch called");
196         if (!this.wifi_service) {
197             this.wifi_service = Cc["@mozilla.org/wifi/monitor;1"].getService(Components.interfaces.nsIWifiMonitor);
198             this.wifi_service.startWatching(this);
199         }
200     },

When an position changes, it calls directly into the geolocation service:

309             var update = Cc["@mozilla.org/geolocation/service;1"].getService(Ci.nsIGeolocationUpdate);
310             update.update(newLocation);

I'd be happy to write this up as a separate bug if you'd like (assuming it is a bug).

Cheers,
Will Wagner
(Assignee)

Comment 10

9 years ago
Created attachment 405342 [details] [diff] [review]
patch v.1

w/ tests.
Attachment #398164 - Attachment is obsolete: true
Attachment #405342 - Flags: review?(Olli.Pettay)
Comment on attachment 405342 [details] [diff] [review]
patch v.1

>@@ -813,95 +815,124 @@ nsGeolocation::GetCurrentPosition(nsIDOM
>                                   nsIDOMGeoPositionErrorCallback *errorCallback,
>                                   nsIDOMGeoPositionOptions *options)
...
>+  if (!nsContentUtils::IsCallerChrome())
>+    return NS_ERROR_FAILURE;
Why this check? It makes it more difficult to use this API in C++.


>+NS_IMETHODIMP
>+nsGeolocation::WatchPosition(nsIDOMGeoPositionCallback *callback,
>+                             nsIDOMGeoPositionErrorCallback *errorCallback,
>+                             nsIDOMGeoPositionOptions *options,
>+                             PRInt32 *_retval NS_OUTPARAM)
...
>+  if (!nsContentUtils::IsCallerChrome())
>+    return NS_ERROR_FAILURE;
Same here.

>+<!--
>+  Test for Persistent Storage in chrome
>+  -->
Really?

Did you check whether window.navigator.geolocation works? Or if it doesn't work, why not? (Because it really should be made working. That is IMO more important than this bug, at least if chrome windows have .navigator object.)
(Assignee)

Comment 12

9 years ago
>>+  if (!nsContentUtils::IsCallerChrome())
>>+    return NS_ERROR_FAILURE;
>Why this check? It makes it more difficult to use this API in C++.


non-chrome callers should fail here -- we are double checking that only chrome callers will have a null mOwner.

I will fix up the comment in the test cases.
(Assignee)

Comment 13

9 years ago
window.navigator.geolocation in chrome windows isn't supported and is probably a separate feature request bug.  I want js components to be able to access geolocation data.
(Assignee)

Comment 14

9 years ago
Created attachment 405946 [details] [diff] [review]
patch v.2

same as last patch, but fixes comments in test cases
Attachment #405342 - Attachment is obsolete: true
Attachment #405946 - Flags: review?(Olli.Pettay)
Attachment #405342 - Flags: review?(Olli.Pettay)
(In reply to comment #12)
> >>+  if (!nsContentUtils::IsCallerChrome())
> >>+    return NS_ERROR_FAILURE;
> >Why this check? It makes it more difficult to use this API in C++.
> 
> 
> non-chrome callers should fail here -- we are double checking that only chrome
> callers will have a null mOwner.

What about C++ callers? Do they have to push chrome JS context to JS stack?
Or push null context.

If we're adding some API for components, it should be useable both from JS and
C++ components.
(Assignee)

Comment 17

9 years ago
i can drop the calls to:

+  if (!nsContentUtils::IsCallerChrome())
+    return NS_ERROR_FAILURE;


It it not required.  Olli, any other issues before I update the patch?
Comment on attachment 405946 [details] [diff] [review]
patch v.2


> nsGeolocation::nsGeolocation(nsIDOMWindow* aContentDom) 
> : mUpdateInProgress(PR_FALSE)
> {
>   // Remember the window
>-  nsCOMPtr<nsPIDOMWindow> window = do_QueryInterface(aContentDom);
>-  if (window)
>-    mOwner = do_GetWeakReference(window->GetCurrentInnerWindow());
>+  if (aContentDom) {
>+    nsCOMPtr<nsPIDOMWindow> window = do_QueryInterface(aContentDom);
>+    if (window)
>+      mOwner = do_GetWeakReference(window->GetCurrentInnerWindow());
This should somehow fail if window, or mOwner is null or if GetCurrentInnerWindow() returns null.

Maybe Init method?
(Assignee)

Comment 19

9 years ago
> This should somehow fail if window, or mOwner is null or if
GetCurrentInnerWindow() returns null.


How could that fail?  aContentDom is a strong reference passed from the calling function which isa DOM window, and don't all content windows have innerwindows?
I want to be pretty neurotic in this case to make sure things are right.

And no, not all the outer windows have inner windows all the time, like
during deletion. Those are the cases where we need to be extremely careful.
(Assignee)

Comment 21

8 years ago
Created attachment 411885 [details] [diff] [review]
patch v.3


I added a init() method.  from the nsGlobalWindow, we will NEVER call init without a content window.  from xpcom (e.g. addons), we will pass a null content window.
Attachment #405946 - Attachment is obsolete: true
Attachment #411885 - Flags: review?(Olli.Pettay)
Attachment #405946 - Flags: review?(Olli.Pettay)

Updated

8 years ago
Attachment #411885 - Flags: review?(Olli.Pettay) → review+
(Assignee)

Comment 22

8 years ago
http://hg.mozilla.org/mozilla-central/rev/f4c04bd52811
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
(Assignee)

Updated

8 years ago
Attachment #411885 - Flags: approval1.9.2?
Attachment #411885 - Flags: approval1.9.2? → approval1.9.2+
Comment on attachment 411885 [details] [diff] [review]
patch v.3

a192=beltzner; tests, and he even updated the reviewers' guide
(Assignee)

Comment 24

8 years ago
http://hg.mozilla.org/mozilla-central/rev/3264564c019b  (fixes return value from init(). r=dholbert via irc)

Comment 25

8 years ago
Does this respect the user setting the pref geo.enabled to false? Has there been any testing to insure there are no workarounds for this pref being set to false?

Comment 26

8 years ago
Do we need to reflect this in devmo somewhere?
Keywords: dev-doc-needed
(Assignee)

Comment 27

8 years ago
@wildcatray -

geo.enabled is a global setting and anyone accessing geolocation w/ this pref set to false will not be able to use geolocation.  http://mxr.mozilla.org/mozilla-central/search?string=sGeoEnabled

I do not understand your second question about workarounds.

@steffen

yes, that is a great idea.

Comment 28

8 years ago
(In reply to comment #27)
> @wildcatray -
> 
> geo.enabled is a global setting and anyone accessing geolocation w/ this pref
> set to false will not be able to use geolocation. 
> http://mxr.mozilla.org/mozilla-central/search?string=sGeoEnabled
> 
> I do not understand your second question about workarounds.
>

Has any effort been made to see if someone can craft an add-on that provides a way to "get around" the fact that the pref is set to false?
(Assignee)

Comment 29

8 years ago
addons can do anything -- they have full permissions.

Addons hosted on addons.mozilla.org go through a review processes.  Reviewers are suppose to look at privacy considerations.  For a outline of what they review, please see:

https://wiki.mozilla.org/Update:Editors/ReviewingGuide

And in particular:

https://wiki.mozilla.org/Update:Editors/ReviewingGuide#Reviewing_Geolocation_stuff

Updated

8 years ago
Depends on: 528259
This is documented now, currently tagged as becoming available in Gecko 1.9.3. Wrote it up before realizing this wasn't landed for 1.9.2. Oh well.

See:

https://developer.mozilla.org/En/Using_geolocation#The_geolocation_object
https://developer.mozilla.org/en/XPCOM_Interface_Reference/NsIDOMGeoGeolocation
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.