Closed Bug 620523 Opened 9 years ago Closed 9 years ago

Error console pref should match Firefox

Categories

(Firefox for Android Graveyard :: General, defect)

defect
Not set

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: wesj, Assigned: wesj)

Details

Attachments

(1 file, 2 obsolete files)

We set up our own pref originally to show/hide the error console. We should use the same pref that Firefox now uses "devtools.errorconsole.enabled".

Also, we should show/hide the console dynamically, since there really isn't any reason not to.
Attached patch Patch v1 (obsolete) — Splinter Review
Patch. Updates to use new pref and attempts to update users with the old pref set. Adds an observer to watch for changes to this.
Attachment #498863 - Flags: review?(mark.finkle)
Comment on attachment 498863 [details] [diff] [review]
Patch v1


>-// JS error console
>-pref("browser.console.showInPanel", false);

Let's make sure "devtools.errorconsole.enabled" exists so it's a simple toggle in about:config and I don't need to actually add a bool pref.

>diff --git a/chrome/content/browser-ui.js b/chrome/content/browser-ui.js

>       // Init the views
>       ExtensionsView.init();
>       DownloadsView.init();
>       PreferencesView.init();
>       ConsoleView.init();
>       FullScreenVideo.init();
>+      ErrorConsoleUI.init();

We already have a ConsoleView with an init() - let's reuse it and not create a new JS object for this

>+  get enabled() {
>+    return Services.prefs.getBoolPref("devtools.errorconsole.enabled");
>+  },
>+  init: function ec_init() {
>+    try {
>+      // update users using the legacy pref
>+      if (Services.prefs.getBoolPref("browser.console.showInPanel")) {
>+        Services.prefs.setBoolPref("devtools.errorconsole.enabled", true);
>+        Services.prefs.clearUserPref("browser.console.showInPanel");
>+      }
>+    } catch(ex) {
>+      // likely don't have an old pref
>+    }
>+    this.updateVisibility();
>+    Services.prefs.addObserver("devtools.errorconsole.enabled", this, false);
>+  },
>+  observe: function ec_observe(aSubject, aTopic, aData) {
>+    this.updateVisibility();
>+  },
>+  updateVisibility: function ec_updateVisibility(aVal, aPref) {
>+    let button = document.getElementById("tool-console");
>+    button.hidden = !this.enabled;
>+  }
>+}

Looks fine but move the ConsoleView and separate methods with a blank line
Attachment #498863 - Flags: review?(mark.finkle) → review-
Attached patch Patch v2 (obsolete) — Splinter Review
Moved to ConsoleView and added to mobile.js. Kinda a screwy observe method now, can take one param if it is being called from the console service or three if its from the prefs service. Tell me if you'd rather separate that into two methods.
Attachment #498863 - Attachment is obsolete: true
Attachment #498875 - Flags: review?(mark.finkle)
Crap. Meant to update the uninit method too. Give me a sec...
Attached patch Patch v3Splinter Review
Uninit listener.
Attachment #498875 - Attachment is obsolete: true
Attachment #498876 - Flags: review?(mark.finkle)
Attachment #498875 - Flags: review?(mark.finkle)
Comment on attachment 498876 [details] [diff] [review]
Patch v3

Looks good to me. Anyone using the old pref will be migrated.
Attachment #498876 - Flags: review?(mark.finkle) → review+
pushed with an additional pref to turn on desktop notifications:

http://hg.mozilla.org/mobile-browser/rev/3b06a19d3cba
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
verified FIXED on build:
Mozilla/5.0 (Android; Linux armv71; rv:2.0b9pre) Gecko/20100104 Namoroka/4.0b9pre Fennec/4.0b4pre
Status: RESOLVED → VERIFIED
Oh yeah: NTT updated to 3.1.1 with the new pref.
bugspam
Assignee: nobody → wjohnston
You need to log in before you can comment on or make changes to this bug.