Closed Bug 1065751 Opened 5 years ago Closed 5 years ago

[AGPS]Gonk GPS provider AGPS: doesn't handle roaming cell

Categories

(Core :: DOM: Geolocation, defect, major)

ARM
Gonk (Firefox OS)
defect
Not set
major

Tracking

()

RESOLVED FIXED
2.2 S2 (19dec)
blocking-b2g 2.1+
Tracking Status
b2g-v2.0 --- wontfix
b2g-v2.0M --- fixed
b2g-v2.1 --- fixed
b2g-v2.1S --- fixed
b2g-v2.2 --- fixed
b2g-master --- unaffected

People

(Reporter: garvan, Assigned: jaliu)

References

Details

Attachments

(4 files, 1 obsolete file)

+++ This bug was initially created as a clone of Bug #1064266 +++

We have code in the network location provider to extract all current cell connections, extract their unique identifiers and use them to do location lookups.
This code needs to look up the mcc and mnc values of each cell connection. But currently the code uses the mcc and mnc values of the SIM card and not those of the active connection.

From Kan-Ru's comment: https://bugzilla.mozilla.org/show_bug.cgi?id=1064266#c5
I thought we use mcc and mnc from network in the GPS provider, but I'm wrong. We need to fix this too:
https://mxr.mozilla.org/mozilla-central/source/dom/system/gonk/GonkGPSGeolocationProvider.cpp?rev=6f40822c1c04#503
See Also: → 1100297
Hi Jamin,
Can you take this bug? Thanks!
blocking-b2g: --- → 2.0M+
Flags: needinfo?(jaliu)
Duplicate of this bug: 1100297
Assignee: nobody → jaliu
Flags: needinfo?(jaliu)
Target Milestone: --- → 2.2 S1 (5dec)
Status: NEW → ASSIGNED
See Also: → 843452
(In reply to Jamin Liu [:jaliu] (PTO: 12/8 ~ 12/10) from comment #3)
> Created attachment 8528902 [details] [diff] [review]
> Support AGPS with roaming cell by retrieving mcc & mnc from
> nsIMobileNetworkInfo. (v1)

Hi Jamin, Do you need review for your patch? Thanks!
Flags: needinfo?(jaliu)
Hi Jamin,
Could you also give me an ETA date? Thanks!
Flags: needinfo?(jaliu)
Whiteboard: [ETA: Dec. 16]
Target Milestone: 2.2 S1 (5dec) → 2.2 S2 (19dec)
Comment on attachment 8528902 [details] [diff] [review]
Support AGPS with roaming cell by retrieving mcc & mnc from nsIMobileNetworkInfo. (v1)

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

Hi Kan-ru,
I'd like to ask you to review the patch when you have time.

I tested this patch on flame v2.2 with a roaming SIM card of India.
However, I only tested it once and I didn't print mcc/mnc as record at that time.
The reason is that I have only one pre-paid SIM card from India with data-connection and it ran out its quota during my first roaming AGPS test. 
(P.S. It did speed up the GPS locating process in that test.)

Nevertheless, I verified mcc/mnc values with roaming SIM card in networkGeolocation when I tested the patch of Bug 1064266. 

Thank you.
Attachment #8528902 - Flags: review?(kchen)
Hi Kan-ru,
This fix is request urgently by partner on 2.0M. Could you help to review it? Thanks!
Flags: needinfo?(kchen)
Comment on attachment 8528902 [details] [diff] [review]
Support AGPS with roaming cell by retrieving mcc & mnc from nsIMobileNetworkInfo. (v1)

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

LGTM
Attachment #8528902 - Flags: review?(kchen) → review+
Flags: needinfo?(kchen)
Thank Kan-Ru for reviewing the patch.

- Convert to hg format
- Add reviewer's name to commit message
Attachment #8528902 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/ad534d0193f4
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Whiteboard: [ETA: Dec. 16]
Jamin, Uplift to 2.0m got conflicts, Could you rebase a patch for 2.0m? Thanks!
Flags: needinfo?(jaliu)
Summary: Gonk GPS provider AGPS: doesn't handle roaming cell → [AGPS]Gonk GPS provider AGPS: doesn't handle roaming cell
Hi Kai-Zhen,
Please push this patch to 2.0m branchsince it's marked as a 2.0m+ bug.
Thank you.
Flags: needinfo?(jaliu)
Does this need to land on v2.1? If so, please request b2g34 approval on the patch.
Flags: needinfo?(jaliu)
[Blocking Requested - why for this release]:

FxOS v2.1 can't reduce the GPS TTFF(Time To First Fix) by using AGPS technology if the device uses roaming network. 
In this case, the AGPS TTFF might be even worse than general GPS since AGPS provides incorrect aiding data.

I'd like to ask for triage to review whether or not this bug is a v2.1 blocker.
The patch is ready. If it's a 2.1+ bug, I will request b2g34 approval later.

Thanks
blocking-b2g: 2.0M+ → 2.1?
Flags: needinfo?(jaliu)
Brian, can you find someone to test this patch for 2.1 please?  We're worried about fall outs when uplifting.
Flags: needinfo?(brhuang)
(In reply to Naoki Hirata :nhirata (please use needinfo instead of cc) from comment #18)
> Brian, can you find someone to test this patch for 2.1 please?  We're
> worried about fall outs when uplifting.

Sure.

Mike, Please help to test this patch. Thank you.
Flags: needinfo?(brhuang) → needinfo?(mlien)
Hi Jamin, could I get the v2.1 local patch for testing?
Flags: needinfo?(mlien) → needinfo?(jaliu)
(In reply to Mike Lien[:mlien] from comment #20)
> Hi Jamin, could I get the v2.1 local patch for testing?

Hi Mike,
Ok, I'll pull v2.1 branch and put v2.1 local patch on Bugzilla later. :)
Flags: needinfo?(jaliu)
Hi! Jamin,

I changed the status as reopen and please help to land this on v2.1. Thanks

--
Keven
Status: RESOLVED → REOPENED
blocking-b2g: 2.1? → 2.1+
Resolution: FIXED → ---
Status: REOPENED → NEW
ni? myself to remind me provide v2.1 patch as long as I've finished my current task.
Flags: needinfo?(jaliu)
Status: NEW → ASSIGNED
Flags: needinfo?(jaliu)
Flags: needinfo?(jaliu)
(In reply to Mike Lien[:mlien] from comment #20)
> Hi Jamin, could I get the v2.1 local patch for testing?

Hi Mike,
I put v2.1 patch at attachment 8551229 [details] [diff] [review].
Thank you for your help. :)
Hi Mike,
Would you mind to test #attachment 8551229 [details] [diff] [review] when you have feel time ?
I'll request for mozilla-b2g34_v2_1 approval if QA think the patch is safe for v2.1. :)
Flags: needinfo?(mlien)
Patch has been verified with v2.1 and it works well. The tested roaming SIM is T-Mobie.

[Only GPS]: it takes 5 minutes and 30 seconds to get the first position
*Test steps (Only GPS):
 1. Flash build contains patch
 2. Launch Geoloc app and start to get position

[AGPS from roaming SIM]:
Slot one with 3G: it takes less than 10 seconds to get the first position
Slot two with 2G: it takes less than 30 seconds to get the first position
*Test steps (Roaming SIM AGPS):
 1. Flash build contains patch
 2. Enable Data connection and Data roaming in Settings app -> Cellular & Data
 3. Launch Geoloc app and start to get position
Flags: needinfo?(mlien)
Comment on attachment 8551229 [details] [diff] [review]
(for v2.1) Support AGPS with roaming cell by retrieving mcc & mnc from nsIMobileNetworkInfo. r=kchen

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
> Bug caused by (feature/regressing bug #): 
(feature) Bug 715788 - Add A-GPS support for gonk

> User impact if declined: 
If user uses roaming network, the GPS TTFF(Time To First Fix) can't be reduced by using AGPS technology.

In this case, the AGPS TTFF might be even worse than general GPS since AGPS provides incorrect aiding data.

> Testing completed:
Mike Lien from Taipei QA team has tested this patch with roaming SIM.
Please refer to #comment 27

> Risk to taking this patch (and alternatives if risky): 
Currently, the use case isn't covered by any automated test case since it's hard to emulate the condition of usage on tinderbox. (Roaming card + AGPS)

> String or UUID changes made by this patch:
No.
Attachment #8551229 - Flags: approval-mozilla-b2g34?
Attachment #8551229 - Flags: approval-mozilla-b2g34? → approval-mozilla-b2g34+
Keywords: checkin-needed
Uplifts don't require checkin-needed :)
Keywords: checkin-needed
Attachment #8556326 - Attachment description: (for v2.1s) Support AGPS with roaming cell by retrieving mcc & mnc from nsIMobileNetworkInfo. → (for v2.1s) Support AGPS with roaming cell by retrieving mcc & mnc from nsIMobileNetworkInfo. r=kchen
Attachment #8551229 - Attachment description: (for v2.1) Support AGPS with roaming cell by retrieving mcc & mnc from nsIMobileNetworkInfo. → (for v2.1) Support AGPS with roaming cell by retrieving mcc & mnc from nsIMobileNetworkInfo. r=kchen
*sigh* Except reopening the bug for uplifts totally breaks actually getting them uplifted. *PLEASE* leave bugs closed if they're fixed on master. Status flags track uplifts and our bug queries won't catch things needing uplift otherwise.

https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/e6c0cae7fee3
Status: ASSIGNED → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.