Re: [OpenJDK 2D-Dev] [12] JDK-8212202: NPE in the print tests after JDK-8153732
Hi Prasanta, The parameters for the function does not tells the order and is
not implied by the way function definition is and more over this function is
called in an interval of minimum refresh time(4 mins). Rather than adding
overhead of confusion since it is not implied by the function I think it is
safe to keep it that way. Depending on reviewer's comments is also not the
right way to depend on. The current definition avoids that but with a small
overhead and I think is not too much of a burden to bear.
Thanks and regards,
Shashi
From: Prasanta Sadhukhan
Sent: Friday, February 15, 2019 12:28 PM
To: Shashidhara Veerabhadraiah ; Philip
Race
Cc: 2d-dev@openjdk.java.net
Subject: Re: [OpenJDK 2D-Dev] [12] JDK-8212202: NPE in the print tests after
JDK-8153732
Hi Shashi,
In this case, we know doCompare() is called from one location only [and also is
not a public method to be called by user] and the check I mentioned is
redundant. It's in while(true) loop so any optimzation we can do to avoid
unnecessary checks will be good in my opinion.
If someone changed the doCompare() call without seeing the implication or
giving thought, then he has to face the repurcussions, but I guess reviewers
would be able to catch that beforehand.
Regards
Prasanta
On 15-Feb-19 12:17 PM, Shashidhara Veerabhadraiah wrote:
Hi Prasanta, doCompare(str1, str2) is a different function and one should not
define a function based on how it is going to be called!! What if someone
changed the caller to this: doCompare(currentRemotePrinters,
prevRemotePrinters). There is no restriction if one calls like this. Here the
function is taking 2 strings and it does not say which one should be passed at
what position. Probably if the function takes different parameters then it sets
an automatic rule on which parameter needs to be passed at which position but
otherwise function definition should take care this.
Hope you agree with me.
Thanks and regards,
Shashi
From: Prasanta Sadhukhan
Sent: Friday, February 15, 2019 12:11 PM
To: Phil Race HYPERLINK
"mailto:philip.r...@oracle.com;; Shashidhara
Veerabhadraiah HYPERLINK
"mailto:shashidhara.veerabhadra...@oracle.com;
Cc: HYPERLINK "mailto:2d-dev@openjdk.java.net"2d-dev@openjdk.java.net
Subject: Re: [OpenJDK 2D-Dev] [12] JDK-8212202: NPE in the print tests after
JDK-8153732
Hi Shashi,
I have one comment:
doCompare(prevRemotePrinters, currentRemotePrinters) is only called from run()
method when "prevRemotePrinters" is already checked to be not null [if
(prevRemotePrinters != null && prevRemotePrinters.length > 0) ]
so I see no point in checking "str1" [which is prevRemotePrinters] to be null
in doCompare(). I guess instead of
413 if (str1 == null && str2 == null) {
414 return false;
415 } else if (str1 == null || str2 == null) {
416 return true;
417 }
you can have
if (str2 == null)
return true;
Regards
Prasanta
On 15-Feb-19 2:48 AM, Phil Race wrote:
+1 .. remember to use the current bug synopsis in the push comment
ie : "[Windows] Exception if no printers are installed."
-phil.
On 2/11/19 12:39 AM, Shashidhara Veerabhadraiah wrote:
Hi Phil, Here is the new Webrev fixing those comments:
HYPERLINK
"http://cr.openjdk.java.net/%7Esveerabhadra/8212202/webrev.06/"http://cr.openjdk.java.net/~sveerabhadra/8212202/webrev.06/
Thanks and regards,
Shashi
From: Philip Race
Sent: Saturday, February 9, 2019 2:25 AM
To: Shashidhara Veerabhadraiah HYPERLINK
"mailto:shashidhara.veerabhadra...@oracle.com;
Cc: Prasanta Sadhukhan HYPERLINK
"mailto:prasanta.sadhuk...@oracle.com;;
HYPERLINK "mailto:2d-dev@openjdk.java.net"2d-dev@openjdk.java.net
Subject: Re: [OpenJDK 2D-Dev] [12] JDK-8212202: NPE in the print tests after
JDK-8153732
413 if (str1 == null && str2 == null) {
414 return false;
415 } else if ((str1 == null && str2 != null) || (str2 == null &&
str1 != null)) {
416 return true;
417 }
When we get to 415 we already know at least one of str1 or str2 is non-null so
415 can just be
} else if (str1 == null || str2 == null) {
-phil.
On 2/6/19, 12:31 AM, Shashidhara Veerabhadraiah wrote:
Hi Phil, Here is the updated Webrev fixing those comments:
HYPERLINK
"http://cr.openjdk.java.net/%7Esveerabhadra/8212202/webrev.05/"http://cr.openjdk.java.net/~sveerabhadra/8212202/webrev.05/
Thanks and regards,
Shashi
From: Phil Race
Sent: Thursday, January 31, 2019 4:36 AM
To: Shashidhara Veerabhadraiah HYPERLINK
"mailto:shashidhara.veerabhadra...@oracle.com;
Cc: Prasanta Sadhukhan HYPERLINK
"mailto:prasanta.sadhuk...@oracle.com;;
HYPERLINK "mailto:2d-dev@openjdk.java.net"2d-dev@openjdk.java.net
Subject: Re: [OpenJDK 2D-Dev] [12] JDK-8212202: NPE in the print tests after
JDK-8153732
Sorry .. this got lost ..
I don't understand this line :
253 jobjectArray
Re: [OpenJDK 2D-Dev] [12] JDK-8212202: NPE in the print tests after JDK-8153732
Hi Shashi,
In this case, we know doCompare() is called from one location only [and
also is not a public method to be called by user] and the check I
mentioned is redundant. It's in while(true) loop so any optimzation we
can do to avoid unnecessary checks will be good in my opinion.
If someone changed the doCompare() call without seeing the implication
or giving thought, then he has to face the repurcussions, but I guess
reviewers would be able to catch that beforehand.
Regards
Prasanta
On 15-Feb-19 12:17 PM, Shashidhara Veerabhadraiah wrote:
Hi Prasanta, doCompare(str1, str2) is a different function and one
should not define a function based on how it is going to be called!!
What if someone changed the caller to this:
doCompare(currentRemotePrinters, prevRemotePrinters). There is no
restriction if one calls like this. Here the function is taking 2
strings and it does not say which one should be passed at what
position. Probably if the function takes different parameters then it
sets an automatic rule on which parameter needs to be passed at which
position but otherwise function definition should take care this.
Hope you agree with me.
Thanks and regards,
Shashi
*From:*Prasanta Sadhukhan
*Sent:* Friday, February 15, 2019 12:11 PM
*To:* Phil Race ; Shashidhara Veerabhadraiah
*Cc:* 2d-dev@openjdk.java.net
*Subject:* Re: [OpenJDK 2D-Dev] [12] JDK-8212202: NPE in the print
tests after JDK-8153732
Hi Shashi,
I have one comment:
doCompare(prevRemotePrinters, currentRemotePrinters) is only called
from run() method when "prevRemotePrinters" is already checked to be
not null [*if (prevRemotePrinters != null && prevRemotePrinters.length
> 0) ]*
so I see no point in checking "str1" [which is prevRemotePrinters] to
be null in doCompare(). I guess instead of
*413 if (str1 == null && str2 == null) {*
*414 return false;*
*415 } else if (str1 == null || str2 == null) {*
*416 return true;*
*417 }*
you can have
if (str2 == null)
return true;
Regards
Prasanta
On 15-Feb-19 2:48 AM, Phil Race wrote:
+1 .. remember to use the current bug synopsis in the push comment
ie : "[Windows] Exception if no printers are installed."
-phil.
On 2/11/19 12:39 AM, Shashidhara Veerabhadraiah wrote:
Hi Phil, Here is the new Webrev fixing those comments:
http://cr.openjdk.java.net/~sveerabhadra/8212202/webrev.06/
*Cc:* Prasanta Sadhukhan
;
2d-dev@openjdk.java.net
*Subject:* Re: [OpenJDK 2D-Dev] [12] JDK-8212202: NPE in the
print tests after JDK-8153732
413 if (str1 == null && str2 == null) {
414 return false;
415 } else if ((str1 == null && str2 != null) ||
(str2 == null && str1 != null)) {
416 return true;
417 }
When we get to 415 we already know at least one of str1 or
str2 is non-null so 415 can just be
} else if (str1 == null || str2 == null) {
-phil.
On 2/6/19, 12:31 AM, Shashidhara Veerabhadraiah wrote:
Hi Phil, Here is the updated Webrev fixing those comments:
http://cr.openjdk.java.net/~sveerabhadra/8212202/webrev.05/
*Cc:* Prasanta Sadhukhan
;
2d-dev@openjdk.java.net
*Subject:* Re: [OpenJDK 2D-Dev] [12] JDK-8212202: NPE in
the print tests after JDK-8153732
Sorry .. this got lost ..
I don't understand this line :
253 jobjectArray emptyArray = env->NewObjectArray(1,
clazz, NULL);
This is returning an array of length 1 and element 0 is NULL.
I think you want
env->NewObjectArray(0, clazz, NULL);
and I don't see why you need to create it there
instead you can just have
304 if (nameArray != NULL) {
305 return nameArray;
306 } else {
307 return env->NewObjectArray(0, clazz, NULL);
308 }
449 if (prevRemotePrinters
Re: [OpenJDK 2D-Dev] [12] JDK-8212202: NPE in the print tests after JDK-8153732
Hi Prasanta, doCompare(str1, str2) is a different function and one should not
define a function based on how it is going to be called!! What if someone
changed the caller to this: doCompare(currentRemotePrinters,
prevRemotePrinters). There is no restriction if one calls like this. Here the
function is taking 2 strings and it does not say which one should be passed at
what position. Probably if the function takes different parameters then it sets
an automatic rule on which parameter needs to be passed at which position but
otherwise function definition should take care this.
Hope you agree with me.
Thanks and regards,
Shashi
From: Prasanta Sadhukhan
Sent: Friday, February 15, 2019 12:11 PM
To: Phil Race ; Shashidhara Veerabhadraiah
Cc: 2d-dev@openjdk.java.net
Subject: Re: [OpenJDK 2D-Dev] [12] JDK-8212202: NPE in the print tests after
JDK-8153732
Hi Shashi,
I have one comment:
doCompare(prevRemotePrinters, currentRemotePrinters) is only called from run()
method when "prevRemotePrinters" is already checked to be not null [if
(prevRemotePrinters != null && prevRemotePrinters.length > 0) ]
so I see no point in checking "str1" [which is prevRemotePrinters] to be null
in doCompare(). I guess instead of
413 if (str1 == null && str2 == null) {
414 return false;
415 } else if (str1 == null || str2 == null) {
416 return true;
417 }
you can have
if (str2 == null)
return true;
Regards
Prasanta
On 15-Feb-19 2:48 AM, Phil Race wrote:
+1 .. remember to use the current bug synopsis in the push comment
ie : "[Windows] Exception if no printers are installed."
-phil.
On 2/11/19 12:39 AM, Shashidhara Veerabhadraiah wrote:
Hi Phil, Here is the new Webrev fixing those comments:
HYPERLINK
"http://cr.openjdk.java.net/%7Esveerabhadra/8212202/webrev.06/"http://cr.openjdk.java.net/~sveerabhadra/8212202/webrev.06/
Thanks and regards,
Shashi
From: Philip Race
Sent: Saturday, February 9, 2019 2:25 AM
To: Shashidhara Veerabhadraiah HYPERLINK
"mailto:shashidhara.veerabhadra...@oracle.com;
Cc: Prasanta Sadhukhan HYPERLINK
"mailto:prasanta.sadhuk...@oracle.com;;
HYPERLINK "mailto:2d-dev@openjdk.java.net"2d-dev@openjdk.java.net
Subject: Re: [OpenJDK 2D-Dev] [12] JDK-8212202: NPE in the print tests after
JDK-8153732
413 if (str1 == null && str2 == null) {
414 return false;
415 } else if ((str1 == null && str2 != null) || (str2 == null &&
str1 != null)) {
416 return true;
417 }
When we get to 415 we already know at least one of str1 or str2 is non-null so
415 can just be
} else if (str1 == null || str2 == null) {
-phil.
On 2/6/19, 12:31 AM, Shashidhara Veerabhadraiah wrote:
Hi Phil, Here is the updated Webrev fixing those comments:
HYPERLINK
"http://cr.openjdk.java.net/%7Esveerabhadra/8212202/webrev.05/"http://cr.openjdk.java.net/~sveerabhadra/8212202/webrev.05/
Thanks and regards,
Shashi
From: Phil Race
Sent: Thursday, January 31, 2019 4:36 AM
To: Shashidhara Veerabhadraiah HYPERLINK
"mailto:shashidhara.veerabhadra...@oracle.com;
Cc: Prasanta Sadhukhan HYPERLINK
"mailto:prasanta.sadhuk...@oracle.com;;
HYPERLINK "mailto:2d-dev@openjdk.java.net"2d-dev@openjdk.java.net
Subject: Re: [OpenJDK 2D-Dev] [12] JDK-8212202: NPE in the print tests after
JDK-8153732
Sorry .. this got lost ..
I don't understand this line :
253 jobjectArray emptyArray = env->NewObjectArray(1, clazz, NULL);
This is returning an array of length 1 and element 0 is NULL.
I think you want
env->NewObjectArray(0, clazz, NULL);
and I don't see why you need to create it there
instead you can just have
304 if (nameArray != NULL) {
305 return nameArray;
306 } else {
307 return env->NewObjectArray(0, clazz, NULL);
308 }
449 if (prevRemotePrinters != null) {
shouldn't this be
449 if (prevRemotePrinters != null &&
prevRemotePrinters.length > 0) {
?
-phil.
On 12/10/18 10:19 PM, Shashidhara Veerabhadraiah wrote:
Hi Phil, I have updated with small code updates:
HYPERLINK
"http://cr.openjdk.java.net/%7Esveerabhadra/8212202/webrev.04/"http://cr.openjdk.java.net/~sveerabhadra/8212202/webrev.04/
Thanks and regards,
Shashi
From: Shashidhara Veerabhadraiah
Sent: Monday, December 10, 2018 3:19 PM
To: Philip Race HYPERLINK
"mailto:philip.r...@oracle.com;
Cc: Prasanta Sadhukhan HYPERLINK
"mailto:prasanta.sadhuk...@oracle.com;;
HYPERLINK "mailto:2d-dev@openjdk.java.net"2d-dev@openjdk.java.net
Subject: RE: [OpenJDK 2D-Dev] [12] JDK-8212202: NPE in the print tests after
JDK-8153732
Hi Phil, Please find the updated Webrev.
HYPERLINK
"http://cr.openjdk.java.net/%7Esveerabhadra/8212202/webrev.03/"http://cr.openjdk.java.net/~sveerabhadra/8212202/webrev.03/
Thanks and regards,
Shashi
From: Philip Race
Sent: Friday, December 7, 2018 8:54
Re: [OpenJDK 2D-Dev] [12] JDK-8212202: NPE in the print tests after JDK-8153732
Hi Shashi,
I have one comment:
doCompare(prevRemotePrinters, currentRemotePrinters) is only called from
run() method when "prevRemotePrinters" is already checked to be not null
[if (prevRemotePrinters != null && prevRemotePrinters.length > 0) ]
so I see no point in checking "str1" [which is prevRemotePrinters] to be
null in doCompare(). I guess instead of
413 if (str1 == null && str2 == null) {414 return false;415 } else if
(str1 == null || str2 == null) {416 return true;417 }you can have if
(str2 == null) return true; Regards Prasanta
On 15-Feb-19 2:48 AM, Phil Race wrote:
+1 .. remember to use the current bug synopsis in the push comment
ie : "[Windows] Exception if no printers are installed."
-phil.
On 2/11/19 12:39 AM, Shashidhara Veerabhadraiah wrote:
Hi Phil, Here is the new Webrev fixing those comments:
http://cr.openjdk.java.net/~sveerabhadra/8212202/webrev.06/
*Cc:* Prasanta Sadhukhan
; 2d-dev@openjdk.java.net
*Subject:* Re: [OpenJDK 2D-Dev] [12] JDK-8212202: NPE in the
print tests after JDK-8153732
Sorry .. this got lost ..
I don't understand this line :
253 jobjectArray emptyArray = env->NewObjectArray(1, clazz,
NULL);
This is returning an array of length 1 and element 0 is NULL.
I think you want
env->NewObjectArray(0, clazz, NULL);
and I don't see why you need to create it there
instead you can just have
304 if (nameArray != NULL) {
305 return nameArray;
306 } else {
307 return env->NewObjectArray(0, clazz, NULL);
308 }
449 if (prevRemotePrinters != null) {
shouldn't this be
449 if (prevRemotePrinters != null &&
prevRemotePrinters.length > 0) {
?
-phil.
On 12/10/18 10:19 PM, Shashidhara Veerabhadraiah wrote:
Hi Phil, I have updated with small code updates:
http://cr.openjdk.java.net/~sveerabhadra/8212202/webrev.04/
*Cc:* Prasanta Sadhukhan
;
2d-dev@openjdk.java.net
*Subject:* RE: [OpenJDK 2D-Dev] [12] JDK-8212202: NPE in the
print tests after JDK-8153732
Hi Phil, Please find the updated Webrev.
http://cr.openjdk.java.net/~sveerabhadra/8212202/webrev.03/
*Subject:* Re: [OpenJDK 2D-Dev] [12] JDK-8212202: NPE in the
print tests after JDK-8153732
First off, I think the high order bit is that if a non-null
array reference is returned there are NO
null String entries in it. Whether a zero length or null
array is returned when there are no printers
is the less important issue.
However an empty array also tells you there are no printers,
and you are less likely to get an NPE from that.
It is arguably easier for the caller, if they don't need the
extra null check first.
FWIW the javax.print public APIs return zero length arrays
instead
Re: [OpenJDK 2D-Dev] [12] Review Request: 8213110 Remove the use of applets in automatic tests
On 30/01/2019 14:53, Phil Race wrote:
Oh .. and I meant to ask what probably is an obvious question but needs to be
asked anyway.
You say you did not try to fix tests that already fail - this is just a
conversion - but did you
confirm there are no NEW failures on any platform ?
Yes, there are no tests which passed before the fix and fails after.
-phil.
On 1/30/19 2:29 PM, Phil Race wrote:
You discuss the life cycle of an applet in jtreg
---
- Call init() and start() methods of the Applet
- Waits for 2 seconds
- Call stop() and destroy() methods of the Applet
- Dispose the frame
--
I see init() and start() being used but I don't see the rest replaced. Why not ?
Do you really mean jtreg() kills the applet just two seconds after start()
returns ?
For the tests where you do something like
main(...) {
obj.init();
obj.start();
eg here :
http://cr.openjdk.java.net/~serb/8213110/webrev.04/test/jdk/java/awt/Focus/AppletInitialFocusTest/AppletInitialFocusTest.java.sdiff.html
would it make sense to invoke that code on the EDT instead of the main thread ?
EventQueue.invokeLater(new Runnable() {
Like already happens here :
http://cr.openjdk.java.net/~serb/8213110/webrev.04/test/jdk/java/awt/FileDialog/FilenameFilterTest/FilenameFilterTest.java.sdiff.html
We have lots of AWT tests that use the main thread so not a blocker but just a
question and
likely could mean further updates to some tests are needed if an exception no
longer propagates
on an expected thread.
-phil.
On 11/26/18 6:29 PM, Sergey Bylokhov wrote:
The "ProblemList.txt" is updated in the new version:
http://cr.openjdk.java.net/~serb/8213110/webrev.04
On 08/11/2018 10:57, Sergey Bylokhov wrote:
Note that for some of the tests I'll need to update the problem list and
replace the .html by the .java.
But since we actively updates the problem lists right now I postponed this task
for a weekend.
On 07/11/2018 15:47, Sergey Bylokhov wrote:
Hello.
Please review the fix for jdk 12.
Bug: https://bugs.openjdk.java.net/browse/JDK-8213110
Webrev: http://cr.openjdk.java.net/~serb/8213110/webrev.03
Description of the bug:
A number of our tests in jdk still launch by applets via "@run applet".
Usually we drop this usage when we update the test for some reason, and
in this fix we will drop them completely for automated tests from the open repo.
I did not update the tests which are specific for Applet API, manual tests, or
tests
which are currently under review for some other bugs.
Note: the "@run applet" means, that the jtreg will do these steps:
- Creates a frame as a holder for the Applet
- Creates an applet(which is undecorated panel) and adds it to the frame
- Sets the size of the frame
- Place the frame to the center of the screen
- Make the frame visible
- Call init() and start() methods of the Applet
- Waits for 2 seconds
- Call stop() and destroy() methods of the Applet
- Dispose the frame
Description of the fix:
- In all cases the usage of the Applet API was dropped
- In the common case when the applet was used as launcher, this code now used
instead:
public static void main(final String[] args) {
TestName app = new TestName();
app.init();
app.start();
}
Example:
http://cr.openjdk.java.net/~serb/8213110/webrev.03/test/jdk/java/awt/Choice/PopdownGeneratesMouseEvents/PopdownGeneratesMouseEvents.java.sdiff.html
- In some cases it was possible to replace init()/start() by the simple
main() method.
Example:
http://cr.openjdk.java.net/~serb/8213110/webrev.03/test/jdk/java/awt/Choice/PopupPosTest/PopupPosTest.java.sdiff.html
- Some of the tests were used the "extend Applet" w/o a reasons:
Example:
http://cr.openjdk.java.net/~serb/8213110/webrev.03/test/jdk/java/awt/Focus/ActualFocusedWindowTest/ActualFocusedWindowBlockingTest.java.sdiff.html
- Some of the tests shows the applet window(which was unrelated to the test
itself) in the center of the screen.
Example:
http://cr.openjdk.java.net/~serb/8213110/webrev.03/test/jdk/java/awt/Focus/ActualFocusedWindowTest/ActualFocusedWindowRetaining.java.sdiff.html
- Some of the tests uses the applet as a place holder for the test components.
In this case it was necessary to change the Applet API to the Frame API,
and complete
the steps which were done by the jtreg:
Example:
http://cr.openjdk.java.net/~serb/8213110/webrev.03/test/jdk/java/awt/Focus/AppletInitialFocusTest/AppletInitialFocusTest.java.sdiff.html
Notes:
- To simplify the review I tried to not change the logic of the tests,
so if the test fail before the fix, then it will fail after the fix.
I would like to create the separate CRs for all additional(if any) cleanup
of these tests.
- Just a few exception from the above is additional calls to:
"setLocationRelativeTo" to place the window to the center of the screen
"robot.waitForIdle()"/"setUndecorated()" to make the tests more stable(when
I
Re: [OpenJDK 2D-Dev] [12] Review Request: 8213110 Remove the use of applets in automatic tests
Hi, Phil.
Thank you for review, see my comments inline:
You discuss the life cycle of an applet in jtreg
---
- Call init() and start() methods of the Applet
- Waits for 2 seconds
- Call stop() and destroy() methods of the Applet
- Dispose the frame
--
I see init() and start() being used but I don't see the rest replaced. Why not ?
Because most of our tests override init() and start() and skip stop()/destroy().
Do you really mean jtreg() kills the applet just two seconds after start()
returns ?
After two seconds it will call stop()/destroy() then dispose the frame and exit.
https://hg.openjdk.java.net/code-tools/jtreg/file/6f00c63c0a98/src/share/classes/com/sun/javatest/regtest/agent/AppletWrapper.java#l157
For the tests where you do something like
main(...) {
obj.init();
obj.start();
eg here :
http://cr.openjdk.java.net/~serb/8213110/webrev.04/test/jdk/java/awt/Focus/AppletInitialFocusTest/AppletInitialFocusTest.java.sdiff.html
would it make sense to invoke that code on the EDT instead of the main thread ?
In this awt test it is not necessary, but some other tests(for Swing library)
should be updated at some point.
EventQueue.invokeLater(new Runnable() {
Like already happens here :
http://cr.openjdk.java.net/~serb/8213110/webrev.04/test/jdk/java/awt/FileDialog/FilenameFilterTest/FilenameFilterTest.java.sdiff.html
We have lots of AWT tests that use the main thread so not a blocker but just a
question and
likely could mean further updates to some tests are needed if an exception no
longer propagates
on an expected thread.
In this test it is needed because otherwise "fd.setVisible(true)" will block the
"main()" thread.
--
Best regards, Sergey.
Re: [OpenJDK 2D-Dev] RFR: JDK-8218682 + JDK-8198411: [TEST_BUG] DashOffset fails in mach5 + Two java2d tests are unstable in mach5
On 14/02/2019 13:02, Alexey Ivanov wrote: BTW do we sure that the usage of IndexColorModel is not a bug? I don't think it's a bug. Likely Windows Server OS is installed in Server Code mode which has limited GUI support. I was able to get IndexColorModel on my desktop by something like this: User1: - sleep for 10 sec - > run java program -> print color model for the current GC -> pause - win+L User2: - Login to User2 -> wait 10 sec User1: - IndexColorModel will be printed, instead of DirectColorModel. -- Best regards, Sergey.
Re: [OpenJDK 2D-Dev] [12] JDK-8212202: NPE in the print tests after JDK-8153732
+1 .. remember to use the current bug synopsis in the push comment
ie : "[Windows] Exception if no printers are installed."
-phil.
On 2/11/19 12:39 AM, Shashidhara Veerabhadraiah wrote:
Hi Phil, Here is the new Webrev fixing those comments:
http://cr.openjdk.java.net/~sveerabhadra/8212202/webrev.06/
Thanks and regards,
Shashi
*From:*Philip Race
*Sent:* Saturday, February 9, 2019 2:25 AM
*To:* Shashidhara Veerabhadraiah
*Cc:* Prasanta Sadhukhan ;
2d-dev@openjdk.java.net
*Subject:* Re: [OpenJDK 2D-Dev] [12] JDK-8212202: NPE in the print
tests after JDK-8153732
413 if (str1 == null && str2 == null) {
414 return false;
415 } else if ((str1 == null && str2 != null) || (str2 ==
null && str1 != null)) {
416 return true;
417 }
When we get to 415 we already know at least one of str1 or str2 is
non-null so 415 can just be
} else if (str1 == null || str2 == null) {
-phil.
On 2/6/19, 12:31 AM, Shashidhara Veerabhadraiah wrote:
Hi Phil, Here is the updated Webrev fixing those comments:
http://cr.openjdk.java.net/~sveerabhadra/8212202/webrev.05/
*Cc:* Prasanta Sadhukhan
; 2d-dev@openjdk.java.net
*Subject:* Re: [OpenJDK 2D-Dev] [12] JDK-8212202: NPE in the print
tests after JDK-8153732
Sorry .. this got lost ..
I don't understand this line :
253 jobjectArray emptyArray = env->NewObjectArray(1, clazz, NULL);
This is returning an array of length 1 and element 0 is NULL.
I think you want
env->NewObjectArray(0, clazz, NULL);
and I don't see why you need to create it there
instead you can just have
304 if (nameArray != NULL) {
305 return nameArray;
306 } else {
307 return env->NewObjectArray(0, clazz, NULL);
308 }
449 if (prevRemotePrinters != null) {
shouldn't this be
449 if (prevRemotePrinters != null &&
prevRemotePrinters.length > 0) {
?
-phil.
On 12/10/18 10:19 PM, Shashidhara Veerabhadraiah wrote:
Hi Phil, I have updated with small code updates:
http://cr.openjdk.java.net/~sveerabhadra/8212202/webrev.04/
*Cc:* Prasanta Sadhukhan
;
2d-dev@openjdk.java.net
*Subject:* RE: [OpenJDK 2D-Dev] [12] JDK-8212202: NPE in the
print tests after JDK-8153732
Hi Phil, Please find the updated Webrev.
http://cr.openjdk.java.net/~sveerabhadra/8212202/webrev.03/
*Subject:* Re: [OpenJDK 2D-Dev] [12] JDK-8212202: NPE in the
print tests after JDK-8153732
First off, I think the high order bit is that if a non-null
array reference is returned there are NO
null String entries in it. Whether a zero length or null array
is returned when there are no printers
is the less important issue.
However an empty array also tells you there are no printers,
and you are less likely to get an NPE from that.
It is arguably easier for the caller, if they don't need the
extra null check first.
FWIW the javax.print public APIs return zero length arrays
instead of null and applications seem to survive :-)
I don't know what you mean by :
> (And anyway we need to handle the first null string reference)?
If you are referring to a small matter of coding in the native
layer, then that is not an insurmountable problem.
Basically if there are problems getting names or whatever in
the native layer, handle it THERE, don't make
everyone else have to deal with it.
One caveat: JNI calls can theoretically fail due to OOME .. in
such a case we are doomed and
the main goal is to not crash in native and to obey all JNI
rules. What is
Re: [OpenJDK 2D-Dev] RFR: JDK-8218682 + JDK-8198411: [TEST_BUG] DashOffset fails in mach5 + Two java2d tests are unstable in mach5
On 14/02/2019 16:04, Sergey Bylokhov wrote: It looks fine. Thank you, Sergey! BTW do we sure that the usage of IndexColorModel is not a bug? I don't think it's a bug. Likely Windows Server OS is installed in Server Code mode which has limited GUI support. Regards, Alexey On 14/02/2019 06:07, Alexey Ivanov wrote: Hi Sergey, Do you have any comments for the latest webrev: http://cr.openjdk.java.net/~aivanov/8218682-8198411/webrev.01/ Do I push the fix? Regards, Alexey On 12/02/2019 18:33, Phil Race wrote: +1 -phil. On 2/12/19 6:24 AM, Alexey Ivanov wrote: Hi Phil, On 11/02/2019 18:32, Phil Race wrote: On 2/11/19 1:44 AM, Alexey Ivanov wrote: Hi Phil, On 08/02/2019 21:02, Phil Race wrote: can you add @key headful to all these tests ? Yes, I can if you think it's required. Yes. I think it is required. Please see the updated webrev: http://cr.openjdk.java.net/~aivanov/8218682-8198411/webrev.01/ These three test are not run in mach5. Regards, Alexey I was thinking whether I shall add a diagnostic message for skipping VolatileImage when IndexColorModel is in effect… sure. -phil. Regards, Alexey -phil. On 2/8/19 12:13 PM, Alexey Ivanov wrote: Hi, Please review the fix for jdk 13: bugs: https://bugs.openjdk.java.net/browse/JDK-8218682 https://bugs.openjdk.java.net/browse/JDK-8198411 webrev: http://cr.openjdk.java.net/~aivanov/8218682-8198411/webrev.00/ Description: The updated DashOffset test proved to fail in mach5. It passed BufferedImage test and then failed VolatileImage test. VolatileImage had different colours instead of the expected white, blue and green. Root cause: The host uses IndexColorModel; the image uses the closest colour. Thus the colours do not match. It's also the reason why DashScaleMinWidth.java and DashZeroWidth.java fail in mach5. Fix: Skip testing VolatileImage where default graphics configuration uses IndexColorModel. I'm removing DashScaleMinWidth.java and DashZeroWidth.java from ProblemList.txt as the tests pass now. Regards, Alexey
Re: [OpenJDK 2D-Dev] RFR 8218854: FontMetrics.getMaxAdvance may be less than the maximum FontMetrics.charWidth
Hi Phil, Thanks for your feedback. I proposed the font in the test case because it's licensed under GNU Affero General Public License, Version 3 (AGPL) with an exemption [2]. My understanding was that we were able to ship it. Anyways, I removed the font and use system installed fonts. On newer Linux distributions, Z003 font should be available anyways. Comment added explaining that bold value comes from freetype 2.6 and is valid at least up to 2.9.1. I was not sure if applying the OBLIQUE_MODIFIER changes as part of this ticket. Done. Line lengths adjusted to not exceed 80 chars. freetypeScaler.c copyright date updated. Webrev 01: * http://cr.openjdk.java.net/~mbalao/webrevs/8218854/8218854.webrev.01/ * http://cr.openjdk.java.net/~mbalao/webrevs/8218854/8218854.webrev.01.zip Are we good to go? Kind regards, Martin.- -- [1] - https://github.com/URWTypeFoundry/Core_35/blob/master/COPYING [2] - https://github.com/URWTypeFoundry/Core_35/blob/master/LICENSE
Re: [OpenJDK 2D-Dev] RFR 8218854: FontMetrics.getMaxAdvance may be less than the maximum FontMetrics.charWidth
Unfortunately we can't check in the font. That would require us to obtain a 3rd party code license and to update the font regularly as new versions are created. Way too onerous. Perhaps instead you can just run through installed fonts ? Can you also add a comment that explains that the values you are using match those in freetype 2.6 -> current (2.9.1) ... And consequently I think you should update the values used by OBLIQUE_MODIFIER at the same time to match current freetype. The current value is from the ancient 2.3 release. Include a comment there too .. Also can you check you are keeping line lengths including comment lines < 80 chars where possible ? -phil. On 2/12/19, 2:40 PM, Martin Balao wrote: Hi, I'd like to propose the following fix for "JDK-8218854: FontMetrics.getMaxAdvance may be less than the maximum FontMetrics.charWidth" [1]: * http://cr.openjdk.java.net/~mbalao/webrevs/8218854/8218854.webrev.00/ Can I have a review? Thanks, Martin.- -- [1] - https://bugs.openjdk.java.net/browse/JDK-8218854
Re: [OpenJDK 2D-Dev] RFR: JDK-8218682 + JDK-8198411: [TEST_BUG] DashOffset fails in mach5 + Two java2d tests are unstable in mach5
It looks fine. BTW do we sure that the usage of IndexColorModel is not a bug? On 14/02/2019 06:07, Alexey Ivanov wrote: Hi Sergey, Do you have any comments for the latest webrev: http://cr.openjdk.java.net/~aivanov/8218682-8198411/webrev.01/ Do I push the fix? Regards, Alexey On 12/02/2019 18:33, Phil Race wrote: +1 -phil. On 2/12/19 6:24 AM, Alexey Ivanov wrote: Hi Phil, On 11/02/2019 18:32, Phil Race wrote: On 2/11/19 1:44 AM, Alexey Ivanov wrote: Hi Phil, On 08/02/2019 21:02, Phil Race wrote: can you add @key headful to all these tests ? Yes, I can if you think it's required. Yes. I think it is required. Please see the updated webrev: http://cr.openjdk.java.net/~aivanov/8218682-8198411/webrev.01/ These three test are not run in mach5. Regards, Alexey I was thinking whether I shall add a diagnostic message for skipping VolatileImage when IndexColorModel is in effect… sure. -phil. Regards, Alexey -phil. On 2/8/19 12:13 PM, Alexey Ivanov wrote: Hi, Please review the fix for jdk 13: bugs: https://bugs.openjdk.java.net/browse/JDK-8218682 https://bugs.openjdk.java.net/browse/JDK-8198411 webrev: http://cr.openjdk.java.net/~aivanov/8218682-8198411/webrev.00/ Description: The updated DashOffset test proved to fail in mach5. It passed BufferedImage test and then failed VolatileImage test. VolatileImage had different colours instead of the expected white, blue and green. Root cause: The host uses IndexColorModel; the image uses the closest colour. Thus the colours do not match. It's also the reason why DashScaleMinWidth.java and DashZeroWidth.java fail in mach5. Fix: Skip testing VolatileImage where default graphics configuration uses IndexColorModel. I'm removing DashScaleMinWidth.java and DashZeroWidth.java from ProblemList.txt as the tests pass now. Regards, Alexey -- Best regards, Sergey.
Re: [OpenJDK 2D-Dev] RFR: JDK-8218682 + JDK-8198411: [TEST_BUG] DashOffset fails in mach5 + Two java2d tests are unstable in mach5
Hi Sergey, Do you have any comments for the latest webrev: http://cr.openjdk.java.net/~aivanov/8218682-8198411/webrev.01/ Do I push the fix? Regards, Alexey On 12/02/2019 18:33, Phil Race wrote: +1 -phil. On 2/12/19 6:24 AM, Alexey Ivanov wrote: Hi Phil, On 11/02/2019 18:32, Phil Race wrote: On 2/11/19 1:44 AM, Alexey Ivanov wrote: Hi Phil, On 08/02/2019 21:02, Phil Race wrote: can you add @key headful to all these tests ? Yes, I can if you think it's required. Yes. I think it is required. Please see the updated webrev: http://cr.openjdk.java.net/~aivanov/8218682-8198411/webrev.01/ These three test are not run in mach5. Regards, Alexey I was thinking whether I shall add a diagnostic message for skipping VolatileImage when IndexColorModel is in effect… sure. -phil. Regards, Alexey -phil. On 2/8/19 12:13 PM, Alexey Ivanov wrote: Hi, Please review the fix for jdk 13: bugs: https://bugs.openjdk.java.net/browse/JDK-8218682 https://bugs.openjdk.java.net/browse/JDK-8198411 webrev: http://cr.openjdk.java.net/~aivanov/8218682-8198411/webrev.00/ Description: The updated DashOffset test proved to fail in mach5. It passed BufferedImage test and then failed VolatileImage test. VolatileImage had different colours instead of the expected white, blue and green. Root cause: The host uses IndexColorModel; the image uses the closest colour. Thus the colours do not match. It's also the reason why DashScaleMinWidth.java and DashZeroWidth.java fail in mach5. Fix: Skip testing VolatileImage where default graphics configuration uses IndexColorModel. I'm removing DashScaleMinWidth.java and DashZeroWidth.java from ProblemList.txt as the tests pass now. Regards, Alexey
