8202990: javafx webview css filter property with display scaling #279

bhaweshkc · 2020-08-11T16:18:37Z

ImageJava.cpp ignores CompositeOperator parameter in drawImage function due to which shadow was getting drawn on top of actual image. apply given composite operator to graphics context before drawing image to fix this issue. another issue is into WCGraphicsPrismContext.java. while blending two layers, applying state to the destination layer was missed due to which image was not getting drawn with right scale in hidpi mode. apply state to fix the issue.

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change must be properly reviewed

Issue

JDK-8202990: javafx webview css filter property with display scaling

Reviewers

Kevin Rushforth (@kevinrushforth - Reviewer)
Arun Joseph (@arun-joseph - Committer)

Download

$ git fetch https://git.openjdk.java.net/jfx pull/279/head:pull/279
$ git checkout pull/279

bridgekeeper · 2020-08-11T16:19:37Z

👋 Welcome back bchoudhary! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request.

mlbridge · 2020-08-11T16:55:13Z

Webrevs

kevinrushforth

With your patch installed, I see a race condition that manifests in one of two ways:

If I load the new .html file with HelloWebView, I see the shadow most of the time, but it is missing some of the time.
The new unit test crashes intermittently

I suspect this might be related to the gc.setCompositeOperation call. Are you certain this is being done on the right thread?

kevinrushforth · 2020-08-12T15:59:28Z

/reviewers 2

openjdk · 2020-08-12T15:59:34Z

@kevinrushforth
The number of required reviews for this PR is now set to 2 (with at least 1 of role reviewers).

arun-joseph · 2020-08-26T17:06:36Z

With your patch installed, I see a race condition that manifests in one of two ways:

I couldn't reproduce the race condition or crash with the fix.

I suspect this might be related to the gc.setCompositeOperation call.

This may not be the cause as the compositeOperator is pushed to a RenderQueue which is decoded from the java side.

arun-joseph

The fix and test looks good.

kevinrushforth · 2020-08-29T00:31:51Z

I spent some time this afternoon going over the fix in more detail and doing extensive testing on both Windows and Mac.

I believe the fix is good. Both by inspection and by instrumenting the code, there are no race conditions or other problems that I can see.

The problem appears to be in the test, or else somewhere in the test harness or the SW pipeline exposed by the test. If I run the new CSSTest in a loop on either Mac or Windows, it will crash intermittently. I then reverted your fix, running the new test (which will throw an expected assertion error), and it still crashes intermittently.

My recommendation is to use a system test, similar to what SVGTest.java does, rather than a unit test in the javafx.web module, which uses WebPage::paint.

kevinrushforth · 2020-08-31T19:09:15Z

I believe that the problem noted above is with the test harness, specifically the WebPageShim::paint method. I filed JDK-8252596 to track fixing the tests.

So the fix for this PR should avoid using it, as suggested above.

kevinrushforth

Looks good. I verified that the system test still catches the bug (that is, it fails without the fix and passes with the fix). I ran it in a loop 100 times on both Mac and Windows without crashing.

tests/system/src/test/java/test/javafx/scene/web/CSSFilterTest.java

arun-joseph

The fix and test looks good.

openjdk · 2020-09-03T17:39:27Z

@bhaweshkc This change now passes all automated pre-integration checks. When the change also fulfills all project specific requirements, type /integrate in a new comment to proceed. After integration, the commit message will be:

8202990: javafx webview css filter property with display scaling

Reviewed-by: kcr, ajoseph

If you would like to add a summary, use the /summary command.
To credit additional contributors, use the /contributor command.
To add additional solved issues, use the /issue command.

Since the source branch of this PR was last updated there have been 18 commits pushed to the master branch:

a5ecfb6: 8252387: Deprecate for removal css Selector and ShapeConverter constructors
8e3616c: 8252062: WebKit build fails with recent VS 2019 compiler
87ad57f: 8251858: Update to Xcode 11.3.1
88c0f97: 8252381: Cherry pick GTK WebKit 2.28.4 changes
c86bd35: 8251941: ListCell: visual artifact when items contain null values
23ad8f4: 8251353: Many javafx scenegraph classes have implicit no-arg constructors
7a4bd9b: 8252060: gstreamer fails to build with gcc 10
ddf8814: 8251555: Remove unused focusedWindow field in glass Window to avoid leak
c3fb321: 8252107: Media pipeline initialization can crash if audio or video bin state change fails
eb9886a: Merge
... and 8 more: https://git.openjdk.java.net/jfx/compare/aefed8102b12f7fa30e30d35fa687e289618caf6...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid automatic rebasing, please merge master into your branch, and then specify the current head hash when integrating, like this: /integrate a5ecfb68105d3f7dbde8bc08bdbe52164e42743c.

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, @arun-joseph) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

bhaweshkc · 2020-09-03T18:16:03Z

/integrate

openjdk · 2020-09-03T18:16:23Z

@bhaweshkc
Your change (at version 62dc459) is now ready to be sponsored by a Committer.

kevinrushforth · 2020-09-03T19:10:29Z

/sponsor

openjdk · 2020-09-03T19:11:19Z

@kevinrushforth @bhaweshkc The following commits have been pushed to master since your change was applied:

