Re: [Rev 02] RFR: 8218973: SVG with masking is not rendering image with mask effect

[Bhawesh Choudhary]

On Fri, 8 May 2020 23:37:51 GMT, Kevin Rushforth  wrote:

>> Bhawesh Choudhary has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   Moved Printing drawing path to non MaskTextureGraphics interface
>
> modules/javafx.web/src/main/java/com/sun/javafx/webkit/prism/WCGraphicsPrismContext.java
>  line 566:
> 
>> 565: filterContext = 
>> PrFilterContext.getInstance(screen);
>> 566: }
>> 567: PrDrawable imagePrDrawable = 
>> PrDrawable.create(filterContext, paintRtTexture);
> 
> Did you test both the SW pipeline and printing paths?

Yes, used -Dprism.order=sw to run HelloWebView test to verify SW Pipeline
Yes, i have added a modified HelloWebView test in attached bug which is used to 
test printing paths.
https://bugs.openjdk.java.net/secure/attachment/88102/HelloWebView.java

-

PR: https://git.openjdk.java.net/jfx/pull/213

Re: [Rev 02] RFR: 8218973: SVG with masking is not rendering image with mask effect

[Bhawesh Choudhary]

On Fri, 8 May 2020 23:25:18 GMT, Kevin Rushforth  wrote:

>> Bhawesh Choudhary has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   Moved Printing drawing path to non MaskTextureGraphics interface
>
> modules/javafx.web/src/main/java/com/sun/javafx/webkit/prism/WCGraphicsPrismContext.java
>  line 549:
> 
>> 548: Texture.WrapMode.CLAMP_NOT_NEEDED);
>> 549: RTTexture maskRtTexture = 
>> g.getResourceFactory().createRTTexture(nativeMaskImage.getWidth(),
>> 550: nativeMaskImage.getHeight(), 
>> Texture.WrapMode.CLAMP_NOT_NEEDED);
> 
> Why do you need to create a second RTT here? I would have thought you could 
> use the mask texture directly (you may need
> to scale the image in order to do that).

main problem with not using second RTTexture is the interface 
MaskTextureGraphics, which accept RTTexture only. also it
difficult to add new API taking Texture instead of RTTexture given that SW 
Pipeline and J2D pipeline has direct
dependency on RTTexture interface.

-

PR: https://git.openjdk.java.net/jfx/pull/213

Re: RFR: 8176270: Adding ChangeListener to TextField.selectedTextProperty causes StringOutOfBoundsException

[Robert Lichtenberger]

On Fri, 8 May 2020 18:50:57 GMT, Kevin Rushforth  wrote:

>> I wasn't aware of PR #73, I only saw the (very old) issue in the JDK Bug 
>> System and started to look into the issue.
>> The fact that two different people start to look into the same issue shows 
>> it is important however :-).
>> Should I close this PR and participate in PR #73?
>
> @effad I just closed PR #73 due to inactivity. If you are interested 
> interested in pursuing this, go ahead and reopen
> this (although you would need to address the feedback that clamping is 
> insufficient).

We're in the process of finishing our products' release. If I find time
I will try and give this (rather nasty) Problem another shot.

On 2020-05-08 20:51, Kevin Rushforth wrote:
>
> @effad  I just closed PR #73
>  due to inactivity. If you are
> interested interested in pursuing this, go ahead and reopen this
> (although you would need to address the feedback that clamping is
> insufficient).
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> , or
> unsubscribe
> .
>

-

PR: https://git.openjdk.java.net/jfx/pull/138

Re: RFR: 8242508: Upgrade to Visual Studio 2019 version 16.5.3

[Ambarish Rapte]

On Wed, 6 May 2020 20:37:10 GMT, Kevin Rushforth  wrote:

