[OpenJDK 2D-Dev] RFR: 8182043: Access to Windows Large Icons [v14]

[Alexey Ivanov]

On Tue, 25 May 2021 23:36:43 GMT, Alexander Zuev <kizune at 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