a5ecfb6: 8252387: Deprecate for removal css Selector and ShapeConverter constructors
8e3616c: 8252062: WebKit build fails with recent VS 2019 compiler
87ad57f: 8251858: Update to Xcode 11.3.1
88c0f97: 8252381: Cherry pick GTK WebKit 2.28.4 changes
c86bd35: 8251941: ListCell: visual artifact when items contain null values
23ad8f4: 8251353: Many javafx scenegraph classes have implicit no-arg constructors
7a4bd9b: 8252060: gstreamer fails to build with gcc 10
ddf8814: 8251555: Remove unused focusedWindow field in glass Window to avoid leak
c3fb321: 8252107: Media pipeline initialization can crash if audio or video bin state change fails
eb9886a: Merge
41ff3fa: 8249537: Update copyright header for files modified in 2020
85821ca: 8251241: macOS: iconify property doesn't change after minimize when resizable is false
b25ffc7: 8251352: Many javafx.base classes have implicit no-arg constructors
2aed5ad: 8240969: WebView does not allow to load style sheet in modularized applications
1c54e61: 8241840: Memoryleak: Closed focused Stages are not collected with Monocle.
f50d218: Merge
208d828: 8228570: Add various documentation clarifications
7a8708b: 8250799: NumberStringConverter and its subclasses are missing documentation for all their constructors

Your commit was automatically rebased without conflicts.

Pushed as commit 3cb3ca8.

bhaweshkc added 2 commits August 11, 2020 16:37

8202990: javafx webview css filter property with display scaling

b8e01de

added Unit Test

e49bae3

bhaweshkc added 2 commits August 11, 2020 21:52

Formatting

04e8f9b

Formatting

91e8bdf

openjdk bot added the rfr Ready for review label Aug 11, 2020

kevinrushforth requested changes Aug 12, 2020

View reviewed changes

kevinrushforth self-requested a review August 26, 2020 18:37

arun-joseph approved these changes Aug 28, 2020

View reviewed changes

Moved test from unit test to system test

a99141d

bhaweshkc requested a review from arun-joseph September 1, 2020 07:10

kevinrushforth approved these changes Sep 1, 2020

View reviewed changes

arun-joseph reviewed Sep 2, 2020

View reviewed changes

tests/system/src/test/java/test/javafx/scene/web/CSSFilterTest.java Outdated Show resolved Hide resolved

arun-joseph reviewed Sep 2, 2020

View reviewed changes

tests/system/src/test/java/test/javafx/scene/web/CSSFilterTest.java Outdated Show resolved Hide resolved

Updated test as per review comment

62dc459

bhaweshkc requested review from kevinrushforth and arun-joseph September 3, 2020 06:13

arun-joseph approved these changes Sep 3, 2020

View reviewed changes

kevinrushforth approved these changes Sep 3, 2020

View reviewed changes

openjdk bot added the ready Ready to be integrated label Sep 3, 2020

openjdk bot added the sponsor Ready to sponsor label Sep 3, 2020

openjdk bot closed this Sep 3, 2020

openjdk bot added integrated Pull request has been integrated and removed sponsor Ready to sponsor ready Ready to be integrated rfr Ready for review labels Sep 3, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8202990: javafx webview css filter property with display scaling #279

8202990: javafx webview css filter property with display scaling #279

bhaweshkc commented Aug 11, 2020 •

edited by openjdk bot

Loading

bridgekeeper bot commented Aug 11, 2020

mlbridge bot commented Aug 11, 2020 •

edited

Loading

kevinrushforth left a comment •

edited

Loading

kevinrushforth commented Aug 12, 2020

openjdk bot commented Aug 12, 2020

arun-joseph commented Aug 26, 2020

arun-joseph left a comment

kevinrushforth commented Aug 29, 2020

kevinrushforth commented Aug 31, 2020

kevinrushforth left a comment

arun-joseph left a comment

openjdk bot commented Sep 3, 2020 •

edited

Loading

bhaweshkc commented Sep 3, 2020

openjdk bot commented Sep 3, 2020

kevinrushforth commented Sep 3, 2020

openjdk bot commented Sep 3, 2020

8202990: javafx webview css filter property with display scaling #279

8202990: javafx webview css filter property with display scaling #279

Conversation

bhaweshkc commented Aug 11, 2020 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Download

bridgekeeper bot commented Aug 11, 2020

mlbridge bot commented Aug 11, 2020 • edited Loading

Webrevs

kevinrushforth left a comment • edited Loading

Choose a reason for hiding this comment

kevinrushforth commented Aug 12, 2020

openjdk bot commented Aug 12, 2020

arun-joseph commented Aug 26, 2020

arun-joseph left a comment

Choose a reason for hiding this comment

kevinrushforth commented Aug 29, 2020

kevinrushforth commented Aug 31, 2020

kevinrushforth left a comment

Choose a reason for hiding this comment

arun-joseph left a comment

Choose a reason for hiding this comment

openjdk bot commented Sep 3, 2020 • edited Loading

bhaweshkc commented Sep 3, 2020

openjdk bot commented Sep 3, 2020

kevinrushforth commented Sep 3, 2020

openjdk bot commented Sep 3, 2020

bhaweshkc commented Aug 11, 2020 •

edited by openjdk bot

Loading

mlbridge bot commented Aug 11, 2020 •

edited

Loading

kevinrushforth left a comment •

edited

Loading

openjdk bot commented Sep 3, 2020 •

edited

Loading