> This is a toolchain upgrade on Windows from the current Visual Studio 2017 
> (version 15.9.16) to Visual Studio 2019
> (version 16.5.3). This will match a recent upgrade done for JDK 15 -- see
> [JDK-8244214](https://bugs.openjdk.java.net/browse/JDK-8244214).  I have run 
> a full build and test using this new
> compiler.
> NOTE: although this isn't strictly dependent on 
> [JDK-8244487](https://bugs.openjdk.java.net/browse/JDK-8244487), which
> is out for review as PR #211 , I plan to wait until that one is approved. I 
> will then integrate PR #211 followed by
> this PR.

lgtm, verified a local build on Windows10.

-

Marked as reviewed by arapte (Reviewer).

PR: https://git.openjdk.java.net/jfx/pull/212

Re: RFR: 8242523: Update the animation and clip envelope classes

[Ambarish Rapte]

On Fri, 24 Apr 2020 00:58:30 GMT, Nir Lisker  wrote:

> Mostly refactoring in preparation of the upcoming fixes. The changes might 
> look like a lot, but it's mostly rearranging
> of methods. Summery of changes:
> ### Animation
> * Added `isNearZero` and `areNearEqual` methods that deal with `EPSILON` 
> checks.
> * Added `isStopped`, `isRunning` and `isPaused` convenience methods.
> * Added `runHandler` method to deal with running the handler.
> * Moved methods to be grouped closer to where they are used rather than by 
> visibility.
> * Removed the static import for `TickCalculation`.
> * Various small subjective readability changes.
> * Behavioral changes: switching `autoReverse` and `onFinished` properties 
> from "Simple" to "Base" properties; and lazily
>   initializing the `cuePoints` map.
> 
> ### Clip Envelopes
> * Added `MultiLoopClipEnvelope` as an intermediate parent for infinite and 
> finite clip envelopes.
> * Rearranged methods order to be consistent.
> * Replaced the `checkBounds` method with a new overload of `Utils.clamp`.
> * Renamed `pos` to `cyclePos`.
> * Extracted common methods: `changedDirection` and `ticksRateChange`
> * Added internal documentation.
> 
> Also corrected a typo in `TicksCalculation` and added an explanation for an 
> animation test.

Overall looks good to me, Added few minor comments.

modules/javafx.graphics/src/main/java/com/sun/scenario/animation/shared/MultiLoopClipEnvelope.java
 line 48:

> 47:
> 48: protected boolean autoReverse() {
> 49: return autoReverse;

I would suggest the name to be `isAutoReverese`

modules/javafx.graphics/src/main/java/com/sun/scenario/animation/shared/ClipEnvelope.java
 line 46:

> 45:  */
> 46: public abstract class ClipEnvelope {
> 47:

I think the removal of line 46 was unintended change.

modules/javafx.graphics/src/main/java/javafx/animation/Animation.java line 142:

> 141: }
> 142:
> 143: /*

Both these methods seem like they belong to Utils.java. If moved to Utils.java 
then the all the calls
`(Math.abs(currentRate - rate) < EPSILON)` can be changed to use these methods 
from Utils.java. If we move these
methods then, Utils.java also needs to declare  `static final double EPSILON = 
1e-12;`  and the EPSILON here can be
initialized as `private static final double EPSILON = Utils.EPSILON;`

modules/javafx.graphics/src/main/java/com/sun/scenario/animation/shared/MultiLoopClipEnvelope.java
 line 61:

> 60:
> 61: protected boolean changedDirection(double newRate) {
> 62: return newRate * rate < 0;

I would recommend to name as `isDirectionChanged`

modules/javafx.graphics/src/main/java/com/sun/scenario/animation/shared/SingleLoopClipEnvelope.java
 line 102:

> 101: long ticksChange = Math.round(currentTick * currentRate);
> 102: ticks = Utils.clamp(0, deltaTicks + ticksChange, cycleTicks);
> 103: AnimationAccessor.getDefault().playTo(animation, ticks, 
> cycleTicks);

This could remain unchanged. The `ticksChange` value is not really used 
elsewhere here.

modules/javafx.graphics/src/main/java/com/sun/scenario/animation/shared/MultiLoopClipEnvelope.java
 line 59:

> 58: return Math.round((ticks - deltaTicks) * Math.abs(newRate / 
> rate));
> 59:  }
> 60:

This is similar to `ClipEnvelope.ticksRateChange()` with a minor difference. 
Can this be removed from here and
`ClipEnvelope.ticksRateChange()` be reused ?

-

Changes requested by arapte (Reviewer).

PR: https://git.openjdk.java.net/jfx/pull/196

Re: [Rev 01] RFR: 8237602: TabPane doesn't respect order of TabPane.getTabs() list

[Ambarish Rapte]

On Fri, 1 May 2020 14:42:34 GMT, Kevin Rushforth  wrote:

>> Ambarish Rapte has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   review-update: change test name
>
> tests/system/src/test/java/test/robot/javafx/scene/TabPanePermuteGetTabsTest.java
>  line 194:
> 
>> 193: @Test
>> 194: public void testPermutGetTabsWithMoreTabs1() {
>> 195: // Step #1
> 
> Typo: Permut --> Permute
> 
> (or, as long as you are changing it anyway, you could rename it now to 
> something other than "permute" since it isn't).

Changed the test name to `testAddingNewTabsWithExistingTabsAtSameIndex`. While 
converting to unit tests, we will have
to change the names of other test from this file.

-

PR: https://git.openjdk.java.net/jfx/pull/201

Re: [Rev 01] RFR: 8237602: TabPane doesn't respect order of TabPane.getTabs() list

[Ambarish Rapte]

> Issue:
> When tabs are permuted as mentioned in the issue description as,
> 1. TabPane.getTabs().setAll(tab0, tab1)
> 2. TabPane.getTabs().setAll(tab0, tab1, tab2, tab3);
> the tab headers do not get permuted in same order as `TabPane.getTabs()`.
> 
> => tab headers should be shown in order as  tab0, tab1, tab2, tab3.
> => but are show in order as  tab2, tab3, tab0, tab1
> 
> Cause:
> Newly added tabs(tab2, tab3) are not inserted at correct index. The index 
> `Change.getFrom()` (0) used from Change does
> not remain valid after the tabs to be moved(tab0, tab1) are removed from 
> `tabsToAdd` list.
> Fix:
> Use the index of first newly added tab, from `TabPane.getTabs()`, which would 
> be always reliable.
> 
> Verification:
> No existing tests fail due to this change.
> Added a system test which fails without and pass with fix.

Ambarish Rapte has updated the pull request incrementally with one additional 
commit since the last revision:

  review-update: change test name

-

Changes:
  - all: https://git.openjdk.java.net/jfx/pull/201/files
  - new: https://git.openjdk.java.net/jfx/pull/201/files/68e8efd2..548badaa

Webrevs:
 - full: https://webrevs.openjdk.java.net/jfx/201/webrev.01
 - incr: https://webrevs.openjdk.java.net/jfx/201/webrev.00-01

  Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod
  Patch: https://git.openjdk.java.net/jfx/pull/201.diff
  Fetch: git fetch https://git.openjdk.java.net/jfx pull/201/head:pull/201

PR: https://git.openjdk.java.net/jfx/pull/201

Re: Unable to allocate direct buffer memory

[Ty Young]

On 5/10/20 3:52 PM, John Hendrikx wrote:

They're related.  32767x1137x4 = 149024316.

It would help to know what your app might be doing.  Although it could 
be a bug in JavaFX, it seems more likely that a canvas/image or cached 
group or something is a bit bigger than reasonable.


I think I've seen such errors in my app before I restricted maximum 
sizes of images which were supplied from uncontrolled sources.


--John




No images are used. The only questionable thing is that 
TilePane/GridPane is used in order to achieve a more responsive UI. I 
don't remember this being an issue in earlier versions either.

Re: Unable to allocate direct buffer memory

[John Hendrikx]

They're related.  32767x1137x4 = 149024316.

It would help to know what your app might be doing.  Although it could 
be a bug in JavaFX, it seems more likely that a canvas/image or cached 
group or something is a bit bigger than reasonable.


I think I've seen such errors in my app before I restricted maximum 
sizes of images which were supplied from uncontrolled sources.


--John

On 07/05/2020 20:51, Ty Young wrote:


On 5/6/20 12:39 PM, Ty Young wrote:

After hours of running my JavaFX application I start getting this
error spammed in console:


java.lang.OutOfMemoryError: Cannot reserve 149024316 bytes of direct
buffer memory (allocated: 149066725, limit: 209715200)
at java.base/java.nio.Bits.reserveMemory(Bits.java:178)
at
java.base/java.nio.DirectByteBuffer.(DirectByteBuffer.java:120)
at java.base/java.nio.ByteBuffer.allocateDirect(ByteBuffer.java:329)
at
javafx.graphics/com.sun.prism.impl.BufferUtil.newByteBuffer(BufferUtil.java:90)

at
javafx.graphics/com.sun.prism.impl.BufferUtil.newIntBuffer(BufferUtil.java:121)

at
javafx.graphics/com.sun.prism.impl.QueuedPixelSource.getUnusedPixels(QueuedPixelSource.java:155)

at
javafx.graphics/com.sun.javafx.tk.quantum.UploadingPainter.run(UploadingPainter.java:156)

at
java.base/java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:515)

at
java.base/java.util.concurrent.FutureTask.runAndReset(FutureTask.java:305)

at javafx.graphics/com.sun.javafx.tk.RenderJob.run(RenderJob.java:58)
at
java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1130)

at
java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:630)

at
javafx.graphics/com.sun.javafx.tk.quantum.QuantumRenderer$PipelineRunnable.run(QuantumRenderer.java:125)

at java.base/java.lang.Thread.run(Thread.java:832)


Ironically every single error is about allocating the exact same
amount of bytes.



Anything on this? I'm also getting:


java.lang.RuntimeException: Requested texture dimensions (32767x1137)
require dimensions (0x1137) that exceed maximum texture size (16384)
at
javafx.graphics/com.sun.prism.es2.ES2RTTexture.create(ES2RTTexture.java:220)

at
javafx.graphics/com.sun.prism.es2.ES2ResourceFactory.createRTTexture(ES2ResourceFactory.java:165)

at
javafx.graphics/com.sun.javafx.tk.quantum.UploadingPainter.run(UploadingPainter.java:124)

at
java.base/java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:515)

at
java.base/java.util.concurrent.FutureTask.runAndReset(FutureTask.java:305)
at javafx.graphics/com.sun.javafx.tk.RenderJob.run(RenderJob.java:58)
at
java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1130)

at
java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:630)

at
javafx.graphics/com.sun.javafx.tk.quantum.QuantumRenderer$PipelineRunnable.run(QuantumRenderer.java:125)

at java.base/java.lang.Thread.run(Thread.java:832)


spammed alternatively.

Re: [Rev 02] RFR: 8218973: SVG with masking is not rendering image with mask effect

[Bhawesh Choudhary]

> Root cause of issue is Specifying a image mask from GraphicsContextJava.cpp 
> in WebKit was not implemented, so masking
> doesn't take place at all while rendering SVGRect. to fix this issue add 
> implementation of function clipToImageBuffer()
> in GraphicsContextJava.cpp and send clip image to WCGraphicsPrismContext.java 
>  While rendering in
> WCGraphicsPrismContext.java if image clip mask is available, use it for 
> rendering using MaskTextureGraphics interface
> otherwise use usual way of rendering.

Bhawesh Choudhary has updated the pull request incrementally with one 
additional commit since the last revision:

  Moved Printing drawing path to non MaskTextureGraphics interface

-

Changes:
  - all: https://git.openjdk.java.net/jfx/pull/213/files
  - new: https://git.openjdk.java.net/jfx/pull/213/files/e66fa3bc..5b85b47d

Webrevs:
 - full: https://webrevs.openjdk.java.net/jfx/213/webrev.02
 - incr: https://webrevs.openjdk.java.net/jfx/213/webrev.01-02

  Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod
  Patch: https://git.openjdk.java.net/jfx/pull/213.diff
  Fetch: git fetch https://git.openjdk.java.net/jfx pull/213/head:pull/213

PR: https://git.openjdk.java.net/jfx/pull/213

Re: [Rev 01] RFR: 8218973: SVG with masking is not rendering image with mask effect

[Bhawesh Choudhary]

> Root cause of issue is Specifying a image mask from GraphicsContextJava.cpp 
> in WebKit was not implemented, so masking
> doesn't take place at all while rendering SVGRect. to fix this issue add 
> implementation of function clipToImageBuffer()
> in GraphicsContextJava.cpp and send clip image to WCGraphicsPrismContext.java 
>  While rendering in
> WCGraphicsPrismContext.java if image clip mask is available, use it for 
> rendering using MaskTextureGraphics interface
> otherwise use usual way of rendering.

Bhawesh Choudhary has updated the pull request incrementally with one 
additional commit since the last revision:

  added dispose of resources

-

Changes:
  - all: https://git.openjdk.java.net/jfx/pull/213/files
  - new: https://git.openjdk.java.net/jfx/pull/213/files/c2729a9c..e66fa3bc

Webrevs:
 - full: https://webrevs.openjdk.java.net/jfx/213/webrev.01
 - incr: https://webrevs.openjdk.java.net/jfx/213/webrev.00-01

  Stats: 3 lines in 1 file changed: 3 ins; 0 del; 0 mod
  Patch: https://git.openjdk.java.net/jfx/pull/213.diff
  Fetch: git fetch https://git.openjdk.java.net/jfx pull/213/head:pull/213

PR: https://git.openjdk.java.net/jfx/pull/213

Re: Unable to allocate direct buffer memory

[Ty Young]

On 5/7/20 3:44 PM, Kevin Rushforth wrote:
This suggests that there might be a memory leak (possibly in uploading 
painter which I see you are using). Is this easily reproducible?





I tried doing software rendering just to see if it made any difference.


I don't know if it fixed any of the other issues, but it definitely made 
things worse. Everytime I resize the application it leaks on-heap memory 
and eventually hits an OutOfMemory exception.



It looks like a bunch of int arrays are being allocated and never 
released. What the hell?

Re: Proposed IntegerSpinner buggy behavior correction - JDK-8242553

[Nir Lisker]

I would say that for doubles, the minimum step size is the one given by ulp
= Math.ulp(double). Then the double case should behave like the
integer case where the ulp is 1 I think.
So, for the angle case of [0, 360), if we are at 360 - ulp, incrementing by
N * ulp will get us to (N-1) * ulp.

On Wed, Apr 22, 2020 at 3:41 PM Kevin Rushforth 
wrote:

> Hi Ajit,
>
> Thanks for writing this up. I think we are clear on what should happen
> for IntegerSpinnerValueFactory and ListSpinnerValueFactory. To answer
> your two questions, I'll take the second one first:
>
> > 2. Do we really care about negative values in no of steps OR
> > amountToStepBy?
> > I am asking this as we already have separate increment() and
> > decrement() methods. Should we bar such negative values? That’s
> > another behavior change though :)
>
> Once you switch to a proper modulo function, the easy answer is that
> negative values will "just work". A negative amountToStepBy might even
> make sense in some use cases (think of an increasing debt value), so
> there is no reason not to support it. A negative number of steps doesn't
> really make sense, since if you want to call increment(-N) you could
> equivalently call decrement(N), but again, it will "just work" as
> expected once the wrapping function is fixed. If we were designing from
> scratch I'd specify that nsteps must be >= 0, but it isn't worth it to
> make the change now (which would require a CSR) to prohibit it.
>
> > 1. Need to decide what would be better way to fix
> > DoubleSpinnerValueFactory wrapAround behavior?
>
> It might be helpful to compare it to the Integer flavor. By definition,
> IntegerSpinnerValueFactory represents a set of discrete values: min and
> max are two distinct values, and there is a well-defined ordering when
> wrapping. If you wrap, it should be a continuous (in both positive and
> negative directions) list of values like so:
>
> min, min+1, min+2, ... max-2, max-1, max, min, min+1, min+2, ...
>
> The formula is simple. After incrementing or decrementing by N *
> amountToStepBy, do the following if wrapAround is true:
>
>  if (val < min || val > max) val = (val - min) % (max - min + 1) + min;
>
> The above works regardless of step size because integers are naturally
> discrete values. It is well-behaved and natural to consider max and min
> being 1 apart from each other when wrapping.
>
> By contrast, it's harder to know what the right answer is for
> DoubleSpinnerValueFactory. It's quite possible that in some cases, the
> app expects the values of DoubleValue factory to also be discrete when
> it comes to wrapping, meaning that min and max are distinct values such
> that if your step was 1.0, you might expect to go from max-1.0 to max to
> min to min+1.0 (although even that definition has problems for
> non-integer values of amountToStepBy, but it's (mostly) solvable). It's
> also quite likely for other cases -- especially ones that would lend
> themselves to wrapAround mode in graphical apps -- where it isn't a set
> of discrete values, but rather is such that min and max represent the
> same value (i.e., the same end result). The simplest example is an angle
> in degrees where 0.0 and 360.0 mean the same thing when used as a
> rotation angle. In this latter use case, stepping from 359.0 to 360.0 to
> 0.0 to 1.0 would cause a discontinuous pause in motion if you mapped the
> output to a rotation angle.
>
> I'm not sure what the right answer is, but the latter seems like a valid
> use case and I would guess at least as common as a "set of discrete
> values" for doubles. Another thing to consider is that even in what I
> will call "discrete" mode (no I'm not really proposing that we add a
> property to control it), you have to take the step size into account;
> otherwise when the step size is < 0.5 you will get stuck at max when
> incrementing by 1 amountToStepBy, unless you have special case logic for
> that.
>
> Any thoughts from other developers?
>
> -- Kevin
>

Re: Mac: Supported MacOS JDKs

[Nir Lisker]

if this is confirmed I can update the page.

On Tue, Apr 14, 2020 at 1:18 PM Florian Kirmaier 
wrote:

> Hi everyone,
>
> it seems to me, that the newest JDK for Mac (MacOSX10.15.sdk) doesn't work
> to build JavaFX.
> It works for me with 10.14 but not with 10.15.
> Can anyone confirm this?
> It would be great to mention this in the build instructions here:
> https://wiki.openjdk.java.net/display/OpenJFX/Building+OpenJFX
>
> Greetings Florian Kirmaier
>

RFR: 8244212: Optionally download media and webkit libraries from latest openjfx EA build

[Jesper Skov]

I have tested on Linux (Fedora 31) only.
It works as intended (with one test failure due to 15-ea+4 being too old now).

UPDATE_STUB_CACHE suggests there is some magic available to help manage the 
stub cache.
But as I could not find anything about it, that section in the documentation 
may need some love.

-

Commit messages:
 - Add build tooling note
 - Downloaded from MavenCentral note in build file
 - Review fixes
 - Allow use of official WebKit/Media libraries for stubs

Changes: https://git.openjdk.java.net/jfx/pull/202/files
 Webrev: https://webrevs.openjdk.java.net/jfx/202/webrev.00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8244212
  Stats: 106 lines in 2 files changed: 97 ins; 3 del; 6 mod
  Patch: https://git.openjdk.java.net/jfx/pull/202.diff
  Fetch: git fetch https://git.openjdk.java.net/jfx pull/202/head:pull/202

PR: https://git.openjdk.java.net/jfx/pull/202

Re: RFR: 8244212: Optionally download media and webkit libraries from latest openjfx EA build

[Johan Vos]

On Thu, 30 Apr 2020 18:56:40 GMT, Jesper Skov 
 wrote:

> I have tested on Linux (Fedora 31) only.
> It works as intended (with one test failure due to 15-ea+4 being too old now).
> 
> UPDATE_STUB_CACHE suggests there is some magic available to help manage the 
> stub cache.
> But as I could not find anything about it, that section in the documentation 
> may need some love.

WEBKIT-MEDIA-STUBS.md line 13:

> 12:
> 13: Note that these take some time to build.
> 14:

... and they have additional requirements for build tools (e.g. cmake)

-

PR: https://git.openjdk.java.net/jfx/pull/202

Re: RFR: 8244212: Optionally download media and webkit libraries from latest openjfx EA build

[Kevin Rushforth]

On Thu, 30 Apr 2020 18:56:40 GMT, Jesper Skov 
 wrote:

> I have tested on Linux (Fedora 31) only.
> It works as intended (with one test failure due to 15-ea+4 being too old now).
> 
> UPDATE_STUB_CACHE suggests there is some magic available to help manage the 
> stub cache.
> But as I could not find anything about it, that section in the documentation 
> may need some love.

Here are a few preliminary comments:

I filed the following JBS bug to track this enhancement:

https://bugs.openjdk.java.net/browse/JDK-8244212

When you are ready for a formal review, you can change the title to:

8244212: Optionally download media and webkit libraries from latest openjfx EA 
build

WEBKIT-MEDIA-STUBS.md line 22:

> 21:
> 22: * Unix libraries (*.so or *.dynlib files)
> 23: 

typo: should be "dylib"

WEBKIT-MEDIA-STUBS.md line 24:

> 23: 
> 24: $projectDir/../caches/sdk/lib
> 25: $projectDir/../caches/modular-sdk/modules_libs/$module

I would just list this one and not the other two. In particular, I don't want 
to encourage developers to copy these
libraries into the JDK since it could cause unexpected results if those 
libraries were loaded at runtime.

WEBKIT-MEDIA-STUBS.md line 36:

> 35:
> 36: ## Officially released libraries
> 37:

I recommend that you add a comment that gradle will download these for you from 
Maven Central.

By the same token, you could indicate in the above section that copying them to 
`../caches/sdk` is a manual step.

WEBKIT-MEDIA-STUBS.md line 53:

> 52:
> 53: Note that this is fine for local work. But a full test *is* required 
> before submitting a PR, see CONTRIBUTING.md.

Can you add a hyperlink to CONTRIBUTING.md?

build.gradle line 1252:

> 1251:
> 1252: // Allows automatic provisioning of webkit+media shared libraries from 
> an official OpenJfx build
> 1253: defineProperty("STUB_RUNTIME_OPENJFX", "")

If you are going to capitalize it, then it should be "OpenJFX".

Can you add "from Maven Central" ?

build.gradle line 1259:

> 1258: def String jdkStubRuntime = cygpath("$JDK_HOME")
> 1259: def String openjfxStubRuntime = cygpath("$projectDir") + 
> "/build/openjfxStub"
> 1260:

I might suggest `buildSrc/build/openjfxStub` so it doesn't get removed by 
`gradle clean`.

build.gradle line 3391:

> 3390: doFirst {
> 3391: if (IS_STUB_RUNTIME_OPENJFX) {
> 3392: println 
> ""

Perhaps this should be inside the `if (!IS_COMPILE_WEBKIT)` ?

build.gradle line 4338:

> 4337:  *  
>   *
> 4338:  * OpenJfx Stubs
>   *
> 4339:  *  
>   *

OpenJFX

-

PR: https://git.openjdk.java.net/jfx/pull/202

Re: RFR: 8244212: Optionally download media and webkit libraries from latest openjfx EA build

[Jesper Skov]

On Thu, 30 Apr 2020 20:10:19 GMT, Kevin Rushforth  wrote:

>> I have tested on Linux (Fedora 31) only.
>> It works as intended (with one test failure due to 15-ea+4 being too old 
>> now).
>> 
>> UPDATE_STUB_CACHE suggests there is some magic available to help manage the 
>> stub cache.
>> But as I could not find anything about it, that section in the documentation 
>> may need some love.
>
> build.gradle line 1259:
> 
>> 1258: def String jdkStubRuntime = cygpath("$JDK_HOME")
>> 1259: def String openjfxStubRuntime = cygpath("$projectDir") + 
>> "/build/openjfxStub"
>> 1260:
> 
> I might suggest `buildSrc/build/openjfxStub` so it doesn't get removed by 
> `gradle clean`.

Neat. I did not know that was a thing. Done.

> build.gradle line 3391:
> 
>> 3390: doFirst {
>> 3391: if (IS_STUB_RUNTIME_OPENJFX) {
>> 3392: println 
>> ""
> 
> Perhaps this should be inside the `if (!IS_COMPILE_WEBKIT)` ?

I assume you mean like this:
`
if (!IS_COMPILE_WEBKIT) {
if (IS_STUB_RUNTIME_OPENJFX) {
   println ...
} else {
   println ...
}
}
`
I do not like the additional indentation level, so I have left it as is.

The semantics should be the same, as long as no one use stubs *and* compile 
webkit.
Which should maybe rather cause a build exception?

But it is not something I feel strongly for, so I am happy to change it :)

-

PR: https://git.openjdk.java.net/jfx/pull/202

Re: RFR: 8244212: Optionally download media and webkit libraries from latest openjfx EA build

[Kevin Rushforth]

On Thu, 30 Apr 2020 19:51:26 GMT, Kevin Rushforth  wrote:

>> I have tested on Linux (Fedora 31) only.
>> It works as intended (with one test failure due to 15-ea+4 being too old 
>> now).
>> 
>> UPDATE_STUB_CACHE suggests there is some magic available to help manage the 
>> stub cache.
>> But as I could not find anything about it, that section in the documentation 
>> may need some love.
>
> I filed the following JBS bug to track this enhancement:
> 
> https://bugs.openjdk.java.net/browse/JDK-8244212
> 
> When you are ready for a formal review, you can change the title to:
> 
> 8244212: Optionally download media and webkit libraries from latest openjfx 
> EA build

I'll take a closer look look once it's out for formal review, but will leave a 
few preliminary comments now. To answer
one question:

> UPDATE_STUB_CACHE suggests there is some magic available to help manage the 
> stub cache.

Yeah, this bit of magic was in our closed repo. I'll see if there is some 
refactoring that might make sense (probably
not, since that bit is done by triggering an `ant` task).

-

PR: https://git.openjdk.java.net/jfx/pull/202