595540 - Show Error Console in Firefox button menu when Error Console is enabled

Status: RESOLVED FIXED

(Firefox :: Menus, defect)

Description

[Pavel Cvrcek [:JasnaPaka]]

If you use Windows Vista/7 you see new Firefox button by default. In bug 593538 old Error Console was hidden by default for Firefox 4.0. When you use preference on about:config page and enable Error Console you will see Error console in main menu but not inside Firefox button. I think in this situation should be Error Console available in submenu Developer.

Comment 1

[Pavel Cvrcek [:JasnaPaka]]

Attachment: Patch (WIP)

Should work but I didn't test it yet.

Comment 2

[Pavel Cvrcek [:JasnaPaka]]

Attachment: Patch (WIP)

Comment 3

[Justin Dolske [:Dolske]]

Comment on attachment 477734 [details] [diff] [review]
Patch (WIP)

>+++ b/browser/base/content/browser.js	Thu Sep 23 00:27:03 2010 +0200
...
>   if (consoleEnabled) {
>     document.getElementById("javascriptConsole").hidden = false;
>     document.getElementById("key_errorConsole").removeAttribute("disabled");
>+    let appMenuJavaScriptConsole = document.getElementById("appmenu_javascriptConsole");
>+    if (appMenuJavaScriptConsole)
>+      appMenuJavaScriptConsole.setAttribute("hidden", false);

Nit: Just use ".hidden = false" as is done a few lines up.


>+++ b/browser/base/content/browser.xul	Thu Sep 23 00:27:03 2010 +0200
...
>+              <menuitem id="appmenu_javascriptConsole"
>+                        hidden="true"
>+                        label="&errorConsoleCmd.label;"
>+                        accesskey="&errorConsoleCmd.accesskey;"
>+                        key="key_errorConsole"
>+                        oncommand="toJavaScriptConsole();"/>  

Alas, the "C" accesskey is already used by the Character Encoding menu here.

But the simple fix is to just remove accesskey for what you're adding, since we're generally not using accesskeys in the appmenu anyway. [Character Encoding shoudn't really have one either, but it's #included from a file shared with the normal menubar.]

r+ with these two changes.

Comment 4

[Pavel Cvrcek [:JasnaPaka]]

Attachment: Patch (fixed)

Patch where both comments are fixed.

Comment 5

[Pavel Cvrcek [:JasnaPaka]]

Attachment: Patch (fixed)

Comment 6

[Dão Gottwald [:dao]]

Comment on attachment 477734 [details] [diff] [review]
Patch (WIP)

>diff -r 0e43f7a9d7a8 browser/base/content/browser.js
>--- a/browser/base/content/browser.js	Wed Sep 22 14:33:54 2010 -0700
>+++ b/browser/base/content/browser.js	Thu Sep 23 00:27:03 2010 +0200
>@@ -1594,16 +1594,19 @@ function delayedStartup(isLoadingBlank, 
>       appMenuInspect.setAttribute("hidden", false);
>   }
> 
>   // Enable Error Console?
>   let consoleEnabled = gPrefService.getBoolPref("devtools.errorconsole.enabled");
>   if (consoleEnabled) {
>     document.getElementById("javascriptConsole").hidden = false;
>     document.getElementById("key_errorConsole").removeAttribute("disabled");
>+    let appMenuJavaScriptConsole = document.getElementById("appmenu_javascriptConsole");
>+    if (appMenuJavaScriptConsole)
>+      appMenuJavaScriptConsole.setAttribute("hidden", false);

Make this:

#ifdef MENUBAR_CAN_AUTOHIDE
    document.getElementById("appmenu_javascriptConsole").hidden = false;
#endif

Also, please change the id from appmenu_javascriptConsole to appmenu_errorConsole.

Comment 7

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

Attachment: updated to tip, addressing dao's comments

Carrying forward review and ui-review, asking for approval.

All of the app menu contents have been moved to browser-appmenu.inc in Bug 585370, so I moved this patch's addition to that file instead of browser.xul.

Comment 8

[Pavel Cvrcek [:JasnaPaka]]

Thanks for updated patch! My notebook for Mozilla work is currently broken so I could not provide updated patch.

Comment 9

[Dão Gottwald [:dao]]

Comment on attachment 495263 [details] [diff] [review]
updated to tip, addressing dao's comments

You changed the id here (indentation is off, btw):

>   if (consoleEnabled) {
>     document.getElementById("javascriptConsole").hidden = false;
>     document.getElementById("key_errorConsole").removeAttribute("disabled");
>+    #ifdef MENUBAR_CAN_AUTOHIDE
>+        document.getElementById("appmenu_errorConsole").hidden = false;
>+    #endif

but not here:

>+          <menuitem id="appmenu_javascriptConsole"

Comment 10

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

Attachment: oops

I knew I'd find some way of messing up in only 8 lines of code...

Oops.

Let's try this again?

Comment 11

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

Attachment: oops2

(In reply to comment #9)
> (indentation is off, btw)

Oh hey, reading is important. 

Sorry for all the bugspam.

Comment 12

[Dão Gottwald [:dao]]

Comment on attachment 495313 [details] [diff] [review]
oops2

Indentation is still off. The patch also won't work, because you indented the ifdef.

Comment 13

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

Attachment: oops3

Oh right.

Comment 14

[Dão Gottwald [:dao]]

Comment on attachment 495335 [details] [diff] [review]
oops3

Looks good, thanks.

Comment 15

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 495335 [details] [diff] [review]
oops3

a=beltzner

Comment 16

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/d66366f42efa