On Mon, 29 Jun 2026 18:53:36 GMT, Andy Goryachev <[email protected]> wrote:
>> ## Summary
>>
>> This PR changes the behavior of `DataFormat` by allowing multiple instances
>> that contain the same set of mime types (ids).
>>
>>
>>
>> ## Problem
>>
>> There seems to be several issues with DataFormat API and implementation
>> discovered during review of the `Clipboard`-related code:
>>
>> 1. `static DataFormat::lookupMimeType(String)` is not thread safe: while
>> iterating over previously registered entries in the `DATA_FORMAT_LIST`
>> another thread might create a new instance (DataFormat L227)
>>
>> 2. `public DataFormat(String...)` constructor might throw an
>> `IllegalArgumentException` if one of the given mime types is already
>> assigned to another `DataFormat`. The origin of this requirement is unclear,
>> but one possible issue I can see is if the application has two libraries
>> that both attempt to create a `DataFormat` for let's say `"text/css"`. Then,
>> depending on the timing or the exact code path, an exception will be thrown
>> for which the library(-ies) might not be prepared. The constructor is also
>> not thread safe.
>>
>> 3. To avoid a situation mentioned in bullet 2, a developer would typically
>> call `lookupMimeType()` to obtain the already registered instance, followed
>> by a constructor call if such an instance has not been found. An example of
>> such code can be seen in webkit/UIClientImpl:299 - but even then, despite
>> that two-step process being synchronized, the code might still fail if *some
>> other* library or the application attempts to create a new instance of
>> DataFormat, since the constructor itself is not synchronized.
>>
>> 4. `DataFormat(new String[] { null })` is allowed but makes no sense!
>>
>> 5. The current implementation uses the `WeakReferenceQueue` which
>> theoretically might, under certain conditions, allow the application to
>> create mismatched `DataFormat`s.
>>
>> Why do we need to have the registry of previously created instances?
>> Unclear. My theory is that the DataFormat allows to have multiple mime-types
>> (ids) - example being `DataFormat.FILES = new
>> DataFormat("application/x-java-file-list", "java.file-list");` - and the
>> registry was added to prevent creation of a `DataFormat` with just one id
>> for some reason.
>>
>> Also, I could not find the origin of the multi-id requirement, or the origin
>> of the `java.file-list` id itself (it is not a valid mime type).
>>
>>
>>
>> ## Solution
>>
>> The proposed solution is to relax the constraint on the constructor to allow
>> multiple instances with the same set of mime types (ids). This might
>> require chan...
>
> Andy Goryachev has updated the pull request with a new target base due to a
> merge or a rebase. The incremental webrev excludes the unrelated changes
> brought in by the merge/rebase. The pull request contains five additional
> commits since the last revision:
>
> - Merge branch 'master' into 8373452.dataformat.2
> - review comments
> - review comments
> - nulls
> - data format
modules/javafx.graphics/src/test/java/test/javafx/scene/input/DataFormatTest.java
line 125:
> 123: String mime = null;
> 124: new DataFormat(mime);
> 125: });
Suggestion:
String mime = null;
assertThrows(NullPointerException.class, () -> {
new DataFormat(mime);
});
Recommendation is to always have one statement in the `assertThrows` /
`assertDoesNotThrows`
modules/javafx.graphics/src/test/java/test/javafx/scene/input/DataFormatTest.java
line 131:
> 129: public void nullArrayIsAllowedForCompatibilityReasons() {
> 130: String[] mimes = null;
> 131: new DataFormat(mimes);
Suggestion:
assertDoesNotThrow(() -> new DataFormat(mimes));
-------------
PR Review Comment: https://git.openjdk.org/jfx/pull/2197#discussion_r3497136657
PR Review Comment: https://git.openjdk.org/jfx/pull/2197#discussion_r3497123101
