On Wed, 26 May 2021 16:36:24 GMT, Michael Strauß <mstra...@openjdk.org> 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: * <p> > 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
