On Tue, 7 May 2024 19:53:57 GMT, Kevin Rushforth <[email protected]> wrote:
>> Alexander Matveev has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> 8282999: Add for support EXT-X-MEDIA tag in HTTP Live Streaming [v2]
>
> modules/javafx.media/src/main/java/com/sun/media/jfxmedia/locator/HLSConnectionHolder.java
> line 480:
>
>> 478: }
>> 479:
>> 480: class PlaylistLoader extends Thread {
>
> Having multiple top-level classes in the same source file is an anti-pattern.
> Is there a good reason that these can't be nested static classes? If not,
> then please make this change. You might be able to then make the nested
> classes private, although there is no harm in leaving them package scope.
When these classes were nested classes I got issues with second instance of
HLSConnectionHolder. If I remember correctly nested classes of second instance
of HLSConnectionHolder were using fields of first HLSConnectionHolder instance.
Maybe because I initiated second instance incorrectly. To avoid any such
potential issues I decided to move away from nested classes. I would prefer to
keep as is or better to move all nested classes under separate package
(com.sun.media.jfxmedia.locator.hls).
-------------
PR Review Comment: https://git.openjdk.org/jfx/pull/1435#discussion_r1593304476
