620523 - Error console pref should match Firefox

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Wesley Johnston (:wesj)]

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.

Comment 1

[Wesley Johnston (:wesj)]

Attachment: 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.

Comment 2

[Mark Finkle (:mfinkle) (use needinfo?)]

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

Comment 3

[Wesley Johnston (:wesj)]

Attachment: 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.

Comment 4

[Wesley Johnston (:wesj)]

Crap. Meant to update the uninit method too. Give me a sec...

Comment 5

[Wesley Johnston (:wesj)]

Attachment: Patch v3

Uninit listener.

Comment 6

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 498876 [details] [diff] [review]
Patch v3

Looks good to me. Anyone using the old pref will be migrated.

Comment 7

[Wesley Johnston (:wesj)]

pushed with an additional pref to turn on desktop notifications:

http://hg.mozilla.org/mobile-browser/rev/3b06a19d3cba

Comment 8

[Aakash Desai [:aakashd]]

verified FIXED on build:
Mozilla/5.0 (Android; Linux armv71; rv:2.0b9pre) Gecko/20100104 Namoroka/4.0b9pre Fennec/4.0b4pre

Comment 9

[Aakash Desai [:aakashd]]

Oh yeah: NTT updated to 3.1.1 with the new pref.

Comment 10

[Dão Gottwald [:dao]]

bugspam