New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
8267554: Support loading stylesheets from data-URIs #536
Conversation
👋 Welcome back mstrauss! A progress list of the required criteria for merging this PR into |
Webrevs
|
/reviewers 2 |
@kevinrushforth |
@kevinrushforth has indicated that a compatibility and specification (CSR) request is needed for this pull request. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
Data URIs also work for user-agent stylesheets. I added documentation to each of the methods. |
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 I'll review the implementation next. |
I've fixed that, and also cleaned up the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Outdated
Show resolved
Hide resolved
modules/javafx.graphics/src/test/java/test/javafx/css/StylesheetTest.java
Outdated
Show resolved
Hide resolved
I added a test for each of |
Changes look good. I'll do a final pass tomorrow. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good.
@mstr2 This change now passes all automated pre-integration checks. ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details. After integration, the commit message for the final commit will be:
You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed. At the time when this comment was updated there had been 6 new commits pushed to the
Please see this link for an up-to-date comparison between the source branch of this pull request and the As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@kevinrushforth, @aghaisas) but any other Committer may sponsor as well. ➡️ To flag this PR as ready for integration with the above commit message, type |
/integrate |
/sponsor |
Going to push as commit 78179be.
Your commit was automatically rebased without conflicts. |
@kevinrushforth @mstr2 Pushed as commit 78179be. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
This PR extends data URI support to allow stylesheets to be loaded from data URIs.
Progress
Issue
Reviewers
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jfx pull/536/head:pull/536
$ git checkout pull/536
Update a local copy of the PR:
$ git checkout pull/536
$ git pull https://git.openjdk.java.net/jfx pull/536/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 536
View PR using the GUI difftool:
$ git pr show -t 536
Using diff file
Download this PR as a diff file:
https://git.openjdk.java.net/jfx/pull/536.diff