On 12/23/2015 7:33 PM, Sergey Bylokhov wrote:
Hi, Semyon.
Thanks for review, see my comments inline.
On 23/12/15 18:37, Semyon Sadetsky wrote:
1. That would be nice to test that modifiers are exactly as expected to
ensure that no extra modifier bits are set.
Yes, but according to the spec it is not guaranteed that some other
modifiers are set or not, so I just check the modifiers which must be
set in this case only.
But I would interpret that as an issue if button 1 is pressed but the
modifiers contains button 2 or 3 as well for example. Since there
complex mapping between modifiers this risk takes place. It is a very
tiny improvement for your test. Please...
2. Test summary and author are omitted
I am not sure what I can add to the summary, other than information
which already existed in the bug description. The author tag is
undesired in the openjdk and its usage is not strictly necessary.
We use to add those fields usually but I don't see any obstacles to make
the test anonymous. :)
3. In the test you are creating a new window for each button. That is
not optimal for test execution time.
It is done intentionally to skip any side effects caused by the
previous keyup/keydown, so each time it is new frame, texteditor,
listeners etc.
Hmm... What side effects you'd expect after a single key press?
4. Test fails on Linux, but you've filed a separate bug already. That
should be ok.
--Semyon
On 11/18/2015 6:50 PM, Sergey Bylokhov wrote:
Hello.
Please review the fix for jdk9.
On macosx when we create a KeyEvent we ignore the mouse state of all
mouse buttons, which means that mouse modifiers are missing.
The only KeyEvent is affected, because when we convert NS modifiers to
java modifiers we use nsToJavaKeyModifiers(), but in all other cases
we use nsToJavaMouseModifiers() which is the same but adds mouse
modifiers.
In the fix:
- The button parameter of nsToJavaMouseModifiers() was removed because
it was unused.
- nsToJavaMouseModifiers() was renamed to nsToJavaModifiers() and now
is used when keyEvent is generated.
- nsToJavaKeyModifiers() was removed because it unused now.
The similar bug for extended(id>=3) mouse buttons was filed for linux:
https://bugs.openjdk.java.net/browse/JDK-8143240
Bug: https://bugs.openjdk.java.net/browse/JDK-8143054
Webrev can be found at:
http://cr.openjdk.java.net/~serb/8143054/webrev.00
