Hi Manajit,
The fix looks good to me.
With Regards,
Avik Niyogi
> On 14-Mar-2016, at 2:42 pm, Manajit Halder <manajit.hal...@oracle.com> wrote:
>
> Hi Semyon & Avik,
>
> Thanks to both of you for the review.
>
> Please review the modified the code.
> http://cr.openjdk.java.net/~arapte/manajit/8149456/webrev.02/
> <http://cr.openjdk.java.net/~arapte/manajit/8149456/webrev.02/>
>
> Regards,
> Manajit
>
>
>> On 14-Mar-2016, at 11:38 am, Semyon Sadetsky <semyon.sadet...@oracle.com
>> <mailto:semyon.sadet...@oracle.com>> wrote:
>>
>> Hi Manajit,
>>
>> The fix itself looks good.
>> Several minor remarks:
>> - In the test setting GridBagLayout to the frame seems meaningless.
>> - The year in the files header should be 2016.
>> - For the future, add "<AWT dev> [9]" in the beginning of review subject.
>>
>> --Semyon
>>
>> On 3/9/2016 4:43 PM, Manajit Halder wrote:
>>> Hi All,
>>>
>>> Please review webrev.01 updated with Avik’s review comments.
>>>
>>> http://cr.openjdk.java.net/~arapte/manajit/8149456/webrev.01/
>>> <http://cr.openjdk.java.net/%7Earapte/manajit/8149456/webrev.01/>
>>>
>>> Thanks,
>>> Manajit
>>>
>>>
>>>> On 09-Mar-2016, at 3:18 pm, Avik Niyogi <
>>>> <mailto:avik.niy...@oracle.com>avik.niy...@oracle.com
>>>> <mailto:avik.niy...@oracle.com>> wrote:
>>>>
>>>> Hi Manajit,
>>>>
>>>> Following are my inputs:
>>>> CRobot.m : Line 359 : retval not needed
>>>> alphabetical objC header list convention not followed. Line 36 should be
>>>> placed at line 32
>>>> CRobotKeyCode.m : Line 171: retval not needed. use else block for cases
>>>> undefined.
>>>> Use non-Swing implementations for test case.
>>>>
>>>> With Regards,
>>>> Avik Niyogi
>>>>> On 09-Mar-2016, at 2:52 pm, Manajit Halder <manajit.hal...@oracle.com
>>>>> <mailto:manajit.hal...@oracle.com>> wrote:
>>>>>
>>>>> Hi All,
>>>>>
>>>>> Kindly review the fix for JDK9.
>>>>>
>>>>> Bug:
>>>>>
>>>>> <https://bugs.openjdk.java.net/browse/JDK-8149456>https://bugs.openjdk.java.net/browse/JDK-8149456
>>>>> <https://bugs.openjdk.java.net/browse/JDK-8149456>
>>>>>
>>>>> Webrev:
>>>>>
>>>>> <http://cr.openjdk.java.net/%7Earapte/manajit/8149456/webrev.00/>http://cr.openjdk.java.net/~arapte/manajit/8149456/webrev.00/
>>>>> <http://cr.openjdk.java.net/~arapte/manajit/8149456/webrev.00/>
>>>>>
>>>>> Issue:
>>>>> [macosx] robot.keyPress do not generate key events (keyPressed and
>>>>> keyReleased) for function keys F13 to F19.
>>>>>
>>>>> Cause:
>>>>> robot.keyPress(keyCode) passes java key code for the particular key
>>>>> pressed to native side (MacOS X). On the MacOS corresponding key code
>>>>> matched and returned to java side. But as per the current implementation
>>>>> keys with java key code below 222 returns a valid key code form MacOS X
>>>>> and for all java key codes above 222 MacOS don’t have any valid key code
>>>>> to return and it returns a default value 127 (undefined) for all of them
>>>>> including function keys F13 to F20. The java key codes for keys F13 to
>>>>> F19 is in range 61440 to 61447.
>>>>>
>>>>> Fix:
>>>>> Create a dictionary of java keys and OS X keys with:
>>>>> key: java key
>>>>> value: OS X key
>>>>>
>>>>> Use the macros defined as macros in header java_awt_event_KeyEvent.h for
>>>>> java keys
>>>>> Define integer constants for OS X keys as shown below:
>>>>>
>>>>> const static int OSX_kVK_ANSI_A = 0x00;
>>>>> const static int OSX_kVK_ANSI_S = 0x01;
>>>>> const static int OSX_kVK_ANSI_D = 0x02;
>>>>> const static int OSX_kVK_ANSI_F = 0x03;
>>>>>
>>>>> Problem in modifying/extending current fix:
>>>>> Why the current fix can’t be extended/modified to support keys F13 to
>>>>> F20.
>>>>>
>>>>> 1) The current fix creates an array with array index as java key and
>>>>> array value as corresponding OS X key.
>>>>> static const unsigned char javaToMacKeyCode[] = {
>>>>> 127, // 0 0 VK_UNDEFINED No_Equivalent
>>>>> 127, // 1 0x1 Not_Used
>>>>> 127, // 2 0x2 Not_Used
>>>>>
>>>>> Here 0, 1 and 2 are the java keys and 127, 127 and 127 are the
>>>>> corresponding key values in OS X for the same key.
>>>>> The array can’t be modified to add support for F13 to F20 because their
>>>>> corresponding values are 61440 to 61447.
>>>>> It won’t be a good solution to extended the array with index 61440 to
>>>>> 61447.
>>>>>
>>>>> 2) The current fix creates an array using a pearl script. The pearl
>>>>> script uses hardcoded values of java keys and OS X keys.
>>>>> Also the Pearl script is not checked in to the java codebase.
>>>>>
>>>>> 3) The current fix not modifiable and not readable.
>>>>>
>>>>> Time comparisons between with and without fix in milliseconds:
>>>>> The time comparison is done by taking the time diff in milliseconds
>>>>> before and after calling robot keypress for all the keys using the test
>>>>> code written for the fix.
>>>>> Time diff taken for 5 test runs:
>>>>>
>>>>> With fix: 1658, 1698, 1705, 1453 and 1516
>>>>> Without fix: 1523, 1579, 1690, 1648 and 1504
>>>>>
>>>>> Note:
>>>>> The following java keys are not matched in the current fix because of
>>>>> the following reasons:
>>>>> 1) They are already matched with their corresponding keys on
>>>>> Mac OS.
>>>>> 2) There are no defined keys on Mac OS with the same
>>>>> functionality for PRINTSCREEN, SCROLL LOCK and PAUSE.
>>>>> 3) The following java keys are not defined in keyTable in
>>>>> AWTEvent.m file.
>>>>>
>>>>> java_awt_event_KeyEvent_VK_PRINTSCREEN
>>>>> java_awt_event_KeyEvent_VK_SCROLL_LOCK
>>>>> java_awt_event_KeyEvent_VK_PAUSE
>>>>>
>>>>> In the previous fix these keys were matched to OSX_F14, OSX_F13 and
>>>>> OSX_F15 respectively.
>>>>>
>>>>>
>>>>> Regards,
>>>>> Manajit
>>>>>
>>>>
>>>
>>
>
