Closed Bug 1282286 Opened 4 years ago Closed 3 years ago

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

Categories

(SeaMonkey :: General, defect)

defect
Not set

Tracking

(seamonkey2.43 unaffected, seamonkey2.44 unaffected, seamonkey2.45 unaffected, seamonkey2.46 unaffected, seamonkey2.47 fixed)

RESOLVED FIXED
seamonkey2.47
Tracking Status
seamonkey2.43 --- unaffected
seamonkey2.44 --- unaffected
seamonkey2.45 --- unaffected
seamonkey2.46 --- unaffected
seamonkey2.47 --- fixed

People

(Reporter: frg, Assigned: frg, NeedInfo)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 4 obsolete files)

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
Blocks: 1284287
Blocks: 1284288
Attached patch 1282286-ForkConsole-WIP.patch (obsolete) — Splinter Review
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?
Assignee: nobody → frgrahl
Status: NEW → ASSIGNED
Attachment #8768146 - Flags: feedback?(philip.chee)
Attachment #8768146 - Flags: feedback?(iann_bugzilla)
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?
Attachment #8769418 - Flags: review?(ewong)
Attached patch 1282286-errorconsole.patch (obsolete) — Splinter Review
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?
Attachment #8768146 - Attachment is obsolete: true
Attachment #8768146 - Flags: feedback?(philip.chee)
Attachment #8768146 - Flags: feedback?(iann_bugzilla)
Attachment #8769446 - Flags: review?(philip.chee)
Attachment #8769446 - Flags: review?(iann_bugzilla)
Attached patch 1282286-errorconsole-V2.patch (obsolete) — Splinter Review
Minor fix for XP icons. Override for console-toolbar.png was not ok.

Tested with Linux and seems to be ok there.
Attachment #8769446 - Attachment is obsolete: true
Attachment #8769446 - Flags: review?(philip.chee)
Attachment #8769446 - Flags: review?(iann_bugzilla)
Attachment #8769449 - Flags: review?(philip.chee)
Attachment #8769449 - Flags: review?(iann_bugzilla)
Attachment #8769418 - Flags: review?(ewong) → review+
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.
Attachment #8769449 - Flags: feedback+
Depends on: 1288981
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.
Attachment #8769449 - Flags: review?(philip.chee)
Attachment #8769449 - Flags: review?(iann_bugzilla)
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?
Attached patch 1282286-errorconsole-V3.patch (obsolete) — Splinter Review
>> 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.
Attachment #8769449 - Attachment is obsolete: true
Attachment #8775128 - Flags: review?(iann_bugzilla)
Summary: Port changes from Bug 1278368 to Seamonkey. Error Console has been replaced by Browser Console. → Bring back Error Console to Seamonkey after removal in Bug 1278368.
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.
Attachment #8775128 - Flags: review?(iann_bugzilla) → review+
Lates round of nits taken care of.

Review+ from IanN carried forward.
Attachment #8775128 - Attachment is obsolete: true
Attachment #8775155 - Flags: review+
Could someone test next mac nightly build if its ok. If not please reopen.
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Blocks: 1289794
Blocks: 1289981
Duplicate of this bug: 1289981
Fixed for 2.47
Target Milestone: --- → seamonkey2.47
Flags: needinfo?(philip.chee)
Depends on: 1296955
You need to log in before you can comment on or make changes to this bug.