On Wed, 23 Jun 2021 13:09:51 GMT, Michael Strauß <mstra...@openjdk.org> wrote:
>> This PR extends data URI support to allow stylesheets to be loaded from data
>> URIs.
>
> Michael Strauß has updated the pull request incrementally with one additional
> commit since the last revision:
>
> cleaned up paragraph tags
I spent some time analyzing the changes to
`StyleManager::loadStylesheetUnPrivileged`, especially around the use of the
local `parse` variable, since you no longer modify it during the processing in
a couple places. I think everything is fine, but I'd like @aghaisas to also
look at this when he reviews it. If @dsgrieve has any comments, they would be
welcome.
The following is the current behavior for the case where `fname` ends with
`.css` or `.bss`:
| `fname` | `.css` exists | `.bss` exists | Result |
| ------- | ------------- | ------------- | ------ |
| _SOMENAME_.bss | YES | NO | fail |
| _SOMENAME_.bss | NO | YES | Use _SOMENAME_.bss instead |
| _SOMENAME_.bss | YES | YES | Use _SOMENAME_.bss |
| _SOMENAME_.css | YES | NO | Use _SOMENAME_.css |
| _SOMENAME_.css | NO | YES | Use _SOMENAME_.bss (NOTE: if `-Dbinary.css=false`
then fail) |
| _SOMENAME_.css | YES | YES | Use _SOMENAME_.bss (NOTE: if
`-Dbinary.css=false` then _SOMENAME_.css) |
In case where `fname` ends with neither `.css` nor `.css`, it is assumed to be
a CSS file and parsed as such.
As near as I can tell, both from reading the code and from running tests, this
is still working as expected. So there should be no regression in behavior.
The code to process a data URI looks good.
As for the tests, can you add at least one test of calling the public
`setUserAgentStylesheet` method using a data URI? Maybe as part of
`StyleManagerTest`? Also, I left a couple suggestions inline.
modules/javafx.graphics/src/main/java/javafx/css/Stylesheet.java line 316:
> 314: final int bssVersion = dataInputStream.readShort();
> 315: if (bssVersion > Stylesheet.BINARY_CSS_VERSION) {
> 316: throw new IOException("Wrong binary CSS version: "
If the `url` string is non-null shouldn't it still be part of the exception
message?
modules/javafx.graphics/src/test/java/test/javafx/css/StylesheetTest.java line
658:
> 656: try {
> 657: inputFile = File.createTempFile("convertCssTextToBinary",
> ".css");
> 658: outputFile = File.createTempFile("convertCssTextToBinary",
> ".bss");
Instead of writing a pair of temp files, maybe you can call `CssParser.parse`
to create a `Stylesheet` and then `Stylesheet.writeBinary` to write to an in
memory `DataOutputStream`? Or alternatively, create the `.bss file` and add it
as a resource under `src/test/resources/...`?
-------------
PR: https://git.openjdk.java.net/jfx/pull/536
