<AWT Dev> [9] Review Request: JDK-8037840 [macosx] Rewrite CWarning window to eliminate the ExecutorService

[Petr Pchelko]

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.petrov at 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.
>>