Re: RFR: 8267554: Support loading stylesheets from data-URIs [v7]
On Fri, 25 Jun 2021 13:54:24 GMT, Michael Strauß 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: > > reset app UA stylesheet to empty stylesheet Marked as reviewed by aghaisas (Reviewer). - PR: https://git.openjdk.java.net/jfx/pull/536
Re: RFR: 8267554: Support loading stylesheets from data-URIs [v7]
On Fri, 25 Jun 2021 13:54:24 GMT, Michael Strauß 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: > > reset app UA stylesheet to empty stylesheet Looks good. - Marked as reviewed by kcr (Lead). PR: https://git.openjdk.java.net/jfx/pull/536
Re: RFR: 8267554: Support loading stylesheets from data-URIs [v6]
On Fri, 25 Jun 2021 13:31:45 GMT, Kevin Rushforth wrote:
>> modules/javafx.graphics/src/test/java/test/com/sun/javafx/css/StyleManagerTest.java
>> line 1171:
>>
>>> 1169: assertEquals(Color.BLUE, rect.getFill());
>>> 1170: } finally {
>>> 1171: Application.setUserAgentStylesheet(uaStylesheet);
>>
>> This line in `finally` block results in `ClassNotFoundException`
>>
>> Run the tests as below to get the log-
>>
>> `gradle --info :graphics:test &> test.log`
>
> Good catch. The reason for this is that the default Stylesheet is in the
> `javafx.controls` module, which is not available when running unit tests in
> the `javafx.graphics` module. I recommend to either `@Ignore` this test or
> else reset it to an empty style sheet (which is effectively what it will be
> for `javafx.graphics` tests, since the platform initialization code that sets
> up the default stylesheet is never called for these tests).
I've decided to reset the UA stylesheet to an empty stylesheet.
-
PR: https://git.openjdk.java.net/jfx/pull/536
Re: RFR: 8267554: Support loading stylesheets from data-URIs [v7]
> 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: reset app UA stylesheet to empty stylesheet - Changes: - all: https://git.openjdk.java.net/jfx/pull/536/files - new: https://git.openjdk.java.net/jfx/pull/536/files/64d72fe3..230cc49e Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx&pr=536&range=06 - incr: https://webrevs.openjdk.java.net/?repo=jfx&pr=536&range=05-06 Stats: 2 lines in 1 file changed: 0 ins; 1 del; 1 mod Patch: https://git.openjdk.java.net/jfx/pull/536.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/536/head:pull/536 PR: https://git.openjdk.java.net/jfx/pull/536
Re: RFR: 8267554: Support loading stylesheets from data-URIs [v6]
On Fri, 25 Jun 2021 12:07:23 GMT, Ajit Ghaisas wrote:
>> Michael Strauß has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> changes per review
>
> modules/javafx.graphics/src/test/java/test/com/sun/javafx/css/StyleManagerTest.java
> line 1171:
>
>> 1169: assertEquals(Color.BLUE, rect.getFill());
>> 1170: } finally {
>> 1171: Application.setUserAgentStylesheet(uaStylesheet);
>
> This line in `finally` block results in `ClassNotFoundException`
>
> Run the tests as below to get the log-
>
> `gradle --info :graphics:test &> test.log`
Good catch. The reason for this is that the default Stylesheet is in the
`javafx.controls` module, which is not available when running unit tests in the
`javafx.graphics` module. I recommend to either `@Ignore` this test or else
reset it to an empty style sheet (which is effectively what it will be for
`javafx.graphics` tests, since the platform initialization code that sets up
the default stylesheet is never called for these tests).
-
PR: https://git.openjdk.java.net/jfx/pull/536
Re: RFR: 8267554: Support loading stylesheets from data-URIs [v6]
On Thu, 24 Jun 2021 23:24:43 GMT, Michael Strauß 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:
>
> changes per review
Code changes are OK.
I found an Exception being logged from newly added test. Please check.
modules/javafx.graphics/src/test/java/test/com/sun/javafx/css/StyleManagerTest.java
line 1171:
> 1169: assertEquals(Color.BLUE, rect.getFill());
> 1170: } finally {
> 1171: Application.setUserAgentStylesheet(uaStylesheet);
This line in `finally` block results in `ClassNotFoundException`
Run the tests as below to get the log-
`gradle --info :graphics:test &> test.log`
-
PR: https://git.openjdk.java.net/jfx/pull/536
Re: RFR: 8267554: Support loading stylesheets from data-URIs [v6]
On Thu, 24 Jun 2021 23:24:43 GMT, Michael Strauß 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: > > changes per review Changes look good. I'll do a final pass tomorrow. - PR: https://git.openjdk.java.net/jfx/pull/536
Re: RFR: 8267554: Support loading stylesheets from data-URIs [v6]
On Thu, 24 Jun 2021 23:24:43 GMT, Michael Strauß 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: > > changes per review I added a test for each of `Application.setUserAgentStylesheet`, `Scene.setUserAgentStylesheet`, and `SubScene.setUserAgentStylesheet`. - PR: https://git.openjdk.java.net/jfx/pull/536
Re: RFR: 8267554: Support loading stylesheets from data-URIs [v6]
> 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: changes per review - Changes: - all: https://git.openjdk.java.net/jfx/pull/536/files - new: https://git.openjdk.java.net/jfx/pull/536/files/c734640f..64d72fe3 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx&pr=536&range=05 - incr: https://webrevs.openjdk.java.net/?repo=jfx&pr=536&range=04-05 Stats: 84 lines in 4 files changed: 52 ins; 9 del; 23 mod Patch: https://git.openjdk.java.net/jfx/pull/536.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/536/head:pull/536 PR: https://git.openjdk.java.net/jfx/pull/536
Re: RFR: 8267554: Support loading stylesheets from data-URIs [v5]
On Thu, 24 Jun 2021 21:12:36 GMT, Kevin Rushforth wrote:
>> Michael Strauß has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> cleaned up paragraph tags
>
> 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?
True.
-
PR: https://git.openjdk.java.net/jfx/pull/536
Re: RFR: 8267554: Support loading stylesheets from data-URIs [v5]
On Wed, 23 Jun 2021 13:09:51 GMT, Michael Strauß 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
Re: RFR: 8267554: Support loading stylesheets from data-URIs [v4]
On Wed, 23 Jun 2021 12:57:10 GMT, Kevin Rushforth wrote: > One thing I noticed is that the diffs don't exactly match what you pasted in > the CSR (looks like a copy/paste issue). The `Scene::setUserAgentStylesheet` > and `SubScene::setUserAgentStylesheet` methods each have three unchanged > lines that are marked with the `+` diff indicator. Can you double-check? I've fixed that, and also cleaned up the `` tags for the new paragraphs. - PR: https://git.openjdk.java.net/jfx/pull/536
Re: RFR: 8267554: Support loading stylesheets from data-URIs [v5]
> 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 - Changes: - all: https://git.openjdk.java.net/jfx/pull/536/files - new: https://git.openjdk.java.net/jfx/pull/536/files/7dfbc0cb..c734640f Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx&pr=536&range=04 - incr: https://webrevs.openjdk.java.net/?repo=jfx&pr=536&range=03-04 Stats: 8 lines in 3 files changed: 0 ins; 2 del; 6 mod Patch: https://git.openjdk.java.net/jfx/pull/536.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/536/head:pull/536 PR: https://git.openjdk.java.net/jfx/pull/536
Re: RFR: 8267554: Support loading stylesheets from data-URIs [v4]
On Tue, 22 Jun 2021 23:15:55 GMT, Michael Strauß 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: > > simplified branch in StyleManager.loadStylesheetUnPrivileged The API and spec changes look good. As I mentioned in a comment added to the CSR, it is ready to move to "Proposed". Once we are farther along in the code review, I'll formally review the CSR. One thing I noticed is that the diffs don't exactly match what you pasted in the CSR (looks like a copy/paste issue). The `Scene::setUserAgentStylesheet` and `SubScene::setUserAgentStylesheet` methods each have three unchanged lines that are marked with the `+` diff indicator. Can you double-check? I'll review the implementation next. - PR: https://git.openjdk.java.net/jfx/pull/536
Re: RFR: 8267554: Support loading stylesheets from data-URIs [v4]
> 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: simplified branch in StyleManager.loadStylesheetUnPrivileged - Changes: - all: https://git.openjdk.java.net/jfx/pull/536/files - new: https://git.openjdk.java.net/jfx/pull/536/files/39ff7583..7dfbc0cb Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx&pr=536&range=03 - incr: https://webrevs.openjdk.java.net/?repo=jfx&pr=536&range=02-03 Stats: 46 lines in 1 file changed: 15 ins; 24 del; 7 mod Patch: https://git.openjdk.java.net/jfx/pull/536.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/536/head:pull/536 PR: https://git.openjdk.java.net/jfx/pull/536
Re: RFR: 8267554: Support loading stylesheets from data-URIs [v3]
On Mon, 21 Jun 2021 15:04:26 GMT, Michael Strauß wrote: >> An overloaded method with a new signature is still a new method that adds to >> the public API, although in an easy to understand way. I agree that the same >> argument could be made for it as for `loadBinary(URL)`, but if neither are >> needed, then maybe we would be better off to deprecate the existing method >> (not as part of this enhancement, of course), rather than adding a similar >> one that also isn't needed as part of the public API. On the other hand, if >> there is a good reason, I don't see a problem adding this new method. >> >> If you do propose to add this API, you will need to add an appropriate >> `@since` tag, and you should write an API-level test for this method as part >> of `StylesheetTest`. > > It seems to me that the only useful thing an application developer could do > with a `Stylesheet` instance is to inspect its contents (the relevant APIs in > javafx.css are public, after all). That seems like a niche use case, but to > the extent that there are applications out there that inspect binary > stylesheets, having the option to load them from a stream seems at least > marginally useful. > > I slightly lean towards adding this public API also to not having to > introducing a new helper class _just_ for this one benign method. If you have > a stronger opinion on this, please go ahead and make the call. Avoiding the need for an accessor isn't sufficient reason, but I do like the symmetry with the existing `loadBinary(URL)` method. Also, one can create a `Stylesheet` by parsing a text file using the various `parse` methods of `CssParser`, so there is ample precedent for doing this. I have no objection to the public method. - PR: https://git.openjdk.java.net/jfx/pull/536
Re: RFR: 8267554: Support loading stylesheets from data-URIs [v3]
On Mon, 21 Jun 2021 14:13:47 GMT, Kevin Rushforth wrote: >> I don't know why JavaFX applications would need this API. But the argument >> should be the same argument as for `loadBinary(URL)`, which is also only >> used internally by `StyleManager`. I think that this isn't opening up any >> new effective API surface, because it's arguably just another overload of >> the existing API. > > An overloaded method with a new signature is still a new method that adds to > the public API, although in an easy to understand way. I agree that the same > argument could be made for it as for `loadBinary(URL)`, but if neither are > needed, then maybe we would be better off to deprecate the existing method > (not as part of this enhancement, of course), rather than adding a similar > one that also isn't needed as part of the public API. On the other hand, if > there is a good reason, I don't see a problem adding this new method. > > If you do propose to add this API, you will need to add an appropriate > `@since` tag, and you should write an API-level test for this method as part > of `StylesheetTest`. It seems to me that the only useful thing an application developer could do with a `Stylesheet` instance is to inspect its contents (the relevant APIs in javafx.css are public, after all). That seems like a niche use case, but to the extent that there are applications out there that inspect binary stylesheets, having the option to load them from a stream seems at least marginally useful. I slightly lean towards adding this public API also to not having to introducing a new helper class _just_ for this one benign method. If you have a stronger opinion on this, please go ahead and make the call. - PR: https://git.openjdk.java.net/jfx/pull/536
Re: RFR: 8267554: Support loading stylesheets from data-URIs [v3]
> 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: added test for Stylesheet.loadBinary(InputStream) - Changes: - all: https://git.openjdk.java.net/jfx/pull/536/files - new: https://git.openjdk.java.net/jfx/pull/536/files/70c29866..39ff7583 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx&pr=536&range=02 - incr: https://webrevs.openjdk.java.net/?repo=jfx&pr=536&range=01-02 Stats: 58 lines in 2 files changed: 40 ins; 17 del; 1 mod Patch: https://git.openjdk.java.net/jfx/pull/536.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/536/head:pull/536 PR: https://git.openjdk.java.net/jfx/pull/536
Re: RFR: 8267554: Support loading stylesheets from data-URIs [v2]
On Sun, 20 Jun 2021 00:22:57 GMT, Michael Strauß wrote:
>> modules/javafx.graphics/src/main/java/javafx/css/Stylesheet.java line 301:
>>
>>> 299: * css version or if an I/O error occurs while reading from the
>>> stream
>>> 300: */
>>> 301: public static Stylesheet loadBinary(InputStream stream) throws
>>> IOException {
>>
>> Why do you need to add this public method to the API? I don't see any
>> discussion as to why JavaFX applications need it. It looks like it is just
>> being used internally by `StyleManager`. Unless there is a compelling reason
>> to add this to the API, you will need to make this method package-scope and
>> use an accessor to access it from `StyleManager`.
>
> I don't know why JavaFX applications would need this API. But the argument
> should be the same argument as for `loadBinary(URL)`, which is also only used
> internally by `StyleManager`. I think that this isn't opening up any new
> effective API surface, because it's arguably just another overload of the
> existing API.
An overloaded method with a new signature is still a new method that adds to
the public API, although in an easy to understand way. I agree that the same
argument could be made for it as for `loadBinary(URL)`, but if neither are
needed, then maybe we would be better off to deprecate the existing method (not
as part of this enhancement, of course), rather than adding a similar one that
also isn't needed as part of the public API. On the other hand, if there is a
good reason, I don't see a problem adding this new method.
If you do propose to add this API, you will need to add an appropriate `@since`
tag, and you should write an API-level test for this method as part of
`StylesheetTest`.
-
PR: https://git.openjdk.java.net/jfx/pull/536
Re: RFR: 8267554: Support loading stylesheets from data-URIs [v2]
On Fri, 18 Jun 2021 18:43:24 GMT, Kevin Rushforth wrote:
> * Do you intend to support setting a user agent stylesheet via a data URL?
> The docs should be explicit about whether or not a data URI can be used for
> the following methods:
>
> *
> [Application::setUserAgentStylesheet](https://github.com/openjdk/jfx/blob/master/modules/javafx.graphics/src/main/java/javafx/application/Application.java#L521)
> *
> [Scene::setUserAgentStylesheet](https://github.com/openjdk/jfx/blob/master/modules/javafx.graphics/src/main/java/javafx/scene/Scene.java#L1697)
> *
> [SubScene::setUserAgentStylesheet](https://github.com/openjdk/jfx/blob/master/modules/javafx.graphics/src/main/java/javafx/scene/SubScene.java#L687)
Data URIs also work for user-agent stylesheets. I added documentation to each
of the methods.
> modules/javafx.graphics/src/main/java/javafx/css/Stylesheet.java line 301:
>
>> 299: * css version or if an I/O error occurs while reading from the
>> stream
>> 300: */
>> 301: public static Stylesheet loadBinary(InputStream stream) throws
>> IOException {
>
> Why do you need to add this public method to the API? I don't see any
> discussion as to why JavaFX applications need it. It looks like it is just
> being used internally by `StyleManager`. Unless there is a compelling reason
> to add this to the API, you will need to make this method package-scope and
> use an accessor to access it from `StyleManager`.
I don't know why JavaFX applications would need this API. But the argument
should be the same argument as for `loadBinary(URL)`, which is also only used
internally by `StyleManager`. I think that this isn't opening up any new
effective API surface, because it's arguably just another overload of the
existing API.
-
PR: https://git.openjdk.java.net/jfx/pull/536
Re: RFR: 8267554: Support loading stylesheets from data-URIs [v2]
> 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: added docs - Changes: - all: https://git.openjdk.java.net/jfx/pull/536/files - new: https://git.openjdk.java.net/jfx/pull/536/files/16119e8c..70c29866 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx&pr=536&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jfx&pr=536&range=00-01 Stats: 48 lines in 3 files changed: 40 ins; 6 del; 2 mod Patch: https://git.openjdk.java.net/jfx/pull/536.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/536/head:pull/536 PR: https://git.openjdk.java.net/jfx/pull/536
Re: RFR: 8267554: Support loading stylesheets from data-URIs
On Fri, 18 Jun 2021 01:23:50 GMT, Michael Strauß wrote:
> This PR extends data URI support to allow stylesheets to be loaded from data
> URIs.
I looked at just the public API and spec, and have two comments:
* I don't see any justification for adding
`Stylesheet::loadBinary(InputStream)` to the public API. See my comments inline.
* Do you intend to support setting a user agent stylesheet via a data URL? The
docs should be explicit about whether or not a data URI can be used for the
following methods:
*
[Application::setUserAgentStylesheet](https://github.com/openjdk/jfx/blob/master/modules/javafx.graphics/src/main/java/javafx/application/Application.java#L521)
*
[Scene::setUserAgentStylesheet](https://github.com/openjdk/jfx/blob/master/modules/javafx.graphics/src/main/java/javafx/scene/Scene.java#L1697)
*
[SubScene::setUserAgentStylesheet](https://github.com/openjdk/jfx/blob/master/modules/javafx.graphics/src/main/java/javafx/scene/SubScene.java#L687)
modules/javafx.graphics/src/main/java/javafx/css/Stylesheet.java line 301:
> 299: * css version or if an I/O error occurs while reading from the
> stream
> 300: */
> 301: public static Stylesheet loadBinary(InputStream stream) throws
> IOException {
Why do you need to add this public method to the API? I don't see any
discussion as to why JavaFX applications need it. It looks like it is just
being used internally by `StyleManager`. Unless there is a compelling reason to
add this to the API, you will need to make this method package-scope and use an
accessor to access it from `StyleManager`.
-
PR: https://git.openjdk.java.net/jfx/pull/536
RFR: 8267554: Support loading stylesheets from data-URIs
This PR extends data URI support to allow stylesheets to be loaded from data URIs. - Commit messages: - Stylesheets can be loaded from data URIs Changes: https://git.openjdk.java.net/jfx/pull/536/files Webrev: https://webrevs.openjdk.java.net/?repo=jfx&pr=536&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8267554 Stats: 237 lines in 4 files changed: 218 ins; 7 del; 12 mod Patch: https://git.openjdk.java.net/jfx/pull/536.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/536/head:pull/536 PR: https://git.openjdk.java.net/jfx/pull/536
