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

[Jiri Vanek]

On 01/27/2014 04:29 PM, Andrew Azores wrote:
> On 01/27/2014 09:39 AM, Jiri Vanek wrote:
>> ...
>>>> +     * @param filePath a {@link String} representing the path of the file to attempt to open
>>>> +     * @throws Exception if any sort of exception occurs during reflective launch of policytool
>>>> +     */
>>>> +    private static void reflectivePolicyToolLaunch(final String filePath) throws Exception {
>>>> +        Class<?> policyTool = Class.forName("sun.security.tools.policytool.PolicyTool");
>>>
>>> What about JRE 6? You could catch a ClassNotFoundException here and then try to get
>>> sun.security.tools.PolicyTool. And, if that fails, well then... Blow up! :-D
>>
>> Well - why yes? The jdk6 is dead. We are dealing with slowly, but its true. The command exec is
>> what matters. The second one is fallback.
>>
>> If you have non-policytool command, non jdk7 system. Please go on and fix. Otherwise I do not
>> believe it is worthy. And if we  will nit pick -what about gnu classpath ? ;)
>>
>> Maybe some  better error message then
>>      JOptionPane.showMessageDialog(frame, message, R("Error"), JOptionPane.ERROR_MESSAGE); ?
>>
>> except that I agree with all what Jacob  pointed out.
>
> What better error message are you looking for? If both launch methods fail, the error message says
> something like "could not find system policy editor. Check that policytool is in your PATH".

aaaa... yyy. I was confised by
   showCouldNotOpenFileDialog(frame, filePath, R("CPPolicyEditorNotFound"));
and  private static void showCouldNotOpenFileDialog(final JFrame frame, final String filePath, final 
String message) {
and  JOptionPane.showMessageDialog(frame, message, R("Error"), JOptionPane.ERROR_MESSAGE);

When the filePath was unused, I thougth you are jsut showing "error".

Sorry.

>
>>>
>>>> +        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.
>
> 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?
>
>>
>> And yes - the  invoke later around the  joptionpane is redundant.
>>
>> Thanx jacob!
>>
>> >
>>
>>
>>
>>
>> J.
>>
>
> 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

Ooh thats interesting. I will need to revisit all the bilion of joption dialogues which I ever 
dislpalyed. The code which shows it really works outside of EDT...

So ok from me...
>
> Thanks,
>
Thanks on my siede.