Kumar,
new version of webrev where i have fixed all the coding style
glitches i've found is here:
http://cr.openjdk.java.net/~kizune/8020802/webrev.04
/Alex
On 10/22/13 19:32, Kumar Srinivasan wrote:
Hi Alex,
thanks for suggestion, dt.jar seems to be a fine test subject.
Fixed @test field and the .jar used in the test.
I am sorry! I misled you yesterday, it should simply be @test
New webrev is here: http://cr.openjdk.java.net/~kizune/8020802/webrev.03
Here are some nits, mostly conventions.
Main.java
Suggestion breaking this line:
204 String tmpbase = (fname == null) ? "tmpjar" :
fname.substring(fname.indexOf(File.separatorChar) + 1);
This makes it clearer
Ref:
http://www.oracle.com/technetwork/java/javase/documentation/codeconventions-136091.html#248
String tmpbase = (fname == null)
?"tmpjar"
:fname.substring(fname.indexOf(File.separatorChar) + 1);
Can tmpbase be made final ?
TestNormal.java
I don't like the way this line is broken up, break after the Paren,
usually it is a comma.
or use a static final for Line.separator, that might make it shorter.
66 Process proc = Runtime.getRuntime()
67 .exec(java_home + File.separator + "bin" + File.separator
+ cmd);
There are other coding convention issues wrt. if/else and while,
please check.
http://www.oracle.com/technetwork/java/javase/documentation/codeconventions-142311.html#449
Thanks
Kumar
/Alex
On 10/22/13 24:33, Kumar Srinivasan wrote:
Alex,
The @test should be just TestNormal not TestNormal.java
if you need an alternate/small jar you can use one in $JDK/lib/
ex: jconsole.jar or dt.jar
Kumar
On 10/21/2013 7:41 AM, Alexander Zuev wrote:
Alan,
thanks for a review, see my comments inline.
On 10/20/13 23:15, Alan Bateman wrote:
On 19/10/2013 16:14, Kumar Srinivasan wrote:
Hi Alex,
This looks good.
Hi Sherman, Alan,
Could one of you review this, please
The "-n" options seems okay but I wonder if there has any thought
given to having an option on jarsigner to normalize it before signing?
That was requirement for PM - looks like they have more usages for
this option than just signing a jar file.
On the patch itself then I think it needs to be cleaned up before
pushing it. From a quick look:
- It's using a raw type for the packer properties, should be
Map<String, String>
Ok, will fix that.
- Is any reason not to use try-with-resources?
Because not all of the resources used are inherited from
AutoCloseable - you can't use File
in try-with-resources block so to use it i will have to rewrite
code a bit. Looks like it's a little bit too late
considering i will need to test it on all the platforms.
- I assume the catching of Exception in createTemporaryFile should
be IOException|SecurityException
Yes, will change that.
- Is the @test tag in the test right?
You mean that @summary does not match the issue summary and does
not include bug ID? Can be fixed too.
- The test appears to extract jfr.jar but I don't think we can
assume this exists
I'm trying to find a .jar file that is in the jdk distribution,
contains class files and of a reasonable size - attempt with
tools.jar caused test to timeout on Windows machine even with
20minutes timeout - and i do not want to make a test
with the timeout greater than even 10 minutes - much less one of
20+minutes.
Any suggestions? I'm putting the jfr.jar so far but can change it
later - test stabilization can be done after the Oct 24th.
- Some of the jar tests use sun.tools.jar.Main and this might be
useful here
I'm trying to check the command-line option and want it to go over
the full path of command-line option parsing - just to be sure.
Do not think it's a big deal.
One other thing, Mathias's mail to jdk8-dev [1] asks for changes
that require translation to be in the master no longer than 10/24.
Does the update to jar.properties count?
Yes, i've been pinged by the documentation team already so i'm
trying to push this stuff as soon as possible.
Of course without giving up on the quality of the code.
The new webrev can be found at
http://cr.openjdk.java.net/~kizune/8020802/webrev.02
/Alex
-Alan
[1]
http://mail.openjdk.java.net/pipermail/jdk8-dev/2013-October/003443.html
