Hi Leonardo, 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 <[email protected]> wrote: > Hi > > 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. > > > On the other hand, regarding serialization vs implements >>> PartialStateHolder, >>> > I've looked for some more externalizable objects: the >>> TagMethodExpression >>> > class is used in the MethodExpressionValueChangeListener, which >>> implements >>> > StateHolder and PartialMethodExpressionValueChangeListener, which >>> implements >>> > PartialStateHolder. >>> >>> > So maybe instead of being Externalizable, >>> > TagMethodExpression should also implement PartialStateHolder. There is >>> a >>> > similar situation with TagValueExpression. >>> >>> I guess - Leonardo should clarify. >>> >>> > 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. > > 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? > > best regards, > > Leonardo Uribe > Regards, Marius
