Error console pref should match Firefox

VERIFIED FIXED

Status

VERIFIED FIXED
8 years ago
7 years ago

People

(Reporter: wesj, Assigned: wesj)

Tracking

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

8 years ago
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.
(Assignee)

Comment 1

8 years ago
Created attachment 498863 [details] [diff] [review]
Patch v1

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

Comment 3

8 years ago
Created attachment 498875 [details] [diff] [review]
Patch v2

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

Comment 4

8 years ago
Crap. Meant to update the uninit method too. Give me a sec...
(Assignee)

Comment 5

8 years ago
Created attachment 498876 [details] [diff] [review]
Patch v3

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+
(Assignee)

Comment 7

8 years ago
pushed with an additional pref to turn on desktop notifications:

http://hg.mozilla.org/mobile-browser/rev/3b06a19d3cba
Status: NEW → RESOLVED
Last Resolved: 8 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.