[7u4] Review request for CR 7134730

[Mike Swingler]

I've taken a look at this diff and have a few suggestions:

diff -r 2e4258236920 src/macosx/bundle/JavaAppLauncher/src/JVMArgs.m
--- a/src/macosx/bundle/JavaAppLauncher/src/JVMArgs.m	Thu Jan 26 21:47:53 2012 +0400
+++ b/src/macosx/bundle/JavaAppLauncher/src/JVMArgs.m	Fri Jan 27 08:43:08 2012 -0500
@@ -77,7 +77,7 @@
 
 NSString *GetJavaRoot(NSDictionary *jvmInfoDict) {
     NSObject *javaRoot = [jvmInfoDict objectForKey:@"$JAVAROOT"];
-    if (![javaRoot isKindOfClass:[NSString class]]) return @"$APP_PACKAGE/Contents/Java";
+    if (![javaRoot isKindOfClass:[NSString class]]) return @"$APP_PACKAGE/Contents/Resources/Java";
     return (NSString *)javaRoot;
 }

I'd actually recommend putting the Java resources (.jars and .jnilibs) into Contents/Java now, and not Contents/Resources/Java, because of issues with code-signing (Resources is designed to be a location for localized content or images, which may be changeable without breaking the bundle's code signature).

I know this is different from traditional app bundles, but since developers will have to re-bundle anyway, this is actually the most compatible thing for them to do moving forward.


Also, I'd simply suggest initializing:
+ NSMutableArray *arguments = [NSMutableArray array];
and not worry about pre-initializing the capacity, or re-assigning the variable later on. It just makes the code a lot cleaner (with no measurable performance cost, since this straight-line code is only executed once).


You can also simply do:

+    [jvmOptions addObjectsFromArray:arguments]

instead of:

+    // Add arguments
+    [arguments enumerateObjectsUsingBlock:^(id obj, NSUInteger idx, BOOL *stop) {
+        [jvmOptions addObject:obj];
+    }];
+

If you are manually adding the arguments list to the jvmOptions array, then you don't need to re-stuff the arguments back into the jvmInfo dictionary, so you can completely remove the lines:

[jvmInfo setObject:arguments forKey:kArgumentsKey];


I'm also a little confused by:
+        expanded = [expanded stringByReplacingOccurrencesOfString:@" " withString:@"\\ "];

Does this mean that HotSpot needs spaces (and possibly other shell-interpreted characters) pre-escaped with backslashes for it? I really don't think this is actually the case, because our previous code did nothing like this. Please remember that the output of this tool is injected directly to HotSpot via API, and is not used to invoke a shell or a shell tool to like "java" or "javac".

Regards,
Mike Swingler
Apple Inc.


On Jan 27, 2012, at 5:46 AM, Greg Brown wrote:

> Sent original review request to 7u4 list by accident. Please see below.
> 
> Begin forwarded message:
> 
>> From: Greg Brown <greg.x.brown at oracle.com>
>> Subject: [7u4] Review request for CR 7134730
>> Date: January 27, 2012 8:44:47 AM EST
>> To: JDK 7u <jdk7u-dev at openjdk.java.net>
>> 
>> This change resolves some path issues in the XCode project that builds the JavaAppLauncher executable and updates the XCode project for XCode 4.2. It also fixes some issues with classpath and command-line argument handling.