1139736 - Map settings "network.debugging.enabled" to preference

Status: RESOLVED FIXED

(Firefox OS Graveyard :: RIL, defect)

Description

[Jessica Jong [:jessica] (inactive)]

Using settings, we can have a checkbox to enable/disable network logs in the FxOS Settings page.

Comment 1

[Jessica Jong [:jessica] (inactive)]

Attachment: patch, v1.

So here is what I observed:

'network.debugging.enabled' is mapped correctly to preference instantly, but because NetworkManager checks this preference only on top of the file, we need to restart b2g to take effect.

However, I noticed that, on the first restart, reading the preference on the top of the file does not give the correct result. If I 'observe' the preference, I can get the correct result after a couple of seconds. But on the second restart, reading the preference on the top of the file gives the correct result.
That means, we need to restart b2g twice for the logging to take effect.

What I can do is force flushing the preference to file when a 'network.debugging.enabled' change is observed by calling 'Services.prefs.savePrefFile(null)', not sure if it's the right thing to do though.

Comment 2

[Jessica Jong [:jessica] (inactive)]

To make it more flexible, I'll just observe 'network.debugging.enabled' changes and define the 'debug' function dynamically in NetworkManager, NetworkService and TetheringService.

Comment 3

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 1: map setting 'network.debugging.enable' to pref.

Comment 4

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 2: define debug function dynamically.

Comment 5

[Jessica Jong [:jessica] (inactive)]

Comment on attachment 8577141 [details] [diff] [review]
Part 1: map setting 'network.debugging.enable' to pref.

Fabrice, may I have your review on this? Thanks.

Comment 6

[Jessica Jong [:jessica] (inactive)]

Comment on attachment 8577142 [details] [diff] [review]
Part 2: define debug function dynamically.

Edgar, may I have your review on this? Thanks.

Comment 7

[Edgar Chen [:edgar]]

Comment on attachment 8577142 [details] [diff] [review]
Part 2: define debug function dynamically.

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

r=me with comments addressed. Thank you.

::: dom/system/gonk/NetworkManager.js
@@ -57,5 @@
>  
> -// Read debug setting from pref.
> -try {
> -  let debugPref = Services.prefs.getBoolPref("network.debugging.enabled");
> -  DEBUG = DEBUG || debugPref;

IMO, we should still keep |DEBUG| being synced with preference value to prevent confusion. Or remove the |DEBUG| lives in global scope.

@@ +62,5 @@
> +  let debugPref;
> +  try {
> +    debugPref = Services.prefs.getBoolPref(PREF_NETWORK_DEBUG_ENABLED);
> +  } catch (e) {
> +    debugPref = false;

Nit: Assign a default value to |debugPref|.

let debugPref = false;
try {
  debugPref = Services.prefs.getBoolPref(PREF_NETWORK_DEBUG_ENABLED);
} catch (e) {}

@@ +66,5 @@
> +    debugPref = false;
> +  }
> +
> +  if (DEBUG || debugPref) {
> +    debug = function (s) {

Nit: No space between `function` and `(`. Here and elsewhere.

::: dom/system/gonk/NetworkService.js
@@ -39,5 @@
>  
> -// Read debug setting from pref.
> -try {
> -  let debugPref = Services.prefs.getBoolPref("network.debugging.enabled");
> -  DEBUG = DEBUG || debugPref;

Ditto

@@ +47,5 @@
> +  let debugPref;
> +  try {
> +    debugPref = Services.prefs.getBoolPref(PREF_NETWORK_DEBUG_ENABLED);
> +  } catch (e) {
> +    debugPref = false;

Ditto

::: dom/system/gonk/TetheringService.js
@@ -106,5 @@
>  let DEBUG = false;
> -// Read debug setting from pref.
> -try {
> -  let debugPref = Services.prefs.getBoolPref("network.debugging.enabled");
> -  DEBUG = DEBUG || debugPref;

Ditto

@@ +113,5 @@
> +  let debugPref;
> +  try {
> +    debugPref = Services.prefs.getBoolPref(PREF_NETWORK_DEBUG_ENABLED);
> +  } catch (e) {
> +    debugPref = false;

Ditto

Comment 8

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 1: map setting 'network.debugging.enable' to pref. r=fabrice

Add r=fabrice. 

Thanks Fabrice!

Comment 9

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 2: define debug function dynamically. r=echen

Add r=echen and address review comment 7.

Thanks Edgar!

Comment 10

[Jessica Jong [:jessica] (inactive)]

try looks good:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=08b07f035035

mochitest-9 is a known failure.

Comment 11

[Jessica Jong [:jessica] (inactive)]

https://hg.mozilla.org/integration/b2g-inbound/rev/846fa7616502
https://hg.mozilla.org/integration/b2g-inbound/rev/883bea484ad4

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/846fa7616502
https://hg.mozilla.org/mozilla-central/rev/883bea484ad4