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.

+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...)

>> 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

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

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?

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

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?

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

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.

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

Agreed to both points.

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?

>> 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?

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?

Minor fix for XP icons. Override for console-toolbar.png was not ok. Tested with Linux and seems to be ok there.

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.

(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?

(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. ;)

>> Why not ifdef? Only two lines which can be put back easily. FRG

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

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

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?

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

Lates round of nits taken care of. Review+ from IanN carried forward.

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

Fixed for 2.47