[rfc][icedtea-web] PolicyEditor

[Andrew Azores]

On 02/13/2014 05:14 PM, Andrew Azores wrote:
> On 02/13/2014 08:55 AM, Jiri Vanek wrote:
>
>>
>> Hi!
>>
>> I have no general issues with this.Considering it was done n best mind;)
>> After reading it, I'm against making it separate project. I would 
>> much ratehr keep it in itew.. and suggestion .. add new luncher? next 
>> to itw-settings javaws.. SimplePolicyEditor?
>
> I didn't get around to that with this patch, but I'll work on that 
> next. Either a new launcher beside itweb-settings and javaws, or a 
> standalone launcher as a separate project... ;) I really do think this 
> idea has some merit. Maybe the editor seems small and simple enough 
> now to stay within ITW, but I think it's already got enough utility to 
> be a replacement for PolicyTool for a lot of users. Most of whom 
> probably are using IcedTea-Web anyway, but not necessarily.
>

Launcher added as well. I think I did it right? Please carefully check 
that I didn't miss any steps or make any errors though.

>> Onemore think I noted After I click the send - yo are allowing the 
>> app to open multiple simple editor windows in time. This may be 
>> dangerous. - maybe some check "underlying file have changed, reload?)
>> (but yah, advaced tool do the same )I'm for -
>>  - have it as modal dialogue in itw-settings
>>  - have it on frame in case of seaprate application (see suggestion 
>> in previous email)
>>  -  otherwse do as you wish ::)
>
> Hmm. I don't think I like the idea of making it modal in this 
> situation (just seems like it is not necessary, and unnecessary 
> modality is really irritating to deal with as a user IMO), but "file 
> has changed, reload/ignore/quit" does sound good. I will look into 
> that next. Besides, even if we make it modal, that doesn't actually 
> protect us against concurrent modification very much. Perhaps 
> itweb-settings control panel can be made to handle the PolicyEditor in 
> the same way that PolicyEditor handles CustomPolicyViewer - if there 
> is no instance, create one. If there is an instance, focus it rather 
> than creating another.

I chose this approach after all. The PolicyPanel will bring the existing 
PolicyEditor to front, rather than making a new one. I think it works 
quite well.

I wanted to implement a "underlying file has changed, 
reload/ignore/quit" type of check, but it seems like this was only made 
into a standard library feature in Java 7, and I didn't see anything in 
our own utilities to help with it. I'd rather avoid adding a worker 
thread to poll the file or something ugly like that as well, but I 
suppose it would be doable. Apache Commons VFS can provide this, but I 
don't know if we want to pull in that dependency just for this.

>
> - "parsing"/"deserializing" is not the most robust. I haven't tested 
> it with random garbage malformed text or with comments in the middle 
> of a block, etc. These hardening improvements will come a bit later 
> on, right now I want to nail down the functionality/visuals. I may end 
> up refactoring the models even further, so hardening it now might be a 
> waste of effort

Comments mostly work, but if you have a line like this:

/* comment */ permission somepermission ... ;

then the whole thing is considered a comment. I think this is okay for a 
small, "lightweight" editor like this, but maybe eventually it will gain 
a proper parser (rather than the simple line-by-line "parsing" it does 
now) to be able to handle this. Double-slash type comments are handled a 
bit more nicely. Similarly, it doesn't properly handle multiple 
permissions on one line:

permission permissionA ...; permission permissionB ...;

and for now this will just have to be "good enough" I think. Anyway, 
this would only happen if someone edited their policy file by hand and 
did this.

>
> I'm planning to add plenty of unit testing to the models introduced 
> with this patch, but I'm out of time today and am taking a short day 
> tomorrow, so I'm not sure if I'll get those done before the end of the 
> week, and I wanted to get this out for at least visuals feedback first.
>
> Thanks,
>

- More unit tests were added, for the new models
- Fixed some bugs with file loading and UI updating. Still, sometimes 
the list view looks strange after a load, as if the topmost list element 
is mostly above the list and can't be scrolled into view, and I'm not 
sure what to do about that. Adding a new codebase forces the list to 
update and it fixes itself at least. The topmost element is always "All 
Applets" anyway so that's not too bad.
- FileUtils.getFileLock() usage was also added where the policy file is 
loaded and saved.
- Launching policyeditor from the command line with exactly one arg is 
interpreted as being meant as a file path, since it's a little annoying 
to use the -file flag when this is also the only flag the program even 
accepts other than -help. The -file flag remains in use simply because 
policytool has it and I'd like to keep that option compatible.

Still OK for head?

Thanks,

-- 
Andrew A

-------------- next part --------------
A non-text attachment was scrubbed...
Name: policy-editor-5.patch
Type: text/x-patch
Size: 130337 bytes
Desc: not available
Url : http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20140219/cb72f7fd/policy-editor-5-0001.patch