Review request for MACOSX_PORT-176: AWT Splashscreen support

[Anthony Petrov]

Hi Eric and Mike,

Thanks for reviewing the fix. I've incorporated suggested changes into 
the latest version of the fix at:

http://cr.openjdk.java.net/~anthony/x-3-splashAndOSXGUILib.2/

Some comments follow inline...


On 12/10/11 01:36, Mike Swingler wrote:
>> 3. On the Mac the SplashScreen is initialized only after the JVM has been started and the main application class has been identified, but, obviously, before calling user's main() method. This is because we need the main class name to set the app's name in the dock, as well as we need to access a couple of Java system properties when an NSApplicationAWT instance is created. java.c and java_md.c contain the corresponding changes.
>
> I'm guessing that it will be logical prerequisite for the app name and icon properties to always be set on JVM instantiation, and not by any user code (which using the splash screen).

Well, as long as the app is a GUI app, and only if it uses the splash 
screen. Otherwise the dock icon should only appear if the app 
initializes AWT later when it's already started running. And this is 
exactly how it works now.


> Two major important points:
>
> Structurally, I think the ownership lifecycle of the QueueingApplicationDelegate is somewhat confused. I think it's really bad for to release the qad from the dealloc of the NSApplicationAWT instance. Perhaps you could add a class method to QueueingApplicationDelegate to manage it's singleton existence, and clear it when it's no longer required. Unless the qad is actually an instance variable of the NSApplicationAWT instance, it's really not proper to release it from (what could potentially be multiple instances of) the -dealloc method.

I've implemented +sharedDelegate in QAD. Note that the instance never 
gets released but I don't think this is a big problem because 1) no 
splash screen - no qad is allocated in the first place; 2) the queue 
gets emptied upon processing the events, the rest of the object (two 
booleans, one pointer, and the queue itself) shouldn't occupy too much 
RAM. So I guess we can live with this. We could add a static "release" 
method, however, I don't like the idea since the pointer returned by 
+sharedDelegate could have been stored elsewhere. We could, of course, 
rely on users of the method to properly pair the -retain/-release calls, 
but do we actually need it?


> Also, the QueuingApplicationDelegate should hold onto it's own NSMutableArray and real delegate as ivars, not global statics.

I agree, and I made the queue an instance variable. However, as soon as 
I'm trying to access an ivar from a block, I get crashed. So I left the 
realDelegate static.


> Two minor nits:
>
> I'm generally leery of any code that blocks itself against the main thread (using [JNFRunLoop performOnMainThreadWaiting:YES …), but in the case of the functions that pass the splash pointer, this sadly seem necessary, because the block can't retain that struct like it can for ObjC types. Any chance that the splash struct could be promoted to (or memory managed by) an ObjC object?

We actually have a retention mechanism for the structure in term of a 
custom lock that the splash screen code uses. Note that most platform 
methods that involve blocks are actually called from shared splash 
screen methods, and these shared methods make sure the structure is 
locked while platform methods are running. If we were to use [... 
perform:NO], then we'd have to setup a monitor on the lock, and catch 
the moment the block has finished executing and only then proceed with 
the rest of the platform and/or shared method. I think this would 
complicate the code very significantly. Besides, in most cases the 
blocks are very simple and only invoke Cocoa methods that just have to 
be called on the AppKit thread. So using [:YES] comes very handy and 
reasonable here.

I don't think we want to turn the Splash structure into an ObjC object 
simply because it might require too many changes for no apparent 
benefits - at least not for any of the other platforms. And given that 
the architecture of the splash screen code already involves a custom 
lock, this seems to be of little usefulness.


> Just as a basic ObjC style thing: you don't have to cast to an object type to call a selector as long as the type at runtime is correct. So this:
>
>     BOOL onMainThread = [(NSNumber*)[args objectAtIndex:0] boolValue];
>     BOOL swtMode = [(NSNumber*)[args objectAtIndex:1] boolValue];
>     BOOL headless = [(NSNumber*)[args objectAtIndex:2] boolValue];
>     BOOL swtModeForWebStart = [(NSNumber*)[args objectAtIndex:3] boolValue];
>     BOOL verbose = [(NSNumber*)[args objectAtIndex:4] boolValue];
>
> Could be simply this:
>
>     BOOL onMainThread = [[args objectAtIndex:0] boolValue];
>     BOOL swtMode = [[args objectAtIndex:1] boolValue];
>     BOOL headless = [[args objectAtIndex:2] boolValue];
>     BOOL swtModeForWebStart = [[args objectAtIndex:3] boolValue];
>     BOOL verbose = [[args objectAtIndex:4] boolValue];

I thought a compiler might issue warnings on this, but it doesn't. Cool! 
I've removed the type casts.

--
best regards,
Anthony