Re: [9] Fix for JDK-6567433 : JComponent.updateUI() may create StackOverflowError
Hi Andrej, Can you please review this? Regards, Ajit -Original Message- From: Ajit Ghaisas Sent: Friday, July 08, 2016 10:32 AM To: Andrej Golovnin Cc: Rajeev Chamyal; Alexander Scherbatiy; swing-dev@openjdk.java.net Subject: RE: [9] Fix for JDK-6567433 : JComponent.updateUI() may create StackOverflowError Hi Andrej, Thanks for your suggestion. I have made the 'updateInProgress' member of these classes transient. This is out of the fact that - 'updateInProgress' member is just an internal field of the class that need not be preserved during serialization. Here is the updated webrev. Request you to review. http://cr.openjdk.java.net/~aghaisas/6567433/webrev.03/ Regards, Ajit -Original Message- From: Andrej Golovnin [mailto:andrej.golov...@gmail.com] Sent: Thursday, July 07, 2016 4:44 PM To: Ajit Ghaisas Cc: Rajeev Chamyal; Alexander Scherbatiy; swing-dev@openjdk.java.net Subject: Re: [9] Fix for JDK-6567433 : JComponent.updateUI() may create StackOverflowError Hi Ajit, one more thing that I have just noticed: /** * Flag to indicate UI update is in progress */ private boolean updateInProgress; I think the field must be transient. In Swing every component is serializable. When updateInProgress is set to true and you serialize/deserialize the component, then the call of the #updateUI()-method on the deserialized instance would never update the UI of the deserialized component because the flag updateInProgress will never change from true to false. Best regards, Andrej Golovnin
Re: [9] Fix for JDK-6567433 : JComponent.updateUI() may create StackOverflowError
Hi Ajit, it looks good for me. Thanks! And you need a reviewer from the Swing team as I don’t have the reviewer role. Best regards, Andrej Golovnin > > -Original Message- > From: Ajit Ghaisas > Sent: Friday, July 08, 2016 10:32 AM > To: Andrej Golovnin > Cc: Rajeev Chamyal; Alexander Scherbatiy; swing-dev@openjdk.java.net > Subject: RE: [9] Fix for JDK-6567433 : JComponent.updateUI() may create > StackOverflowError > > Hi Andrej, > > Thanks for your suggestion. > > I have made the 'updateInProgress' member of these classes transient. > This is out of the fact that - 'updateInProgress' member is just an > internal field of the class that need not be preserved during serialization. > > Here is the updated webrev. Request you to review. > http://cr.openjdk.java.net/~aghaisas/6567433/webrev.03/ > > Regards, > Ajit > > > > -Original Message- > From: Andrej Golovnin [mailto:andrej.golov...@gmail.com] > Sent: Thursday, July 07, 2016 4:44 PM > To: Ajit Ghaisas > Cc: Rajeev Chamyal; Alexander Scherbatiy; swing-dev@openjdk.java.net > Subject: Re: [9] Fix for JDK-6567433 : JComponent.updateUI() may create > StackOverflowError > > Hi Ajit, > > one more thing that I have just noticed: > > /** > * Flag to indicate UI update is in progress > */ > private boolean updateInProgress; > > I think the field must be transient. In Swing every component is > serializable. When updateInProgress is set to true and you > serialize/deserialize the component, then the call of the #updateUI()-method > on the deserialized instance would never update the UI of the deserialized > component because the flag updateInProgress will never change from true to > false. > > Best regards, > Andrej Golovnin
Re: [9] Fix for JDK-6567433 : JComponent.updateUI() may create StackOverflowError
The fix looks good to me. Thanks, Alexandr. On 7/12/2016 3:58 PM, Andrej Golovnin wrote: Hi Ajit, it looks good for me. Thanks! And you need a reviewer from the Swing team as I don’t have the reviewer role. Best regards, Andrej Golovnin -Original Message- From: Ajit Ghaisas Sent: Friday, July 08, 2016 10:32 AM To: Andrej Golovnin Cc: Rajeev Chamyal; Alexander Scherbatiy; swing-dev@openjdk.java.net Subject: RE: [9] Fix for JDK-6567433 : JComponent.updateUI() may create StackOverflowError Hi Andrej, Thanks for your suggestion. I have made the 'updateInProgress' member of these classes transient. This is out of the fact that - 'updateInProgress' member is just an internal field of the class that need not be preserved during serialization. Here is the updated webrev. Request you to review. http://cr.openjdk.java.net/~aghaisas/6567433/webrev.03/ Regards, Ajit -Original Message- From: Andrej Golovnin [mailto:andrej.golov...@gmail.com] Sent: Thursday, July 07, 2016 4:44 PM To: Ajit Ghaisas Cc: Rajeev Chamyal; Alexander Scherbatiy; swing-dev@openjdk.java.net Subject: Re: [9] Fix for JDK-6567433 : JComponent.updateUI() may create StackOverflowError Hi Ajit, one more thing that I have just noticed: /** * Flag to indicate UI update is in progress */ private boolean updateInProgress; I think the field must be transient. In Swing every component is serializable. When updateInProgress is set to true and you serialize/deserialize the component, then the call of the #updateUI()-method on the deserialized instance would never update the UI of the deserialized component because the flag updateInProgress will never change from true to false. Best regards, Andrej Golovnin
Re: 8160438: [PIT][macosx] [TEST_BUG] javax/swing/plaf/nimbus/8057791/bug8057791.java fails
On 7/12/2016 8:24 AM, Avik Niyogi wrote:
Hi All,
A gentle reminder, please review my code changes.
With Regards,
Avik Niyogi
On 08-Jul-2016, at 4:24 pm, Yuri Nesterenko wrote:
It pass for me if I provide some time to process native events
after the user activity simulation. For instance,
setAutoDelay(50) for robot does that plus waitForIdle()
after every mouseMove(). In this case, the part with that additional
check and a (misleading, I think) comment about the color profiles
may be removed. The test would take much more time though, and
@run main/timeout=600 bug8057791
line would be necessary.
Some more comments to the previous ones:
- runColorTestCase() uses JList and should be run on EDT
- "if (tryNimbusLookAndFeel()) {" It is supposed that the Nimbus L&F
is supported on all platforms. May be it is better to fail the test if
the Nimbus L&F is not set.
- "if (osName.contains("Mac")) { isMac = true; }" can be simplified
to "return osName.contains("Mac")"
- " if (!"".equals(errorString)) {" can be simplified to
!errorString.isEmpty()
Thanks,
Alexandr.
Thanks,
-yan
On 07/08/2016 04:28 AM, Avik Niyogi wrote:
The test does not pass if mac specific check is not done for
backgroundcolor.
The check is required to get the same values from BufferedImage as they
are the same as found with Digital Color Meter.
This test case fixes that.
Please provide inputs if this fix will get a +1 or if not any positive
inputs to modify the test case will be welcome.
With Regards,
Avik Niyogi
On 07-Jul-2016, at 9:51 pm, Semyon Sadetsky
mailto:semyon.sadet...@oracle.com>> wrote:
On 7/7/2016 6:30 PM, Yuri Nesterenko wrote:
On 07/07/2016 06:15 PM, Semyon Sadetsky wrote:
On 7/7/2016 5:58 PM, Yuri Nesterenko wrote:
On 07/07/2016 05:35 PM, Yuri Nesterenko wrote:
On 07/07/2016 05:04 PM, Semyon Sadetsky wrote:
On 07.07.2016 16:35, Avik Niyogi wrote:
Hi Semyon,
Thank you for the review comment.
In Mac OS X, *System Preferences > Displays > Colors > Display
Profile* section, the default value is *Color LCD*.
This causes a failure in some test cases which uses robot.The colour
configuration it expects to use is the *Generic RGB Profile*.
That is what “Non-generic display settings” means.
AFAIK there are instruction that tests should be executed using color
profile with no color corrections on OS X. I guess there are many
other
tests that fail with color correction.
If this is a root cause than the bug doesn't need to be fixed.
Well, I filed this bug and I used Generic RGB on all my
test machines. There may be additional precautions in the tests
about that but misconfiguration is not the root case here.
That said, I feel vary about attempts to guess Apple colors
wary I mean
in non-generic profiles.
Yuri. Do you mean that the non-generic is not a root case?
No: I had Generic RGB everywhere, and it failed for me.
I should say that the new version of the test properly
fails with b120 and pass with current PIT. And colors
are visibly different in the two builds: so the test works OK now.
That contradicts with the description of the fix.
Probably the test does unnecessary care about the non-Generic profiles.
159 if (!foundMatch && isMac()) {
160 //One more chance for Mac due to non-Generic
display setting
161 detectedColor = new Color(img.getRGB(x, y), true);
162 if (detectedColor.equals(colorCheck)) {
163 foundMatch = true;
164 break checkLoops;
165 }
166 }
Does it mean that the code above, which is necessary to serve
non-Generic profiles on OS X, may be removed and the test still passes?
--Semyon
-yan
--Semyon
-yan
--Semyon
Regarding “Negative scenarios”, these include cases where
colours are
different from the expected background or foreground colors
is checked with the robot and BufferedImage respectively to
*eliminate
false positives due to coincidentally finding a match* by using
robot
or BufferedImage.
Please find my changes appropriating the inputs provided.
I removed the variable foundMatch as I found that it is not required
at all and incorporated the suggestion to use return instead of a
labelled break.
http://cr.openjdk.java.net/~aniyogi/8160438/webrev.01/
https://bugs.openjdk.java.net/browse/JDK-8160438
*Webrev:
*
