On Mon, 28 Jul 2025 19:00:35 GMT, Alexey Ivanov <aiva...@openjdk.org> wrote:
>> Prasanta Sadhukhan has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> ArgType name change
>
> src/java.desktop/share/classes/javax/swing/ImageIcon.java line 64:
>
>> 62: * are preloaded using MediaTracker to monitor the loaded state
>> 63: * of the image.
>> 64: * If the image source parameter to a constructor or method is non-null,
>
> Suggestion:
>
> * of the image.
> *
> * <p>
> * If the image source parameter to a constructor or method is non-null,
>
> I suggest putting the discussion on non-null parameters in its own paragraph
> for ease of reading the javadoc.
ok
> src/java.desktop/share/classes/javax/swing/ImageIcon.java line 206:
>
>> 204: * a string representation of the URL.
>> 205: * @param location the URL for the image
>> 206: * @throws {@code NullPointerException} if (@code null) URL is
>> passed.
>
> Suggestion:
>
> * @throws NullPointerException if a {@code null} URL is passed
ok
> src/java.desktop/share/classes/javax/swing/ImageIcon.java line 217:
>
>> 215: * @param image the image
>> 216: * @param description a brief textual description of the image
>> 217: * @throws {@code NullPointerException} if (@code null) Image is
>> passed.
>
> Suggestion:
>
> * @throws NullPointerException if a {@code null} image is passed
>
> Here, _“image”_ shouldn't be capitalised, you mean it as the literal
> _“image”_ rather than a type.
>
> I still prefer specifying the parameter name:
> Suggestion:
>
> * @throws NullPointerException if {@code image} is {@code null}
ok
> test/jdk/javax/swing/ImageIcon/ImageIconTest.java line 39:
>
>> 37: import javax.swing.ImageIcon;
>> 38:
>> 39: public class ImageIconTest {
>
> Suggestion:
>
> public class ImageIconNullParametersTest {
>
> Be more specific?
it is not just testing for NullParameters
> test/jdk/javax/swing/ImageIcon/ImageIconTest.java line 52:
>
>> 50: fos.write(invalidData);
>> 51: }
>> 52: String fileName = (new File(System.getProperty("test.src", "."),
>> imgName)).getName();
>
> Why do we need jumping over the hoops?
>
> You're creating an image, put it into the current directory that defaults to
> `scratch` directory in jtreg. The `scratch` directory is automatically
> cleaned by jtreg between tests, thus you won't even leave files in the source
> tree of JDK.
used file.deleteOnExit
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/25767#discussion_r2239691809
PR Review Comment: https://git.openjdk.org/jdk/pull/25767#discussion_r2239693016
PR Review Comment: https://git.openjdk.org/jdk/pull/25767#discussion_r2239693481
PR Review Comment: https://git.openjdk.org/jdk/pull/25767#discussion_r2239695863
PR Review Comment: https://git.openjdk.org/jdk/pull/25767#discussion_r2239697154
