Closed Bug 1259598 Opened 4 years ago Closed 4 years ago

Add new information fields to geolocation stumbles

Categories

(Cloud Services :: Location, defect)

defect
Not set

Tracking

(firefox49 fixed)

RESOLVED FIXED
Tracking Status
firefox49 --- fixed

People

(Reporter: telliott, Assigned: vng)

References

Details

Attachments

(1 file, 4 obsolete files)

Combain has asked for the following fields to be added, in approximate order of importance:

posage (time gps reading was taken)
netage (time wifi reading was taken)
gpsheading
gpsspeed
pressure

(they'd also like a trackid and some platform/device information, but I'm not committing to those until I talk to privacy)

This should be near-trivial. If it's not, please let me know.
This is a preliminary patch to add the following fields to the stumbler:
* GPS heading
* GPS speed
* wifi age

This also adds a new pressure sensor scanner to capture the ambient pressure in hPa

Code is based on the mainline MozStumbler code here: https://github.com/mozilla/MozStumbler/pull/1755
Attachment #8739571 - Flags: review?(michelangelo)
The patch description says it adds wifi age, but I can't see any code that actually does that part here. Otherwise looks good to me, as its the same code as in MozStumbler.
Missed a changeset in the previous patch. This now includes the 'age' field.
Attachment #8739571 - Attachment is obsolete: true
Attachment #8739571 - Flags: review?(michelangelo)
Attachment #8739672 - Flags: review?(michelangelo)
Comment on attachment 8739672 [details] [diff] [review]
revised patch - missed the 'age' commit last time

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

sr+ from the MLS service side. Looks all good to me.
Attachment #8739672 - Flags: superreview+
Comment on attachment 8739672 [details] [diff] [review]
revised patch - missed the 'age' commit last time

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

LGTM, but I'm no reviewer.:)
Clearing up the r? flag as I'm not sure I should review this.
Attachment #8739672 - Flags: review?(michelangelo) → review?
Comment on attachment 8739672 [details] [diff] [review]
revised patch - missed the 'age' commit last time

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

Looks like that I was mistaken in thinking I needed some sort of formal blessing to review things.
Apologies for the slowdown: n00b mistake.:)

LGTM!
Attachment #8739672 - Flags: review? → review+
Michelangelo - can you add the checkin-needed keyword to this bug?  I don't have the right permissions to do it.
Flags: needinfo?(michelangelo)
checkin-needed
Keywords: checkin-needed
Flags: needinfo?(michelangelo)
Depends on: 1266635
Sorry :Tomcat - I completely missed adding the PressureScanner in my patch.  I'll make sure everything goes through the try server this time.
Flags: needinfo?(vng)
Attached patch added_pressurescanner.patch (obsolete) — Splinter Review
Missed adding the PressureScanner to the hg commit before, so the patch was not complete.
Attachment #8739672 - Attachment is obsolete: true
Comment on attachment 8744412 [details] [diff] [review]
added_pressurescanner.patch

This patch is identical to the previously reviewed patch, but includes the PressureScanner class.  I missed adding it to patch last time.
Attachment #8744412 - Flags: superreview?(hschlichting)
Attachment #8744412 - Flags: review?(michelangelo)
Comment on attachment 8744412 [details] [diff] [review]
added_pressurescanner.patch

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

Looks good to me. But the new file is missing the MPL license header. I think that is required for moz-central code.
Attachment #8744412 - Flags: superreview?(hschlichting) → superreview+
Adds MPL2 to the PressureScanner code.
Attachment #8744412 - Attachment is obsolete: true
Attachment #8744412 - Flags: review?(michelangelo)
Comment on attachment 8744432 [details] [diff] [review]
Added MPL2 to pressure scanner code

This adds the MPL2.0 license to the top of the PressureScanner code as per review
Attachment #8744432 - Flags: superreview?(hschlichting)
Attachment #8744432 - Flags: review?(michelangelo)
Attachment #8744432 - Flags: superreview?(hschlichting) → superreview+
Attachment #8744432 - Flags: review?(michelangelo) → review+
Hi Victor,

could you take a look at this again, seems we got an error we had before again. Backed this out because we were running as example in https://treeherder.mozilla.org/logviewer.html#?job_id=8993382&repo=fx-team into the  /builds/slave/fx-team-and-api-15-00000000000/build/src/mobile/android/stumbler/java/org/mozilla/mozstumbler/service/stumblerthread/Reporter.java:30: error: cannot find symbol  errors, that caused a bustage last time.
Flags: needinfo?(vng)
I've backed out all the PressureScanner code entirely.  The problem was a bad mozconfig file, but in practice - we never have the ability to turn on 'active' scanning anyway.  Removing the PressureScanner seems like the right thing to do as it would just be adding bloat to Fennec for no good reason.

I've verified the JSON dumps on my own device, the age fields look good - but the GPS heading and and speed values always seem to be zero.
Flags: needinfo?(vng)
Attachment #8745581 - Flags: superreview?(hschlichting)
Attachment #8745581 - Flags: review?(michelangelo)
Attachment #8744432 - Attachment is obsolete: true
Attachment #8745581 - Flags: review?(michelangelo) → review+
Comment on attachment 8745581 [details] [diff] [review]
no-pressure.patch

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

There's still constants related to pressure remaining in the patch, but they don't actually hurt much and might make diffing to the mozstumbler code easier.

So I'm ok with this.
Attachment #8745581 - Flags: superreview?(hschlichting) → superreview+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/53765e008b97
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.