437948 - Geolocation

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, enhancement, P2)

Description

[Christian Sejersen]

http://locationaware.org/wiki/Working_Draft
http://code.google.com/p/google-gears/wiki/GeolocationAPI

Comment 1

[Doug Turner (:dougt)]

Attachment: patch v.1

This is a first cut of geolocation.  As I mentioned in other places, it is implementing a subset of the google gears API and is subject to change.

There are a few things that still need to be done that I know about.  Included in the patch, there is a TODO file that describes what is left to do.  Some of them are not critical to fix, but rather nice-to-have.  I will fix up the whitespace in nsGeolocation.cpp in the next patch.

Btw, the NS_OSSO build flag which I am using hasn't landed yet and is being tracked by

Comment 2

[Doug Turner (:dougt)]

Attachment: mochitests

these are the start of the mochitests for geolocation.  dom/tests/mochitests/geolocation.

More will follow.

Comment 3

[Doug Turner (:dougt)]

in dom/src/geolocation/Makefile.in,
LOCAL_INCLUDES  = $(MOZ_GTK2_CFLAGS)
should be:
LOCAL_INCLUDES  += $(MOZ_GTK2_CFLAGS)

Comment 4

[Doug Turner (:dougt)]

Attachment: patch v.2

oh, looks like i also missed geolocation.properties.  Here is a new patch that includes everything.

Comment 5

[Doug Turner (:dougt)]

Comment on attachment 325428 [details] [diff] [review]
patch v.2

new patch coming up.  factoring out some of the servicy stuff from nsGeolocator into nsGeolocatorService.

Comment 6

[Doug Turner (:dougt)]

Attachment: w.i.p. patch....

factored a bunch of stuff out of the nsGeolocator class into a nsGeolocatorService class.  Need further cleanup, and testing of the nsIGeolocatorProvider stuff.

Comment 7

[Doug Turner (:dougt)]

Comment on attachment 325848 [details] [diff] [review]
w.i.p. patch....

(oh, and I am sure that the maemo stuff is now broken)

Comment 8

[Doug Turner (:dougt)]

Attachment: patch v.3

finished factoring stuff into the service.
maemo support.

Comment 9

[Doug Turner (:dougt)]

going to need to tweak the maemo impl over the weekend, nothing major.  The rest of it is basically ready to be review.

Comment 10

[Doug Turner (:dougt)]

Attachment: Geoprovider in JS

Here is a sample geolocation provider that you can use to test the geolocation code.  Basically, it is a js component that registers as a provider and returns a location when asked.  It does not support watchLocation(), but easily could be adapted.

Comment 11

[Doug Turner (:dougt)]

the printf's in nsGeolocatorService::GetLastKnownPosition, should be removed.

Comment 12

[Doug Turner (:dougt)]

also, exclude the change to file memory/jemalloc/Makefile.in

Comment 13

[Doug Turner (:dougt)]

note, when this lands, we will have to include:

apt-get install liblocation-dev

to the build setup instructions

Comment 14

[Doug Turner (:dougt)]

Attachment: patch v.4

Comment 15

[pbrobinson]

What about using geoclue which would allow the use of more geo apis with a single api
http://www.freedesktop.org/wiki/Software/GeoClue

I think its already used in some devices like the new TomTom linux device and possibly available on openmoko phones too.

Comment 16

[Doug Turner (:dougt)]

I am not against adding GeoClue support -- I just could not find anything about this on the device I using and that we are targeting for Fennec.

I have a interface between the dom specific code and the location provider, so it is possible to build the glue code that is needed.  In the next version of the patch, I have a NMEA Stream implemenation -- so if your geo solution exposed a serial port that is accessible via a file, you're all set.

Comment 17

[pbrobinson]

Hmm. Not my solution... I just randomly play with it :)

It does dbus and I seem to remember the moz codebase already supports dbus as do most linux mobile devices so that wouldn't be adding extra deps. No idea what or how hard it would be to implement though.

