User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.1; de; rv:1.8.1.3) Gecko/20070309 Firefox/2.0.0.3 Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; de; rv:1.9a1) Gecko/20070213 Sunbird/0.3.1 A command line handler added by an extension should be able to suppress the main Sunbird window from opening by setting cmdline.preventDefault to true. This works for other Mozilla applications, but not for Sunbird. The effect can be reproduced with all extensions that try to use that mechanism, for example "Minimize to Tray" or "XPCOMViewer". While XPCOMViewer isn't really hurt by this behaviour, The command line switches of Minimize to Tray (and any other extension that tries to do something in the background) are made effectively useless. Reproducible: Always Steps to Reproduce: 1.Install XPCOMViewer Extension in Sunbird. 2.Open Sunbird normally. 3.call "sunbird -xpcomviewer" from the command line Actual Results: In addition to the running Sunbird window, the xpcomviewer-Window is opened, as expected. But in addition to that, a second Sunbird main window appears! Expected Results: Only the xpcomviewer-Window should open. As written above, Sunbird for some reason does not evaluate the preventDefault-Flag of command line handlers.

1. I can confirm this with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.4pre) Gecko/20070502 Calendar/0.5pre. 2. I investigated the problem and cleaned up/reworked the code in calendarService.js. I already have a calendarService.js in my local tree, that works as expected.

With this patch I propose to remove the old calendarService.js and add a new calCmdLineHandler.js that is basically the same Firefox and Thunderbird use. The old calendarService.js also contained some leftovers from ICALProtocolHandler (which was replaced with calProtocolHandler.js) and an in my opinion unused ICALContentHandler (back from the days Calendar was an extension for Firefox and you could click on webcal links). And finally... it solves this bug. :)

Great! Thank you. Will this patch go into Sunbird 0.5 or a later version?

No, this patch will not make 0.5. Now for some real comments: The new file should not live in resources/content. It's not a content file, it is a component. I think it should be in base/src, next to all the other components. And what I don't see: How does it fix the bug? (that's why I prefer to split rewrites from bug fixes)

Comment on attachment 264288 [details] [diff] [review] Patch v1 setting this to r-, to try to get answers to my questions. (and to get it off my list)

Well, have you read my bug description? Martin's patch moves the opening of Sunbirds main window into a command line handler that can be suppressed by other command line hanlers by setting the appropriate flag - as it should be. You are of course right about the new file beeing a component, but the old file also was a component and lived in resources/content... so that is more a matter of taste.

And what is 'the appropriate flag'? I can't tell it by just looking at the patch. That's why i said that i prefer to split the rewrite form the fix) The preventDefault flag that was mentioned in comment 0 is used in the new file only once, and it is in the old file in exactly the same place. So I don't think that's the trick. And _if_ the code is being moved to a new file, the new file should be in the right location. The code has always lived in the wrong directory. That has been a bug. Not fixing that bug while creating a new file would be the wrong thing to do. That way, the code will ilkely never be put in the right place. I'd like to prevent that.

Well, you are absolutely right, the reason is non-obvious. After much head-scratching and testing I just found out what really is the important difference between the patch and the original: The current calendarService.js adds the command line handler TWICE, once to the category "command-line-handler" (correct!), once to the category "command-line-argument-handlers" - this causes the bug and is completely superfluous! So, would you like a minimal patch that just removes the offending lines from the current calendarService.js? Moving the file to the correct directory can then be left to a structural cleanup.

The two categories are there for a reason: one is for the old xpfe stuff, and the other for newer toolkit apps. I think we can now indeed remove the old xpfe code, although it must be tested with both sunbird and lightning. Also, is that really what fixes the bug? It would surprise me. Another change I noticed is the change of name, from m-calendar to x-default. I'd say that that means the new code is now default, and preventDefault knows what to prevent.

We don't support older than Thunderbird 1.5, which is new toolkit, right. And yes, it fixes the bug. The name x-default does not matter - I tested this first - so be surprised. ;-) The preventDefault line is not needed at all for Sunbird, because it is there to prevent the default handler from executing - which made sense when we were an extension, but not now. You didn't answer the question: do you want a minimal patch or not?

I don't really care about a minimal patch or a full patch. All I want to know is what actually was changed, and how it fixes the bug. And what I don't want, is a half patch. Either we leave the code in the same file, or we move it to the right location. Moving to a new file in the wrong location is not right.

OK, here is a patch that modifies calendarService.js in place. Necessary is the change to remove the old handler category; the renaming to x-default and the removal of unneeded attributes is purely cosmetic as well as the senseless setting of the preventDefault-Flag - this IS the default handler, there is nothing to suppress afterwards. This fixes the bug, because the handler was actually called twice, the second time beeing suppressed by preventDefault having been set by the first call. The call to the handler via "command-line-argument-handlers" could not be suppressed by other handlers setting preventDefault to true. Lightning is not affected by this patch because it doesn't have this file.

Comment on attachment 272496 [details] [diff] [review] New version of bugfix only modifying calendarService.js Norbert, don't forget to ask for review when attaching new patches.

Oops, thank you. It is only my second patch; I'm not used to this buerocracy yet. ;-)

Double oops: first version contained stupid errors even when it solved the problem; this one is better.

Comment on attachment 272758 [details] [diff] [review] Reworked Version of the patch This patch does not fix the bug. I tested it, and I still got two windows. That didn't really surprise me, because it just removes unused code. What could that do? What I did find out was that removing the setting of preventDefault made two sunbird windows show up. That made me figure out how things really should work. The idea is that there is one default commandline handler [1]. That handler opens a windows which is specified by a pref, toolkit.defaultChromeURI. Now, the sunbird commandline handler also opens a window. It sets the preventDefault flag to ensure that the toolkit default handler doesn't also open a window. The point is that preventDefault is no magic thing. It is something that the command line handlers should test for, if they want to be a default. The sunbird handler doesn't do that. It just opens the window, no matter what. One possible way to fix the bug here would be to make the handler check before opening a window. A different approach would be to leave the opening of the window to the default handler, and have the sunbird handler only do the parsing of the real command line options. PRoblem with that approach is that it is a bit harder to get the options into the sunbird window javascript. Not setting a review flag, because the patch itself doesn't look wrong. It's good cleanup. It just doesn't fix the bug here. Need to decide if it should be in different bug. [1] http://lxr.mozilla.org/seamonkey/source/toolkit/components/nsDefaultCLH.js

Well, you are right. It turns out that I apparently forgot to delete compreg.dat and xpti.dat in my Sunbird profile each time I modified my patch, and this ruined my testing. So now here is a version of the patch that fixes the bug and changes nothing else. The cleanup will have to be done elsewhere though. Note, however, that adding a test for the preventDefault-Flag is not enough. It is also mandatory to rename the handler to x-default, else the fix does not work. This is probably due to the order in which the handler routines are being called. Please note also that the handler still needs to set the preventDefault-Flag when it opens the Sunbird main window or else a second small window without contents will open additionally.

Comment on attachment 279444 [details] [diff] [review] Again reworked version of the patch yeah, this looks good. r=mvl

Checked in on HEAD and MOZILLA_1_8_BRANCH -> FIXED

Filed bug 395511 for the clean up of calendarService.js.