On Tue, 25 May 2021 23:36:43 GMT, Alexander Zuev <kiz...@openjdk.org> wrote:
>> Fix updated after first round of review. > > Alexander Zuev has updated the pull request incrementally with one additional > commit since the last revision: > > null file now properly causes IllegalArgumentException > Small fixed in JavaDoc src/java.desktop/share/classes/javax/swing/filechooser/FileSystemView.java line 296: > 294: > 295: if (f == null){ > 296: throw new IllegalArgumentException("File reference should be > specified"); Shall the exception message be more specific: "The file reference should not be null"? src/java.desktop/share/classes/javax/swing/filechooser/FileSystemView.java line 300: > 298: > 299: if(!f.exists()) { > 300: return null; Shall it throw `FileNotFoundException` or `IllegalArgumentException` if the file doesn't exist? It could more convenient to return `null` rather than catch an exception. The space is missing between if and the opening parenthesis. test/jdk/javax/swing/JFileChooser/FileSystemView/SystemIconTest.java line 58: > 56: > 57: static void negativeTests() { > 58: ImageIcon icon; Can it be icon? This test doesn't use the fact that the returned object is `ImageIcon`. test/jdk/javax/swing/JFileChooser/FileSystemView/SystemIconTest.java line 67: > 65: if (!gotException) { > 66: throw new RuntimeException("Negative size icon should throw > invalid argument exception"); > 67: } A suggestion: throw the exception inside try-block and ignore the expected exception, then `gotException` can be dropped. Suggestion: try { fsv.getSystemIcon(new File("."), -1, 16); throw new RuntimeException("Negative size icon should throw invalid argument exception"); } catch (IllegalArgumentException iae) { // expected } Shall the test also exercise 0 as an invalid parameter? Shall the test also pass an invalid height? ------------- PR: https://git.openjdk.java.net/jdk/pull/2875