[rfc][icedtea-web] PolicyEditor model refactor

[Andrew Azores]

On 05/30/2014 11:26 AM, Andrew Azores wrote:
> 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,
>

Oops, sorry. There was also an imports change in FileUtils that was 
meant to be cleaned first. That won't be included if/when this is pushed.

Thanks,

-- 
Andrew A