Comment 18

[Doug Turner (:dougt)]

if you want to find out :-)

apply the last patch to a mozilla-central tree, take a look at the nsIGeolocationProvider.idl, and see if you can come up with an implementation that does the dbus stuff you're talking about.

Comment 19

[Doug Turner (:dougt)]

Attachment: patch v.5

fixes up shutdown stall.
adds a default nmea stream parser (source from David M Howard.  BSD License).

still need to work on making sure we don't call out into non-existent windows.  will talk to jst about that.

Comment 20

[Johnny Stenback (:jst)]

Comment on attachment 327492 [details] [diff] [review]
patch v.5

- In nsGeolocatorService::StartDevice():

+    mProvider = do_GetService(NS_GEOLOCATION_PROVIDER_CONTRACTID);
+    if (!mProvider)
+    {
+      // guess not, lets try a default one:  
+#ifdef NS_OSSO
+      mProvider = new MaemoLocationProvider();
+#endif
+    }
+
+    nsresult rv;
+    nsXPIDLCString nmeaPath;
+    nsCOMPtr<nsIPrefBranch2> prefs = do_GetService(NS_PREFSERVICE_CONTRACTID);
+    if (prefs)
+      rv = prefs->GetCharPref("geo.nmea.path", getter_Copies(nmeaPath));
+
+    if (NS_SUCCEEDED(rv) && prefs != nsnull)
+      mProvider = new NMEAStreamLocationProvider(nmeaPath);

If NS_OSSO is defined and we successfully got the prefs we'll leak the MaumoLocationProvider allocated above (inside #ifdef NS_OSSO).

- In nsGeolocator::Update(nsIDOMGeolocation *somewhere):

[...]
+  // need to check if the page is still alive, right? jst?

Yes, per our earlier discussion on irc...

