492684 - use preference to control logging in NetworkGeolocationProvider

Status: VERIFIED FIXED

(Core :: DOM: Geolocation, defect)

Description

[Doug Turner (:dougt)]

right now, to debug problems, we need to ask users to modify a file in the installation.  It is better if we use a preference to enable/disable this logging functionality.

http://mxr.mozilla.org/mozilla-central/source/dom/src/geolocation/NetworkGeolocationProvider.js

Comment 1

[Doug Turner (:dougt)]

Attachment: patch v.1

Comment 2

[Bartłomiej Brzozowiec (BartZilla)]

Comment on attachment 383794 [details] [diff] [review]
patch v.1

>--- a/dom/src/geolocation/NetworkGeolocationProvider.js
>+++ b/dom/src/geolocation/NetworkGeolocationProvider.js
...
>+    if (prefService.getBoolPref("geo.wifi.enableLog"))
...
How about adding this pref to all.js (firefox.js) so it would show up in about:config?

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+2]]

I don't think that it should be added. It's a hidden one like app.update.log.all which is only used for logging purposes.

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(In reply to comment #1)
> Created an attachment (id=383794) [details]
> patch v.1

Doug, you should remove the commented out code in the patch.

Comment 5

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 383794 [details] [diff] [review]
patch v.1

Does this really need to be live? Could just check the pref once at component initialization... maybe the difference doesn't matter.

Comment 6

[Doug Turner (:dougt)]

it really doesn't matter.  as far as perf goes, we are hitting the network and that really is the bottle neck.

Comment 7

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 383794 [details] [diff] [review]
patch v.1

Still kind of bothers me that we'll end up unnecessarily calling getService/getBoolPref multiple times per geolocation call, but it's not in the startup path and not particularly perf-sensitive so I guess I can live with it...

If you're not going to add a default value, you'll have to add a try/catch to avoid this throwing in the pref-unset case. r=me with that.

Comment 8

[Doug Turner (:dougt)]

Attachment: how about...

Comment 9

[:Gavin Sharp [email: [e-mail]]]

Could store a reference to the console service in startup(), too. And maybe dump() in addition to logStringMessage?

Comment 10

[Nochum Sossonko [:Natch]]

The LOGs in startup won't ever work with that afaict...

Comment 11

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 383960 [details] [diff] [review]
how about...

indeed

Comment 12

[Doug Turner (:dougt)]

Attachment: patch v.1

Comment 13

[Doug Turner (:dougt)]

Attachment: patch v.2

Comment 14

[Nochum Sossonko [:Natch]]

Comment on attachment 386582 [details] [diff] [review]
patch v.2

> Cc["@mozilla.org/consoleservice;1"].getService(Ci.nsIConsoleService).logStringMessage(aMsg);

Somewhat off topic, but I think you'd be better off just dump()'ing the string + '\n', makes it easier to copy and paste and is cleaner...

Comment 15

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 386582 [details] [diff] [review]
patch v.2

>diff --git a/dom/src/geolocation/NetworkGeolocationProvider.js b/dom/src/geolocation/NetworkGeolocationProvider.js

> function LOG(aMsg) {

>+    if (gLoggingEnabled)
>+    {
>+        aMsg = ("*** WIFI GEO: " + aMsg);
>+        Cc["@mozilla.org/consoleservice;1"].getService(Ci.nsIConsoleService).logStringMessage(aMsg);
>     }

Do we want to dump() too? Can make it easier to collect full logs.

>+function WifiGeoPositionProvider() {
>+    try {
>+        this.prefService = Cc["@mozilla.org/preferences-service;1"].getService(Ci.nsIPrefBranch);
>+        gLoggingEnabled = this.prefService.getBoolPref("geo.wifi.logging.enabled");
>+    } catch (e) {}

Leave the prefService initialization outside of the try/catch?

>+                    let branch = this.prefService.getBranch("geo.wifi.access_token.");

This call requires a QI to nsIPrefService. Just add it to the initialization in the constructor?

(Is there a test that would have failed for that?)

r=me with those addressed.

Comment 16

[Doug Turner (:dougt)]

Comment on attachment 386582 [details] [diff] [review]
patch v.2

there is a litmus test. we probably could add a mochitest, but I will just do that in another bug (493122)

Comment 17

[Doug Turner (:dougt)]

http://hg.mozilla.org/mozilla-central/rev/24bd46c67347

Comment 18

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Verified fixed on trunk with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090716 Minefield/3.6a1pre ID:20090716032001

Comment 19

[Daniel Veditz [:dveditz]]

Comment on attachment 386582 [details] [diff] [review]
patch v.2

minus for 1.9.1.4 since a workaround apparently exists (and 3.6 comes soon enough).