Geolocation fix "jerks around"

RESOLVED FIXED in Firefox 18

Status

()

Core
DOM: Device Interfaces
P2
normal
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: cjones, Assigned: dougt)

Tracking

unspecified
mozilla19
ARM
Gonk (Firefox OS)
Points:
---

Firefox Tracking Flags

(blocking-basecamp:+, firefox18 fixed, firefox19 fixed)

Details

Attachments

(1 attachment, 3 obsolete attachments)

STR
 (1) Open Maps app
 (2) Initially, we get a coarse fix.  IP or wifi positioning?
 (3) Wait for a precise GPS fix

After the GPS fix, I see the current location in Maps "jerk" back and forth between the coarse fix in (2) and the precise fix in (3).

I would expect that once a high-precision locator becomes available, we use it exclusively and ignore or disable low-precision sources.
Suggest blocking-basecamp for major usability issue.  This makes geolcation in the Maps app pretty much useless.
(Assignee)

Comment 2

5 years ago
The wifi/ip position and the gps position are both being sent to the document. We use to have code to filter/blend locations.

I agree that it should be fixed for basecamp.  Assigning.  I can look at it shortly.
Assignee: nobody → doug.turner
blocking-basecamp: ? → +
Priority: -- → P2
(Assignee)

Comment 3

5 years ago
Created attachment 677171 [details] [diff] [review]
patch v.1

It would be really cool if someone could try this patch out
Attachment #677171 - Flags: review?
Works great!
(Assignee)

Comment 5

5 years ago
Comment on attachment 677171 [details] [diff] [review]
patch v.1

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

jdm, do you think you can take a look?
Attachment #677171 - Flags: review? → review?(josh)

Comment 6

5 years ago
Comment on attachment 677171 [details] [diff] [review]
patch v.1

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

I'd like somebody else to sign off on the math; it went over my head :/

::: dom/src/geolocation/nsGeolocation.cpp
@@ +709,5 @@
>    }
>    return NS_OK;
>  }
>  
> +PRBool

Plain old bool, please, and associate return value changes.

@@ +716,5 @@
> +  if (!aSomewhere) {
> +    return false;
> +  }
> +  
> +  if (mProviders.Count() == 1 || mLastPosition == nullptr) {

!mLastPosition

@@ +722,5 @@
> +  }
> +
> +  nsresult rv;
> +  DOMTimeStamp oldTime;
> +  rv = mLastPosition->GetTimestamp(&oldTime);

Do you intend to do anything with oldTime?

@@ +744,5 @@
> +  NS_ENSURE_SUCCESS(rv, PR_FALSE);
> +
> +
> +  DOMTimeStamp newTime;
> +  rv = aSomewhere->GetTimestamp(&newTime);

Were you going to be using this, too?

@@ +779,5 @@
> +                       (cos(rNewLat) * cos(rOldLat) * cos(rOldLon - rNewLon)) ) * 6378137; 
> +
> +  // The threshold is when the distance between the two positions exceeds the
> +  // worse (larger value) of the two accuracies.
> +  double max_accuracy = PR_MAX(oldAccuracy, newAccuracy);

NS_MAX
Attachment #677171 - Flags: review?(josh) → review+
(Assignee)

Comment 7

5 years ago
Created attachment 677666 [details] [diff] [review]
patch v.2

Kan-Ru, would you mind also testing this patch? This is much more straight forward and probably works just fine.
Attachment #677171 - Attachment is obsolete: true
Will the old "better" position expire? If I get a accurate fix via gps then go into a building, all later updates from IP positioning will be discarded and I will not get any update.

Also the large movement case could happen in real life. Imagine that I get a accurate fix, then I take the subway, on the subway I have wifi so I could get a coarse location.
(Assignee)

Comment 9

5 years ago
Comment on attachment 677666 [details] [diff] [review]
patch v.2

That's fair.

we should use patch v.1.  bz said he wanted comments about the math used.  We should also look at the time and expire as Kan-Ru suggests.
Attachment #677666 - Flags: review-
(Assignee)

Comment 10

5 years ago
Created attachment 678799 [details] [diff] [review]
patch v.2
Attachment #677666 - Attachment is obsolete: true
Attachment #678799 - Flags: review?(bzbarsky)
Comment on attachment 678799 [details] [diff] [review]
patch v.2

This looks good.

