[rfc][icedtea-web] PolicyEditor model refactor

[Omair Majid]

* Andrew Azores <aazores at redhat.com> [2014-05-30 10:09]:
> >>+        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.

In general, I find the idiom of "get a return value, modify that to
update the object that returned it" a bad idea because it spreads state
around in the codebase. It's best avoided. Anyway, please do consider
(eventually) fixing this.

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

You can use a slightly different syntax to get hold of an outer class
from an inner one: `NameOfParentClass.this`. In this case, it's:

  PolicyEditor.this

Thanks,
Omair

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