[rfc][icedtea-web] PolicyEditor model refactor

[Omair Majid]

Hi Andrew,

* Andrew Azores <aazores at redhat.com> [2014-05-15 11:55]:
> I've refactored PolicyEditor so that the policy file model is now in a
> separate class, moving PolicyEditor toward MVC design. There still isn't
> really a separation between View and Controller though.

I am barely familiar with this code, but I have added my comments below.

> All the unit tests should still be passing, other than the two that are
> marked Known To Fail. I've also done fairly extensive manual testing and it
> still seems to work properly as far as I can tell.

Okay, then I assume things are working and will mostly pay attention to
other things.

> +++ 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?

> +        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?

> +++ b/netx/net/sourceforge/jnlp/security/policyeditor/PolicyEditor.java

> +                            cpViewer = new CustomPolicyViewer(weakThis.get(), codebase, policyFile);

Why not `this` instead of `weakThis.get()`?

> +        if (!policyFile.getFile().exists()) {
> +            try {
> +                policyFile.getFile().createNewFile();

This sounds like a createOrGet() method that can be added to FileUtils.


> +++ 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?

> +        fileWatcher = new MD5SumWatcher(file);
> +        fileWatcher.update();

Any reason you are continuing to use this rather than using the
new-to-Java 7 WatchService?

> +        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();
    }


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

Thanks,
Omair

-- 
PGP Key: 66484681 (http://pgp.mit.edu/)
Fingerprint = F072 555B 0A17 3957 4E95  0056 F286 F14F 6648 4681