777594 - cannot turn off/on Geolocation

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[Evelyn Hung]

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

[Tim Guan-tin Chien [:timdream] (please needinfo)]

I propose can patch shell.js and link the about:config pref to mozSetting.

Is that OK?

Comment 2

[Tim Guan-tin Chien [:timdream] (please needinfo)]

The about:config value would be geo.enabled

http://www.mozilla.org/en-US/firefox/geolocation/

Comment 3

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

I would prefer that we did this in the geolocation code.  Doug how hard would be to set up a setting->pref mapper there?

Comment 4

[Doug Turner (:dougt)]

easy.  let me look when I get home.

Comment 5

[Doug Turner (:dougt)]

try using the pref "geo.enabled"

Comment 6

[Doug Turner (:dougt)]

sorry, ignore comment #5.

Comment 7

[Doug Turner (:dougt)]

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?

Comment 8

[Doug Turner (:dougt)]

Attachment: work in progress - patch v.0

Comment 9

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

It's not gaia/b2g only.  wtf would be the point of that.

Comment 10

[Doug Turner (:dougt)]

cjones - ask gregor.  that was my understand from IRC last night.

Comment 11

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(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).

Comment 12

[Doug Turner (:dougt)]

that assumption may be incorrect.  i wasn't able to tests last night.

Comment 13

[Gregor Wagner [:gwagner]]

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

Comment 14

[Doug Turner (:dougt)]

O.o  okay.  good!

Comment 15

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(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.

Comment 16

[Doug Turner (:dougt)]

Attachment: patch v.1

jdm, can you take a look?

Comment 17

[Josh Matthews [:jdm]]

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.

Comment 18

[Doug Turner (:dougt)]

> 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.

Comment 19

[Doug Turner (:dougt)]

> 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.

Comment 20

[Doug Turner (:dougt)]

Attachment: patch v.2

Comment 21

[Josh Matthews [:jdm]]

(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

[Josh Matthews [:jdm]]

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 :)

Comment 23

[Doug Turner (:dougt)]

> 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...

Comment 24

[Doug Turner (:dougt)]

https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=33a387ea92de

Comment 25

[Matt Brubeck (:mbrubeck)]

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

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f5d191758459
https://hg.mozilla.org/mozilla-central/rev/9b056f7ae1cd

Comment 27

[Evelyn Hung]

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.

Comment 28

[Doug Turner (:dougt)]

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

[Tim Guan-tin Chien [:timdream] (please needinfo)]

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.

Comment 30

[Doug Turner (:dougt)]

right, if geolocation.enabled is false, you should never see a prompt. 

Can we verify that mozSettings is correctly being set in airplane mode?