Review request for MACOSX_PORT-176: AWT Splashscreen support

[Mike Swingler]

On Dec 9, 2011, at 7:13 AM, Anthony Petrov wrote:

> Hi Mike et al.,
> 
> Here's the final version of the fix:
> 
> http://cr.openjdk.java.net/~anthony/x-3-splashAndOSXGUILib.1/
> 
> To summarize essential changes introduced with the fix:
> 
> 1. The NSApplicationAWT code has been moved to a separate library libosxapp.dylib so that it be available from both AWT and splash screen code.
> 
> 2. The QueuingApplicationDelegate class allows us to postpone creating the real ApplicationDelegate instance later when AWT has finally started up. All the events delivered to the QAD will be posted to the real delegate once it's installed.
> 
> 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).

> 4. The splash screen code now uses blocks and [JNFRunLoop performOnMainThreadWaiting] method instead of the AppKitThreadHelper utility class.
> 
> I'm going to push this fix early next week. Thanks in advance for any additional reviews!


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.

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

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?

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];

Lifecycle issues with the QueuingApplicationDelegate aside, good work so far!
Mike Swingler
Apple Inc.