1282286 - Bring back Error Console to Seamonkey after removal in Bug 1278368.

Status: RESOLVED FIXED

(SeaMonkey :: General, defect)

Description

[Frank-Rainer Grahl (:frg)]

Bug 1278368 removed the error console. make installer is currently broken because of it. 

We need to either do this too or fork the code over to suite. I am for forking if feasible. The new one has much more functionality but is clumsy to use if you only want to check things out quick.

Comment 1

[rsx11m]

+1 for forking. So, the idea is to keep the old error console in parallel to the new devtools?
Similar to the DOM Inspector? (though that one is an extension anyway...)

Comment 2

[Frank-Rainer Grahl (:frg)]

>> So, the idea is to keep the old error console in parallel to the new devtools?

That would be my plan. Like it is now in 2.45 and 2.46.

FRG

Comment 3

[Frank-Rainer Grahl (:frg)]

I have started with a fork. Needs some tinkering before a patch is ready. If the decision later is to remove it I will just use it in my private tree :)

FRG

Comment 4

[Frank-Rainer Grahl (:frg)]

Attachment: 1282286-ForkConsole-WIP.patch

WIP patch. Works with classic theme. Will likely not work with modern theme. Testing this next.

Paths might not be optimal. 

Took out an ifdef in custom.css for windows. Would need a preprocessor step otherwise and already has a -moz-os-version: windows-xp in it so I think it's not needed.

Questions: Where should the icons reside finally. Chrome locations ok?

Comment 5

[Frank-Rainer Grahl (:frg)]

Oh I forgot. Should we keep the Ctrl+Shift+J access key as it is taken by the new console?

Comment 6

[David E. Ross]

Bug reports against the old Error Console were recently closed because Firefox's new Browser Console supposedly does not have those problems.  With the forking to retain Error Console in SeaMonkey, will those bug reports be reopened?  More important, will any of them be fixed?

Comment 7

[rsx11m]

My guess is we'd have to reopen/refile them as SeaMonkey bugs.

Comment 8

[Frank-Rainer Grahl (:frg)]

I am against just reopening and assigning them to Seamonkey. When I look at old Seamonkey bugs I see a lot which might no longer be valid. Lets first port it and then new ones can be filed if necessary in followp bugs.

Comment 9

[Frank-Rainer Grahl (:frg)]

Btw. The devtools browser console will be there too so if you need new functionality please file a bug against it. I just want the old one around for quick checks if something is wrong. The fancy stuff should go in the new console.

FRG

Comment 10

[rsx11m]

Agreed to both points.

Comment 11

[Edmund Wong (:ewong)]

Comment on attachment 8768146 [details] [diff] [review]
1282286-ForkConsole-WIP.patch

Review of attachment 8768146 [details] [diff] [review]:
-----------------------------------------------------------------

shouldn't there be entries in the suite/installer/package-manifests.in?

i.e.
@RESPATH@/components/jsconsole-clhandler.manifest
@RESPATH@/components/jsconsole-clhandler.js

or something like that?

Comment 12

[Frank-Rainer Grahl (:frg)]

Attachment: 1282286-interimfix.patch

>> shouldn't there be entries in the suite/installer/package-manifests.in?

They are still there. The patch doesn't have them because they were currently not removed. 

But that also means that make installer will fail right now. How about an interim patch until the console is ready for action again?

Comment 13

[Frank-Rainer Grahl (:frg)]

Attachment: 1282286-errorconsole.patch

This one works. Set windowtype to suite:console from global:console and changed -jsconsole command line parameter to -oldconsole so that it does not interfere with the Borwser console (which did over ride this starting with 2.45 anyway).

Need to test on Linux and Windows XP. Anyone able to do and test an osx build?

Comment 14

[Frank-Rainer Grahl (:frg)]

Attachment: 1282286-errorconsole-V2.patch

Minor fix for XP icons. Override for console-toolbar.png was not ok.

Tested with Linux and seems to be ok there.

Comment 15

[Frank-Rainer Grahl (:frg)]

Installer temporary fix

https://hg.mozilla.org/comm-central/rev/2a96f2cb7620

I will put it back into the main patch once I got some feedback on this one.

Comment 16

[Matt A. Tobin]

