/hg/icedtea-web: Improve PolicyTool launch method in PolicyPanel

[Jacob Wisor]

On 01/27/2014 04:29 PM, Andrew Azores wrote:
> On 01/27/2014 09:39 AM, Jiri Vanek wrote:
> [...]
>>>> +        Class<?>[] signature = new Class<?>[] { String[].class };
>>>
>>> Redundant. No need to create a new instance here at run-time.
>>>
>>>> +        Method main = policyTool.getDeclaredMethod("main", signature);
>>>
>>> Just substitute "signature" with "String[].class".
>>>
>>>> +        Object args = new String[] { "-file", filePath };
>>>
>>> Why is "args" of type Object? String[] should be fine and Method.invoke()
>>> won't complain because
>>> String[] inherits from Object. ;-)
>>>
>> Especially this.. My overlook.
>
> Method#invoke has signature(Object, Object...) - the first being the object the
> method is to be invoked on, and the second being a varargs list for the method
> call. Yes, String[] inherits from Object, and it also confuses the varargs
> invocation. Hopefully this notation will make sense, but what I am doing here
> results in this method call being invoked:
>
> PolicyTool.main({"-file", filePath})
>
> and what you are suggesting results in this:
>
> PolicyTool.main("-file", filePath)
>
> These are not the same and in fact the second call will fail because there is no
> PolicyTool.main(String, String) method, so you get IllegalArgumentException:
> wrong number of arguments.

Oh I see. You avoid doing an explicit cast to Object later because the compiler 
is unsure whether an implicit cast from String[] to Object[] should be 
interpreted as a vararg (Object[]) or a formal parameter (Object). Since the 
compiler has been "automagically" creating Object[] arrays for varargs ever 
since then your code is okay.

> The "signature" variable is indeed not necessary but I'm really surprised that
> we're getting to the level of nitpicking on inlining variables here.
> Does this matter so much that it's worth patching?

This would just save some unnecessary operations on the stack. So, no probably 
not. For some it is just a matter of style.

But, you *should* probably change the call from Class.getDeclaredMethod() to 
Class.getMethod() because Class.getDeclaredMethod() assumes access to all, 
including non-public methods which causes a SecurityManager.checkMemberAccess(). 
This may lead to failure if the default SecurityManager is set. 
Class.getMethod() by definition accesses public members only hence it will not 
error out even with the default SecurityManager set (unless configured to deny 
access to public members). Or, has Class.getDeclaredMethod() been intentional too?

> Should we not be ensuring that we only invoke JOptionPane.showMessageDialog from
> the EDT? We have two different convenience methods that make this call, and
> *usually* these methods are all called from the main thread, but there is an
> occasion where we call showCouldNotOpenFileDialog from policyToolLaunchHelper,
> and this time it's occurring in a new thread. So we can either wrap this one
> particular call in an invokeLater, or just be completely sure that these two
> convenience methods are always used safely and put the invokeLater directly
> within them. In most cases, yes, it ends up being redundant, but it also means
> in every case it's done safely, doesn't it?
>
> Here's a small StackOverflow discussion I found while double checking myself on
> this.
>
> https://stackoverflow.com/questions/13863713/which-thread-to-launch-joptionpane

Oh, you want to make sure the call to JOptionPane.showMessageDialog() happens on 
the EDT for it to block. Then this is okay. Good job! :-) I have missed your 
intention because I assumed you just wanted to make sure IcedTea-Web's JFrame 
would process all events before getting to JOptionPane.showMessageDialog().

Jacob