1022279 - NetworkGeolocationProvider should honor wifi.enabled setting changes

Status: RESOLVED FIXED

(Core :: DOM: Geolocation, defect)

Description

[Doug Turner (:dougt)]

On FirefoxOS, it would be nice if the NetworkGeolocationProvider could know when the wifi settings changed so that could switch over to just reporting CELL changes.

Comment 1

[Doug Turner (:dougt)]

Attachment: bug_1022279

jdm, can you review.

Garvan, I think this is what you were thinking.

Comment 2

[Kan-Ru Chen [:kanru] (UTC+9)]

Comment on attachment 8436431 [details] [diff] [review]
bug_1022279

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

steal.

I notice that we used result.value in settingsCallback and now use result as boolean directly. After looking at SettingsService.js, I think it is correct to use result as value directly.

Comment 3

[Doug Turner (:dougt)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/819fc3d1f260

Comment 4

[:garvan]

Use cases: 
1) Wifi is on, App using watch position and wifi gets shut off.
2) Wifi is off, App using watch position and wifi gets turned on.

Is this case handled? I expected to see the settings callback address this.

Comment 5

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/819fc3d1f260

Comment 6

[Preeti Raghunath(:Preeti)]

Can you please help understand how this impacts the user?

Comment 7

[Doug Turner (:dougt)]

without the 4 changesets I nominated, html5 geolocation will behavior incorrectly.  developers will get location information from potentially 4 location sources each with different accuracies.  These changes improve this and provide a nice filter so that web applications do not have to worry about the implementation details.  I would expect without these changes, most geolocation applications will not work correctly.

Comment 8

[Preeti Raghunath(:Preeti)]

Can we check on 1.3?

Comment 9

[Doug Turner (:dougt)]

Preeti,
What are you asking?

Comment 10

[Doug Turner (:dougt)]

preeti ping?

Comment 11

[Jason Smith [:jsmith]]

Let me try asking the question I think that was trying to being asked here.

Doug - Why is this specifically needed for 1.4? This isn't a regression, right? comment 0 hints that this is nice to have, so triage is confused why this is needed.

Comment 12

[Preeti Raghunath(:Preeti)]

Doug,

Sorry for my delayed response.

I was trying to ask if this issue is a regression? What is the priority of the geolocation feature in 1.4? I know it impacts Dolphin so I will nominate this for 1.4D instead.

Comment 13

[Doug Turner (:dougt)]

Thanks Jason 

> Why is this specifically needed for 1.4?

read the comment 7

> This isn't a regression, right?

Not is a serious bug in the geolocation stack.  We patch it for 1.3T, but did not fix it for dolphin.

> comment 0 hints that this is nice to have, so triage is confused why this is needed.

I see.  Here is the rub:

Approve this and the other 3 bugs, or geolocation is going to suck on this platform.

Comment 14

[Jason Smith [:jsmith]]

Actually wait on that blocking flag - we're planning on taking this to 1.4D. So we don't need this on 1.4 right now.

Comment 15

[Preeti Raghunath(:Preeti)]

On 6/23 a decision was made to not provide Dolphin a separate branch but retain 1.4 as ground for Dolphin bugs. Hence making it 1.4+

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/da9599f0416f

Comment 17

[Chris Peterson [:cpeterson]]

Doug: Ryan says you need to rebase this patch for 1.4.

Comment 18

[Doug Turner (:dougt)]

https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/daa446763919

Comment 19

[:garvan]

There is bug in the 1.4 version of this patch, a line got dropped:

This line http://mxr.mozilla.org/mozilla-central/source/dom/system/NetworkGeolocationProvider.js#153
is missing from 1.4:
http://mxr.mozilla.org/mozilla-b2g30_v1_4/source/dom/system/NetworkGeolocationProvider.js#163

Comment 20

[Chris Peterson [:cpeterson]]

status-b2g1.4=affected as per comment 19

Comment 21

[:garvan]

Is ok on aurora
http://mxr.mozilla.org/mozilla-aurora/source/dom/system/NetworkGeolocationProvider.js#154

Comment 22

[Chris Peterson [:cpeterson]]

Garvan: can you please post a new 1.4 patch to add the missing line for wifi.enabled setting? dougt can't because he is on PTO this week.

Comment 23

[:garvan]

Attachment: 0001-bug1022279-1.4-patch-missing-this-line.patch

Patch for 1.4 to add the missing line.

Comment 24

[Chris Peterson [:cpeterson]]

Comment on attachment 8449442 [details] [diff] [review]
0001-bug1022279-1.4-patch-missing-this-line.patch

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

LGTM

Comment 25

[Chris Peterson [:cpeterson]]

checkin-needed for mozilla-b2g30_v1_4

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/04dd47e806fe

Also, please don't reopen bugs for issues with branch uplifts. That's what status flags are for. Bug resolution tracks the status on trunk.