109811 - Refactor launch window opening code out of nsAppRunner.cpp and into AppShell

Status: RESOLVED WORKSFORME

(SeaMonkey :: UI Design, defect)

Description

[Andrew Thompson]

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.

Comment 1

[Simon Fraser [no longer active]]

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

Comment 2

[Andrew Thompson]

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?

Comment 3

[Simon Fraser [no longer active]]

> 1/ widget is already linked in with startup

Not any more  :)

Boy, I'm making your life hard.

Comment 4

[Andrew Thompson]

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?

Comment 5

[Andrew Thompson]

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

Comment 6

[Mike Pinkerton (not reading bugmail)]

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?

Comment 7

[Simon Fraser [no longer active]]

The "appshell" in widget is a different beast.

Comment 8

[Andrew Thompson]

Attachment: Moves opening windows functionality from nsAppRunner.cpp to the AppShell service

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

Comment 9

[Simon Fraser [no longer active]]

Spam some more XP folks.

Comment 10

[Alec Flett]

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)

Comment 11

[Andrew Thompson]

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

Comment 12

[Andrew Thompson]

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

Comment 13

[Steve Dagley]

Setting TFV: to match 90823 since this blocks it

Comment 14

[Andrew Thompson]

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 15

[Dan M]

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.

Comment 16

[Dan M]

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?

Comment 17

[Andrew Thompson]

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

Comment 18

[Conrad Carlen (not reading bugmail)]

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

Comment 19

[Andrew Thompson]

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.

Comment 20

[David G King]

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

Comment 21

[Benjamin Smedberg]

Nobody touch this bug without consulting me, please