RFR: 8278759 : PointerEvent: buttons property set to 0 when mouse down [v2]

[Hima Bindu Meda]

On Thu, 24 Feb 2022 23:53:04 GMT, Kevin Rushforth <kcr at openjdk.org> wrote:

>> Hima Bindu Meda has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Refactor variable names and update the formatting
>
> modules/javafx.web/src/main/java/com/sun/webkit/WebPage.java line 844:
> 
>> 842:                                                 //intermediate mouse events that can change text selection.
>> 843:                 && twkProcessMouseEvent(getPage(), me.getID(),
>> 844:                                         me.getButton(), me.getButtons(), me.getClickCount(),
> 
> I wonder if `buttonMask` would be a better (more descriptive) name on the Java side, and consequently, in the JNI call? Within WebKit, using `buttons` makes sense, since that's what they call it. This is just a suggestion. I don't mind if you leave it as is.

Agree.Updated the name to "buttonMask" and refactored accordingly.

> modules/javafx.web/src/main/java/javafx/scene/web/WebView.java line 1089:
> 
>> 1087:         }
>> 1088:         final int buttonMask = ((ev.isPrimaryButtonDown() ? WCMouseEvent.BUTTON1 : WCMouseEvent.NOBUTTON) | (ev.isMiddleButtonDown() ? WCMouseEvent.BUTTON2 : WCMouseEvent.NOBUTTON) |
>> 1089:                                     (ev.isSecondaryButtonDown() ? WCMouseEvent.BUTTON3 : WCMouseEvent.NOBUTTON));
> 
> Minor: I would wrap this so each of the three bits is on its own line (so the first line isn't so long). Also, the indentation should either be 8 spaces or line up with the RHS expression on the previous line.

Formatted as per comment and aligned. Please review.

> modules/javafx.web/src/main/native/Source/WebCore/platform/PlatformMouseEvent.h line 48:
> 
>> 46:     enum SyntheticClickType : int8_t { NoTap, OneFingerTap, TwoFingerTap };
>> 47: #if PLATFORM(JAVA)
>> 48:     enum MouseButtonPressed : uint8_t { NoButtonPress = 0, LeftButtonPress, RightButtonPress, MiddleButtonPress = 4 };
> 
> Do you think `MouseButtonMask`, `LeftButtonMask`, etc., would be better names than `...Pressed`?

As the w3c spec also mentions that buttons gives the current state of the device buttons as a bitmask, MouseButtonMask is more suitable.Made the changes accordingly.

> tests/system/src/test/java/test/robot/javafx/web/PointerEventTest.java line 89:
> 
>> 87: 
>> 88:     private static CountDownLatch loadLatch;
>> 89:     private static CountDownLatch startupLatch;
> 
> I would reverse the order here, since startup latch is triggered first. Alternatively, you can use a single latch and initialize it to 2.

Used single latch and initialised to 2.

> tests/system/src/test/java/test/robot/javafx/web/PointerEventTest.java line 128:
> 
>> 126:     }
>> 127: 
>> 128:     public String mouseButtonDrag(MouseButton... button) {
> 
> I recommend changing the name of the parameter to `buttonArray` (or `buttonArr` or just `buttons` if you want it shorter), since this varargs parameter is an array of potentially more than one button.

changed to buttons

> tests/system/src/test/java/test/robot/javafx/web/PointerEventTest.java line 150:
> 
>> 148:         });
>> 149: 
>> 150:         return buttonMask;
> 
> if you move the `Integer.parseInt` from the caller to here (and change the return type to `int`) it will simplify the callers a bit. Just a suggestion.

changes done as per the comment.

> tests/system/src/test/java/test/robot/javafx/web/PointerEventTest.java line 156:
> 
>> 154:     public void testLeftButtonDrag() {
>> 155:         int result = Integer.parseInt(mouseButtonDrag(MouseButton.PRIMARY));
>> 156:         Assert.assertEquals(LEFT_BUTTON_DRAG,result);
> 
> Minor: there should be a space after the `,`. Also, you might consider using a static import for `assertEquals`.
> 
> This applies to all of the test methods.

done.

> tests/system/src/test/java/test/robot/javafx/web/PointerEventTest.java line 201:
> 
>> 199:         new Thread(() -> Application.launch(TestApp.class, (String[])null)).start();
>> 200:         waitForLatch(loadLatch, 10, "Timeout waiting for loading webpage().");
>> 201:         waitForLatch(startupLatch, 15, "Timeout waiting for FX runtime to start");
> 
> Since `startupLatch` will trigger first, you should either switch the two calls, or use a single latch (initialized to 2) with a single call to `waitForLatch`.

Added single latch initialised to 2.

> tests/system/src/test/java/test/robot/javafx/web/PointerEventTest.java line 215:
> 
>> 213:     public void resetTest() {
>> 214:         Util.runAndWait(() -> {
>> 215:             robot.mouseRelease(MouseButton.PRIMARY,MouseButton.MIDDLE,MouseButton.SECONDARY);
> 
> Minor: add a space after each `,`

done.

-------------

PR: https://git.openjdk.java.net/jfx/pull/742