nsAppRunner.cpp currently contains various routines like DoCommandLines which are generally useful as they can handle tasks like opening all the windows the user wants opening on startup (ie they read the preferences and act on them). eg, Mac needs these to handle the re-open events sent by the OS when the application is clicked in the dock. The trouble is its a lot of C function inside xpfe/bootstrap/nsAppRunner.cpp, and thus not callable from elsewhere in the application. I propose to move this stuff out into bootstrap/appshell and expose it (probably not via public/idl, but I'm open to the suggestion) to other components in the application. There's actually a comment in the source code suggesting this should be done one day, so I guess today is the day. Here is what will need doing: 1. Move the C functions 2. Make them extern 3. Decide where to put the headers, public or src 4. On Mac OS we need to move AppShell{Debug}.shlb from dist/components to dist/ Essential Files. 5. Test on Windows and Linux, make sure all the above works there too.

>4. On Mac OS we need to move AppShell{Debug}.shlb from dist/components to dist/ Essential Files. No can do. We don't want to change an XPCOM component DLL into something you link with. That's bad.

OK, I need an interface so I can call this code. Startup needs this code to. So I can only see 2 possibilities: Duplicate the code into widget/mac and leave a copy in apprunner. Move the code into widget: 1/ widget is already linked in with startup 2/ widget deals with windows, as does this code 3/ I can remove all the #ifdef platform statements and do a platform specific implementation. Problem: to do that I'll need to make it true C++ with inheritance etc, to make sure each platform finds the right instance of the code. This means I will break all the platforms I can't test on for sure. 0% chance of getting that checked in before 1.0 I think. Unless you can suggest another library that's linked to apprunner and isn't split by platform?

> 1/ widget is already linked in with startup Not any more :) Boy, I'm making your life hard.

Actually, I looked again, and there's very little non-XP code in the bit I've extracted. There was lots elsewhere in the file, but this is pretty much XP code. OK, I'll change tack: I notice this in nsAppRunner.cpp: static nsresult GetNativeAppSupport(nsINativeAppSupport** aNativeApp) { NS_ENSURE_ARG_POINTER(aNativeApp); *aNativeApp = nsnull; nsCOMPtr<nsIAppShellService> appShellService(do_GetService(kAppShellServiceCID)); if (appShellService) appShellService->GetNativeAppSupport(aNativeApp); return *aNativeApp ? NS_OK : NS_ERROR_FAILURE; } So I think I can add these functions to nsIAppShellService (or something along those lines) and keep AppShell as an XPCOM component I can use from within nsAppRunner.cpp? I see that infact this command line processing / window opening stuff is the "odd man out" almost everything else in main1() is done through appShell's XPCOM interface ... Howzat?

OK, I just nailed this. All of the startup/new window code is now in nsAppShellService, and is exposed through that interface (only needed 2 new functions in the idl, the entry points to it are confined to only those 2 places). This means it can happily be an XPCOM component and host the code anyway, removing the need for steps 2, 3 and 4 in the original plan....

isn't there already an appShell living in widget and created by the appshell service? Shouldn't the code live in there or am i confused?

The "appshell" in widget is a different beast.

Can someone apply on Win32 and Linux and test for me please?

Spam some more XP folks.

I'm really glad to see this stuff cleaned up, but I can't decide if I like the approach. On one hand, there is a lot of command-line stuff that should be handled in a service of some kind instead of linked into nsAppRunner On the other hand, nsIAppShellService is already vaguely confusing and has a mishmash of unrelated methods already. As far as this specific patch, the three OpenWindow() calls are all kind over overlapping and could be combined into one routine if we just provided defaults for height and width (and we know that that we want SIZE_TO_CONTENT as the defaults) and forced people to pass NS_LITERAL_STRING("") as the arguments if they didn't want any arguments. Maybe we should split out nsIAppShellService, which seems more like it should be about running an application, into two or more interfaces? Something like nsIAppWindowService (i.e. an application-level window manager layered on top of nsIWindowWatcher) Also, I'm not a huge fan of passing around nsICmdLineService, but I realize that its required if we're going to simulate a command line when someone launches a seperate process (i.e. if the 2nd process puts "-mail" on the command line, we want to marshal that over to the 1st process with a simulated "-mail") - lets only pass it around when we actually need it, and not just for the sake of caching the service. (I haven't done the homework to know if this is what we're doing or not)

Hi Alec, welcome to the "creep the scope on lordpixel" party. This was a 20 line patch until smfr caught me. ;) Ah well, in for a penny. The thing with OpenWindow is already done, Simon suggested it already. I'll respin tomorrow probably. As to the other stuff - I'm not opposed, but I'm not strongly opionated enough to come up with a design by myself. Also - it may be lower risk to do this as the first step then work out a grand plan later. This is a means to an end for me... I'll look into WindowWatcher and WindowMediator a little, but more concrete suggestions would be welcome...

Still don't have a tree that builds. However, I don't want this bug to die, because its blocking something I really want to get checked in. This is the worst possible state for a bug as far as I can tell. There's a vague wishlist of what might need to be implemented but no strong requirements. The current patch meets my needs and I want to commit it soon, but I will work on improvements if anyone has anything concrete. So please, either give me things to fix, or tell me what you'd like to see "eventually" so I can file a new bug for that. Or give me reviews. Please. I have too many bugs which go this way, its getting me down :(

Setting TFV: to match 90823 since this blocks it

Dan M - can you comment on this bug? I note you've done a lot of work on nsWindowCreator.cpp which is in the same vein as this. Where do you think the code which manages which windows are opened on startup should be refactored to?

Comment on attachment 58053 [details] [diff] [review] Moves opening windows functionality from nsAppRunner.cpp to the AppShell service I can be happy with what you're trying to do, but I don't much like the code arrangement. What's up with all the static, classless functions in nsAppShellService.h? Do they have to use closures? I realize that the point of the exercise is to expose these things, but they really feel like dirty little utility functions that shouldn't be visible outside their source file. I'd be a lot happier if you could factor all those functions into a simpler interface, and consider adding it to nsIAppShellService.

Oh. I see you even /asked/ me to comment. nsAppRunner isn't my favourite code; I don't know it well. But as far as I can tell it boils down to LaunchApplication(WithArgs)? and Ensure1Window. The rest is all secret internal cruft. It makes sense to me to put some kind of ParseAndExecuteArgs method and maybe an EnsureWindow method on nsIAppRunner. Do you need to expose the rest of tha cruft?

Well, you've hit the nail right on the head. The problem is there is no nsIAppRunner, I assume because its not an XPCOM component, its our main() entry point. To be re-usable this stuff has gotta move. So I see two problems: 1/ Not everyone agrees AppShell is the place for this stuff. However so far no one has suggested an alternative course and made a case for it, and AppShell certainly works 2/ The C closures stuff is ugly because as Dan points out, it makes us have to expose all sorts of stuff that should really be private. It probably needs re- writing to not be in C. I guess this means I need to figure out what its doing and see how to convert it, which I don't exactly relish, given my lack of experience with such things. Does that about sum up or have I missed something someone feels needs to be done...?

I have something similar to this patch in my patch for bug 97622.

Well Conrad's patch on bug 97622 appears to address a lot of what this bug is about, though nsAppRunner.cpp still has most of its nasty old C code. Arguably this could be closed, or that code could still be cleaned up. In any case Conrad's changes are enough for me to unblock bug 90823.

Hmmm, I think the target milestone needs to be updated from Mozilla 1.2a to something that is in the future.

Nobody touch this bug without consulting me, please