[rfc][icedtea-web] PolicyEditor model refactor

[Andrew Azores]

On 05/30/2014 10:25 AM, Omair Majid wrote:
> * 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.

I don't like that idiom either, hence why I'm not exposing any methods 
to allow it and am making it clear that the exposed methods can't be 
used for it ;)

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

Eh, sure. weakThis is already around and has valid uses (I think), but I 
can change this to use the direct reference as well. CustomPolicyViewer 
doesn't handle a null parent very well anyway.

New patch only makes the weakThis -> PolicyEditor.this change.

Thanks,

-- 
Andrew A

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