Re: RFR: 8272756: Remove unnecessary explicit initialization of volatile variables in java.desktop
On Fri, 20 Aug 2021 09:41:15 GMT, Сергей Цыпанов wrote: > This is a continuation of > > - https://bugs.openjdk.java.net/browse/JDK-6736490 > - https://bugs.openjdk.java.net/browse/JDK-8035284 > - https://bugs.openjdk.java.net/browse/JDK-8145680 > - https://bugs.openjdk.java.net/browse/JDK-8251548 > > As mentioned in JDK-6736490: > > _An explicit initialization of a volatile class instance variable, such as > private volatile Object = null; or private volatile int scale = 0; is > unnecessary since the Java spec automatically initializes objects to null and > primitive type short, int, long, float and double to 0 and boolean to false. > Explicit initialization of volatile variable to a value the same as the > default implicit initialized value results in an unnecessary store and membar > operation._ src/java.desktop/share/classes/com/sun/imageio/plugins/tiff/TIFFIFD.java line 64: > 62: Set tags = essentialTags; > 63: if (tags == null) { > 64: essentialTags = Set.of( What is the purpose of the local tags here? - PR: https://git.openjdk.java.net/jdk/pull/5197
Integrated: 8267161 : Write automated test case for JDK-4479161
On Mon, 9 Aug 2021 18:36:07 GMT, lawrence.andrews wrote: > 1) Automated the manual test case. > 2) Removed html dependent file > 3) Removed javax.swing.JApplet dependent. > 4) Test case can be executed independently as well with jtreg framework. > 5) Added methods to know that JFrame and Other component is visible before > starting the user action via Robot. > > @shurymury This pull request has now been integrated. Changeset: d85560ed Author:lawrence.andrews Committer: Alexey Ivanov URL: https://git.openjdk.java.net/jdk/commit/d85560ed0f10a586b659db1c0201373457f1a5a9 Stats: 172 lines in 2 files changed: 88 ins; 50 del; 34 mod 8267161: Write automated test case for JDK-4479161 Reviewed-by: serb, aivanov - PR: https://git.openjdk.java.net/jdk/pull/5058
Re: RFR: 8267161 : Write automated test case for JDK-4479161 [v6]
On Fri, 20 Aug 2021 15:01:35 GMT, Alexey Ivanov wrote: >> lawrence.andrews has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Fixed review comments > > test/jdk/java/awt/im/4959409/bug4959409.java line 87: > >> 85: keyPressedEventLatch.countDown(); >> 86: jLabel.setText("keyPressed receive for Shift+1"); >> 87: System.out.println("keyPressed receive for >> Shift+1"); > > Here, it was correct: “received”. Over curiously did search and replace . Corrected - PR: https://git.openjdk.java.net/jdk/pull/5058
Re: RFR: 8267161 : Write automated test case for JDK-4479161 [v7]
On Fri, 20 Aug 2021 15:10:41 GMT, lawrence.andrews wrote: >> 1) Automated the manual test case. >> 2) Removed html dependent file >> 3) Removed javax.swing.JApplet dependent. >> 4) Test case can be executed independently as well with jtreg framework. >> 5) Added methods to know that JFrame and Other component is visible before >> starting the user action via Robot. >> >> @shurymury > > lawrence.andrews has updated the pull request incrementally with one > additional commit since the last revision: > > Fixed the right word Marked as reviewed by aivanov (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/5058
Re: RFR: 8267161 : Write automated test case for JDK-4479161 [v7]
> 1) Automated the manual test case. > 2) Removed html dependent file > 3) Removed javax.swing.JApplet dependent. > 4) Test case can be executed independently as well with jtreg framework. > 5) Added methods to know that JFrame and Other component is visible before > starting the user action via Robot. > > @shurymury lawrence.andrews has updated the pull request incrementally with one additional commit since the last revision: Fixed the right word - Changes: - all: https://git.openjdk.java.net/jdk/pull/5058/files - new: https://git.openjdk.java.net/jdk/pull/5058/files/f85ad4f4..c4b2649f Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=5058&range=06 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=5058&range=05-06 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/5058.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5058/head:pull/5058 PR: https://git.openjdk.java.net/jdk/pull/5058
Re: RFR: 8267161 : Write automated test case for JDK-4479161 [v6]
On Fri, 20 Aug 2021 14:50:51 GMT, lawrence.andrews wrote: >> 1) Automated the manual test case. >> 2) Removed html dependent file >> 3) Removed javax.swing.JApplet dependent. >> 4) Test case can be executed independently as well with jtreg framework. >> 5) Added methods to know that JFrame and Other component is visible before >> starting the user action via Robot. >> >> @shurymury > > lawrence.andrews has updated the pull request incrementally with one > additional commit since the last revision: > > Fixed review comments test/jdk/java/awt/im/4959409/bug4959409.java line 87: > 85: keyPressedEventLatch.countDown(); > 86: jLabel.setText("keyPressed receive for Shift+1"); > 87: System.out.println("keyPressed receive for > Shift+1"); Here, it was correct: “received”. - PR: https://git.openjdk.java.net/jdk/pull/5058
Re: RFR: 8267161 : Write automated test case for JDK-4479161 [v5]
On Fri, 20 Aug 2021 13:45:23 GMT, Alexey Ivanov wrote: >> lawrence.andrews has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Added WindowListener to check Frame is opened and simplified the testcase > > test/jdk/java/awt/im/4959409/bug4959409.java line 120: > >> 118: }); >> 119: >> 120: clickTextField(robot, points[0].x + rect[0].width / 2, >> points[0].y + rect[0].height / 2); > > I'd probably wrap this line, it doesn't fit even in 120 columns. done > test/jdk/java/awt/im/4959409/bug4959409.java line 131: > >> 129: >> 130: if (!keyPressedEventLatch.await(TIMEOUT, TimeUnit.SECONDS)) { >> 131: throw new RuntimeException("Did not received keyPressed for >> Shift + 1 , test failed"); > > The grammar: > Suggestion: > > throw new RuntimeException("Did not receive keyPressed for Shift > + 1 , test failed"); done > test/jdk/java/awt/im/4959409/bug4959409.java line 147: > >> 145: >> 146: public static void main(String[] args) throws InterruptedException, >> InvocationTargetException, >> 147: AWTException { > > Suggestion: > > public static void main(String[] args) throws Exception { > > Shorter. done - PR: https://git.openjdk.java.net/jdk/pull/5058
Re: RFR: 8267161 : Write automated test case for JDK-4479161 [v6]
> 1) Automated the manual test case. > 2) Removed html dependent file > 3) Removed javax.swing.JApplet dependent. > 4) Test case can be executed independently as well with jtreg framework. > 5) Added methods to know that JFrame and Other component is visible before > starting the user action via Robot. > > @shurymury lawrence.andrews has updated the pull request incrementally with one additional commit since the last revision: Fixed review comments - Changes: - all: https://git.openjdk.java.net/jdk/pull/5058/files - new: https://git.openjdk.java.net/jdk/pull/5058/files/b40a1353..f85ad4f4 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=5058&range=05 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=5058&range=04-05 Stats: 13 lines in 1 file changed: 1 ins; 3 del; 9 mod Patch: https://git.openjdk.java.net/jdk/pull/5058.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5058/head:pull/5058 PR: https://git.openjdk.java.net/jdk/pull/5058
Re: RFR: 8267161 : Write automated test case for JDK-4479161 [v5]
On Fri, 20 Aug 2021 13:39:58 GMT, Alexey Ivanov wrote: >> lawrence.andrews has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Added WindowListener to check Frame is opened and simplified the testcase > > test/jdk/java/awt/im/4959409/bug4959409.java line 59: > >> 57: private static JLabel jLabel; >> 58: >> 59: public static void createUIAndTest() throws InterruptedException, >> InvocationTargetException, AWTException { > > `throws Exception` would be enough and shorter: > Suggestion: > > public static void createUIAndTest() throws Exception { > > > It's a test code, any exception means the test fails, so we don't care much > about which specific exceptions can be thrown. Okay > test/jdk/java/awt/im/4959409/bug4959409.java line 80: > >> 78: >> 79: jTextField.addKeyListener(new KeyAdapter() { >> 80: > > I'd remove this blank line for consistency with the anonymous class above. Done > test/jdk/java/awt/im/4959409/bug4959409.java line 92: > >> 90: } else { >> 91: jLabel.setText("Did not received keyPressed for >> Shift+1"); >> 92: System.out.println("Did not received keyPressed >> for Shift+1"); > > The grammar: > Suggestion: > > jLabel.setText("Did not receive keyPressed for > Shift+1"); > System.out.println("Did not receive keyPressed for > Shift+1"); done - PR: https://git.openjdk.java.net/jdk/pull/5058
Re: RFR: 8267161 : Write automated test case for JDK-4479161 [v5]
On Thu, 19 Aug 2021 18:54:48 GMT, lawrence.andrews wrote: >> 1) Automated the manual test case. >> 2) Removed html dependent file >> 3) Removed javax.swing.JApplet dependent. >> 4) Test case can be executed independently as well with jtreg framework. >> 5) Added methods to know that JFrame and Other component is visible before >> starting the user action via Robot. >> >> @shurymury > > lawrence.andrews has updated the pull request incrementally with one > additional commit since the last revision: > > Added WindowListener to check Frame is opened and simplified the testcase Marked as reviewed by aivanov (Reviewer). test/jdk/java/awt/im/4959409/bug4959409.java line 59: > 57: private static JLabel jLabel; > 58: > 59: public static void createUIAndTest() throws InterruptedException, > InvocationTargetException, AWTException { `throws Exception` would be enough and shorter: Suggestion: public static void createUIAndTest() throws Exception { It's a test code, any exception means the test fails, so we don't care much about which specific exceptions can be thrown. test/jdk/java/awt/im/4959409/bug4959409.java line 80: > 78: > 79: jTextField.addKeyListener(new KeyAdapter() { > 80: I'd remove this blank line for consistency with the anonymous class above. test/jdk/java/awt/im/4959409/bug4959409.java line 92: > 90: } else { > 91: jLabel.setText("Did not received keyPressed for > Shift+1"); > 92: System.out.println("Did not received keyPressed > for Shift+1"); The grammar: Suggestion: jLabel.setText("Did not receive keyPressed for Shift+1"); System.out.println("Did not receive keyPressed for Shift+1"); test/jdk/java/awt/im/4959409/bug4959409.java line 120: > 118: }); > 119: > 120: clickTextField(robot, points[0].x + rect[0].width / 2, > points[0].y + rect[0].height / 2); I'd probably wrap this line, it doesn't fit even in 120 columns. test/jdk/java/awt/im/4959409/bug4959409.java line 131: > 129: > 130: if (!keyPressedEventLatch.await(TIMEOUT, TimeUnit.SECONDS)) { > 131: throw new RuntimeException("Did not received keyPressed for > Shift + 1 , test failed"); The grammar: Suggestion: throw new RuntimeException("Did not receive keyPressed for Shift + 1 , test failed"); test/jdk/java/awt/im/4959409/bug4959409.java line 147: > 145: > 146: public static void main(String[] args) throws InterruptedException, > InvocationTargetException, > 147: AWTException { Suggestion: public static void main(String[] args) throws Exception { Shorter. - PR: https://git.openjdk.java.net/jdk/pull/5058
RFR: 8272756: Remove unnecessary explicit initialization of volatile variables in java.desktop
This is a continuation of - https://bugs.openjdk.java.net/browse/JDK-6736490 - https://bugs.openjdk.java.net/browse/JDK-8035284 - https://bugs.openjdk.java.net/browse/JDK-8145680 - https://bugs.openjdk.java.net/browse/JDK-8251548 As mentioned in JDK-6736490: _An explicit initialization of a volatile class instance variable, such as private volatile Object = null; or private volatile int scale = 0; is unnecessary since the Java spec automatically initializes objects to null and primitive type short, int, long, float and double to 0 and boolean to false. Explicit initialization of volatile variable to a value the same as the default implicit initialized value results in an unnecessary store and membar operation._ - Commit messages: - 8272756: Remove unnecessary explicit initialization of volatile fields in java.desktop Changes: https://git.openjdk.java.net/jdk/pull/5197/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=5197&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8272756 Stats: 50 lines in 25 files changed: 0 ins; 0 del; 50 mod Patch: https://git.openjdk.java.net/jdk/pull/5197.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5197/head:pull/5197 PR: https://git.openjdk.java.net/jdk/pull/5197