I corrected the things you required in the JIRA issues. When you have the time, please have a look over them. See inline my comments and questions:On Sun, Jul 11, 2010 at 3:57 AM, Leonardo Uribe <firstname.lastname@example.org> wrote:
Sorry for the late response, but I was looking other important issues (I usually review issues with JSR-314 component first). I reviewed your patches and I committed some of them, and made comments on the others.
To save ValueExpressions, javax.faces.component._DeltaStateHelper uses an InternalMap, but that one does not take into account PartialStateHolder interface. So in this case does not apply.> On the other hand, regarding serialization vs implements PartialStateHolder,I guess - Leonardo should clarify.
> I've looked for some more externalizable objects: the TagMethodExpression
> class is used in the MethodExpressionValueChangeListener, which implements
> StateHolder and PartialMethodExpressionValueChangeListener, which implements
> So maybe instead of being Externalizable,
> TagMethodExpression should also implement PartialStateHolder. There is a
> similar situation with TagValueExpression.
In MethodExpression it is possible, but to do that, we need to solve two questions:
- If we have an immutable object that implements Serializable interface and a similar one that implements PartialStateHolder, which one is saved / restored faster? which one has bigger state size? It is worth to implement PartialStateHolder or keep it as Serializable?. To solve that we need to try some simple performance tests with possible candidates.
--> Ok. How should I do that. This meaning do you have any suggestions for objects or should I create some mock objects with identical data and then compare the two strategies (in time and state size)?
- Does all methods that store MethodExpression variables ( UICommand.actionExpression) should handle PartialStateHolder interfaces? In theory yes (I add this support before but then I revert it to the current behavior because there is no evidence why use PartialStateHolder in this case), but if we don't expect MethodExpression implementations implements PartialStateHolder, just let it as is.
--> By this you mean classes with MethodExpressions that use also serialization and not implements PartialStateHolder. I've found a couple of such classes: EventHandler.Listener and LegacyMethodBinding.
Look this class: org.apache.myfaces.view.facelets.tag.jsf.core.SetPropertyActionListenerHandler . It has a inner class called SetPropertyListener that implements Serializable. javax.faces.component._DeltaList is used to store FacesListeners and that one handles PartialStateHolder instances. That one is other valid case.
--> Ok. So should I transform this class into one implementing PartialStateHolder?