+PRBool
+nsGeolocator::AskForPermission()
+{
+  // jst says there are easier ways to do this.  look in content utils.
>+  // ->>  PRBool isCallerChrome = nsContentUtils::IsCallerChrome();
+  // but I still need the principal's uri to display.  hmm.

hmm indeed.

+  nsCOMPtr<nsIScriptSecurityManager> secman(do_GetService(NS_SCRIPTSECURITYMANAGER_CONTRACTID));

You could use nsContentUtils::GetSecurityManager() to avoid a little bit of code here...

do_GetService(NS_STRINGBUNDLE_CONTRACTID);
+    if (!bundleService)
+      return PR_FALSE;
+    
+    nsCOMPtr<nsIStringBundle> bundle;
+    bundleService->CreateBundle(GEOLOCATION_PROPERTIES, getter_AddRefs(bundle));

nsContentUtils::GetLocalizedString() could help you cut down on the lines of code here...

Other than that this looks good at a high level. I'll still go through this once more once an updated patch is available...

Comment 21

[Doug Turner (:dougt)]

Attachment: patch v.6

adds checks before calling out into the dom of a closed windows.  also prevents calling out into the document of a window that has since changed (similar to what the xmlhttprequest does)

Comment 22

[Doug Turner (:dougt)]

Attachment: patch v.7

This removes the UI from the patch and allows an application to control it.

For example:

  var geolocationService = Cc["@mozilla.org/geolocation/service;1"].
                           getService(Ci.nsIGeolocationService);

  geolocationService.prompt = function(url) {alert(url.spec); return 1;};

The definition of this function is in the idl.

Comment 23

[Doug Turner (:dougt)]

Attachment: patch v.8

this creates an asynchronous prompt callback.  This is required so that the application can use non-modal UIs.

Now the interface to this looks like:

var geolocationService = Cc["@mozilla.org/geolocation/service;1"].
                           getService(Ci.nsIGeolocationService);

getNotificationBox(window/browser)                                                                
  geolocationService.prompt = function(request) {}

I also included some minor code clean up.

Comment 24

[Johnny Stenback (:jst)]

Comment on attachment 327976 [details] [diff] [review]
patch v.8

- In NMEAStreamLocationProvider::~NMEAStreamLocationProvider():

+{
+  mKeepGoing = PR_FALSE;
+}

This is pointless, right? Nothing should be looking at members of this class after the destructor has run...

- In NMEAStreamLocationProvider::Run():

+          // Lets throttle this back a bit:
+          PRTime now = PR_Now();
+          if (now - lastUpdated > 2000)

Only throttle to 2ms updates? I thought all GPS chips have a minumum update frequency of 1s anyways...

- In dom/src/geolocation/nmeap*

Shouldn't all this nmea code ideally be an extension, installed conditionally depending on what location devices are available etc? From reading through this code is doesn't seem like it's gone through a lot of beating on, and there's definitely code cleanup (duplication etc), debug code that's not inside #ifdef's that should be done in it before we take it into our tree, if we need to.

- In nsGeolocationRequest::Allow():

+  printf("allow\n");

Remove this before landing.

- In nsGeolocationRequest::AllowButFuzz():

+  printf("allow but fuzz\n");

Ditto.

- In nsGeolocationRequest::SendLocation():

+  //TODO mFuzzLocation.  Needs to be defined what we do here.
+  if (mFuzzLocation)
+  {
+    // need to make a copy because nsIDOMGeolocation is
+    // readonly, and we are not sure of its implementation.
+
+    double lat, lon, alt, herror, verror;
+    DOMTimeStamp time;
+    location->GetLatitude(&lat);
+    location->GetLongitude(&lon);
+    location->GetAltitude(&alt);
+    location->GetHorizontalAccuracy(&herror);
+    location->GetVerticalAccuracy(&verror);
+    location->GetTimestamp(&time); 
+
+    // do something to the numbers...  TODO.

Yeah, really :). How about we don't give out the numbers at all before we figure out how to fuzz this and actually do it.

+    nsGeolocation* somewhere = new nsGeolocation(lat,
+                                                 lon,
+                                                 alt,
+                                                 herror,
+                                                 verror,
+                                                 time);
+    somewhere->AddRef();
+    mCallback->OnRequest(location);
+    somewhere->Release();

Use nsRefPtr<> here to avoid manual reference counting, and don't you want to pass in somewhere there instead of location?

- In nsGeolocationRequest::SetOwner():

+    if (window) {
+      mOwner = window->GetCurrentInnerWindow();
+}
+}
+}

Fix closing brace indentation.

nsGeolocatorService::nsGeolocatorService() {

Stick the opening brace on its own line for consistency's sake.

+  nsCOMPtr<nsIPrefBranch2> prefs = do_GetService(NS_PREFSERVICE_CONTRACTID);
+  if (prefs) {
+    nsresult rv = prefs->GetIntPref("geo.timeout", &mTimeout);
+    if (NS_FAILED(rv))
+      mTimeout = 6000;
+  }

You should be able to do mTimeout = nsContentUtils::GetIntPref("geo.timeout", 6000) here to save on some code.

- In nsGeolocatorService::Update():

+  for (PRUint32 i = 0; i< mGeolocators.Length(); i++)
+      mGeolocators[i]->Update(somewhere);

Stick with 2-space indentation :)

+nsIDOMGeolocation*
+nsGeolocatorService::GetLastKnownPosition()
+{
+  nsIDOMGeolocation* location = nsnull;
+  if (mProvider)
+    mProvider->GetCurrentLocation(&location);
+  return location;    
+}

This should return already_AddRefed<nsIDOMGeolocation> to make it a bit harder to leak.

- In nsGeolocatorService::StartDevice():

