Developer Settings: add checkbox to disable GPS Locations

RESOLVED FIXED in mozilla35

Status

()

RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: garvan, Assigned: garvan)

Tracking

Trunk
mozilla35
ARM
Gonk (Firefox OS)
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 5 obsolete attachments)

(Assignee)

Description

4 years ago
GPS-enabled devices use combinations of the GPS and network geolocation. In order to debug, or QA the logic of the GonkGPSGeolocationProvider, we need to be able to toggle the GPS locations on and off. 

Debug Scenarios:
- boot device, turn off GPS location, load geolocation app
--> tests the logic for the cold-start case, which is to use any available network location
- load geolocation toggle GPS locations off
--> tests the logic for temporary dropouts of the GPS, which in areas of poor network geolocation coverage, the last GPS location is used. If network coverage is high, than it may be used.
(Assignee)

Comment 1

4 years ago
Created attachment 8476749 [details] [diff] [review]
Part 1: Gaia: added dev checkbox for ignoring GPS location

I added the option: "Ignore GPS locations".

I chose this name for these reasons:
- I want an option that is off by default (vs. "Enable GPS locations"), it is clearer to me to see debugging options that need to be turned on, not off.
- "Disable GPS" would be incorrect wording, all GPS functionality is intact (which is handy because one can look at the debug logging and see what the equivalent GPS location is to the current network location)
- It most accurately reflects what the code will do, which is only to ignore these locations and look for a network location instead.
Attachment #8476749 - Flags: review?(kchen)
(Assignee)

Comment 2

4 years ago
Created attachment 8476753 [details] [diff] [review]
Part 2: gecko: implement gps ignore flag

GonkGPSLocationProvider gets a new flag for this setting.

Also added code to listen for Gaia settings changed, so the debug flags can be toggled on the fly.
Attachment #8476753 - Flags: review?(kchen)
(Assignee)

Comment 3

4 years ago
Created attachment 8476770 [details] [diff] [review]
Part 3: code cleanup related to the debug flags

The debug flag related code needed some cleanup IMHO.

- change #define to const char* for consistency
- change int to bool for bool flags
- make debug log messages more concise, multi-line messages (that could be one-line) make watching the debug log difficult
- put debug flags into the format gDebug_<debug flag name>. We have 2 debug flags now,  we will likely have more in future.
Attachment #8476770 - Flags: review?(kchen)
Attachment #8476749 - Flags: review?(kchen) → review?(ehung)
Comment on attachment 8476753 [details] [diff] [review]
Part 2: gecko: implement gps ignore flag

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

::: dom/system/gonk/GonkGPSGeolocationProvider.cpp
@@ +828,2 @@
>      obs->RemoveObserver(this, kNetworkConnStateChangedTopic);
> +#endif

We could remove observer unconditionally.
Attachment #8476753 - Flags: review?(kchen) → review+
Attachment #8476770 - Flags: review?(kchen) → review+
(Assignee)

Comment 6

4 years ago
Note to self, make sure to rebase on top of bug 1033274 when that lands

Updated

4 years ago
Attachment #8476749 - Flags: review?(ehung) → review+

Comment 7

4 years ago
Oh, could you send a Pull Request for your gaia patch? Here is a guidance for your reference.
 
https://developer.mozilla.org/en-US/Firefox_OS/Developing_Gaia/Submitting_a_Gaia_patch

Thanks!
Flags: needinfo?(gkeeley)
(Assignee)

Comment 8

4 years ago
Thanks for the review Evelyn, I'll review that link and follow the PR process for Gaia.
Flags: needinfo?(gkeeley)
(Assignee)

Comment 9

4 years ago
Comment on attachment 8476749 [details] [diff] [review]
Part 1: Gaia: added dev checkbox for ignoring GPS location

Marking obsolete, proper PR is on Gaia github.
(Assignee)

Updated

4 years ago
Attachment #8476749 - Attachment is obsolete: true
(Assignee)

Comment 10

4 years ago
Created attachment 8480219 [details] [review]
Part 1: Gaia: added dev checkbox for ignoring GPS location

Part 1 is on Gaia github:
https://github.com/mozilla-b2g/gaia/pull/23388

I didn't carry over the r+, although it is the same code as the patch on the bug; it is my first Gaia PR, so my Gaia commit process should be eyeballed to see that I followed the instructions correctly.

(I didn't realize this PR link was to go into an bugzilla attachment, or I would have done it when I obsoleted the attached patch in the prev comment).
Attachment #8480219 - Flags: review?(ehung)

Comment 11

4 years ago
Comment on attachment 8480219 [details] [review]
Part 1: Gaia: added dev checkbox for ignoring GPS location

Everything is good! Thank you. :)
Attachment #8480219 - Flags: review?(ehung) → review+
(Assignee)

Comment 12

4 years ago
Created attachment 8483210 [details] [diff] [review]
Part 2: gecko: implement gps ignore flag

Rebased on latest m-c, carrying over r+ from Kan-Ru.
Attachment #8476753 - Attachment is obsolete: true
Attachment #8483210 - Flags: review+
(Assignee)

Comment 13

4 years ago
Created attachment 8483211 [details] [diff] [review]
Part 3: code cleanup related to the debug flags

Rebased on today's m-c, carrying over r+ from Kan-Ru's previous review.
Attachment #8476770 - Attachment is obsolete: true
Attachment #8483211 - Flags: review+
(Assignee)

Comment 14

4 years ago
Created attachment 8483250 [details] [diff] [review]
Part 3: code cleanup related to the debug flags

see previous comment. Needed one more change for rebase.
Attachment #8483211 - Attachment is obsolete: true
Attachment #8483250 - Flags: review+
(Assignee)

Comment 15

4 years ago
Created attachment 8483492 [details] [diff] [review]
Part 2: gecko: implement gps ignore flag

rebase of part 2 on m-c, carrying over previous r+,

Try is Green:

Your Try Server build (f411a5c79165) was successfully completed on builder try-linux.
It should be available for download at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/gkeeley@mozilla.com-f411a5c79165/try-linux/firefox-35.0a1.en-US.linux-i686.tar.bz2
The full log for this build run is available at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/gkeeley@mozilla.com-f411a5c79165/try-linux/try-linux-bm83-try1-build3883.txt.gz.

For an overview of all results see TBPL.
Attachment #8483210 - Attachment is obsolete: true
Attachment #8483492 - Flags: review+
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/2cd0cf185415
https://hg.mozilla.org/mozilla-central/rev/6f40822c1c04
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
You need to log in before you can comment on or make changes to this bug.