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

Wed Jan 29 07:03:23 PST 2014

On 01/28/2014 05:29 PM, Jacob Wisor wrote:
> On 01/28/2014 07:45 PM, Andrew Azores wrote:
>> On 01/27/2014 05:50 PM, Jacob Wisor wrote:
>>> 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?
>>>
>>> Jacob
>>
>> No, to be honest I didn't put a whole lot of thought into which to 
>> choose here.
>> I don't think I had any particular reasoning for choosing 
>> getDeclaredMethod, it
>> just happened. This I think is a good enough change to warrant a new 
>> commit,
>> along with making other local variables and formal params final where 
>> they can
>> be (which is pretty much everywhere in this class), adding a class-level
>> Javadoc, and older PolicyTool package fallback.
>>
>> The backport patch is included here as well, because it also 
>> incorporates these
>> suggestions of course. Seems better to keep these two patches in the 
>> same place
>> since the code review between them should be just about identical 
>> (other than
>> the change to accommodate lack of DirectoryValidator, they are 
>> identical).
>
> Looks good to me. Just a few last nits:
>
> +        final JLabel aboutLabel = new JLabel("<html>" + 
> R("CPPolicyDetail") + "</html>");
> +
> +        final String fileUrlString = 
> config.getProperty(DeploymentConfiguration.KEY_USER_SECURITY_POLICY);
> +        final JButton showUserPolicyButton = new 
> JButton(R("CPButPolicy"));
> +        showUserPolicyButton.addActionListener(new 
> ViewPolicyButtonAction(frame, fileUrlString));
> +
> +        final String pathPart = 
> localFilePathFromUrlString(fileUrlString);
> +        showUserPolicyButton.setToolTipText(R("CPPolicyTooltip", 
> FileUtils.displayablePath(pathPart, 60)));
> +
> +        final JTextField locationField = new JTextField(pathPart);
> +        locationField.setEditable(false);
>
> This is not wrong but often, every time a UI class needs to receive 
> refactoring and some methods need to be added, developers end up 
> declaring references to components this class handles as fields. This 
> becomes necessary in order to share those components between methods. 
> Or, they need to be accessed after initialization. Sure, sometimes 
> they can be provided via formal parameters to those methods during 
> initialization, but sometimes they just can't, so the only way to 
> share them is to store their references in fields, hence double work. 
> This would not be a problem if components were easily unambiguously 
> retrievable via names or some other sort of keys from the current 
> object, but unfortunately this is not the case. So please consider 
> declaring aboutLabel, showUserPolicyButton, and locationField as 
> fields. I am aware that many other IcedTea-Web's UI classes have been 
> coded this way but this is not actually an example of best practice. I 
> am also aware that this coding technique helps to save some memory at 
> run-time but imho the increased maintainability of code greatly 
> outweighs any marginal memory savings it offers.
>

Eh, right now it's not needed at all since the purpose of the panel is 
really just to give a button for launching an editor. If some maintainer 
in the future needs the class refactored that way, then it shouldn't be 
much extra work to do it at that time to just extract those three 
variables into fields as needed. I don't disagree that it might be more 
maintainable if they were fields and that the memory savings this way 
are completely inconsequential, but I'd like to avoid having this patch 
end up in infinite review ;)

>
> +                    } catch (Exception e) {
>
> Hmm, I would honestly be more happy if we would be catching explicitly 
> named exceptions here.
>
> +                        e.printStackTrace();
> +                        showCouldNotOpenFileDialog(frame, filePath, 
> R("CPPolicyEditorNotFound"));
> +                    }
> +                }
> +            }
> +        }).start();
> +    }
> +
> [...]
> +    private static void reflectivePolicyToolLaunch(final String 
> filePath) throws Exception {
>
> Same goes here, throw explicit exceptions. But I do not insist.

Yea, if only we were using source level 7, then I could generalize this 
to ReflectiveOperationException. Unfortunately in 6 the best I can do is 
Exception. So it's either live with Exception or have four different 
handlers, each doing the exact same thing. This code is so completely 
non-mission critical that I think it's okay to catch Exception.

>
> +    private static String localFilePathFromUrlString(final String url) {
> +        try {
> +            final URL u = new URL(url);
> +            return u.getPath();
> +        } catch (MalformedURLException e) {
> +            return url;
>
> This should probably return null, an empty string, or even escalate 
> this MalformedURLException.
>
> Otherwise looks good.
>
> Jacob

I definitely don't think it's worth escalating the exception. This 
method is only being used to get text for a tooltip and the 
locationField... this is also why the exception handling is just to 
return some ugly-looking and/or maybe incorrect (in that it isn't a URL 
format) text rather than nothing at all.

Thanks,

-- 
Andrew A