It might still make sense to mention that this comes out of applying the spherical law of cosines to the triangle formed by our two points and the north pole.
Attachment #678799 - Flags: review?(bzbarsky) → review+
Please address comment 6 before landing.
(Assignee)

Comment 13

5 years ago
https://hg.mozilla.org/mozilla-central/rev/30b340a71112
https://hg.mozilla.org/releases/mozilla-aurora/rev/db23dcb2f68d
status-firefox18: --- → fixed
status-firefox19: --- → fixed
(Assignee)

Updated

5 years ago
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
(In reply to Josh Matthews [:jdm] from comment #6)
> Comment on attachment 677171 [details] [diff] [review]
> patch v.1
> 
> Review of attachment 677171 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'd like somebody else to sign off on the math; it went over my head :/
> 
> ::: dom/src/geolocation/nsGeolocation.cpp
> @@ +709,5 @@
> >    }
> >    return NS_OK;
> >  }
> >  
> > +PRBool
> 
> Plain old bool, please, and associate return value changes.

Silly you, you still missed this :)
(Assignee)

Comment 15

5 years ago
https://hg.mozilla.org/releases/mozilla-aurora/rev/d84a2be98735
https://hg.mozilla.org/mozilla-central/rev/ed13d73c61bb

backed out for bug 690913.
Status: RESOLVED → REOPENED
status-firefox18: fixed → ---
status-firefox19: fixed → ---
Resolution: FIXED → ---
We recently disabled test_geolocation.js due to intermittent failure (we're enabling webAPI tests sometime tomorrow or Thursday on tbpl): https://hg.mozilla.org/integration/mozilla-inbound/rev/a22328aa1130#l2.9

Could this be the cause?
(Assignee)

Comment 17

5 years ago
I don't think so..  It's a real bug (yeah!!! for regression tests)

I am trying out this:
  https://tbpl.mozilla.org/?tree=Try&rev=7208a31a6f90
The patch at https://hg.mozilla.org/try/rev/c367403ef2c0 should probably update mIsFirstUpdate.
(Assignee)

Comment 19

5 years ago
Created attachment 679469 [details] [diff] [review]
patch v.3

a bit more involved.
Attachment #678799 - Attachment is obsolete: true
Attachment #679469 - Flags: review?(josh)
Comment on attachment 679469 [details] [diff] [review]
patch v.3

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

The delta check ensures that even if a user is on the move and keeps receiving positions with strictly worse accuracy than the cached value, a long-running app using watchPosition will eventually receive an update, right? r=me if that's the case.

::: dom/src/geolocation/nsGeolocation.cpp
@@ +518,5 @@
> +  // in the case when newly detected positions are all less accurate than the cached one.
> +  //
> +  // Fixes bug 596481
> +  if (mIsFirstUpdate || aIsBetter) {
> +    mIsFirstUpdate = PR_FALSE;

One day we'll fix this nervous PR tic of yours.
Attachment #679469 - Flags: review?(josh) → review+

Comment 21

5 years ago
Try run for c565b9be1ddf is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=c565b9be1ddf
Results (out of 17 total builds):
    failure: 17
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/dougt@mozilla.com-c565b9be1ddf

Comment 22

5 years ago
Try run for c565b9be1ddf is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=c565b9be1ddf
Results (out of 18 total builds):
    failure: 18
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/dougt@mozilla.com-c565b9be1ddf
(Assignee)

Comment 23

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/c98b2e425857

Comment 24

5 years ago
https://hg.mozilla.org/mozilla-central/rev/c98b2e425857
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
(Assignee)

Comment 25

5 years ago
https://hg.mozilla.org/releases/mozilla-aurora/rev/38bff1f270d0
status-firefox18: --- → fixed
status-firefox19: --- → fixed
(In reply to :Ms2ger from comment #14)
> (In reply to Josh Matthews [:jdm] from comment #6)
> > Comment on attachment 677171 [details] [diff] [review]
> > patch v.1
> > 
> > Review of attachment 677171 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > I'd like somebody else to sign off on the math; it went over my head :/
> > 
> > ::: dom/src/geolocation/nsGeolocation.cpp
> > @@ +709,5 @@
> > >    }
> > >    return NS_OK;
> > >  }
> > >  
> > > +PRBool
> > 
> > Plain old bool, please, and associate return value changes.
> 
> Silly you, you still missed this :)

And managed to miss it again!
You need to log in before you can comment on or make changes to this bug.