Closed
Bug 777594
Opened 12 years ago
Closed 12 years ago
cannot turn off/on Geolocation
Categories
(Firefox OS Graveyard :: General, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: jj.evelyn, Assigned: dougt)
References
Details
Attachments
(1 file, 2 obsolete files)
9.69 KB,
patch
|
jdm
:
review+
|
Details | Diff | Splinter Review |
https://github.com/mozilla-b2g/gaia/issues/2833 To turn on/off geolocation, we need a solution in Gecko for this. It would be better if there is a configuration in mozSettings, like wifi.enabled or bluetooth.enabled, and then we could simply set this value in gaia.
Comment 1•12 years ago
|
||
I propose can patch shell.js and link the about:config pref to mozSetting. Is that OK?
Comment 2•12 years ago
|
||
The about:config value would be geo.enabled http://www.mozilla.org/en-US/firefox/geolocation/
I would prefer that we did this in the geolocation code. Doug how hard would be to set up a setting->pref mapper there?
Assignee | ||
Comment 5•12 years ago
|
||
try using the pref "geo.enabled"
Assignee | ||
Comment 6•12 years ago
|
||
sorry, ignore comment #5.
Assignee | ||
Comment 7•12 years ago
|
||
Since mozSettings is gaia/b2g only, I do not want to add support for mozSetting in geolocation. I think doing what Tim suggest is the right thing for now. if mozSetting is extended to desktop, we can look again. If I was able to test this feature on desktop as well as b2g, I'd be more open to accepting such a change. Gregor, do you have a bug that tracks making mozSetting work on desktop?
Assignee | ||
Comment 8•12 years ago
|
||
It's not gaia/b2g only. wtf would be the point of that.
Assignee | ||
Comment 10•12 years ago
|
||
cjones - ask gregor. that was my understand from IRC last night.
Comment 11•12 years ago
|
||
(In reply to Doug Turner (:dougt) from comment #8) > Created attachment 646021 [details] [diff] [review] > work in progress - patch v.0 I am going to assume that this patch works with mozSettings key "geolocation.enabled" and push the Gaia code so when this landed it would just work(TM).
Assignee | ||
Comment 12•12 years ago
|
||
that assumption may be incorrect. i wasn't able to tests last night.
Comment 13•12 years ago
|
||
I guess that was some miscommunication. I thought you asked if we are planning to switch the desktop settings to mozSettings. You can test mozSettings on desktop. We currently run mozSettings tests in https://hg.mozilla.org/mozilla-central/file/98c2a42a3aef/dom/settings/tests/test_settings_basics.html
Assignee | ||
Comment 14•12 years ago
|
||
O.o okay. good!
Assignee | ||
Updated•12 years ago
|
Attachment #646021 -
Flags: feedback?(anygregor)
Comment 15•12 years ago
|
||
(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TW) from comment #11) > I am going to assume that this patch works with mozSettings key > "geolocation.enabled" and push the Gaia code so when this landed it would > just work(TM). https://github.com/mozilla-b2g/gaia/pull/2847/files#L0R69 This is how I will turn off Geolocation in Gaia.
Assignee | ||
Comment 16•12 years ago
|
||
jdm, can you take a look?
Attachment #646021 -
Attachment is obsolete: true
Attachment #646021 -
Flags: feedback?(anygregor)
Attachment #646343 -
Flags: review?(josh)
Comment 17•12 years ago
|
||
Comment on attachment 646343 [details] [diff] [review] patch v.1 Review of attachment 646343 [details] [diff] [review]: ----------------------------------------------------------------- I think this is a good way to solve it. I'd like to the next version with my comments addressed and containing a test for watchPosition as well. ::: dom/src/geolocation/nsGeolocation.cpp @@ +596,5 @@ > > return NS_OK; > } > + > + if (!strcmp("mozsettings-changed", aTopic)) This observer is not the prettiest I've ever seen. However, this is the bed we've made; can we at least move the body into a separate method so it doesn't dominate the rest of Observe? @@ +617,5 @@ > + if (!JS_ParseJSON(cx, dataStr.get(), dataStr.Length(), &val) || !val.isObject()) { > + return NS_OK; > + } > + > + JSObject &obj(val.toObject()); Whether this is correct code or not, I've never seen it used anywhere else. JSObject *obj = &val.toObject(). @@ +637,5 @@ > + if (value.toBoolean() == false) > + { > + // turn things off > + for (PRUint32 i = 0; i< mGeolocators.Length(); i++) > + { Move this onto the previous line. @@ +643,5 @@ > + } > + StopDevice(); > + Update(nsnull); > + mLastPosition = nsnull; > + sGeoEnabled = PR_FALSE; false @@ +646,5 @@ > + mLastPosition = nsnull; > + sGeoEnabled = PR_FALSE; > + } > + else { > + sGeoEnabled = PR_TRUE; true @@ +1033,5 @@ > > + if (!sGeoEnabled) { > + nsCOMPtr<nsIRunnable> ev = new RequestAllowEvent(false, request); > + NS_DispatchToMainThread(ev); > + return NS_OK; *_retval isn't initialized here. I think we need to append a callback to the watch list. ::: dom/tests/mochitest/geolocation/test_mozsettings.html @@ +46,5 @@ > +toggleGeolocationSetting(false, function() { > + ok(true, "turned off geolocation via mozSettings"); > + setTimeout(function() { > + navigator.geolocation.getCurrentPosition(successCallbackAfterMozsetting, failureCallbackAfterMozsetting); > + }, 500); // need to wait a bit for all of these async callbacks to finish This is worrying. When does onsuccess fire? If you just need to be finished executing onsuccess, you could use SimpleTest.executeSoon.
Attachment #646343 -
Flags: review?(josh)
Assignee | ||
Comment 18•12 years ago
|
||
> Whether this is correct code or not, I've never seen it used anywhere else. JSObject *obj = &val.toObject(). The Gonk code for Volume management used is. > Move this onto the previous line. When in Rome. The style in this file rules. > *_retval isn't initialized here. I think we need to append a callback to the watch list. Good call.
Assignee | ||
Comment 19•12 years ago
|
||
> This is worrying. When does onsuccess fire?
setTimeout of zero doesn't do the trick. onSuccess is called, but it looks like the way we are dispatching the observer event is being re-queued. It first gets fired and the SettingsChangeNotifier.jsm gets it. Then it is redisptach to the actual observers. So, just putting this test at the end of the event queue doesn't work.
Assignee | ||
Comment 20•12 years ago
|
||
Attachment #646343 -
Attachment is obsolete: true
Attachment #646418 -
Flags: review?(josh)
Comment 21•12 years ago
|
||
(In reply to Doug Turner (:dougt) from comment #18) > > Move this onto the previous line. > > When in Rome. The style in this file rules. It's funny you should mention this, since the style in the file is overwhelmingly in favour of my request :)
Comment 22•12 years ago
|
||
Comment on attachment 646418 [details] [diff] [review] patch v.2 Review of attachment 646418 [details] [diff] [review]: ----------------------------------------------------------------- test_mozsettingsWatch is missing from the patch, but I'll let that slide this time :)
Attachment #646418 -
Flags: review?(josh) → review+
Assignee | ||
Comment 23•12 years ago
|
||
> It's funny you should mention this, since the style in the file is overwhelmingly in favour of my request :)
I'll land another patch of whitespace only that makes this file consistent with your request...
Assignee | ||
Comment 24•12 years ago
|
||
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=33a387ea92de
Comment 25•12 years ago
|
||
The tests from this bug fail on Android; filed follow-up bug 778256 and disabled the tests: https://hg.mozilla.org/integration/mozilla-inbound/rev/9b056f7ae1cd
Comment 26•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/f5d191758459 https://hg.mozilla.org/mozilla-central/rev/9b056f7ae1cd
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Reporter | ||
Comment 27•12 years ago
|
||
How to verify geolocation has been turned off? In Gaia, when users check on Airplane mode, the mozSettings key "geolocation.enabled" will be disabled (set to false). However, when visit a website with geolocation function, it still pop up for granting access.
Assignee | ||
Comment 28•12 years ago
|
||
What you're doing sounds like the right way to verify. When you said: "the mozSettings key "geolocation.enabled" will be disabled" Is the mozSetting key disabled or isn't it disabled when you put the phone in airplane mode.
Comment 29•12 years ago
|
||
I guess Evelyn had hard time testing that because of bug 779455. I think the real question here is: if Geolocation is disabled, why is there still a mozChromeEvent permission prompt from Gecko? If this is expected Gecko behavior, then Gaia should ignore the prompt base on the mozSetting value. If not, we get yet another Bugzilla bug to fix.
Assignee | ||
Comment 30•12 years ago
|
||
right, if geolocation.enabled is false, you should never see a prompt. Can we verify that mozSettings is correctly being set in airplane mode?
You need to log in
before you can comment on or make changes to this bug.
Description
•