Closed Bug 1200310 Opened 5 years ago Closed 4 years ago

FxOS Geo Stumbling: don't scan cell+wifi if stumbling data is at daily limit

Categories

(Core :: DOM: Geolocation, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla43
Tracking Status
firefox43 --- fixed

People

(Reporter: garvan, Unassigned)

Details

Attachments

(2 files)

This patch refactors the code to check if cell+wifi collection is needed prior to initiating scanning. It is not power efficient to scan cell+wifi if it is not needed.
This is a cleanup item. We have guards to ensure only one worker thread is used. The MozStumbler worker thread 'WriteStumbleOnThread' has 2 guards, one for 'is stumbling' and another for 'is uploading'. This patch cleans this up to use one guard on the thread. Since WriteStumbleOnThread also controls the start/stop of uploading, it just needs one guard.

This makes it easier to do the second patch which directly addresses this bug
Attachment #8654969 - Flags: review?(josh)
Check to see if there is a file waiting to upload, because in this case, we don't store cell+wifi scans so the code should not perform this scan.

Note that the WriteStumbleOnThread is still triggered so that it can:
- check the age of the stumble data file (we call this a 'done' stumble file as it has reached max size, and is waiting to be uploaded)
- try upload if the file is 1 day old or greater
Attachment #8654973 - Flags: review?(josh)
Attachment #8654969 - Flags: review?(josh) → review+
Comment on attachment 8654973 [details] [diff] [review]
Part2: check-if-file-waiting-before-scanning.diff

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

::: dom/system/gonk/mozstumbler/MozStumbler.cpp
@@ +76,5 @@
>  
>  void
>  MozStumble(nsGeoPosition* position)
>  {
> +  if (WriteStumbleOnThread::IsFileWaitingForUpload()) {

We're checking this on a different thread than it's being written. Can we make this atomic as well?
Attachment #8654973 - Flags: review?(josh) → review+
> ::: dom/system/gonk/mozstumbler/MozStumbler.cpp
> @@ +76,5 @@
> >  
> >  void
> >  MozStumble(nsGeoPosition* position)
> >  {
> > +  if (WriteStumbleOnThread::IsFileWaitingForUpload()) {
> 
> We're checking this on a different thread than it's being written. Can we
> make this atomic as well?

Will do, I think it will be safer for future use; currently there is no 'testandset' operation on this so all usage is atomic. Thanks for the review!
https://hg.mozilla.org/integration/mozilla-inbound/rev/b542d5f36d6bb87025b0287ed89c7f9edb83ff0c
Bug 1200310 - Instead of thread guard flags for 'is stumbling' AND 'is uploading', we can use just one flag. r=jdm

https://hg.mozilla.org/integration/mozilla-inbound/rev/d7edc758649a9a5a43fbe038b2e190dca4072f25
Bug 1200310 - No need to wifi+cell scan if stumble file is full, just trigger upload. r=jdm
https://hg.mozilla.org/mozilla-central/rev/b542d5f36d6b
https://hg.mozilla.org/mozilla-central/rev/d7edc758649a
Status: UNCONFIRMED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
You need to log in before you can comment on or make changes to this bug.