Closed Bug 1139736 Opened 7 years ago Closed 6 years ago

Map settings "network.debugging.enabled" to preference

Categories

(Firefox OS Graveyard :: RIL, defect)

x86_64
Linux
defect
Not set
normal

Tracking

(firefox39 fixed)

RESOLVED FIXED
2.2 S8 (20mar)
Tracking Status
firefox39 --- fixed

People

(Reporter: jessica, Assigned: jessica)

References

Details

Attachments

(2 files, 3 obsolete files)

Using settings, we can have a checkbox to enable/disable network logs in the FxOS Settings page.
Attached patch patch, v1. (obsolete) — Splinter Review
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.
To make it more flexible, I'll just observe 'network.debugging.enabled' changes and define the 'debug' function dynamically in NetworkManager, NetworkService and TetheringService.
Attachment #8576582 - Attachment is obsolete: true
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.
Attachment #8577141 - Flags: review?(fabrice)
Comment on attachment 8577142 [details] [diff] [review]
Part 2: define debug function dynamically.

Edgar, may I have your review on this? Thanks.
Attachment #8577142 - Flags: review?(echen)
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
Attachment #8577142 - Flags: review?(echen) → review+
Attachment #8577141 - Flags: review?(fabrice) → review+
Add r=fabrice. 

Thanks Fabrice!
Attachment #8577141 - Attachment is obsolete: true
Attachment #8578450 - Flags: review+
Add r=echen and address review comment 7.

Thanks Edgar!
Assignee: nobody → jjong
Attachment #8577142 - Attachment is obsolete: true
Attachment #8578451 - Flags: review+
Blocks: 1144494
You need to log in before you can comment on or make changes to this bug.