[OpenJDK 2D-Dev] [10] Review Request: 8184435 Cleanup of javadoc in javax.print package

2017-07-16 Thread Sergey Bylokhov 

Hello,
Please review the fix for jdk10.
The cleanup was done in the same way as for datatransfer, sound and 
accessibility packages(see links in the CR).


I suggest to check the specdiff first, because for some methods the 
specification was reworked. CSR will be filed after technical review.


Bug: https://bugs.openjdk.java.net/browse/JDK-8184435
Webrev can be found at: http://cr.openjdk.java.net/~serb/8184435/webrev.07
Specdiff: 
http://cr.openjdk.java.net/~serb/8184435/specdiff.07/overview-summary.html


In this fix the javadoc is updated and the next rules were applied:
 -  should be replaced by {@tag }
 - 80 column limit
 - description of the class/method/field should be followed by dot
 - @param, @return should not end with a dot, except a case when more 
than one sentences are used

 - empty line after description/before the first tag was added
 - unnecessary empty lines were removed
 - sets of spaces in the middle of text were deleted
 - @param, @throws, @return should be aligned, to be more readable
 - unnecessary imports should be removed
 - the "null"/"true"/"false"/"this"/"ClassName" should be wrapped in 
{@code } when necessary

 - the order of different tags were unified across the package
... etc

There are also some mixing of different "reference usage", for example 
"InputStream" vs "input stream", "String" vs "string", etc. I tried to 
fix some of them.


--
Best regards, Sergey.

Re: [OpenJDK 2D-Dev] [10]JDK-8183341:Better cleanup for javax/imageio/AllowSearch.java

2017-07-16 Thread Shashidhara Veerabhadraiah 
Hi All, Here is the new webrev with the fixes for the comments:

 



 

Thanks and regards,

Shashi

 

From: Philip Race 
Sent: Friday, July 14, 2017 7:46 PM
To: 2d-dev@openjdk.java.net
Subject: Re: [OpenJDK 2D-Dev] [10]JDK-8183341:Better cleanup for 
javax/imageio/AllowSearch.java

 

I think it best to send an updated webrev since I'd like to make sure the
changes are made everywhere as requested.

-phil.

On 7/14/17, 5:17 AM, Kevin Rushforth wrote: 

As long as you are fixing the 'if(' ... can you add curly braces around the 
body of the target statement?

The following pattern:

    if (condition)
    statement;

is not in keeping with our coding style and can be a source of bugs later if a 
statement should be added.

Thanks.

-- Kevin


Ajit Ghaisas wrote: 

A nit… 

There should be a space between if and ( -- on lines 65 and 70.

You can make this change before pushing. No need to have a new webrev just for 
this.

 

Regards,

Ajit

 

From: Shashidhara Veerabhadraiah 
Sent: Friday, July 14, 2017 10:01 AM
To: HYPERLINK "mailto:2d-dev@openjdk.java.net"2d-dev@openjdk.java.net
Subject: Re: [OpenJDK 2D-Dev] [10]JDK-8183341:Better cleanup for 
javax/imageio/AllowSearch.java

 

Hi All, Anything more comments on this? Can I close this now?

 

Thanks and regards,

Shashi

 

From: Jayathirth D V 
Sent: Wednesday, July 12, 2017 3:14 PM
To: Shashidhara Veerabhadraiah mailto:shashidhara.veerabhadra...@oracle.com"shashidhara.veerabhadra...@oracle.com>
Cc: HYPERLINK "mailto:2d-dev@openjdk.java.net"2d-dev@openjdk.java.net
Subject: RE: [OpenJDK 2D-Dev] [10]JDK-8183341:Better cleanup for 
javax/imageio/AllowSearch.java

 

Hello Shashi,

 

Changes are fine.

 

Thanks,

Jay

 

From: Shashidhara Veerabhadraiah 
Sent: Wednesday, July 12, 2017 2:50 PM
To: Jayathirth D V; Sergey Bylokhov; Prahalad Kumar Narayanan
Cc: HYPERLINK "mailto:2d-dev@openjdk.java.net"2d-dev@openjdk.java.net
Subject: RE: [OpenJDK 2D-Dev] [10]JDK-8183341:Better cleanup for 
javax/imageio/AllowSearch.java

 

Thanks for that suggestion Jay. I have modified with some changes so that we 
don’t run into a null pointer exception!! And here is the new Webrev for the 
same:

 

HYPERLINK 
"http://cr.openjdk.java.net/%7Epkbalakr/shashi/8183341/webrev_02/"http://cr.openjdk.java.net/~pkbalakr/shashi/8183341/webrev_02/

 

Thanks and regards,

Shashi

 

From: Jayathirth D V 
Sent: Wednesday, July 12, 2017 11:53 AM
To: Shashidhara Veerabhadraiah mailto:shashidhara.veerabhadra...@oracle.com"shashidhara.veerabhadra...@oracle.com>
Cc: HYPERLINK "mailto:2d-dev@openjdk.java.net"2d-dev@openjdk.java.net
Subject: RE: [OpenJDK 2D-Dev] [10]JDK-8183341:Better cleanup for 
javax/imageio/AllowSearch.java

 

Hello Shashi,

 

The lines before the try{} block in test() function in the test case can throw 
IOException, like createImageInputStream() call present at line no 50.
In that case we will not reach finally block and temporary file will not be 
deleted.
Including complete code of test() function in try{} and deleting resources in 
finally block will be a better option.
 

Thanks,

Jay

 

From: Shashidhara Veerabhadraiah 
Sent: Wednesday, July 12, 2017 11:03 AM
To: Sergey Bylokhov; Prahalad Kumar Narayanan
Cc: HYPERLINK "mailto:2d-dev@openjdk.java.net"2d-dev@openjdk.java.net
Subject: Re: [OpenJDK 2D-Dev] [10]JDK-8183341:Better cleanup for 
javax/imageio/AllowSearch.java

 

Sergey, Thank you for the excellent suggestion. I have updated the Webrev 
accordingly.

 

Prahalad, With the use of Sergey’s suggestion of adding an finally block solves 
the problem of not deleting files on a fail case. 

  . Was the bug reproducible at your end ?

Yes, it was reproducible. We can see the temp files hanging at %temp% folder.

  . If yes, was it reproducible when the test succeeded /or when the test 
failed ?

Yes, it was reproducible on success for sure. The fail case was not tested as 
that would require me to do an undo of an earlier changeset change. But I think 
the output would remain the same as the pass case.

  . Is there a difference in the VM's termination based on the outcome of 
the test case.

Not sure since the fail case was not tested. But per the code, it is sure that 
it will have differences. The finally block should fix this problem.

  . How many such test cases exist that need similar clean up ?

    . grep on 'deleteOnExit' within jdk/test/javax/imageio/ gives me 
atleast 10 instances.

    . grep on 'File.createTempFile' within same directory gives me 29 
instances.

Am not sure what to do with respect to them. Should I raise new bugs if there 
is problem with them(if not on the bug db) and fix it?

 

Updated Webrev:

HYPERLINK 
"http://cr.openjdk.java.net/%7Epkbalakr/shashi/8183341/webrev_01/"http://cr.openjdk.java.net/~pkbalakr/shashi/8183341/webrev_01/

 

Thanks and regards,

Shashi

 

From: Sergey Bylokhov 
Sent: Wednesday, July 12, 2017 12: