On Thu, 24 Feb 2022 23:53:04 GMT, Kevin Rushforth <k...@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
