[7u4] Review request for CR 7134730

[Kumar Srinivasan]

Hi Greg,

I scanned  the webrev,  here are my comments:

1. General comments the copyright is not formatted right, it is now:
2 * Copyright 2012 Oracle and/or its affiliates. All rights reserved.

it should be, note the comma after the year.

2 * Copyright 2012, Oracle and/or its affiliates. All rights reserved.

and several files missing Copyrights.


2.  main.m // disclaimer I don't fully understand this language 
(Objective C ?)
      FULL_VERSION and DOT_VERSION is always  is hard-coded to
      1.7.0 what happens when we go to 1.8.0 ?  Can we not set this
      in some automated way.

3. It looks the cp_wildcard expansion is set to TRUE in 
JLI_LaunchfFxnPtr, do
    we really need this ? the classpath is going to be hard-coded right 
? IMO
    we don't require cp wildcarding, we can set that to FALSE.

4. I also find the version string hard-coded in build.xml, we need to 
have a mechanism
    such that version information is derived from the make/build logic.

5. The test  is great, but how will the test be executed ? typically
    in the jdk, the tests are located under jdk/test/......, and these 
are all run by
    jtreg automatically on a periodic basis (nightly, weekly, promotion 
etc), we
   need this hooked up into that framework.


6. AppBundlerTask.java.html

Does the ant code get built and run with jdk6 ? it that is the case then 
the following
may be moot, but of course you can use for-each and simplify some code 
below.

   71     private ArrayList<File>  classPath = new ArrayList<File>();
   72     private ArrayList<File>  nativeLibraries = new ArrayList<File>();
   73     private ArrayList<String>  options = new ArrayList<String>();
   74     private ArrayList<String>  arguments = new ArrayList<String>();

you can set the assiged type to List.
private List<File> classPath = new ArrayList<>();

lines 135....

  135     public void addConfiguredClassPath(FileSet classPath) {
  136         File parent = classPath.getDir();
  137
  138         DirectoryScanner directoryScanner = classPath.getDirectoryScanner(getProject());
  -139         String[] includedFiles = directoryScanner.getIncludedFiles();
  140
  -141         for (int i = 0; i<  includedFiles.length; i++) {
  -142             this.classPath.add(new File(parent, includedFiles[i]));
  -143         }
	      for (String name : directoryScanner.getIncludedFiles() {
		  this.classPath.add(new File(parent, name);
               }
   144     }


similarly lines 453 and 477.

Test.java, I would make the test program dump out the args as well,
and pass some dummy args to ensure that args are passed correctly
to the app's main.

I noticed that you set the starting heapsize and maximum heap size ?

Kumar


>> Why are these three files in the webrev not under the standard GPL license used for the rest of OpenJDK (and the rest of the files in the webrev)?:
>>
>> http://cr.openjdk.java.net/~gkbrown/7134730/webrev.00/src/macosx/bundle/build.xml.html
>> http://cr.openjdk.java.net/~gkbrown/7134730/webrev.00/src/macosx/bundle/appbundler/native/main.m.html
>> http://cr.openjdk.java.net/~gkbrown/7134730/webrev.00/src/macosx/bundle/build.properties.html
> An oversight. They should be - I will add the GPL header.
> G
>