<AWT Dev> [9] Review Request: 8035568 [macosx] Cursor management unification.

[Sergey Bylokhov]

Hi, Petr.
Is it possible in LWMouseInfoPeer that windowPeer == null?

On 7/17/14 4:03 PM, Petr Pchelko wrote:
> Hello,
>
> Thank you for the review, the new version is here:
> http://cr.openjdk.java.net/~pchelko/9/8035568/webrev.01/
>
> Mike wrote:
>> Has this change been tested on Retina? I don't see any effort to convert in and out of the screen coordinate system.
> Yes, it works fine on Retina. Where should we convert the coords?  CGEventGetLocation gives us coors in logical pixels and that's exactly what we need. Am I missing something?
>
> Anthony wrote:
>> 1. src/macosx/classes/sun/lwawt/LWMouseInfoPeer.java
>>>   55         // Most likely the cached window under cursor is correct and we do not need the native check.
>> Perhaps instead it would make sense to describe here when the first condition may fail and the native check could actually become useful? Otherwise the current comment doesn't add much value to understanding the code.
> Done. I've removed the non-native check as it might be wrong.
>
>> . src/macosx/native/sun/awt/AWTWindow.m
>>> -    AWT_ASSERT_APPKIT_THREAD;
>>>
>>> +    [ThreadUtilities performOnMainThreadWaiting:YES block:^{
>> This looks okay. But I'm wondering whether this could cause any dead locks potentially? I'd suggest to run other tests that may involve (maybe indirectly) calling the nativeGetTopmostPlatformWindowUnderMouse method (grab/ungrab? focus? modal dialogs? tooltips/popups? maybe something else).
> Normally nativeGetTopmostPlatformWindowUnderMouse is called on the Appkit thread. It's called from EDT only in isWindowUnderMouse function, and it's not used too much in out code.
>
>> 3. src/macosx/native/sun/awt/CCursorManager.m
>>> -    [ThreadUtilities performOnMainThreadWaiting:YES block:^(){
>> Is it OK to call Core Graphics functions on a thread other than the main thread?
> According to Apple, yes: "Quartz is thread safe on the whole, but individual Quartz objects are not. In general, you can operate on any object on any thread as long as you guarantee that no two threads are operating on the same object simultaneously. The easiest way to achieve this is to not share your objects between threads."
>
> With best regards. Petr.
>
> On 11 июля 2014 г., at 15:15, Anthony Petrov <anthony.petrov at oracle.com> wrote:
>
>> Hi Petr,
>>
>> The fix looks good to me overall. A few comments:
>>
>> 1. src/macosx/classes/sun/lwawt/LWMouseInfoPeer.java
>>>   55         // Most likely the cached window under cursor is correct and we do not need the native check.
>> Perhaps instead it would make sense to describe here when the first condition may fail and the native check could actually become useful? Otherwise the current comment doesn't add much value to understanding the code.
>>
>>
>> 2. src/macosx/native/sun/awt/AWTWindow.m
>>> -    AWT_ASSERT_APPKIT_THREAD;
>>>
>>> +    [ThreadUtilities performOnMainThreadWaiting:YES block:^{
>> This looks okay. But I'm wondering whether this could cause any dead locks potentially? I'd suggest to run other tests that may involve (maybe indirectly) calling the nativeGetTopmostPlatformWindowUnderMouse method (grab/ungrab? focus? modal dialogs? tooltips/popups? maybe something else).
>>
>>
>> 3. src/macosx/native/sun/awt/CCursorManager.m
>>> -    [ThreadUtilities performOnMainThreadWaiting:YES block:^(){
>> Is it OK to call Core Graphics functions on a thread other than the main thread?
>>
>> --
>> best regards,
>> Anthony
>>
>> On 7/8/2014 2:19 PM, Petr Pchelko wrote:
>>> Hello, AWT Team.
>>>
>>> Please review the fix for the issue:
>>> https://bugs.openjdk.java.net/browse/JDK-8035568
>>> The fix is available at:
>>> http://cr.openjdk.java.net/~pchelko/9/8035568/webrev.00/
>>>
>>> We are using 2 different methods for getting a cursor position in Robot and in LWAWT. I've changed our implementation to use Carbon method.
>>> nativeGetCursorPosition is a very hot method and changing it's implementation makes it run 35 times faster. Also we will never deadlock on it any more.
>>> However, I needed to change the isWindowUnderMouse implementation. The problem's that LWWindowPeer.windowUnderCursor is updated on mouse events
>>> and generated mouse events, so sometimes it may be not updated when called from a component resize handler. Luckily we can test it using native code.
>>> isWindowUnderMouse is not a hot method at all, in real apps it's called very rarely (never called after a couple of hours of real IDE usage) so it's not a problem that it
>>> runs slower now.
>>>
>>> I've run all cursor/mouse tests. A couple of tests failed because they didn't have proper synchronization and we are too fast for them now. I've fixed it and open-sourced the tests.
>>>
>>> With best regards. Petr.
>>>


-- 
Best regards, Sergey.

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/awt-dev/attachments/20140717/80f11df6/attachment-0001.html>