Hello, Artem. Thank you for the review, the updated version of the fix is available here: http://cr.openjdk.java.net/~pchelko/8006634/webrev.02/
With best regards. Petr. On Feb 18, 2013, at 2:52 PM, Artem Ananiev wrote: > Hi, Petr, > > a few comments about the fix: > > 1. Having two ctors in CToolkitThreadBlockedHandler seems to be redundant. > One one is used in the current code. > > 2. LWCToolkit.flushPendingEventsOnAppkit() is used in a single place, in > CPlatformWindow. Could we get rid of this method completely and call > LWCToolkit.invokeAndWait() from CPlatformWindow directly? > > 3. LWCToolkit.invokeAndWait(Runnable, Component): invocation event, created > in this method, must have "component" as the source, not the toolkit instance. > > Thank you for such great refactoring and cleanup! > > Artem > > On 2/14/2013 6:27 PM, Petr Pchelko wrote: >> Thank you, Sergey. >> >> Please have a look at the updated version of the fix at: >> http://cr.openjdk.java.net/~pchelko/8006634/webrev.01/ >> >>> I suggest do not remove LWCToolkit.flushPendingEventsOnAppkit and reuse it >>> in CDropTargetContextPeer.flushEvents Is it possible? >> Done. >> >>> Can you please take a look to this >>> http://java.net/jira/browse/MACOSX_PORT-768 Looks like the workaround from >>> those issue is not necessary now. >> There is another bug: http://bugs.sun.com/view_bug.do?bug_id=7199783 The >> changes in CCursorManager will be made in the fix for that bug. >> >>> - It would be really helpful to add additional doce about runloops. >> I have extended the documentation on the doAWTRunLoop method a bit, but I >> have now idea what else to write there. >> >>> The test will be welcome, because this CR fix the possible deadlock, when >>> we use performOnMT inside DND . >> The nested loop is run to synchronously process SunDropTargetEvents. There >> is no API to set a listener on this event without changing the JDK code. >> When these events are processed, the new event (DropTargetEvent) is created >> which is that dispatched asynchronously. So I did not find any way to make a >> deadlock occur without hacking the JDK code. >> >>> Synchronisation in CToolkitThreadBlockedHandler is not necessary? >> It extends a mutex, so the client is responsible for the synchronization. >> This is the same behavior as on other platforms. >> >>> There are some changes related to VoiceOver could you please check that it >>> works? >> Yes, I've tried it for a bit, it seems to work. >> >>> I'm not sure but looks like stopAWTRunLoop should stop runloop >>> synchronously since after that we release CFRelease(mediatorObject). Is it >>> possible that when we call endRunLoop the object will be released? >> The mediator object is retained two times, because it is used by 2 threads. >> Than it is released first time in endRunLoop and second time in the end of >> doAWTRunLoop, so it can't be already released when we end a runloop. There >> is no difference if we run stopRunLoop with waiting YES or NO, it is done on >> the main thread to avoid using @syncronized on a BOOL value. Making access >> to it @synchronized is about 10 times slower for some reason) >> >> With best regards. Petr. >> >> On Feb 14, 2013, at 1:18 PM, Sergey Bylokhov wrote: >> >>> Hi, Petr. >>> From the quick review: >>> - I suggest do not remove LWCToolkit.flushPendingEventsOnAppkit and reuse >>> it in CDropTargetContextPeer.flushEvents Is it possible? >>> - Can you please take a look to this >>> http://java.net/jira/browse/MACOSX_PORT-768 Looks like the workaround from >>> those issue is not necessary now. >>> - It would be really helpful to add additional doce about runloops. >>> - The test will be welcome, because this CR fix the possible deadlock, when >>> we use performOnMT inside DND . >>> - Synchronisation in CToolkitThreadBlockedHandler is not necessary? >>> - There are some changes related to VoiceOver could you please check that >>> it works? >>> - I'm not sure but looks like stopAWTRunLoop should stop runloop >>> synchronously since after that we release CFRelease(mediatorObject). Is it >>> possible that when we call endRunLoop the object will be released? >>> >>> 14.02.2013 10:58, Petr Pchelko wrote: >>>> Hello, AWT Team. >>>> >>>> Please review the fix for the issue: >>>> http://bugs.sun.com/view_bug.do?bug_id=8006634 >>>> The fix is available at: >>>> http://cr.openjdk.java.net/~pchelko/8006634/webrev.00/ >>>> >>>> This fix makes a lot of changes in nested event loops: >>>> >>>> 1. CToolkitThreadBlockedHandler was made a subclass of Mutex as it is made >>>> on other platforms. Shared code relies on that with logic for >>>> entering/exiting nested event loops. Previously it did not matter as >>>> CToolkitThreadBlockedHandler did not make a actual nested loop. Now it >>>> makes a native nested loop via doAWTRunLoop. >>>> >>>> 2. doAWTRunLoop method is changed >>>> a. Now we have a boolean flag processEvents. If it is set to false, we >>>> do not process native events while in the nested loop, same as how it was >>>> done before the fix. If it is set to true, events are processed while in >>>> the loop. Attention is needed when using this new feature, because we >>>> short-circuit native event processing and there could be some cases when >>>> it breaks Appkit. (resizing windows with a mouse could be an example) >>>> However we need this feature in the Drag and Drop. >>>> b. I have got rid of an feature to run the nested loop in >>>> NSDefaultRunLoopMode, it was never used. >>>> >>>> 3. We decided that we are getting rid of the deadlock detection mechanism: >>>> it is rarely used, nobody have ever seen the error message from it. I have >>>> tested a lot without this feature and it looks like everything is fine, >>>> however I am absolutely not sure about this decision. I am including Peter >>>> and Ragini as Accessibility was using this functionality and I had to >>>> change Accessibility code a bit. >>>> >>>> 4. Some native methods from the LWCToolkit were made package-private. The >>>> comment stated that deploy used them, but I've cloned there code and they >>>> apparently do not use it any more. Looks like the comment is from Apple >>>> times and we do not event have a WebkitPluginObject any more. >>>> >>>> With best regards. Petr. >>> >>> >>> -- >>> Best regards, Sergey. >>> >>
