On Fri, 27 Sep 2024 22:01:13 GMT, Harshitha Onkar <[email protected]> wrote:
>> Following tests are added as part of this PR: >> >> 1. /java/awt/dnd/DnDHTMLToOutlookTest/DnDHTMLToOutlookTest.java - **Manual >> PassFailJFrame (PFJ) test**, problemlisted on all platforms. >> >> 2. java/awt/dnd/DragSourceMotionListenerTest.java - **Automated test**, >> problemlisted on windows >> >> 3. /java/awt/dnd/DragToAnotherScreenTest.java - **Manual PFJ test** >> >> 4. /java/awt/dnd/RejectDragTest.java - **Automated test**, problemlisted on >> macOS > > Harshitha Onkar has updated the pull request incrementally with one > additional commit since the last revision: > > removed redundant frame.setLocation() test/jdk/ProblemList.txt line 468: > 466: java/awt/Dialog/MakeWindowAlwaysOnTop/MakeWindowAlwaysOnTop.java 8266243 > macosx-aarch64 > 467: java/awt/dnd/BadSerializationTest/BadSerializationTest.java 8277817 > linux-x64,windows-x64 > 468: java/awt/dnd/DragSourceMotionListenerTest.java 8225131 windows-all `DragSourceMotionListenerTest` passes for me on Windows. Does it need to be problem-listed? test/jdk/ProblemList.txt line 469: > 467: java/awt/dnd/BadSerializationTest/BadSerializationTest.java 8277817 > linux-x64,windows-x64 > 468: java/awt/dnd/DragSourceMotionListenerTest.java 8225131 windows-all > 469: java/awt/dnd/RejectDragTest.java 7124259 macosx-all I wonder if `RejectDragTest` still fails on macOS. Latest comment in [JDK-7124259](https://bugs.openjdk.org/browse/JDK-7124259) is dated February 2020. The problem could be fixed by now. test/jdk/ProblemList.txt line 470: > 468: java/awt/dnd/DragSourceMotionListenerTest.java 8225131 windows-all > 469: java/awt/dnd/RejectDragTest.java 7124259 macosx-all > 470: java/awt/dnd/DnDHTMLToOutlookTest/DnDHTMLToOutlookTest.java 8027424 > generic-all DnDHTMLToOutlookTest passes for me with Mozilla Thunderbird 128.2.3 as well as with Microsoft Word and Outlook version 2302 on Windows. I haven't tested on Linux or macOS. test/jdk/java/awt/dnd/DnDHTMLToOutlookTest/DnDHTMLToOutlookTest.java line 71: > 69: > 70: mainPanel = new Panel(); > 71: mainPanel.setLayout(new FlowLayout()); If we could make the panel larger, it would be easier to move it around on the screen. It's too small, and you can't grab it by title bar unless you resize the window. This test involves interacting with other applications, so it's good to be able to move the drag source, the button, on the screen. test/jdk/java/awt/dnd/DragSourceMotionListenerTest.java line 82: > 80: private static final Point testPoint2 = new Point(); > 81: private static volatile Point srcPoint; > 82: private static volatile Dimension d; `d` shouldn't be `volatile`; it should rather be a local variable in all the `invokeAndWait` calls — it's used only there. test/jdk/java/awt/dnd/DragSourceMotionListenerTest.java line 152: > 150: srcPoint = source.getLocationOnScreen(); > 151: d = source.getSize(); > 152: srcPoint.translate(d.width / 2, d.height / 2); This is thread-safe only because you use `invokeAndWait`, not because `srcPoint` is a volatile variable. It's only the assignment to a volatile variable that creates the *happens-before* relation to the following reads. It is easier to reason the correctness of the code if you assign to a volatile variable once; otherwise, additional modifications to the state of the assigned reference aren't guaranteed to be seen by other threads which read the updated reference from the variable. This case is still thread-safe, even without using `volatile` modifiers because of `invokeAndWait`, provided there are no other threads but main and EDT which read the value of `srcPoint`. Since it's common to add the `volatile` modifier, we should follow the rules for using it: Suggestion: Point p = source.getLocationOnScreen(); Dimension d = source.getSize(); p.translate(d.width / 2, d.height / 2); srcPoint = p; In the suggested snippet, the write to `srcPoint` occurs when the object `p` that has be read from another thread already has the state we want. When another thread reads the new value `srcPoint`, it is guaranteed to see the result of `translate` operation, which wasn't the case for the code as it's written (if it were not for `invokeAndWait`). test/jdk/java/awt/dnd/DragSourceMotionListenerTest.java line 165: > 163: dstOutsidePoint.translate(3 * d.width / 2, d.height / 2); > 164: }); > 165: testPoint1.setLocation(dstOutsidePoint); EDT is not guaranteed to see the updated values of `testPoint2` fields without additional synchronisation, and you need it for `sourceAdapter.dragMouseMoved`. test/jdk/java/awt/dnd/DragSourceMotionListenerTest.java line 172: > 170: dstInsidePoint.translate(d.width / 2, d.height / 2); > 171: }); > 172: testPoint2.setLocation(dstInsidePoint); EDT is not guaranteed to see the updated values of `testPoint2` fields without additional synchronisation, and you need it for `sourceAdapter.dragDropEnd`. Nether testPoint1 nor testPoint2 are used on main thread (except for the object instantiation). Move `setLocation` calls into `invokeAndWait`: then EDT is guaranteed to see the updated fields values of the `Point` objects. test/jdk/java/awt/dnd/DragSourceMotionListenerTest.java line 229: > 227: mouseReleaseEvent.countDown(); > 228: clickedComponent = (Component)e.getSource(); > 229: } Suggestion: if (e.getID() == MouseEvent.MOUSE_RELEASED) { clickedComponent = (Component)e.getSource(); mouseReleaseEvent.countDown(); } For thread-safety, you have to assign `clickedComponent` before releasing the latch; otherwise, the thread that's released from `await` is not guaranteed to see the updated value of `clickedComponent`. test/jdk/java/awt/dnd/DragSourceMotionListenerTest.java line 235: > 233: mouseReleaseEvent = new CountDownLatch(1); > 234: robot.waitForIdle(); > 235: reset(); I suggest moving `reset()` before you create a new instance of `CountDownLatch` and therefore before you write its reference to the volatile field `mouseReleaseEvent`. test/jdk/java/awt/dnd/DragSourceMotionListenerTest.java line 240: > 238: robot.mouseRelease(InputEvent.BUTTON1_DOWN_MASK); > 239: robot.waitForIdle(); > 240: robot.delay(500); Waiting is somewhat redundant here… you wait for the event using a `CountDownLatch`. test/jdk/java/awt/dnd/DragSourceMotionListenerTest.java line 245: > 243: } > 244: > 245: Component c = clickedComponent; Accessing `clickedComponent` is not thread-safe here. For it to be thread-safe, you have to assign the value before `mouseReleaseEvent.countDown()`. test/jdk/java/awt/dnd/DragToAnotherScreenTest.java line 60: > 58: > 59: If on multi-monitor screens then please position > 60: the drag and drop windows on different screens. This can be done automatically… I mean `PassFailJFrame` won't be able to handle positioning of the second frame on another screen. Yet you can do it in the code. test/jdk/java/awt/dnd/DragToAnotherScreenTest.java line 73: > 71: If the second label changes its text to drag me > 72: after the drop and you DO NOT see any error messages > 73: in the log area press PASS else FAIL. I think we can make it semi-automatic then. If the test determines an error condition, the test could fail automatically. However, to give a better experience for the tester, the test could display a message before calling `forceFail`. Another way could be disabling the `Pass` button, yet it's not possible at the moment. test/jdk/java/awt/dnd/RejectDragTest.java line 147: > 145: startPoint.translate(50, 50); > 146: endPoint.translate(150, 150); > 147: }); The same problem occurs here: evaluate the correct values for the fields of `startPoint` and `endPoint` and only then assign a reference to the volatile field. (As in `DragSourceMotionListenerTest.java`) test/jdk/java/awt/dnd/RejectDragTest.java line 169: > 167: > 168: public static int sign(int n) { > 169: return Integer.compare(n, 0); Actually, [the specification](https://docs.oracle.com/en/java/javase/21/docs/api/java.base/java/lang/Integer.html#compare(int,int)) does not guarantee only `0`, `1`, and `-1` are returned, yet OpenJDK implementation does return these three values only: https://github.com/openjdk/jdk/blob/f1bf469b4ee07b48b629a126111e307d3cab7fd7/src/java.base/share/classes/java/lang/Integer.java#L1408-L1410 We could use this, I guess. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/21213#discussion_r1781211627 PR Review Comment: https://git.openjdk.org/jdk/pull/21213#discussion_r1781247125 PR Review Comment: https://git.openjdk.org/jdk/pull/21213#discussion_r1781096791 PR Review Comment: https://git.openjdk.org/jdk/pull/21213#discussion_r1781110291 PR Review Comment: https://git.openjdk.org/jdk/pull/21213#discussion_r1781112427 PR Review Comment: https://git.openjdk.org/jdk/pull/21213#discussion_r1781127928 PR Review Comment: https://git.openjdk.org/jdk/pull/21213#discussion_r1781169609 PR Review Comment: https://git.openjdk.org/jdk/pull/21213#discussion_r1781174131 PR Review Comment: https://git.openjdk.org/jdk/pull/21213#discussion_r1781141616 PR Review Comment: https://git.openjdk.org/jdk/pull/21213#discussion_r1781162547 PR Review Comment: https://git.openjdk.org/jdk/pull/21213#discussion_r1781137066 PR Review Comment: https://git.openjdk.org/jdk/pull/21213#discussion_r1781137675 PR Review Comment: https://git.openjdk.org/jdk/pull/21213#discussion_r1781257340 PR Review Comment: https://git.openjdk.org/jdk/pull/21213#discussion_r1781261574 PR Review Comment: https://git.openjdk.org/jdk/pull/21213#discussion_r1781222555 PR Review Comment: https://git.openjdk.org/jdk/pull/21213#discussion_r1781233684
