On Wed, 7 May 2025 19:28:46 GMT, Phil Race <p...@openjdk.org> wrote:
> The implementations that accept a URLConnection, a URL and an InputStream all
> do the same.
Yes, but these methods do not declare any exceptions to be thrown.
The new method declares `IOException` can be thrown, therefore we can let an
`IOException` escape.
>> src/java.desktop/share/classes/com/sun/media/sound/JavaSoundAudioClip.java
>> line 113:
>>
>>> 111: clip.init(uc.getInputStream());
>>> 112: } catch (final Exception ignored) {
>>> 113: // Playing the clip will be a no-op if an exception
>>> occured in inititialization.
>>
>> Suggestion:
>>
>> // Playing the clip will be a no-op if an exception occurred in
>> initialization.
>
> As I wrote previously I am not going fixing typos in methods I didn't touch
> and actually expect to remove when the Applet API is removed.
Yes, you wrote but you had updated the comments in both `create(URLConnection)`
and `create(URL)`, and both comments contain typos.
I wouldn't have noticed if these hadn't been included in the diff.
Fixing these typos only takes accepting the suggestions on GitHub. After you
accept, if at all, use `git pull origin soundclip_api` in your local repo to
get the changes before adding changes to your local branch.
>> test/jdk/javax/sound/SoundClip/SoundClipTest.java line 43:
>>
>>> 41: if (!isSoundcardInstalled()) {
>>> 42: return;
>>> 43: }
>>
>> It should throw `jtreg.SkippedException` instead of pretending the test
>> passes.
>
> Well perhaps, but this is what every other similar jtreg sound test does.
Maybe `jtreg.SkippedException` didn't exist when the tests were written.
Tests that throw `jtreg.SkippedException` were considered *passed*, but not any
more.
I don't insist, though.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/24991#discussion_r2081947336
PR Review Comment: https://git.openjdk.org/jdk/pull/24991#discussion_r2081974458
PR Review Comment: https://git.openjdk.org/jdk/pull/24991#discussion_r2081990095