(In reply to Frank-Rainer Grahl from comment #15)
> Installer temporary fix
> 
> https://hg.mozilla.org/comm-central/rev/2a96f2cb7620
> 
> I will put it back into the main patch once I got some feedback on this one.

Why not ifdef?

Comment 17

[Hartmut Figge]

(In reply to Frank-Rainer Grahl from comment #15)
> Installer temporary fix
> 
> https://hg.mozilla.org/comm-central/rev/2a96f2cb7620
> 
> I will put it back into the main patch once I got some feedback on this one.

I have used that since June 29. ;)

Comment 18

[Frank-Rainer Grahl (:frg)]

>> Why not ifdef?

Only two lines which can be put back easily.

FRG

Comment 19

[Ian Neal]

Comment on attachment 8769449 [details] [diff] [review]
1282286-errorconsole-V2.patch

This looks good to me and certainly works, but I would like to have history imported from the original files. I think Ratty may have done it before but there is also a few guides available on how to do it.
f+ for the moment.

Comment 20

[Ian Neal]

Comment on attachment 8769449 [details] [diff] [review]
1282286-errorconsole-V2.patch

Now the pain of file copy/history is over, this patch can be rebased. Thanks.

Comment 21

[Ian Neal]

File copy/history landed as https://hg.mozilla.org/comm-central/pushloghtml?changeset=fa786e755299

Comment 22

[Ian Neal]

Comment on attachment 8769449 [details] [diff] [review]
1282286-errorconsole-V2.patch

I did flatten the structure so that console/content/ -> console/ but if you prefer to have a content/ folder then that is not an issue.
>+++ b/suite/common/console/jsconsole-clhandler.js
>@@ -0,0 +1,40 @@
>+/* -*- indent-tabs-mode: nil; js-indent-level: 4 -*- */
>+/* vim:sw=4:sr:sta:et:sts: */
Nit: This line is incomplete, my editor complains that */ is an unknown option for sts

>+    if (!cmdLine.handleFlag("oldconsole", false))
Maybe errorconsole instead of oldconsole?

>+      wwatch.openWindow(null, "chrome://communicator/content/console/console.xul", "_blank",
Nit: bring "_blank", onto the following line.
>+                        "chrome,dialog=no,all", cmdLine);

>+  helpInfo : "  --oldconsole        Open the Error console.\n",
Maybe errorconsole instead of oldconsole?

>+++ b/suite/common/console/jsconsole-clhandler.manifest
>+category command-line-handler t-jsconsole @mozilla.org/suite/console-clh;1
Should this be changed from "t-jsconsole" to something like "s-errorconsole"?

>+++ b/suite/common/console/tests/.eslintrc
This didn't make into the file copy, do we actually want it?

>+++ b/suite/common/console/tests/chrome.ini
>@@ -0,0 +1,4 @@
>+[DEFAULT]
>+skip-if = buildapp == 'b2g'
Does this make any sense to us?

>+++ b/suite/common/console/tests/test_hugeURIs.xul
>@@ -0,0 +1,64 @@
>+<?xml version="1.0"?>
>+<?xml-stylesheet type="text/css" href="chrome://global/skin"?>
global or communicator?
Have you tested this test?

>+++ b/suite/common/jar.mn
>-% overlay chrome://global/content/console.xul chrome://communicator/content/consoleOverlay.xul
>+% overlay chrome://communicator/content/console/console.xul chrome://communicator/content/consoleOverlay.xul
Could we not merge consoleOverlay.xul into console.xul now? (and associated files) - maybe in a follow-up patch so we can get this landed quickly.

> % overlay chrome://help/content/help.xul chrome://communicator/content/helpOverlay.xul
Similarly for this in helpviewer?

Comment 23

[Frank-Rainer Grahl (:frg)]

Attachment: 1282286-errorconsole-V3.patch

>> I did flatten the structure so that console/content/ -> console/ but if you 

I am fine with it.

>> Maybe errorconsole instead of oldconsole?

I choose option 3 and named it suiteconsole. errorconsole is used in m-c for the devtools console. But the window and menu still say 'Error Console' so you decide if suiteconsole is ok or just use errorconsole.

>> Should this be changed from "t-jsconsole" to something like "s-errorconsole"?
I left it as it is. Changing the manifest file was educated guesswork anyway and it corresponds to the source name this way:) 

>> >+++ b/suite/common/console/tests/.eslintrc
>> This didn't make into the file copy, do we actually want it?

I removed the tests dir. The fix made it a long time ago into the tree and I doubt we will make any further changes to the console unless its broken.

>> Could we not merge consoleOverlay.xul into console.xul now?

I will do a follow-up patch.

Comment 24

[Ian Neal]

Comment on attachment 8775128 [details] [diff] [review]
1282286-errorconsole-V3.patch

>+++ b/suite/common/console/console.css
>@@ -1,66 +1,66 @@
> /* This Source Code Form is subject to the terms of the Mozilla Public
>  * License, v. 2.0. If a copy of the MPL was not distributed with this
>  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> 
Nit: Extra blank line.
> 
> .console-box {

>+++ b/suite/common/console/console.xul
>-<?xml-stylesheet href="chrome://global/skin/" type="text/css"?> 
>-<?xml-stylesheet href="chrome://global/skin/console/console.css" type="text/css"?> 
>-<?xml-stylesheet href="chrome://global/content/console.css" type="text/css"?> 
>+<?xml-stylesheet href="chrome://communicator/skin/" type="text/css"?>
>+<?xml-stylesheet href="chrome://communicator/content/console/console.css" type="text/css"?>
>+<?xml-stylesheet href="chrome://communicator/skin/console/console.css" type="text/css"?>
Why the switch in order?

>+++ b/suite/common/console/consoleBindings.xml
>@@ -1,24 +1,24 @@
> <?xml version="1.0"?>
> <!-- This Source Code Form is subject to the terms of the Mozilla Public
>    - License, v. 2.0. If a copy of the MPL was not distributed with this
>    - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
> 
Nit: Extra blank line.
> 
>-<!DOCTYPE bindings SYSTEM "chrome://global/locale/console.dtd">

>+++ b/suite/themes/classic/jar.mn

>+#ifdef XP_WIN
>+% override chrome://communicator/skin/console/console-toolbar.png   chrome://communicator/skin/console/console-toolbar-XP.png      osversion<6
>+#endif
>\ No newline at end of file
Nit: can this be fixed whilst you're here?

r=me with those addressed.

Comment 25

[Frank-Rainer Grahl (:frg)]

Attachment: 1282286-errorconsole-V3a.patch

Lates round of nits taken care of.

Review+ from IanN carried forward.

Comment 26

[Frank-Rainer Grahl (:frg)]

https://hg.mozilla.org/comm-central/rev/15822e1bf8c8

Comment 27

[Frank-Rainer Grahl (:frg)]

Could someone test next mac nightly build if its ok. If not please reopen.

Comment 29

[Rainer Bielefeld]

Fixed for 2.47