Hi Kevin, attached please find a revised patch. My comments are inlined.
> Am 28.07.2016 um 18:03 schrieb Kevin Rushforth <kevin.rushfo...@oracle.com>: > > Hi > > Alexander Nyssen wrote: >> >> Hi, >> >> I have added my comments below: >> >> >>> Am 28.07.2016 um 17:22 schrieb Kevin Rushforth <kevin.rushfo...@oracle.com >>> <mailto:kevin.rushfo...@oracle.com>>: >>> >>> I got the attachment, since Alexander also CCed me directly. I will attach >>> it shortly. >> >> Thanks! > > Done. > >>> I do have two comments on this: >>> >>> 1) We are past Feature Freeze, so all Enhancements need formal JDK 9 R-team >>> approval [1][2]. In this case, the justification can be internal API that >>> is no longer accessible in JDK 9 due to Jigsaw (I would be very reluctant >>> to consider any other Enhancement request this late in the process), but I >>> will need to look at it and then take it through the approval process, >>> provided that I feel it is in scope. >> >> I was not aware about this, but I would of course appreciate if it could be >> included (due to Jigsaw). Thanks for considering it at least. > > I'll take a closer look tomorrow or Monday (no more time today). At first > glance it seems like something reasonable to take forward. That sounds promising. Thanks! > >>> 2) Some of the changes you list seem unrelated to this enhancement and are >>> better done as separate issues (e.g., the rework of the SWTCursorsTest). >>> Also, I am unconvinced of the need to force GTK 2; in fact it seems at odds >>> with the work we have done with JEP 283 [3]. >> >> Well, the test case refactoring is somehow related, as I introduced the >> common SWT rule while introducing the second SWT test. However, I could >> provide it as a separate contribution if that was wished (and a JIRA issue >> was provided), but the rest of this contribution of course requires it as a >> prerequisite. If this enhancement could not be included in JDK 9, I would >> have to provide it as a separate contribution, as I would have to >> re-introduce FXCanvasTest in other succeeding bugfix contributions >> (JDK-8143596, JDK-8143596). > > I see. I did take a quick look at this and the test changes seem fine as part > of this. I see you created the new test with 'hg cp' (or similar) which > records it as a copy of the SWTCursorsTest.java file, which given the number > of changes is not needed (and not really useful), but that's easy to fix. Done (I copied it within IntelliJ and the IDE seems to have applied hg copy). > > There are several white space changes in FXCanvas.java that should be > reverted. Our policy is that we do not make unrelated changes, including > white space changes, in portions of a file that aren't otherwise modified by > a patch. Done (I used the IntelliJ formatter). > >> The GTK2 flag I introduced just affects SWT. As the swt library that is >> bundled is rather old (3.7.2) that seemed to be safer (we have observed >> quite a few problems when running SWT on GTK3). We can of course remove it >> if tests are not affected by it. > > We don't actually bundle swt itself, although we do download an old copy to > link against, and to run tests against. In any case, given that our minimum > Linux platform for JDK 9 is Ubuntu 16.04, it might not have GTK2 installed by > default. Please revert this change to build.gradle. If test issues arise on > Linux we will deal with it at that time (possibly by moving to a newer > version of swt to run tests). I removed the SWT option. However, the previous logger message is no longer valid and should be removed, so the patch still contains a change to build.gradle. > > Thanks. > > — Kevin Regards, Alexander > > >> >>> >>> — Kevin >> >> Regards, >> Alexander >> >>> >>> [1] http://mail.openjdk.java.net/pipermail/jdk9-dev/2016-June/004485.html >>> <http://mail.openjdk.java.net/pipermail/jdk9-dev/2016-June/004485.html> >>> [2] http://openjdk.java.net/projects/jdk9/fc-extension-process >>> <http://openjdk.java.net/projects/jdk9/fc-extension-process> >>> [3] https://bugs.openjdk.java.net/browse/JDK-8145568 >>> <https://bugs.openjdk.java.net/browse/JDK-8145568> >>> >>> >>> Phil Race wrote: >>>> The mailing list rejects attachments so we got nothing. >>>> >>>> -phil. >>>> >>>> On 7/28/2016 8:06 AM, Alexander Nyssen wrote: >>>>> Hi Kevin, all, >>>>> >>>>> attached please find a patch that fixes JDK-8160325. The patch comprises >>>>> the following changes: >>>>> >>>>> - Provided static FXCanvas#getFXCanvas(Scene) method to obtain the >>>>> FXCanvas instance embedding the given Scene instance. >>>>> - Added EmbeddedWindow.getHost() so the HostInterface can be retrieved. >>>>> - Added FXCanvasTest with a test method to test correct behavior of >>>>> FXCanvas#getFXCanvas(Scene). >>>>> - Introduced SwtTest JUnit MethodRule to have more concise tests and >>>>> ensure it is also used by SWTCursorsTest. >>>>> - Ensured SWT tests are executed using GTK2 on Linux. >>>>> >>>>> I reworked the existing SWTCursorsTest while introducing FXCanvasTest to >>>>> be more concise. >>>>> >>>>> Regards, >>>>> Alexander >>>>> >>>> >>
