276075 - Provide Thunderbird-compatible package of Venkman

Status: VERIFIED FIXED

(Other Applications Graveyard :: Venkman JS Debugger, defect)

Description

[Nickolay_Ponomarev]

Please, make Venkman compatible with Thunderbird.

Comment 1

[James Ross]

*rolls eyes*

Comment 2

[Rob North]

Ooh someone beat me to raising this bug ;-)

I've hacked the Firefox thunderbird to make it work on Thunderbird.
I've put it on a web site here:

http://www.zen8134.zen.co.uk.nyud.net:8090/Venkman.html

Comment 3

[Rob North]

I'll need some help working out how to transform the hacked Venkman into
one that is suitable for patching the trunk (or thunderbird branch).

Comment 4

[James Ross]

Rob, does that version also work on Firefox? Mozilla?

My conclusions from hacking around at 3:30am last night was that it is not
possible to make an XPI that supports both Firefox and Thunderbird without some
major re-writing of the way it works, i.e. separate main app XUL files for each.

I'd also be interested to know what bits you changed, although I think know all
the bits that matter just to make it work in Tbird alone. :)

Comment 5

[Rob North]

No. My version only works on Thunderbird.
I think the install.js could be used to select which components are registered,
depending on the app that Venkman's being installed on.
I'm sure one of the XPIs I use does this between Mozilla and Firefox.

It's based on the 0.9.84 version for Firefox.

To build it I basically did the following:

*changed the GUID for the target app.
*Changed the target objects that venkman overlay binds to, from Firefox files to
Thunderbird files.
*Changed the menu and toolbar components that Venkman overlay binds to from
Firefox names to Thunderbird names.
*Removed an import for "chrome://browser/content/contentAreaUtils.js"
*Changes the extension description to make it clear it's only for Thunderbird.

I'll put a patch up here, showing the changes from 0.9.84 to the Thunderbird
version.

Comment 6

[Rob North]

Attachment: Diff from Venkman 0.9.84 for Firefox to Thunderbird version.

Ok here's the patch (Or rather diff) I promised. 
NOTE: For reference/enlightenment only. Not indended to be applied to CVS.

P.S. Should I have flagged this as a patch? I assume given my caveats above the
answer is no!

Comment 7

[James Ross]

