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

Tue Jan 28 14:29:20 PST 2014

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.

+                    } 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.

+    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