[rfc][icedtea-web] PolicyEditor model refactor

[Andrew Azores]

On 05/29/2014 06:19 PM, Omair Majid wrote:
>> +++ b/netx/net/sourceforge/jnlp/security/policyeditor/CustomPolicyViewer.java
>> +    public CustomPolicyViewer(final PolicyEditor parent, final String codebase, final PolicyFileModel policyFile) {
>> +        this.parent = new WeakReference<>(parent);
>> +    private void updateCustomPermissions() {
>> +        parent.get().setChangesMade();
> This seems rather strange. You are storing a WeakReference and then
> calling get() and then operating on the result. Is there a reason you
> are not just keeping a normal reference to it?

I honestly can't remember what reasoning I had for it in the first 
place, and I can't come up with any reason for it now. Fixed.

>
>> +        customPermissions.addAll(policyFile.getCopyOfCustomPermissions().get(codebase));
> Is there a reason why you can't just have a `getCustomPermissions`
> method that returns an immutable version of data (preferred) or a copy
> of data rather than a separate `getCopyOfCustomPermisions`? Is there a
> case where you would want a `getCustomPermissions` method that returns a
> mutable non-copy?

It might come up that a mutable non-copy would be needed, although it 
isn't now. But it seems to me that that's a plausible method that could 
exist, so I gave the method a name that more explicitly specifies what 
you actually get back when you call it. Perhaps that's better done via 
javadoc? I think there deserves to be some indication that what comes 
back is a copy (immutable or not), so that anyone else working with this 
doesn't look at it and think that this would be a convenient shortcut to 
adding batches of permissions to the model.

>
>> +++ b/netx/net/sourceforge/jnlp/security/policyeditor/PolicyEditor.java
>> +                            cpViewer = new CustomPolicyViewer(weakThis.get(), codebase, policyFile);
> Why not `this` instead of `weakThis.get()`?

Well, in this context 'this' is actually an anonymous Runnable, not the 
PolicyEditor.

>
>> +        if (!policyFile.getFile().exists()) {
>> +            try {
>> +                policyFile.getFile().createNewFile();
> This sounds like a createOrGet() method that can be added to FileUtils.

Actually, taking a closer look, it's not really needed. createNewFile 
just returns false if the file already existed, so there's not much 
point to checking its existence beforehand.

>
>
>> +++ b/netx/net/sourceforge/jnlp/security/policyeditor/PolicyFileModel.java
>> +    private final Map<String, Map<PolicyEditorPermissions, Boolean>> codebasePermissionsMap = new HashMap<>();
> Can you add a comment about what the `Boolean` is for?

Sure, added.

>
>> +        fileWatcher = new MD5SumWatcher(file);
>> +        fileWatcher.update();
> Any reason you are continuing to use this rather than using the
> new-to-Java 7 WatchService?

I was planning to migrate to that eventually - do you mind if it waits 
for a later patch?

>
>> +        final FileLock fileLock = FileUtils.getFileLock(file.getAbsolutePath(), false, true);
> After you have acquired a lock, you should always free it, even in the
> face of exceptions. See if it's possible to use this idiom:
>
>      FileLock lock = FileUtils.getFileLock(...)
>      try {
>          ...
>      } finally {
>          lock.release();
>      }

Fixed.

>
>> +        sb.append("\n/* Generated by PolicyEditor at ").append(new SimpleDateFormat("yyyy-MM-dd HH:mm:ss").format(Calendar.getInstance().getTime())).append(" */").append(System.getProperty("line.separator"));
> Please split this into multiple lines.

Also fixed.

>
> Thanks,
> Omair
>

Thanks for the review! New patch attached. I'd probably recommend just 
vimdiff-ing it against the previous patch, or similar...

-- 
Andrew A

-------------- next part --------------
A non-text attachment was scrubbed...
Name: policyfilemodel-4.patch
Type: text/x-patch
Size: 48915 bytes
Desc: not available
URL: <http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20140530/01901fd2/policyfilemodel-4-0001.patch>