Yeah, it would have to use install.js only as install.rdf simply can't do
different things for different apps... :(

I've got it working *apart* from the "Removed an import for..." issue, which is
a pain. Why missing things couldn't just be skipped I don't know.

I'm kinda tempted to create a bug to track getting trunk Venkman to work on
everything*, but in four days time I'm back to my full-time job so probably wont
be able to mess about with it so much.

* == Trunk Mozilla, Firefox and Thunderbird, plus older Moz, e.g. 1.7, and
FF/Tbird Aviary branch (1.0).

Comment 8

[Nickolay_Ponomarev]

Excuse me if I'm missing something obvious, but isn't it possible to just use
mozIJSSubScriptLoader to load the contentAreaUtils.js file conditionally?

Comment 9

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: port patch in previous attachment to CVS trunk

Rob/James: here's a port that makes the existing trunk work with Thunderbird when building with --enable-extensions=venkman.  I cloned all references to firefox into a similar directory structure for Thunderbird since I assume this is necessary for the XPIs to work.  Is this the correct thing to have done?  I'd like to make it work with Sunbird also before requesting a review...

Comment 10

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: ported patch, v2

Comment 11

[gekacheka]

Attachment: Add info to overlay thunderbird and sunbird

(patch -l -p 2 -i file.patch)

Modify venkman overlay to add JavaScript Debugger to tools menu and toolbar pallete in Thunderbird and Sunbird.

Similar to v2, but also specifies changes to content/contents.rdf, and does not create the empty content/tb/contents.rdf.

An xpi with the changes is contained in:
http://www.geocities.com/gekacheka/venkman/venkman0.9.85.sb1.xpi.zip

(I tried creating a chrome.manifest so content/contents.rdf would not be necessary, but couldn't get it to work, so this will have to do.  The manifest I tried is inside the xpi, but named "chrome.manifest.broken".)

These changes make Venkman usable in Sunbird.  Basic functionality seems to work (e.g., stopping on errors, browsing call stack).

Intermittently the debugger does not appear upon clicking "JavaScript Debugger".  Seems like this has been the case before, so it is probably not a problem with these changes.

Open problem:  It looks like help functions depend on "browser.xul" (via "getBrowserURL") to display the help files.  I don't think Thunderbird and Sunbird contain browser.xul, so another approach to displaying the files will be needed.  They are internal files with a special url protocol ("x-jsd:help"), so it cannot just pass the url to an external browser.  Perhaps it could be filed as a separate bug.

Comment 12

[Aleks Totic]

Attachment: venkman-0.9.87 patch

This is the venkman-0.9.87 source patch. You will also need chrome.manifest to complete the patch, I'll add this separately since I do not know how to get CVS to give me a diff for files not in the tree.
This patch:
- makes venkman work on firefox & thunderbird
- moves venkman to the ff 1.5 chrome.manifest (no mo RDF)
- makes -venkman command line option work again
- integrates 0.9.86 fixes that were floating on the web

Comment 13

[Aleks Totic]

Attachment: 0.9.87 chrome.manifest

This is chrome.manifest, to be placed inside venkman/xpi after the 0.9.87 patch is applied

Comment 14

[Aleks Totic]

I've just made Venkman work on Thunderbird & Firefox. The patches to the source tree are attached to this bug. It'd be good to commit the patches, so we have a refence version everyone is using again.

- makes venkman work on firefox & thunderbird 
- moves venkman to the ff 1.5 chrome.manifest (no mo RDF) 
- makes -venkman command line option work again 
- integrates 0.9.86 fixes that were floating on the web
- bumps the version number to 0.9.87

You can find the binary at:
http://www.totic.org/develop/venkman-0.9.87.xpi

Comment 15

[Karsten Düsterloh]

Comment on attachment 211825 [details] [diff] [review]
venkman-0.9.87 patch

Some nits first:
- use |diff -pu8| as your diff is very hard to read
- attach patches as patches, not as plain text
- don't use tabs, just spaces
- request reviews via the review flags, usually noone volunteers without ;-)

>Index: resources/content/command-manager.js
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/venkman/resources/content/command-manager.js,v
>retrieving revision 1.18
>diff -r1.18 command-manager.js
>235c235
><         document.documentElement.appendChild (parentElem);
>---
>>         document.firstChild.appendChild (parentElem);

This is wrong, see bug 319822.

>Index: resources/content/venkman-overlay.xul
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/venkman/resources/content/venkman-overlay.xul,v
>retrieving revision 1.8
>diff -r1.8 venkman-overlay.xul
>62c62
><   <menuitem id="venkman-menu"  position="5" label="&venkmanCmd.label;"
>---
>>   <menuitem position="5" label="&venkmanCmd.label;"

Why are you dropping the id?

>Index: resources/content/venkman-scripts.xul
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/venkman/resources/content/venkman-scripts.xul,v
>retrieving revision 1.11
>diff -r1.11 venkman-scripts.xul
>56a57
>>     <!--
>59a61
>>     -->

This will break SeaMonkey, right?

>Index: resources/content/venkman-views.js
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/venkman/resources/content/venkman-views.js,v
>retrieving revision 1.28
>diff -r1.28 venkman-views.js
>2813c2813
><     if (target && target.localName == "margin" && event.button == 0)
>---
>>     if (target && target.localName == "margin")

Why is this needed?

Comment 16

[Aleks Totic]

Attachment: Fixes for the reviewer's comments

All of your comments were valid:
BAD: document.documentElement.appendChild (parentElem);
BAD:Why are you dropping the id?
BAD:This will break SeaMonkey, right?
BAD:Why is this needed?

I started off the 0.9.86 code base that someone released on the net, and all these changes came from this release. I've now removed all these changes, and the new patch is all my code. It works in my tree.

Resubmitting for checkin

Comment 17

[James Ross]

Comment on attachment 211925 [details] [diff] [review]
Fixes for the reviewer's comments

You've used tabs all over the place. Please keep indentation consistent with existing code.

Full review comments to follow...

Comment 18

[James Ross]

Comment on attachment 211925 [details] [diff] [review]
Fixes for the reviewer's comments

>Index: js/venkman-service.js

>-const CLINE_SERVICE_CTRID =
>+const CLINE_SERVICE_CONTRACTID =

Unnecessary change. Why?

>+function openDebuggerWindow(args)

You inconsistently pass |null| or nothing to this function. Why does it have |args| at all, if nothing useful is ever passed? If passing the argument |null| or |undefined| is actually useful for anything specific, please at minimum comment at the call site or in this function explaining what these special values mean and will do that is different from nothing being passed.

>+{
>+	var ass = Components.classes[ASS_CONTRACTID].getService(nsIAppShellService);
>+	window = ass.hiddenDOMWindow;

Nit: strict JS warning: undeclared variable "window".

>+	window.openDialog("chrome://venkman/content/venkman.xul", "_blank",
>+				"chrome,menubar,toolbar,resizable,dialog=no", args);

>-CLineService.prototype.commandLineArgument = "-venkman";
>-CLineService.prototype.prefNameForStartup = "general.startup.venkman";
>-CLineService.prototype.chromeUrlForTask = "chrome://venkman/content";
>-CLineService.prototype.helpText = "Start with JavaScript Debugger.";
>-CLineService.prototype.handlesArgs = false;
>-CLineService.prototype.defaultArgs = "";
>-CLineService.prototype.openWindowWithArgs = false;

You've removed support for the old command-line system, which is a completely unacceptable change.

>+        if (cmdLine.handleFlag("venkman", false))
>+        {
>+        	openDebuggerWindow(null);
>+        }

Style: omit braces if the condition and content is only 1 line each, such as in this case.

> CLineFactory.createInstance =
>-function clf_create (outer, iid) {
>+function (outer, iid)
>+{

Changing the bracing style is acceptable (though ultimately pointless), but removing the function's name is not.

>-    return new CLineService();
>+    return new CLineService().QueryInterface(iid);
> }
> 
>+

Again, a pointless change.

>-        var ass =
>-            Components.classes[ASS_CONTRACTID].getService(nsIAppShellService);
>-        window = ass.hiddenDOMWindow;
>-
>-        var args = new Object();
>-        args.url = channel.URI.spec;
>-
>-        window.openDialog("chrome://venkman/content/venkman.xul", "_blank",
>-                          "chrome,menubar,toolbar,resizable,dialog=no", args);
>-    }
>+    	openDebuggerWindow();

You've completely lost the URI involved here. I don't know if Venkman actually uses it, but I consider the change here unacceptable.

> Module.registerSelf =
> function (compMgr, fileSpec, location, type)
> {

I cannot review the code changes in here, due to the damn tabs everwhere making lines appear different when they are not. I will review this once you've fixed the indentation throughout.

> Module.unregisterSelf =
> function(compMgr, fileSpec, location)
> {
>+   debug ("venkman unregistering");

Why?

> Module.getClassObject =
>-function (compMgr, cid, iid) {
>+function vk_mod_getClassObject(compMgr, cid, iid) {
>+	debug("venkman get class object");

The addition of the function name is acceptable, but why add the debug that is always going to get called?


>Index: xpi/install.rdf

>+		<!-- target: Mozilla Thunderbird, version 0.9 to 1.6 -->
>+	    <em:targetApplication>
>+	      <Description>
>+	        <em:id>{3550f703-e582-4d05-9a08-453d09bdfdc6}</em:id>
>+	        <em:maxVersion>1.5+</em:maxVersion>

I don't really like this value. If it works on trunk, set it to 1.6.

>+	        <em:minVersion>0.9</em:minVersion>
>+	      </Description>
>+	    </em:targetApplication>
>+	
>+	    <!-- target: Flock, version 1.0 -->

I *really* dislike this addition, but there are probably no grounds I can actually disallow it on.

>-		<em:homepageURL>http://www.hacksrus.com/~ginda/venkman/</em:homepageURL>
>+		<em:homepageURL>http://www.mozilla.org/projects/venkman/</em:homepageURL>

Unacceptable change (and no explanation, too!).


>Index: xpi/makexpi.sh

> sed "s|@REVISION@|$VERSION|g" < $XPIFILES/install.rdf > $XPIROOT/install.rdf
>+cp $XPIFILES/chrome.manifest $XPIROOT/chrome.manifest
> echo   ".       done"

You should add a "echo -n ." line and reduce the spaces before the word "done" by one, in line with the code throughout this file.

> 
>-

Why are you making so many random changes?

> echo   ".  done"
> 
> 
>+

Ditto.

Comment 19

[Aleks Totic]

Comment on attachment 211925 [details] [diff] [review]
Fixes for the reviewer's comments

Replaced by the new patch

Comment 20

[Aleks Totic]

Attachment: Hopefully final revision for Thunderbird 0.9.87 patch

This patch is in response to the review comments. All complaints should be fixed, and the changes to the original source have been kept to a minimum.

- there are no tabs in the file besides the ones that were there already
- CLINE_SERVICE_CTRID is back to what it was. Personally, I thought CLINE_SERVICE_CONTRACTID was more readable, and consistent with other moz code.
- Fixed: Nit: strict JS warning: undeclared variable "window". 
- Fixed: You've removed support for the old command-line system. Back in, thought that old system was obsolete but I guess SeaMonkey still uses it?
>> + return new CLineService().QueryInterface(iid); 
- this remains as is. It is valid code, considering we can return different classes depending upon request.
>> You've completely lost the URI involved here. I don't know if Venkman actually uses it, but I consider the change here unacceptable.
Fixed: Looks like Venkman never used it, but I moved the arguments back for consistency with the older code.
>> You should add a "echo -n ." line and reduce the spaces before the word "done" by one, in line with the code throughout this file.
That line was there before, kept as is.

Regarding the homepage URL, the http://www.hacksrus.com/~ginda/venkman/ was a neglected child, out of date regarding compatibility, net patches such as the getahead.co.uk patch for ff 1.5. Unless someone takes ownership of hacksrus again, I think http://www.mozilla.org/projects/venkman/ is a better choice.

BTW, harsh review process, my first intro to the mozilla code after a while. But most of the comments I agree with. Personally, I'd have just checked stuff in if it worked, as it was an improvement over the bit-rot from before. But, the result is better code overall.

Comment 21

[James Ross]

Sorry if the review comments seemed overly harsh. As you say, we do this whole review thing so that the code that does go in is of a higher quality.

I do want to have a look through the new patch, as I'm not entirely sure my comments made sense (judging from your comments), and just check it's ok.

Comment 22

[Aleks Totic]

Attachment: Use this patch for review, the other one came down with a flu

Old patch just smelled funny. Use this one instead.

Comment 23

[timeless]

Comment on attachment 212662 [details] [diff] [review]
Use this patch for review, the other one came down with a flu

> function findDebuggerWindow ()
you're changing the behavior of this function. i hope that's ok.

>+/* nsISupports */
>+CLineService.prototype.QueryInterface =
>+function handler_QI(iid)
>+{
I'm not fond of this format, you're calling iid.equals(null) in some cases. I don't even remember how well that works.
>+        iid.equals(nsICommandLineHandler) ||
>+        iid.equals(nsICmdLineHandler))

personally, i prefer partly implementing classinfo and iterating over the array to implement QI.

>+function handler_handle(cmdLine)
>+{
the indentation here is not consistent:
>+    try
>+    {
      4
>+        if (cmdLine.handleFlag("venkman", false))
          4
>+             openDebuggerWindow(null);
               5
>+   }
     3
>+    catch (e)
      4

>+CLineService.prototype.helpInfo =
>+ "  -venkman         Starts a Javascript debugger.\n"
You misspelled JavaScript. And it's not just some random JavaScript Debugger, it's Venkman. It could be that other such debuggers are available for the process which means your help should clearly indicate which one is being started, the command names will be viewed by users as random.

>@@ -444,19 +480,18 @@ function jsdh_handle(contentType, window
>-        window.openDialog("chrome://venkman/content/venkman.xul", "_blank",
>-                          "chrome,menubar,toolbar,resizable,dialog=no", args);
>-    }
      4
>+        openDebuggerWindow(args);
          4
>+     }
       5
please don't mess up indentation.

>+    catman.addCategoryEntry("command-line-handler",
>                             "venkman command line handler",
>                             CLINE_SERVICE_CTRID, true, true);
> 
>+
why did you add this line?
>     debug("*** Registering x-jsd protocol handler.\n");

>+    catman.deleteCategoryEntry("command-line-argument-handlers",
>+                               CLINE_SERVICE_CTRID, true);
>+    catman.deleteCategoryEntry("command-line-handler",
>                                CLINE_SERVICE_CTRID, true);

While deleteCategoryEntry doesn't throw today, i don't actually trust it not to and would rather we wrapped the calls in tries (silver is free to overrule me here).

> <!-- Firefox -->
> <menupopup id="menu_ToolsPopup">
>   <menuitem id="tb-venkman-menu" oncommand="start_venkman()" 
>       insertafter="javascriptConsole,devToolsSeparator"
>       label="&vnkMenu.label;" accesskey="&vnkMenu.accesskey;"/>

>+<!-- Thunderbird -->
>+<menupopup id="taskPopup">
>+  <menuitem id="tb-venkman-menu" oncommand="start_venkman()" 
>+      insertafter="javaScriptConsole,devToolsSeparator"
>+      label="&vnkMenu.label;" accesskey="&vnkMenu.accesskey;"/>
This isn't legal, you're defining two nodes with the same id in the same file, that's an xml violation.

I don't see any justification for this. you didn't change any strings:
>-const __vnk_version        = "0.9.85";
>-const __vnk_requiredLocale = "0.9.85";
>+const __vnk_version        = "0.9.87";
>+const __vnk_requiredLocale = "0.9.87";

> 		<!-- target: Mozilla Firefox, version 0.9 to 1.6 -->
...
> 		</em:targetApplication>
>-		
>+
>+		<!-- target: Mozilla Thunderbird, version 0.9 to 1.6 -->
this can't be properly aligned, event the comment has a different starting column
>+	    <em:targetApplication>

>+cp $XPIFILES/chrome.manifest $XPIROOT/chrome.manifest
> echo   ".       done"

you ignored silver's -n comment. heck. you clearly ignored silver's previous whitespace complaints.

Comment 24

[Aleks Totic]

Wow, another rejection. I give up. The code works, with no runtime errors, which has not been the case for about a year now. Correcting a few whitespace errors, the dual id def inside xul file problem is not worth the effort. Hopefully, it'll be a good starter code for someone.

Comment 25

[Brendan Eich [:brendan]]

(In reply to comment #24)
> Wow, another rejection. I give up. The code works, with no runtime errors,
> which has not been the case for about a year now. Correcting a few whitespace
> errors, the dual id def inside xul file problem is not worth the effort.

Valid XML is a virtue by itself, I hear ;-).

> Hopefully, it'll be a good starter code for someone.

I'm not sure for whom, though.  If you aren't willing to fix all the nits picked and do what the module owner and a peer ask, it's going to be hard to get your patch in.  Nit-picking is part of good ownership, to keep code hygiene high.  The more substantive comments seem like good ownership to me to.

On the other hand, something is wrong when Venkman doesn't work with the newer XUL apps out of the box.  That state of affairs can be due to many reasons, not all Venkman owners/peers' fault, but something is being owned badly if this bug won't be fixed until the volunteer contributes exactly the right patch.  Silver, why not use what's good in the patch and check it in yourself?

/be

Comment 26

[James Ross]

Aleks, your effort is very much appreciated. If you don't wish to complete the standard process of patch review that's up to you, but I hope you understand both that this process is required, and that your work so far will not all be in vein.

Brendan, I will review what is good and what is not this weekend (currently I'm at the other end of the UK from my computer). The problem for me is that I am very stretched for time between projects, and have a hard enough job just making time for the review comments here. Never the less, I will set aside as much as I can this weekend on sorting this bug out.

Venkman has been effectively unowned for months (and still is in many respects); getting it back on track is hard work, and I've already been let down a number of times by the Mozilla Foundation with regard to support for this.

Comment 27

[Aleks Totic]

> This isn't legal, you're defining two nodes with the same id in the same file, 
> that's an xml violation. 

True. I forgot about the duplicate id rule, and the parser did not complain. The 2nd id can be changed to tb-venkman-menu2. File this under "this will break when we use a stricter parser"

> I don't see any justification for this. you didn't change any strings: 
>-const __vnk_version = "0.9.85"; ]
>-const __vnk_requiredLocale = "0.9.85"; 
>+const __vnk_version = "0.9.87"; 
>+const __vnk_requiredLocale = "0.9.87";

This is because venkman complains when its version and locale version do not match.

In general, what got me frustrated is that when I used to receive patches, if explaining the problems took longer than fixing them myself, I'd just fix it and check the code in. This is certainly true of minor whitespace/spelling stuff.

My 1st attempt was obviously broken in many ways (tabs, etc). 2nd had a fatal flaw of removing support for old CmdLine style (which I am still not sure why is it needed, I thought this was a good thing since it made code cleaner). 3rd one was good with the exception of duplicate id in XUL file. The old code was not exactly up to TeX quality either. There are a few tabs in the original file, total lack of comments, and if you compare it to the code that does the same in other extensions, they are very inconsistent. Oh, and venkman spews endless vnk: dbg statements on command line when running in debug. So, my few rough edges fit in nicely.

The whole process felt like a giant pain, when I was just trying to be nice. Since Venkman has been unusable on Tb for so long, I figured that anything that fixes it would be happily accepted. Instead, I get "Javascript" spellcheck.

I'll post another patch when idle cycles accumulate. Funny thing is, creating the new patch takes less time than writing this comment, but I just can't bear to see the poor code picked apart again.

Comment 28

[Brendan Eich [:brendan]]

(In reply to comment #26)
> Venkman has been effectively unowned for months (and still is in many
> respects); getting it back on track is hard work, and I've already been let
> down a number of times by the Mozilla Foundation with regard to support for
> this.

I don't know what happened here -- can you mail me some details?  I'll try to get this fixed.

/be

Comment 29

[Wayne Mery (:wsmwk)]

(In reply to comment #28)
> (In reply to comment #26)
> > Venkman has been effectively unowned for months (and still is in many
> > respects); getting it back on track is hard work, and I've already been let
> > down a number of times by the Mozilla Foundation with regard to support for
> > this.
> 
> I don't know what happened here -- can you mail me some details?  I'll try to
> get this fixed.
> 
> /be

Is there a workaround to get venkman working with *trunk* build (for those of us that use windows binaries)? 
[http://www.zen8134.zen.co.uk.nyud.net:8090/Venkman.html won't install]

With respect to venkman working in Thunderbird on an ongoing basis ... given the Moz's long standing position of the strategic importance of extensions, more recently formalized with new focus and plans [1], I would think venkman would be a priority and this bug in particular would be critical, or at least major. 

An additional data point, http://www.geekrant.org/category/internet/mozilla/extensions/ (IMO a not a rant) argues Thunderbird is a stepchild to FF when it comes to support of extension development. In the context of venkman's status I certainly agree. 

[1]
* http://weblogs.mozillazine.org/mitchell/archives/2006/04/extensions_developers_mike_sha_1.html
* http://shaver.off.net/diary/2006/04/04/fertilizer/

Comment 30

[James Ross]

Thunderbird is a bastard child of XPFE and Toolkit as well.

Comment 31

[Brendan Eich [:brendan]]

I don't care what begot what app, and its legitimacy status by someone's lights.

I do care about firefox -venkman and thunderbird -venkman working.  That would be great.  If the patch is 90% right, why not help it get in?  How is shooting it down again a good thing?  It's not like the direction and even most of the patch content are bad, right?

Silver: if you mailed me details about the Foundation not doing right by you, I may have lost them in a big bad disk corruption incident the other week.  Please resend, or send if you haven't.

/be

Comment 32

[James Ross]

There are no details to mail you with, it's just people's words backed up by nothing. I think "let down" in the original comment was the wrong wording to use, it was more like "not backed up by". That doesn't help us here, though...

As for the patch, I think it is trying to 'fix' too much at once. I certainly don't have time to review each revision of a patch that big that often, and I stand by the majority of timeless' comments on the latest patch.

I'm not helping by fixing the patch because I don't have *time* to do that. I pointed out all the things I saw wrong with it, and they were not all fixed (or rebutted) so I don't know what good re-iterating those problems will do.

If it is wanted so much (I sure don't need Venkman in TBird), I think it will have to be split up in to things that can be reviewed in less time. Starting with just one bit would help, too, as some of the problems existed throughout the entire patch (e.g. whitespace/indentation issues).

If I'm sleepless tonight, maybe I will work some smaller bits of this patch into something workable, but I really should be sleeping.

Comment 33

[James Ross]

That reminds me.

Aleks, you've not included chrome.manifest in the diff but have added it to the XPI-making code. Could you include it somewhere and explain why you need it?

Comment 34

[James Ross]

Ok, so on close inspection of the last patch it doesn't look nearly as bad as I remembered. It does still have issues, like duplicate IDs, bad indentation, needless changes, and some as-yet unexplained bits.

I'll try and give some review comments for what I can tonight. If I can get the patch to apply to CVS at all, I might have time to fix up the issues myself, we'll just have to see.

Comment 35

[James Ross]

While Brendan may not care about which app begot which, I've just found out why it really matters.

 - The Thunderbird Tools menu has id="taskPopup".
 - The Mozilla Suite/SeaMonkey Tools menu has id="taskPopup".

Venkman appears (correctly) in the Web Development submenu of Tools in SeaMonkey, but - and this is where the problem is - it also ends up at the bottom of the main Tools menu, thanks to the Thunderbird menu item in the overlay. (I get a strong deja vu mentioning this, but don't know where or when I might have mentioned it before.)

If I understand the many colours ChromeReg has been through over the years (and branches), we can apply a new (Thunderbird specific) overlay file just for Thunderbird via chrome.manifest on the 1.5 branch and newer branches, right?

An alternative (and much more a hack) would be to hide (or add) the menu item via JavaScript, which stands some (small) chance of knowing if it is Mozilla Suite/SeaMonkey or Thunderbird.

Comment 36

[James Ross]

Ok, I've got a working set of JS code to nuke the TBird menu item if it finds it isn't running on TBird (thank you nsIXULAppInfo!). Patch is a sec...

Comment 37

[Benjamin Smedberg]

> branches), we can apply a new (Thunderbird specific) overlay file just for
> Thunderbird via chrome.manifest on the 1.5 branch and newer branches, right?

Correct.

Comment 38

[James Ross]

Attachment: [checked-in] Complete patch v3

Ok, so this is for the most part the previous patch (thanks again for the effort you have contributed Aleks). I've just fixed all my review comments (I think!), and sorted out the menu issue (appologies to bsmedberg for not using chrome.manifest here).

With this, it works on Thunderbird 1.5/1.0, Firefox 1.5/1.0, SeaMonkey 1.0 and Mozilla Suite 1.7.x (and should continue working on all versions back to 1.0, as well as trunk).

Comment 39

[James Ross]

Small note: on the 1.8 branch (i.e. FF/TB 1.5) you cannot re-open Venkman after closing it (this is not a new issue with this patch). No idea why, but it works fine on trunk, meaning I can't debug it.

Comment 40

[Aleks Totic]

>Aleks, you've not included chrome.manifest in the diff but have added it to the
>XPI-making code. Could you include it somewhere and explain why you need it?

This line: 

RCS file: /cvsroot/mozilla/extensions/venkman/xpi/makexpi.sh,v
+cp $XPIFILES/chrome.manifest $XPIROOT/chrome.manifest

makes sure that the file gets into the xpi.

This is the new chrome.manifest way of extension packaging that replaces contents.rdf.

> Ok, I've got a working set of JS code to nuke the TBird menu item if it finds 
> it isn't running on TBird (thank you nsIXULAppInfo!). Patch is a sec...

Sorry about the menu item appearing twice in SeaMonkey, I did not test this. An alternative way (involving no coding) would be to split tbird overlay in a separate file (venkman-overlay-tb.xul), and replace the following line in chrome.manifest:

overlay chrome://messenger/content/mailWindowOverlay.xul chrome://venkman/content/venkman-overlay.xul

with 
overlay chrome://messenger/content/mailWindowOverlay.xul chrome://venkman/content/venkman-overlay-tb.xul

I've attached the new overlay files/chrome.manifest as a zip file if you are interested. Tested in ff1.5/tb1.5/seamonkey1.5.

Aleks

PS: Looks like everything finished while I was writing this, so you can just ignore this comment. Happy this made it in.

Comment 41

[Aleks Totic]

Attachment: Alternative overlays to avoid duplicate menus in SeaMonkey

Alternative overlay strategy for Thunderbird.

Comment 42

[timeless]

Comment on attachment 217514 [details] [diff] [review]
[checked-in] Complete patch v3

>Index: resources/content/venkman-overlay.js
>@@ -39,6 +39,37 @@
>+function isThunderbird()
>+{
>+    var cls = Components.classes[XULAPPINFO_CONTRACTID];
>+    if (!cls)
>+        return false;
>+
>+    var appinfo = cls.getService(nsIXULAppInfo);
this will throw on failure, it will never return null
>+    if (!appinfo)

please fix that

Comment 43

[James Ross]

Comment on attachment 217514 [details] [diff] [review]
[checked-in] Complete patch v3

Checked in with try/catch fix for getService throwing a hissy.

Comment 44

[James Ross]

I have realised that nsIXULAppInfo doesn't work on Thunderbird 1.0, but since that's reaching its EOL and only the menu item is affected (-venkman will still work AFICT), it doesn't matter. If anyone wants to contribute a fix for the function isThunderbird that supports TB 1.0, feel free. I think this bug is - finally - dead.

I'll be pushing the XPI into Addons now, although I fully expect it to be 2 or 3 days before it gets approved, due to their current backlog.

Comment 45

[James Ross]

Slightly ahead of expected schedule, 0.9.87 is now live at:
  https://addons.mozilla.org/firefox/216/

Share and enjoy.