Hello, Anthony. I’ve updated the patch: http://cr.openjdk.java.net/~pchelko/9/8037840/webrev.02/
Now the CancelableFuture methods are final. With best regards. Petr. 19 марта 2014 г., в 8:49 после полудня, Anthony Petrov <anthony.pet...@oracle.com> написал(а): > Is there a reason to not declare the @Override CancelableRunnable.run() as > final? Allowing descendants to override it may result in violation of the > cancelable contract. > > -- > best regards, > Anthony > > On 3/19/2014 7:21 PM, Petr Pchelko wrote: >> Hello, Anthony. >> Thank you for the review. >> >>> src/macosx/native/sun/awt/LWCToolkit.m: >>> I'd like to see the NewGlobalRef/DeleteGlobalRef calls to be more balanced. >>> Both should be performed either in the utility @interface, or in the JNI >>> method (I'd prefer the former). >> Unfortunately that's impossible. We must call the NewGlobalRef on the >> calling thread (in the JNI method), while DeleteGlobalRef could only be >> called after the task was executed (either in perform or in dealloc). >> Dealloc looks better for me as it's a common pattern to release resources in >> dealloc. >> >> Other comments are addressed in the new version of the fix: >> http://cr.openjdk.java.net/~pchelko/9/8037840/webrev.01/ >> >> With best regards. Petr. >> >> On 19.03.2014, at 18:58, Anthony Petrov <anthony.pet...@oracle.com> wrote: >> >>> Hi Petr, >>> >>> src/macosx/native/sun/awt/LWCToolkit.m: >>> I'd like to see the NewGlobalRef/DeleteGlobalRef calls to be more balanced. >>> Both should be performed either in the utility @interface, or in the JNI >>> method (I'd prefer the former). >>> >>> src/macosx/classes/sun/lwawt/macosx/CWarningWindow.java >>> From Object-Oriented Design perspective, the CancelableRunnable doesn't >>> enforce the Cancelable contract. I'd suggest to redesign this class as >>> follows (pseudo-code): >>> >>> CancelableRunnable { >>> private boolean flag; >>> public abstract void perform(); >>> >>> final run() { >>> if (flag) { >>> perform(); >>> } >>> } >>> } >>> >>> and the implementation classes would only ever override the perform() >>> method. This way we hide the flag from descendant classes, and also ensure >>> the correct behavior of any descendant class after its cancel() method is >>> invoked. >>> >>> Also note the single 'l' in Cancelable. There's already CancelablePrintJob >>> class in JDK, so I'd suggest to keep using the same flavor of English. >>> >>> -- >>> best regards, >>> Anthony >>> >>> On 3/19/2014 6:15 PM, Petr Pchelko wrote: >>>> Hello, AWT Team. >>>> >>>> Please review the fix for the issue: >>>> https://bugs.openjdk.java.net/browse/JDK-8037840 >>>> The fix is available at: >>>> http://cr.openjdk.java.net/~pchelko/9/8037840/webrev/ >>>> >>>> This is another fix for the SecurityWarning window. The problem is that we >>>> create a new Java thread to animate the warning icon. And we have a thread >>>> per icon. >>>> That's an overkill and a waste of resources. The idea of this fix is to >>>> use Appkit to schedule delayed tasks. >>>> >>>> You might wonder, why I'm rewriting the code I've just fixed in >>>> JDK-8037776. The idea is that JDK-8037776 is a completely safe fix to >>>> backport it to 8u20. This fix is a bit more risky, so it will stay in 9 >>>> without a backport to 8u20. >>>> >>
