Re: RFR: 8267551: Support loading images from inline data-URIs [v13]
On Sat, 5 Jun 2021 16:12:36 GMT, Kevin Rushforth wrote: >> `resource` is never `null` at this point, and it is already trimmed. Also, >> contrary to the comment, I cannot see this method being used anywhere in >> tests, so I made it private. > > If that is guaranteed by all callers of this method (which is easier to check > now that you made it private), this is fine. This method is only used once, and only after `resource` has already been dereferenced. So if it was indeed `null`, we would have errored out with `NullPointerException` before calling this method. - PR: https://git.openjdk.java.net/jfx/pull/508
Re: RFR: 8267551: Support loading images from inline data-URIs [v13]
On Fri, 4 Jun 2021 22:00:03 GMT, Kevin Rushforth wrote:
>> Michael Strauß has updated the pull request with a new target base due to a
>> merge or a rebase. The pull request now contains 15 commits:
>>
>> - Merge branch 'master' into feature/image-datauri
>>
>># Conflicts:
>>#
>> modules/javafx.graphics/src/test/java/test/com/sun/javafx/iio/ImageStorageTest.java
>> - Accept regular file paths, change javadoc
>> - Merge branch 'openjdk:master' into feature/image-datauri
>> - Rename DataURI.isValidURI
>> - Reverted a change
>> - Allow leading and trailing whitespace in data URI
>> - Removed test
>> - Changed data URI detection
>> - Merge branch 'master' into feature/image-datauri
>> - Moved test
>> - ... and 5 more:
>> https://git.openjdk.java.net/jfx/compare/7b7050c4...315523c5
>
> modules/javafx.graphics/src/main/java/com/sun/javafx/util/DataURI.java line
> 76:
>
>> 74:
>> 75: if (Character.isWhitespace(uri.charAt(0))) {
>> 76: uri = uri.trim();
>
> Same question as above.
Removed the conditional branch.
-
PR: https://git.openjdk.java.net/jfx/pull/508
Re: RFR: 8267551: Support loading images from inline data-URIs [v13]
On Sat, 5 Jun 2021 15:52:01 GMT, Kevin Rushforth wrote:
>> My idea was to avoid repeatedly allocating a new String with the data
>> contained in the URI. Since `matchScheme` is called at least twice (maybe
>> more), that would be a total of at least three copies of the URI data until
>> we parse it. Granted, this would only apply for URIs that contain leading or
>> trailing whitespace (since `trim()` is specified to return the same String
>> instance if it contains no leading or trailing whitespace).
>
> I see. How about something like this then?
>
>
> if (uri == null || uri.isEmpty()) {
> return false;
> }
>
> if (Character.isWhiteSpace(uri.charAt(0)) {
> uri = uri.trim();
> }
>
> if (uri.length() < 6) {
> return false;
> }
>
>
> The rest of the method can then work on a trimmed string.
I replaced it with another implementation that is effectively the same as this.
-
PR: https://git.openjdk.java.net/jfx/pull/508
Re: RFR: 8267551: Support loading images from inline data-URIs [v13]
On Sat, 5 Jun 2021 17:10:29 GMT, John Hendrikx wrote: >> Maybe add a brief comment to that effect? > > Ok clear thanks I added a comment that explains that. - PR: https://git.openjdk.java.net/jfx/pull/508
Re: RFR: 8267551: Support loading images from inline data-URIs [v13]
On Sat, 5 Jun 2021 16:10:59 GMT, Kevin Rushforth wrote: >> In this specific case, image loading has failed for some reason. The call to >> `DataURI.tryParse` is only there to potentially call `DataURI.toString()` >> for a truncated log output, instead of logging the entire `url` String. If >> data-URI truncation is not wanted, this code can be reverted. > > Maybe add a brief comment to that effect? Ok clear thanks - PR: https://git.openjdk.java.net/jfx/pull/508
Re: RFR: 8267551: Support loading images from inline data-URIs [v13]
On Sat, 5 Jun 2021 15:44:38 GMT, Michael Strauß wrote:
>> modules/javafx.graphics/src/main/java/javafx/css/converter/URLConverter.java
>> line 98:
>>
>>> 96: }
>>> 97:
>>> 98: private URL resolve(String stylesheetUrl, String resource) {
>>
>> Why was this change done? It seems unnecessary and possibly unwanted.
>
> `resource` is never `null` at this point, and it is already trimmed. Also,
> contrary to the comment, I cannot see this method being used anywhere in
> tests, so I made it private.
If that is guaranteed by all callers of this method (which is easier to check
now that you made it private), this is fine.
-
PR: https://git.openjdk.java.net/jfx/pull/508
Re: RFR: 8267551: Support loading images from inline data-URIs [v13]
On Sat, 5 Jun 2021 15:49:16 GMT, Michael Strauß wrote:
>> modules/javafx.graphics/src/main/java/com/sun/javafx/css/StyleManager.java
>> line 776:
>>
>>> 774: } else {
>>> 775: logger.warning("Error loading
>>> image: " + url);
>>> 776: }
>>
>> So if I understand this correctly, an Image is first loaded via the standard
>> mechanism, and if that fails for any reason (`image.isError()`) a second
>> attempt is made through `DataURI` ?
>>
>> Would it not be better to register a new protocol so `new Image` would just
>> succeed for data uri's ?
>
> In this specific case, image loading has failed for some reason. The call to
> `DataURI.tryParse` is only there to potentially call `DataURI.toString()` for
> a truncated log output, instead of logging the entire `url` String. If
> data-URI truncation is not wanted, this code can be reverted.
Maybe add a brief comment to that effect?
-
PR: https://git.openjdk.java.net/jfx/pull/508
Re: RFR: 8267551: Support loading images from inline data-URIs [v13]
On Sat, 5 Jun 2021 15:08:29 GMT, Michael Strauß wrote: >> modules/javafx.graphics/src/main/java/com/sun/javafx/util/DataURI.java line >> 185: >> >>> 183: >>> 184: return originalUri.substring(0, originalUri.length() - >>> originalData.length()) >>> 185: + originalData.substring(0, 14) + "..." + >>> originalData.substring(originalData.length() - 14); >> >> Why truncate it? > > The idea was to not clutter logs with large pieces of data. Do you think that > the entire data string should be logged? Your explanation is fine. And since this is an internal class, it doesn't matter all that much. - PR: https://git.openjdk.java.net/jfx/pull/508
Re: RFR: 8267551: Support loading images from inline data-URIs [v13]
On Sat, 5 Jun 2021 15:17:09 GMT, Michael Strauß wrote:
>> In that case, it might be clearer and simpler to just call `trim()` on the
>> input String before doing anything with it, unless there is a reason not to.
>
> My idea was to avoid repeatedly allocating a new String with the data
> contained in the URI. Since `matchScheme` is called at least twice (maybe
> more), that would be a total of at least three copies of the URI data until
> we parse it. Granted, this would only apply for URIs that contain leading or
> trailing whitespace (since `trim()` is specified to return the same String
> instance if it contains no leading or trailing whitespace).
I see. How about something like this then?
if (uri == null || uri.isEmpty()) {
return false;
}
if (Character.isWhiteSpace(uri.charAt(0)) {
uri = uri.trim();
}
if (uri.length() < 6) {
return false;
}
The rest of the method can then work on a trimmed string.
-
PR: https://git.openjdk.java.net/jfx/pull/508
Re: RFR: 8267551: Support loading images from inline data-URIs [v13]
On Sat, 5 Jun 2021 08:21:14 GMT, John Hendrikx wrote:
>> Michael Strauß has updated the pull request with a new target base due to a
>> merge or a rebase. The pull request now contains 15 commits:
>>
>> - Merge branch 'master' into feature/image-datauri
>>
>># Conflicts:
>>#
>> modules/javafx.graphics/src/test/java/test/com/sun/javafx/iio/ImageStorageTest.java
>> - Accept regular file paths, change javadoc
>> - Merge branch 'openjdk:master' into feature/image-datauri
>> - Rename DataURI.isValidURI
>> - Reverted a change
>> - Allow leading and trailing whitespace in data URI
>> - Removed test
>> - Changed data URI detection
>> - Merge branch 'master' into feature/image-datauri
>> - Moved test
>> - ... and 5 more:
>> https://git.openjdk.java.net/jfx/compare/7b7050c4...315523c5
>
> modules/javafx.graphics/src/main/java/com/sun/javafx/css/StyleManager.java
> line 776:
>
>> 774: } else {
>> 775: logger.warning("Error loading
>> image: " + url);
>> 776: }
>
> So if I understand this correctly, an Image is first loaded via the standard
> mechanism, and if that fails for any reason (`image.isError()`) a second
> attempt is made through `DataURI` ?
>
> Would it not be better to register a new protocol so `new Image` would just
> succeed for data uri's ?
In this specific case, image loading has failed for some reason. The call to
`DataURI.tryParse` is only there to potentially call `DataURI.toString()` for a
truncated log output, instead of logging the entire `url` String. If data-URI
truncation is not wanted, this code can be reverted.
-
PR: https://git.openjdk.java.net/jfx/pull/508
Re: RFR: 8267551: Support loading images from inline data-URIs [v13]
On Fri, 4 Jun 2021 22:09:09 GMT, Kevin Rushforth wrote:
>> Michael Strauß has updated the pull request with a new target base due to a
>> merge or a rebase. The pull request now contains 15 commits:
>>
>> - Merge branch 'master' into feature/image-datauri
>>
>># Conflicts:
>>#
>> modules/javafx.graphics/src/test/java/test/com/sun/javafx/iio/ImageStorageTest.java
>> - Accept regular file paths, change javadoc
>> - Merge branch 'openjdk:master' into feature/image-datauri
>> - Rename DataURI.isValidURI
>> - Reverted a change
>> - Allow leading and trailing whitespace in data URI
>> - Removed test
>> - Changed data URI detection
>> - Merge branch 'master' into feature/image-datauri
>> - Moved test
>> - ... and 5 more:
>> https://git.openjdk.java.net/jfx/compare/7b7050c4...315523c5
>
> modules/javafx.graphics/src/main/java/javafx/css/converter/URLConverter.java
> line 98:
>
>> 96: }
>> 97:
>> 98: private URL resolve(String stylesheetUrl, String resource) {
>
> Why was this change done? It seems unnecessary and possibly unwanted.
`resource` is never `null` at this point, and it is already trimmed. Also,
contrary to the comment, I cannot see this method being used anywhere in tests,
so I made it private.
-
PR: https://git.openjdk.java.net/jfx/pull/508
Re: RFR: 8267551: Support loading images from inline data-URIs [v13]
On Fri, 4 Jun 2021 22:14:12 GMT, Kevin Rushforth wrote:
>> Michael Strauß has updated the pull request with a new target base due to a
>> merge or a rebase. The pull request now contains 15 commits:
>>
>> - Merge branch 'master' into feature/image-datauri
>>
>># Conflicts:
>>#
>> modules/javafx.graphics/src/test/java/test/com/sun/javafx/iio/ImageStorageTest.java
>> - Accept regular file paths, change javadoc
>> - Merge branch 'openjdk:master' into feature/image-datauri
>> - Rename DataURI.isValidURI
>> - Reverted a change
>> - Allow leading and trailing whitespace in data URI
>> - Removed test
>> - Changed data URI detection
>> - Merge branch 'master' into feature/image-datauri
>> - Moved test
>> - ... and 5 more:
>> https://git.openjdk.java.net/jfx/compare/7b7050c4...315523c5
>
> modules/javafx.graphics/src/main/java/javafx/scene/image/Image.java line 612:
>
>> 610: /**
>> 611: * Constructs an {@code Image} with content loaded from the
>> specified
>> 612: * image file. The {@code url} parameter can be one of the
>> following:
>
> I would not use the words "image file" in the first sentence. I recommend to
> restore this to "url".
Done.
> modules/javafx.graphics/src/main/java/javafx/scene/image/Image.java line 624:
>
>> 622: *
>> 623: * If a URL uses the "data" scheme, the data must be base64-encoded
>> 624: * and the MIME type must either be empty or a subtype of the
>
> These two paragraphs might be better as part of the class docs (which is
> where we list the supported image formats).
Done.
> modules/javafx.graphics/src/main/java/javafx/scene/image/Image.java line 638:
>
>> 636: /**
>> 637: * Constructs an {@code Image} with content loaded from the
>> specified
>> 638: * image file. The {@code url} parameter can be one of the
>> following:
>
> Similar comment. Maybe something like this for the first sentence?
>
>
> Constructs an {@code Image} with content loaded from the specified url using
> the specified parameters.
Done.
-
PR: https://git.openjdk.java.net/jfx/pull/508
Re: RFR: 8267551: Support loading images from inline data-URIs [v13]
On Fri, 4 Jun 2021 22:06:31 GMT, Kevin Rushforth wrote:
>> Michael Strauß has updated the pull request with a new target base due to a
>> merge or a rebase. The pull request now contains 15 commits:
>>
>> - Merge branch 'master' into feature/image-datauri
>>
>># Conflicts:
>>#
>> modules/javafx.graphics/src/test/java/test/com/sun/javafx/iio/ImageStorageTest.java
>> - Accept regular file paths, change javadoc
>> - Merge branch 'openjdk:master' into feature/image-datauri
>> - Rename DataURI.isValidURI
>> - Reverted a change
>> - Allow leading and trailing whitespace in data URI
>> - Removed test
>> - Changed data URI detection
>> - Merge branch 'master' into feature/image-datauri
>> - Moved test
>> - ... and 5 more:
>> https://git.openjdk.java.net/jfx/compare/7b7050c4...315523c5
>
> modules/javafx.graphics/src/main/java/com/sun/javafx/util/DataURI.java line
> 191:
>
>> 189: public boolean equals(Object o) {
>> 190: if (this == o) return true;
>> 191: if (o == null || getClass() != o.getClass()) return false;
>
> Can this be replaced by `o instanceof DataURI`?
Sure.
-
PR: https://git.openjdk.java.net/jfx/pull/508
Re: RFR: 8267551: Support loading images from inline data-URIs [v13]
On Sat, 5 Jun 2021 15:07:36 GMT, Kevin Rushforth wrote: >> I implemented it in this way so that this requirement is not imposed onto >> the caller, similar to `java.net.URL` does not impose this requirement onto >> its callers. I can imagine that `DataURI` might be used in other places, so >> it might make sense to make it more robust with regards to leading >> whitespace? > > In that case, it might be clearer and simpler to just call `trim()` on the > input String before doing anything with it, unless there is a reason not to. My idea was to avoid repeatedly allocating a new String with the data contained in the URI. Since `matchScheme` is called at least twice (maybe more), that would be a total of at least three copies of the URI data until we parse it. Granted, this would only apply for URIs that contain leading or trailing whitespace (since `trim()` is specified to return the same String instance if it contains no leading or trailing whitespace). - PR: https://git.openjdk.java.net/jfx/pull/508
Re: RFR: 8267551: Support loading images from inline data-URIs [v13]
On Fri, 4 Jun 2021 22:03:26 GMT, Kevin Rushforth wrote:
>> Michael Strauß has updated the pull request with a new target base due to a
>> merge or a rebase. The pull request now contains 15 commits:
>>
>> - Merge branch 'master' into feature/image-datauri
>>
>># Conflicts:
>>#
>> modules/javafx.graphics/src/test/java/test/com/sun/javafx/iio/ImageStorageTest.java
>> - Accept regular file paths, change javadoc
>> - Merge branch 'openjdk:master' into feature/image-datauri
>> - Rename DataURI.isValidURI
>> - Reverted a change
>> - Allow leading and trailing whitespace in data URI
>> - Removed test
>> - Changed data URI detection
>> - Merge branch 'master' into feature/image-datauri
>> - Moved test
>> - ... and 5 more:
>> https://git.openjdk.java.net/jfx/compare/7b7050c4...315523c5
>
> modules/javafx.graphics/src/main/java/com/sun/javafx/util/DataURI.java line
> 175:
>
>> 173:
>> 174: public byte[] getData() {
>> 175: return data;
>
> This returns a reference to the byte array rather than a copy. Is this what
> is wanted? If so, it would be good to add a comment.
I added a comment that clarifies this.
> modules/javafx.graphics/src/main/java/com/sun/javafx/util/DataURI.java line
> 185:
>
>> 183:
>> 184: return originalUri.substring(0, originalUri.length() -
>> originalData.length())
>> 185: + originalData.substring(0, 14) + "..." +
>> originalData.substring(originalData.length() - 14);
>
> Why truncate it?
The idea was to not clutter logs with large pieces of data. Do you think that
the entire data string should be logged?
-
PR: https://git.openjdk.java.net/jfx/pull/508
Re: RFR: 8267551: Support loading images from inline data-URIs [v13]
On Sat, 5 Jun 2021 14:56:15 GMT, Michael Strauß wrote: >> modules/javafx.graphics/src/main/java/com/sun/javafx/util/DataURI.java line >> 47: >> >>> 45: } >>> 46: >>> 47: int firstNonWhitespace = 0, length = uri.length(); >> >> Why do you need to trim leading spaces? The input URL strings should already >> be trimmed by the caller. > > I implemented it in this way so that this requirement is not imposed onto the > caller, similar to `java.net.URL` does not impose this requirement onto its > callers. I can imagine that `DataURI` might be used in other places, so it > might make sense to make it more robust with regards to leading whitespace? In that case, it might be clearer and simpler to just call `trim()` on the input String before doing anything with it, unless there is a reason not to. - PR: https://git.openjdk.java.net/jfx/pull/508
Re: RFR: 8267551: Support loading images from inline data-URIs [v13]
On Fri, 4 Jun 2021 21:59:28 GMT, Kevin Rushforth wrote: >> Michael Strauß has updated the pull request with a new target base due to a >> merge or a rebase. The pull request now contains 15 commits: >> >> - Merge branch 'master' into feature/image-datauri >> >># Conflicts: >># >> modules/javafx.graphics/src/test/java/test/com/sun/javafx/iio/ImageStorageTest.java >> - Accept regular file paths, change javadoc >> - Merge branch 'openjdk:master' into feature/image-datauri >> - Rename DataURI.isValidURI >> - Reverted a change >> - Allow leading and trailing whitespace in data URI >> - Removed test >> - Changed data URI detection >> - Merge branch 'master' into feature/image-datauri >> - Moved test >> - ... and 5 more: >> https://git.openjdk.java.net/jfx/compare/7b7050c4...315523c5 > > modules/javafx.graphics/src/main/java/com/sun/javafx/util/DataURI.java line > 47: > >> 45: } >> 46: >> 47: int firstNonWhitespace = 0, length = uri.length(); > > Why do you need to trim leading spaces? The input URL strings should already > be trimmed by the caller. I implemented it in this way so that this requirement is not imposed onto the caller, similar to `java.net.URL` does not impose this requirement onto its callers. I can imagine that `DataURI` might be used in other places, so it might make sense to make it more robust with regards to leading whitespace? - PR: https://git.openjdk.java.net/jfx/pull/508
Re: RFR: 8267551: Support loading images from inline data-URIs [v13]
On Fri, 4 Jun 2021 21:56:32 GMT, Kevin Rushforth wrote:
>> Michael Strauß has updated the pull request with a new target base due to a
>> merge or a rebase. The pull request now contains 15 commits:
>>
>> - Merge branch 'master' into feature/image-datauri
>>
>># Conflicts:
>>#
>> modules/javafx.graphics/src/test/java/test/com/sun/javafx/iio/ImageStorageTest.java
>> - Accept regular file paths, change javadoc
>> - Merge branch 'openjdk:master' into feature/image-datauri
>> - Rename DataURI.isValidURI
>> - Reverted a change
>> - Allow leading and trailing whitespace in data URI
>> - Removed test
>> - Changed data URI detection
>> - Merge branch 'master' into feature/image-datauri
>> - Moved test
>> - ... and 5 more:
>> https://git.openjdk.java.net/jfx/compare/7b7050c4...315523c5
>
> modules/javafx.graphics/src/main/java/com/sun/javafx/iio/ImageStorage.java
> line 333:
>
>> 331: loader = getLoaderBySignature(theStream, listener);
>> 332: }
>> 333: } catch (Exception e) {
>
> Is this change needed?
If a MIME type other than `image` was specified in the data URL,
`IllegalArgumentException` is thrown. If we only catch `IOException` here, the
IAE will not be wrapped into a `ImageStorageException`.
-
PR: https://git.openjdk.java.net/jfx/pull/508
Re: RFR: 8267551: Support loading images from inline data-URIs [v13]
On Fri, 4 Jun 2021 21:54:12 GMT, Kevin Rushforth wrote:
>> Michael Strauß has updated the pull request with a new target base due to a
>> merge or a rebase. The pull request now contains 15 commits:
>>
>> - Merge branch 'master' into feature/image-datauri
>>
>># Conflicts:
>>#
>> modules/javafx.graphics/src/test/java/test/com/sun/javafx/iio/ImageStorageTest.java
>> - Accept regular file paths, change javadoc
>> - Merge branch 'openjdk:master' into feature/image-datauri
>> - Rename DataURI.isValidURI
>> - Reverted a change
>> - Allow leading and trailing whitespace in data URI
>> - Removed test
>> - Changed data URI detection
>> - Merge branch 'master' into feature/image-datauri
>> - Moved test
>> - ... and 5 more:
>> https://git.openjdk.java.net/jfx/compare/7b7050c4...315523c5
>
> modules/javafx.graphics/src/main/java/com/sun/javafx/iio/ImageStorage.java
> line 290:
>
>> 288: /**
>> 289: * Load all images present in the specified input. For more details
>> refer to
>> 290: * {@link #loadAll(InputStream, ImageLoadListener, double, double,
>> boolean, float, boolean)}.
>
> `ImageLoadListener` isn't imported, so still needs to be qualified (although
> we don't actually generate any docs since this is an implementation class).
`ImageLoadListener` is in the same package as `ImageStorage`, so it should be
implicitly imported.
-
PR: https://git.openjdk.java.net/jfx/pull/508
Re: RFR: 8267551: Support loading images from inline data-URIs [v13]
On Wed, 26 May 2021 16:36:24 GMT, Michael Strauß wrote:
>> This PR adds support for loading images from [inline data
>> URIs](https://en.wikipedia.org/wiki/Data_URI_scheme), which is also widely
>> supported by web browsers. This enables developers to package small images
>> in CSS files, rather than separately deploying the images alongside the CSS
>> file.
>
> Michael Strauß has updated the pull request with a new target base due to a
> merge or a rebase. The pull request now contains 15 commits:
>
> - Merge branch 'master' into feature/image-datauri
>
># Conflicts:
>#
> modules/javafx.graphics/src/test/java/test/com/sun/javafx/iio/ImageStorageTest.java
> - Accept regular file paths, change javadoc
> - Merge branch 'openjdk:master' into feature/image-datauri
> - Rename DataURI.isValidURI
> - Reverted a change
> - Allow leading and trailing whitespace in data URI
> - Removed test
> - Changed data URI detection
> - Merge branch 'master' into feature/image-datauri
> - Moved test
> - ... and 5 more:
> https://git.openjdk.java.net/jfx/compare/7b7050c4...315523c5
modules/javafx.graphics/src/main/java/com/sun/javafx/css/StyleManager.java line
776:
> 774: } else {
> 775: logger.warning("Error loading image:
> " + url);
> 776: }
So if I understand this correctly, an Image is first loaded via the standard
mechanism, and if that fails for any reason (`image.isError()`) a second
attempt is made through `DataURI` ?
Would it not be better to register a new protocol so `new Image` would just
succeed for data uri's ?
-
PR: https://git.openjdk.java.net/jfx/pull/508
Re: RFR: 8267551: Support loading images from inline data-URIs [v13]
On Wed, 26 May 2021 16:36:24 GMT, Michael Strauß wrote:
>> This PR adds support for loading images from [inline data
>> URIs](https://en.wikipedia.org/wiki/Data_URI_scheme), which is also widely
>> supported by web browsers. This enables developers to package small images
>> in CSS files, rather than separately deploying the images alongside the CSS
>> file.
>
> Michael Strauß has updated the pull request with a new target base due to a
> merge or a rebase. The pull request now contains 15 commits:
>
> - Merge branch 'master' into feature/image-datauri
>
># Conflicts:
>#
> modules/javafx.graphics/src/test/java/test/com/sun/javafx/iio/ImageStorageTest.java
> - Accept regular file paths, change javadoc
> - Merge branch 'openjdk:master' into feature/image-datauri
> - Rename DataURI.isValidURI
> - Reverted a change
> - Allow leading and trailing whitespace in data URI
> - Removed test
> - Changed data URI detection
> - Merge branch 'master' into feature/image-datauri
> - Moved test
> - ... and 5 more:
> https://git.openjdk.java.net/jfx/compare/7b7050c4...315523c5
I haven't tested this yet, but have done a first pass on the spec changes and
most of the implementation (I'll finish next week).
Overall, the docs look good. Can you also update the list of supported image
formats in the `Image` class docs just before line 76?
modules/javafx.graphics/src/main/java/com/sun/javafx/iio/ImageStorage.java line
290:
> 288: /**
> 289: * Load all images present in the specified input. For more details
> refer to
> 290: * {@link #loadAll(InputStream, ImageLoadListener, double, double,
> boolean, float, boolean)}.
`ImageLoadListener` isn't imported, so still needs to be qualified (although we
don't actually generate any docs since this is an implementation class).
modules/javafx.graphics/src/main/java/com/sun/javafx/iio/ImageStorage.java line
333:
> 331: loader = getLoaderBySignature(theStream, listener);
> 332: }
> 333: } catch (Exception e) {
Is this change needed?
modules/javafx.graphics/src/main/java/com/sun/javafx/util/DataURI.java line 47:
> 45: }
> 46:
> 47: int firstNonWhitespace = 0, length = uri.length();
Why do you need to trim leading spaces? The input URL strings should already be
trimmed by the caller.
modules/javafx.graphics/src/main/java/com/sun/javafx/util/DataURI.java line 76:
> 74:
> 75: if (Character.isWhitespace(uri.charAt(0))) {
> 76: uri = uri.trim();
Same question as above.
modules/javafx.graphics/src/main/java/com/sun/javafx/util/DataURI.java line 175:
> 173:
> 174: public byte[] getData() {
> 175: return data;
This returns a reference to the byte array rather than a copy. Is this what is
wanted? If so, it would be good to add a comment.
modules/javafx.graphics/src/main/java/com/sun/javafx/util/DataURI.java line 185:
> 183:
> 184: return originalUri.substring(0, originalUri.length() -
> originalData.length())
> 185: + originalData.substring(0, 14) + "..." +
> originalData.substring(originalData.length() - 14);
Why truncate it?
modules/javafx.graphics/src/main/java/com/sun/javafx/util/DataURI.java line 191:
> 189: public boolean equals(Object o) {
> 190: if (this == o) return true;
> 191: if (o == null || getClass() != o.getClass()) return false;
Can this be replaced by `o instanceof DataURI`?
modules/javafx.graphics/src/main/java/javafx/css/converter/URLConverter.java
line 98:
> 96: }
> 97:
> 98: private URL resolve(String stylesheetUrl, String resource) {
Why was this change done? It seems unnecessary and possibly unwanted.
modules/javafx.graphics/src/main/java/javafx/scene/image/Image.java line 612:
> 610: /**
> 611: * Constructs an {@code Image} with content loaded from the specified
> 612: * image file. The {@code url} parameter can be one of the following:
I would not use the words "image file" in the first sentence. I recommend to
restore this to "url".
modules/javafx.graphics/src/main/java/javafx/scene/image/Image.java line 624:
> 622: *
> 623: * If a URL uses the "data" scheme, the data must be base64-encoded
> 624: * and the MIME type must either be empty or a subtype of the
These two paragraphs might be better as part of the class docs (which is where
we list the supported image formats).
modules/javafx.graphics/src/main/java/javafx/scene/image/Image.java line 638:
> 636: /**
> 637: * Constructs an {@code Image} with content loaded from the specified
> 638: * image file. The {@code url} parameter can be one of the following:
Similar comment. Maybe something like this for the first sentence?
Constructs an {@code Image} with content loaded from the specified url using
the specified parameters.
-
PR: https://git.openjdk.java.net/jfx/pull/508
Re: RFR: 8267551: Support loading images from inline data-URIs [v13]
> This PR adds support for loading images from [inline data > URIs](https://en.wikipedia.org/wiki/Data_URI_scheme), which is also widely > supported by web browsers. This enables developers to package small images in > CSS files, rather than separately deploying the images alongside the CSS file. Michael Strauß has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 15 commits: - Merge branch 'master' into feature/image-datauri # Conflicts: # modules/javafx.graphics/src/test/java/test/com/sun/javafx/iio/ImageStorageTest.java - Accept regular file paths, change javadoc - Merge branch 'openjdk:master' into feature/image-datauri - Rename DataURI.isValidURI - Reverted a change - Allow leading and trailing whitespace in data URI - Removed test - Changed data URI detection - Merge branch 'master' into feature/image-datauri - Moved test - ... and 5 more: https://git.openjdk.java.net/jfx/compare/7b7050c4...315523c5 - Changes: https://git.openjdk.java.net/jfx/pull/508/files Webrev: https://webrevs.openjdk.java.net/?repo=jfx=508=12 Stats: 566 lines in 8 files changed: 501 ins; 23 del; 42 mod Patch: https://git.openjdk.java.net/jfx/pull/508.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/508/head:pull/508 PR: https://git.openjdk.java.net/jfx/pull/508