+    // Check for the nmea stream provider
+    if (!mProvider)
+    {
+      // lets look to see if there is a nmea file we can read from

So this would need to move into the extension then too, one way or another...

- In nsGeolocator::~nsGeolocator():

+  printf(" DELETING NSGEOLOCATOR !!!!!!!!!! \n");

Delete before checking in.

I think that's it, fix that and this can land IMO. I can look at an updated diff if you think there's a need to.

Comment 25

[Justin Dolske [:Dolske]]

(In reply to comment #24)

> Only throttle to 2ms updates? I thought all GPS chips have a minumum update
> frequency of 1s anyways...

That's been the case, although 5Hz models are readily available now. Supposedly 100Hz modules exist, but cost a small fortune.

Comment 26

[Doug Turner (:dougt)]

thanks jst.  I am going to remove the NMEA stuff for now and put it in an extension (or make another patch/bug).

justin is right, we need to throttle any/all nmea serial devices.  updates are faster than what we can process on high end machines.

Comment 27

[Doug Turner (:dougt)]

Attachment: patch v.9

removal of the nmea stuff, fixing all jst comments, and fixes for merge.  This should be what lands next.

Comment 28

[Doug Turner (:dougt)]

http://hg.mozilla.org//mozilla-central/index.cgi/rev/e5014dc700ad

new bugs for follow ups.  there will be more as the draft spec is changing.

jst, thanks for the review.

Comment 29

[Benjamin Smedberg]

I backed this out this morning, as it caused unit-test failures on the Firefox tree. test_419731.js was failing on windows consistently, and went green after the backout.

Comment 30

[Doug Turner (:dougt)]

test_419731.js continues to fail after the back out on: WINNT 5.2 mozilla-central qm-win2k3-moz2-01 dep unit test

Comment 31

[Doug Turner (:dougt)]

ERROR FAIL leaked 40 bytes during test execution (should have leaked no more than 0 bytes)
ERROR FAIL leaked 1 instance of nsGeolocatorService with size 36 bytes
ERROR FAIL leaked 1 instance of nsTArray_base with size 4 bytes

We are a bit 

Comment 32

[Doug Turner (:dougt)]

we are a bit leaky.

Comment 33

[Myk Melez [:myk] [@mykmelez]]

(In reply to comment #30)
> test_419731.js continues to fail after the back out on: WINNT 5.2
> mozilla-central qm-win2k3-moz2-01 dep unit test

This is bug 444432.

Comment 34

[Doug Turner (:dougt)]

Attachment: patch v.10 -- fixes memory leak

This fixes two things:

1) first there was a memory leak.  We were not releasing the nsGeolocatorService service.  There basically was a cycle between the service (which lives for the entire life of the app), and the nsGeolocator objects (the object that provides geolocation information to the application).  I broke this cycle by adding a method to the nsGeolocator which can be called at application shutdown.  This method unhooks itself from the nsGeolocatorService.

before the patch running mochitests:

ERROR FAIL leaked 44 bytes during test execution (should have leaked no more than 0 bytes)
ERROR FAIL leaked 1 instance of nsGeolocatorService with size 40 bytes
ERROR FAIL leaked 1 instance of nsTArray_base with size 4 bytes

After the mochitests:

SUCCESS no leaks detected!

2)

In the mochitest, i removed waiting for a dialog.  We are still testing the geolocation lastLocation property, but we are not testing to see if a dialog is raise.  We are moving to use the infobar, and that will land in a subsequent patch.  Don't worry, if no prompt is set by the application, we do not hand out location information.  Fail safe.  However we do go through the motions of creating the service (so, I am not hiding the leak by changing the test).


Asking jst for another blessing.

Comment 35

[Reed Loden [:reed]]

Comment on attachment 328937 [details] [diff] [review]
patch v.10 -- fixes memory leak

>+# The Initial Developer of the Original Code is
>+# Doug Turner <[e-mail]>.

Shouldn't that be MoCo or MoFo, considering you're a Mozilla Corporation employee?

Comment 36

[Doug Turner (:dougt)]

I fixed the license headers locally.

Comment 37

[Johnny Stenback (:jst)]

Comment on attachment 328937 [details] [diff] [review]
patch v.10 -- fixes memory leak

- In dom/tests/mochitest/geolocation/test_lastPosition.html 

+Moving to the info var very soon.  right now, there is no

"info bar".

Comment 38

[Doug Turner (:dougt)]

turns out that this need a bit more leak fixing.  When I landed this patch, included (by mistake) browser code that deals with the prompts.  This caused a leak.  Today, i chased down the last leaks in the test cases i am using.  

Comment 39

[Doug Turner (:dougt)]

Attachment: patch v.11

more memory leak fixes.

Comment 40

[Doug Turner (:dougt)]

Attachment: geolocation ownership graph

Spend some time tracking down leaks/cycles.  Here is the final result.  new patch will be coming up.

The main changes is allowing the navigator to explictly shutdown the geolocation object when the docshell changes.  This allows us to not hold a strong reference to the window that the geolocation object was created in (the mOwner memory field).

I also included a test case which opens a bunch of windows that create a geolocation object, then closes them.  Previously, we would have leaked or possibly crashed.

I hope this graph is useful to someone other then me. :-)

