[rfc][icedtea-web] multiple remember actions

[Jiri Vanek]

On 04/29/2014 09:15 PM, Andrew Azores wrote:
> On 04/24/2014 04:36 AM, Jiri Vanek wrote:
>> Hi!
>>
>> Based on previous two enhancements - [rfc][icedtea-web] replace jlabel by  jeditorpane and  Re:
>> [rfc][icedtea-web] conditional disabling of manifes tattributes
>>
>> this is enabling to save matching alaca action
>>  - benefits: one file, one  lock , one id, one table, one metadata and unlimited number of actions
>>  - drawbacks - suspiciously simply, maybe not so transparent, once more columns will be added, new
>> editing
>>
>> The patch grown due to changed signatures of methods. I will try to summarize:
>>
>>
>> * netx/net/sourceforge/jnlp/controlpanel/UnsignedAppletActionTableModel.java"
>>  - original columnrnemaed to "Unsigned applet action" added "Library action" column
>>  - the value of those two columns is get from source.getAppletSecurityActions().* methods
>>
>> * netx/net/sourceforge/jnlp/controlpanel/UnsignedAppletsTrustingListPanel.java
>>  - adapted "toString" method
>>  - added unset to list of combobox values for first two columns cell editors
>>  - filter of  "byValue" are now selecting via *or* - if any of action have the desired value, then
>> it is true
>
> See AppletSecurityActions comment about Iterable - if that advice is taken then it can be used here
> already.
>
> Rather than:
> +                boolean r = false;
> +                for (int i = 0; i < AppletSecurityActions.REMEBER_COLUMNS_COUNT; i++) {
> +                    ExecuteAppletAction o = (ExecuteAppletAction)
> entry.getModel().getValueAt(entry.getIdentifier(), i);
> +                    r = r || (o.equals(ExecuteAppletAction.ALWAYS) ||
> o.equals(ExecuteAppletAction.NEVER));
> +                }
> +                return r;
>
> perhaps:
>
> +                for (int i = 0; i < AppletSecurityActions.REMEBER_COLUMNS_COUNT; i++) {
> +                    ExecuteAppletAction o = (ExecuteAppletAction)
> entry.getModel().getValueAt(entry.getIdentifier(), i);
> +                    if (o.equals(ExecuteAppletAction.ALWAYS) || o.equals(ExecuteAppletAction.NEVER)) {
> +                        return true;
> +                    }
> +                }
> +                return false;
>
> but this is mostly just stylistic. In the first case the parens around the second disjunction are
> also not really needed :)
>
>>
>>
>> * netx/net/sourceforge/jnlp/resources/Messages.properties"
>>  - enabled date in SUnsignedAllowedBefore and SUnsignedRejectedBefore - thsi was actualy doone in
>> ocde. I do not know why this was forgotten)
>>  - removed SAppletTitle - it was nasty :)
>>  - added nnew name of column 1 and new name fornew column 2
>
> "Missing Resource: APPEXTSECguiTableModelTableColumnActionUB" ?
>
>>
>> * netx/net/sourceforge/jnlp/resources/Messages_cs.properties
>>  also removed  SAppletTitle
>>
>> * netx/net/sourceforge/jnlp/runtime/JNLPRuntime.java
>>  - added stup for return of international date (jnlpruntime is holding the locales, thats why it
>> its here)
>
> There is a trailing semicolon after the closing brace on this new function
>
>>
>>
>>  * netx/net/sourceforge/jnlp/runtime/ManifestAttributesChecker.java
>>  -  showMatchingALACAttributePanel is receiving whole file (for purposes of reuse in reusable
>> dialogue)) instead of just file.title
>>
>> * netx/net/sourceforge/jnlp/security/SecurityDialogs.java few minor warning-clean up which I could
>> not look onto.
>>  - showMatchingALACAttributePanel - to get file instead file.title and adapted to new  dialogue
>>   - checking action for always/never
>>   - using text for yes/no
>>   - getting response
>>   - storing response
>>
>> * netx/net/sourceforge/jnlp/security/appletextendedsecurity/AppletSecurityActions.java
>>   - allowed public access via set(int id) and get(int id) - needed for generalization
>
> "Remember" misspelled as "Remeber" in constant name.
>
> Can this class be made to implement Iterable so getRealCount() + getAction(int) isn't the only way?
>
>>
>> * netx/net/sourceforge/jnlp/security/appletextendedsecurity/UnsignedAppletActionEntry.java
>>   - changed signatures of :
>>   ExecuteAppletAction getUnsignedAppletAction() -> AppletSecurityActions getAppletSecurityActions()
>>  - and
>>  setUnsignedAppletAction(ExecuteAppletAction unsignedAppletAction) ->
>> etAppletSecurityActions(AppletSecurityActions actions)
>>
>>
>> * netx/net/sourceforge/jnlp/security/appletextendedsecurity/UnsignedAppletActionStorage.java
>>  - added ID parameter to
>>   getMatchingItem
>>   getMatchingItemByDocumentBase
>>   getMatchingItemByCodeBase
>>   getMatchingItemByBases
>>  - this is a bit unhappy. But th e mechanism needs to find strong value first. So it mus know
>> whichone it is looking fr :( so we cannot jsut return Actions and select later )
>>
>> * netx/net/sourceforge/jnlp/security/appletextendedsecurity/UnsignedAppletTrustConfirmation.java
>>  - get and forward  the ID parameter
>>  - separated get action and get entry logic
>>  - one bug fix - when somebody  have visited applet, and have not clicked remember, and later he
>> selected remember for codebase, then the "codebase" change never propagated. Now it does.
>>  - forwarding correct id into depth
>>
>>
>> *
>> netx/net/sourceforge/jnlp/security/appletextendedsecurity/impl/UnsignedAppletActionStorageExtendedImpl.java
>>
>>  - used the *or* action as in table
>>  - made aware of columns abstraction
>>
>> * netx/net/sourceforge/jnlp/security/appletextendedsecurity/impl/UnsignedAppletActionStorageImpl.java
>>  - again propagated ID
>
> What is the use case for a null ID? IOW why Integer and not just int? It seems to me that in other
> places, the ID really just is an int as well...
>
>>
>> * netx/net/sourceforge/jnlp/security/dialogs/MatchingALACAttributePanel.java
>>  - moved to new impl into apptrustwarningpanel  with other similar panels
>>
>> * netx/net/sourceforge/jnlp/security/dialogs/apptrustwarningpanel/AppTrustWarningDialog.java:
>>  - added shower for MatchingAlaca
>>
>> * netx/net/sourceforge/jnlp/security/dialogs/apptrustwarningpanel/AppTrustWarningPanel.java
>>  - added pluginbridge back - maybe best to o here is to abstract getTitle() into jnlpfile. But as
>> naother work...
>>
>> * netx/net/sourceforge/jnlp/security/dialogs/apptrustwarningpanel/MatchingALACAttributePanel.java:
>>  - actual extension of AppTrustWarningPanel for matching alaca
>>  - getAppletTitle is emty, as it is handle in plain text
>>
>> *
>> netx/net/sourceforge/jnlp/security/dialogs/apptrustwarningpanel/PartiallySignedAppTrustWarningPanel.java
>>
>>  - removed duplicate code of getAppletTitle
>>  - shown the date of action
>
> With the changed main text, the window is no longer tall enough on my setup. The last line of text
> has the bottom of it clipped off because it's too tall. I guess this is actually a problem with the
> parent class not setting its window size correctly.
>
>>
>> *
>> netx/net/sourceforge/jnlp/security/dialogs/apptrustwarningpanel/UnsignedAppletTrustWarningDialog.java
>>  - removed - this is actually bug. It should be removed when generalized "remember" dialogue came in
>>
>>
>> *
>> netx/net/sourceforge/jnlp/security/dialogs/apptrustwarningpanel/UnsignedAppletTrustWarningPanel.java
>>  - shown the date of action
>>
>>
>> *
>> tests/netx/unit/net/sourceforge/jnlp/security/appletextendedsecurity/impl/UnsignedAppletActionStorageImplTest.java
>>
>>  - tests adapted to new ID style
>>  - well righ now I realised the high-level of tests of new logic are missing (I added the test
>> datat but forgot to add also testcases ) - well wil be added in next round
>>
>>
>> Thanx!
>>
>> J.
>
> There are a few spots with bad formatting eg one too many or one too few spaces of indent:
> - UnsignedAppletActionTableModel
> - AppletSecurityActions
> - MatchingALACAttributePanel
>
> please just "Fix Formatting" on at least these files before push.
>
> The "Unsigned action" classes should really be renamed now, and you even have a comment to this
> effect in UnsignedAppletActionEntry. Can this also be done? Maybe just as a separate changeset, but
> I do think it should be done.
>
> Overall okay I think, I can't find much to actually complain about in this one other than style
> nitpicks :D

Thank you!

Pushed. Withall hints fixed (yah, also the iterable ;) I explained the id change in Changelog, It 
shdould be ok also for you.


I'm now going to refactor all the names. Do you have any preferences on this?


J.