> It now can be reviewed as usual at:
>
> http://cr.openjdk.java.net/~prappo/8080272/webrev.00/
>
> Feel free to review. Thanks.
Let me start then.
1. I've seen several cases where current behaviour
while ((n = in.read(buffer)) > 0)
~~~
has been changed to
while ((read = this.read(buffer, 0, TRANSFER_BUFFER_SIZE)) >= 0)
~~~
Those things jump out at you, but don't seem to be harmful, since the only case
where:
java.io.InputStream.read(byte[], int off, int len)
java.io.InputStream#read(byte[])
may return 0, is when len == 0. And it's never the case here. Unless, of course,
some misbehaving implementation of InputStream is used.
2.
jdk/src/java.xml.crypto/share/classes/com/sun/org/apache/xml/internal/security/utils/JavaUtils.java
Some of the refactored methods there seem to be unused:
getBytesFromFile(String)
writeBytesToFilename(String, byte[])
Should we remove them instead?
3. Thanks to Patrick I've learned
AutoCloseable s = ...
try (s) {
~~~
}
is not a bug, but new javac 9 feature [1]. Learned it the hard way.
4. I wonder if javax.management.loading.MLet.loadLibraryAsResource and
sun.net.www.MimeLauncher.run could be refactored with more appropriate
Files.copy(is, file.toPath())
as it seems to fit there perfectly.
5. Now we don't need the com.sun.media.sound.StandardMidiFileWriter.bufferSize
after we removed its sole client.
6. I've run into several cases where code use explicit `flush()` for
ByteArrayOutputStream, BufferedOutputStream. For the former one it's never
needed. The latter one does it automatically on `close()`.
Should we still call them?
7. org.jcp.xml.dsig.internal.dom.Utils.readBytesFromStream is a little bit
awkward since it used 1024 as some sort of a cut-off.
It might've had something to do with EOF detection, but I'm not sure.
8. jdk/src/java.desktop/windows/classes/sun/print/Win32PrintJob.java
} catch (FileNotFoundException fnfe) {
notifyEvent(PrintJobEvent.JOB_FAILED);
throw new PrintException(fnfe.toString());
} catch (IOException ioe) {
notifyEvent(PrintJobEvent.JOB_FAILED);
throw new PrintException(ioe.toString());
}
It seems ok to remove the FileNotFoundException branch. Or I'm missing
something.
9. I don't think I would agree with some changes in sun.net.www.MimeLauncher.run
as they obviously make the new version not equivalent to the old one. Non
swallowed IOException, additional null check. Way too much for the cleanup
change.
Summary
-------
The change seems ok to me, given that (7, 9) are justified or fixed.
Patrick needs a _R_eviewer for his changes to be accepted. Please help him with
this.
-------------------------------------------------------------------------------
[1] https://blogs.oracle.com/darcy/entry/concise_twr_jdk9