Comment 41

[Doug Turner (:dougt)]

Attachment: Geolocation implementation matching ownership graph above.

Comment 42

[Doug Turner (:dougt)]

jst took a look.  i am changing how I get the mURI to:

    doc->NodePrincipal()->GetURI(getter_AddRefs(mURI));

This will account for getting the right uri when people use javascript: uri, or data: uris in their documents.

Comment 43

[Doug Turner (:dougt)]

46b8c45ac877
2008-07-15 16:37 -0700	Doug Turner - Initial geolocation implementation. bug=437948, r/sr=jst

2e96b409dbad
2008-07-15 18:33 -0700	Doug Turner - Fix maemo platform geolocation bustage


The unit tests have passed on all 3 platforms. 

Comment 44

[(not currently active) Ted Mielczarek]

I've been trying to use dougt's geolocation extension with nightlies and failing. I think you have a packaging problem:

"nsIGeolocationService" in Components.interfaces
false

^^ from the JS Shell in extensiondev, in a 20080821034539 nightly build. The unit tests succeed because they don't test packaged builds, they run out of the objdir. See bug 383136.

Comment 45

[Doug Turner (:dougt)]

Attachment: adds the xpt file to static packages

this adds the generated xpt file to the packaging.

Comment 46

[Doug Turner (:dougt)]

changeset:   18342:811388a5f8f8
tag:         tip
user:        Doug Turner <[e-mail]>
date:        Sun Aug 24 07:30:01 2008 -0700
summary:     Bug 437948.  Add the xpt file for geolocation to the packaging files. r=[e-mail]

Comment 47

[Eric Shepherd [:sheppy]]

Initial documentation for using Geolocation is here.  I'll be working on it some more, including adding a live example as well as documenting the interfaces behind it, but this should be a good starting point.

http://developer.mozilla.org/En/Using_geolocation

Comment 48

[Eric Shepherd [:sheppy]]

It's been pointed out that unless "velocity" includes direction information, it should be called "speed" instead.

Comment 49

[Eric Shepherd [:sheppy]]

Will Firefox 3.1 be including support for Skyhook, or will people be on their own to get a provider for location information?

Comment 50

[Justin Dolske [:Dolske]]

(In reply to comment #49)
> Will Firefox 3.1 be including support for Skyhook

AFAIK, no. Currently Firefox 3.1 just has the framework in place. To actually do anything, location provider modules will need to be written to plug into it. (Although Fennec on the N810 supports the built-in GPS, and dougt has an AMO extensinon for manually specifying a static position.)

Comment 51

[Doug Turner (:dougt)]

there are absolutely NO plans to include Skyhook in Firefox.  none.  zero.

Comment 52

[Eric Shepherd [:sheppy]]

No wonder the API does nothing when I try to use it then. :)

Comment 53

[Eric Shepherd [:sheppy]]

Geolocation documentation has been done for a while.

Comment 54

[Joel Maher ( :jmaher ) (UTC -8)]

verified with beta3