Closed
Bug 1259598
Opened 8 years ago
Closed 8 years ago
Add new information fields to geolocation stumbles
Categories
(Cloud Services :: Server: Location, defect)
Cloud Services
Server: Location
Tracking
(firefox49 fixed)
RESOLVED
FIXED
Tracking | Status | |
---|---|---|
firefox49 | --- | fixed |
People
(Reporter: telliott, Assigned: vng)
References
Details
Attachments
(1 file, 4 obsolete files)
4.84 KB,
patch
|
mds
:
review+
hschlichting
:
superreview+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•8 years ago
|
||
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
Reporter | ||
Updated•8 years ago
|
Attachment #8739571 -
Flags: review?(michelangelo)
Comment 2•8 years ago
|
||
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.
Assignee | ||
Comment 3•8 years ago
|
||
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 4•8 years ago
|
||
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 5•8 years ago
|
||
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 6•8 years ago
|
||
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+
Assignee | ||
Comment 7•8 years ago
|
||
Michelangelo - can you add the checkin-needed keyword to this bug? I don't have the right permissions to do it.
Flags: needinfo?(michelangelo)
Assignee | ||
Updated•8 years ago
|
Flags: needinfo?(michelangelo)
Comment 10•8 years ago
|
||
backed out for bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=8899270&repo=fx-team
Flags: needinfo?(vng)
Comment 11•8 years ago
|
||
Backout: https://hg.mozilla.org/integration/fx-team/rev/244de786f714
Assignee | ||
Comment 12•8 years ago
|
||
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)
Assignee | ||
Comment 13•8 years ago
|
||
Missed adding the PressureScanner to the hg commit before, so the patch was not complete.
Attachment #8739672 -
Attachment is obsolete: true
Assignee | ||
Comment 14•8 years ago
|
||
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 15•8 years ago
|
||
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+
Assignee | ||
Comment 16•8 years ago
|
||
Adds MPL2 to the PressureScanner code.
Attachment #8744412 -
Attachment is obsolete: true
Attachment #8744412 -
Flags: review?(michelangelo)
Assignee | ||
Comment 17•8 years ago
|
||
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)
Updated•8 years ago
|
Attachment #8744432 -
Flags: superreview?(hschlichting) → superreview+
Updated•8 years ago
|
Attachment #8744432 -
Flags: review?(michelangelo) → review+
Assignee | ||
Comment 18•8 years ago
|
||
The build looks ok now : https://treeherder.mozilla.org/#/jobs?repo=try&revision=749a6ad5d68f
Keywords: checkin-needed
Comment 19•8 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/cc427bee3a5a
Keywords: checkin-needed
Comment 20•8 years ago
|
||
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)
Comment 21•8 years ago
|
||
Backout: https://hg.mozilla.org/integration/fx-team/rev/69493bc2a238
Assignee | ||
Comment 22•8 years ago
|
||
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)
Assignee | ||
Updated•8 years ago
|
Attachment #8744432 -
Attachment is obsolete: true
Updated•8 years ago
|
Attachment #8745581 -
Flags: review?(michelangelo) → review+
Assignee | ||
Comment 23•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5ab59482945d
Comment 24•8 years ago
|
||
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+
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Comment 25•8 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/53765e008b97
Keywords: checkin-needed
Comment 26•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/53765e008b97
You need to log in
before you can comment on or make changes to this bug.
Description
•