Hi 2012/11/15 Mark Struberg <[email protected]>: > >> But you reverted the changes I did months ago. Didn't you? oh yes, you >> "refactor" it and get rid what you don't like. > > That is OSS Leo. We take what we find and improve it. This is not a revert - > this is evolution! > > There is no such thing like 'my code' in the ASF. >
Mark, please, try to understand my position. Each change has the potential to introduce a bug. The ASF way is code in community. I'm not against evolution. What I want is just some time to think carefully the problem from start before do anything that could be harmful. > >> That's unfair. I don't think that's true. > Look at what happened with the RelativeResourceHandler and where it lives now > for example. Just to name one in the not so far past. > I'm not going further with commenting any of your personal attacks but will > further again only focus on technical arguments. > Ok. > > As I said before, we can rollback the viewId to the hashCode if you like. I'm > perfectly fine with that. > Ok > I'm also fine with moving the classes to another package root. But I'm -1 on > the original 15++ inner classes implementation because this is unmaintainable. > Yes, I know that. I'm not objecting that. But I just want to do the effort of refactor those classes just once, after thinking about the trade-offs. > > Again: if we set the key back to hashCode then there is no functional change > to the previous version! It's just own classes instead of inner classes + > removing duplicated code. > Yes, and we can do that but after the release. > > Actually you really can blame me for something (actually not only me but all > other community members as well): to not have reviewed that part earlier. I > confess for that part. This is a complicated area and we should have given > feedback way earlier as this is hard to get right. But it doesn't get easier > by having large changes developed locally and then committing it in one big > step. > I know the changes proposed goes in the right direction, but there is no need to rush for them. > >> Since the plan is do a release next week, could >> we revert the changes, and put them on hold > > I still don't see the reason for it. The code is fundamentally the same > (after moving back to the hashCode) and most likely will pass the TCK. > Please give it a try or provide fundamental feedback. The classes are still > the same, they only got moved from inner classes to own classes. > There is not enough time. The plan for the next weeks is focus on JSF 2.2, because the public review will be coming soon (in December). My attention is focused is that, and this problem for me is a distraction. We need to review and contribute the spec ASAP. This is the time were we can fix early the problems, and if we wait after the spec goes out, we'll have to live with that until the next year The plan next week was the release, because based on my calculations I will only have time for another one at mid January / February. Please Mark, I'm begging you for the good of the community, don't block this release. Let me put on hold the changes, fix the problem with the solution with minimal changes and do the release. > >> https://jira.springsource.org/browse/SWF-1365 > This bug got fixed and released more than 2 years ago in SWF. This was even > done before swf-2.0 went final. I see no reason to keep any old code in > MyFaces only for that. > I have followed the problem, it was not solved until SWF-1540 got fixed (14/Jun/2012). regards Leonardo > LieGrue, > strub > > > > ----- Original Message ----- >> From: Leonardo Uribe <[email protected]> >> To: MyFaces Development <[email protected]>; Mark Struberg >> <[email protected]> >> Cc: >> Sent: Friday, November 16, 2012 12:11 AM >> Subject: Re: StateSaving review >> >> Hi >> >> 2012/11/15 Mark Struberg <[email protected]>: >>> Leo, it would be great if you reflect a bit. >>> >>>> I have checked the code. I know what SerializedViewKey does. >>> >>> good! no other arguments? Well, then all is fine it seems. >>> >>> >>>> it is YOU the one who are reverting code done by other people >>> >>> The only code I ever reverted in MyFaces was your revert of my code change. >> That's it. >> >> But you reverted the changes I did months ago. Didn't you? oh yes, you >> "refactor" it and >> get rid what you don't like. >> >>> But actually I'm not the only one who got code reverted by you, and >> those people are fed up as well and partly left the community already >> because of >> that. >>> >> >> That's unfair. I don't think that's true. I'm just exercising my >> right >> to participate, that's it. People can disagree from time to time. >> That's part of the life, if you can't deal with it, it's your >> problem. >> But I'm always willing to look for alternatives and resolve >> differences, like I'm doing right now, and that's what makes the >> difference. >> >>> >>>> I'm sorry to be so direct, but as a member of the community, I >>>> have the right to participate and I'm willing to exercise that >> right. >>> >>> That's perfectly fine as well. So please start your excercise with >> listening to other people sometimes. >>> * I listed 5++ flaws in the design. >>> * I removed a few classes which contained 99% copy & paste. >>> * I removed a few classes which are not used anymore since over a year >>> * I removed an old resources folder which was just lying around for no >> reason. >>> * I moved the remaining inner classes to toplevel and an own package to >> make it easier to maintain and overlook the whole complexity. >>> * I added the viewId to _one_ of the dozen key implementations. Actually we >> don't need it. We also don't need the viewId hashCode which was there >> before. So I don't care, let's move this back to your hashCode solution >> and fix it properly in the next release. >>> * I cleaned up your generics because it always only used the String type >> anyway. And this cannot change in the future because this fact comes directly >> from the JSF spec. >>> Otherwise there has been no fundamental change to your code. The mess you >> see is all yours so to say (sorry for the sarcasm). >>> >> >> That's your opinion, not mine. But I can see some advance. At last, there is >> one list of the changes proposed. Since the plan is do a release next >> week, could >> we revert the changes, and put them on hold while the necessary steps are >> done? >> Otherwise a release will not be possible for a long time (In fact, you >> have kidnapped >> the release). In that way, we can dedicate enough time and resources to get >> it out once for all. Do you agree with that? Could you let me complete >> the planned >> schedule? >> >> Anyway, what's the deal to put the changes in 2.1.x branch? why you are so >> desperate for that? You have not given to me any reason why you can't work >> with >> 2.1.x-client-window branch, which is best for what you are trying to >> do, or why you >> can wait just 2 weeks to commit the changes. >> >> Come on Mark, is not a big deal, right? >> >>> >>>> The concrete case is spring webflow does not support PSS algorithm, so >>>> we need to switch back and enable compatibility mode with JSF 1.2. >>> >>> Spring WebFlow uses it's own StateManager which natively extends >> javax.faces.StateManager. There is no MyFaces code involved anymore since >> ages! >>> >> http://static.springsource.org/spring-webflow/docs/2.0.x/javadoc-api/org/springframework/faces/webflow/FlowViewStateManager.html >>> >>> I also googled quite some time and fine nothing pointing to any problems >> with the MyFaces StateManager. Not even on stackoverflow. >>> >> >> Look this: >> >> https://jira.springsource.org/browse/SWF-1365 >> >> Because that problem, I decided to preserve JspStateManagerImpl long time >> ago, >> specifically when MYFACES-3117 was solved, because the change over >> ResponseStateManager imposed a change over all state management in MyFaces. >> Is there a problem? don't care if exists or not, what's important is >> provide >> a way to go back to the old behavior. Be conservative, keep code stable. >> >> regards, >> >> Leonardo Uribe >> >>> >>> >>> LieGrue, >>> strub >>> >>> >>> >>> ----- Original Message ----- >>>> From: Leonardo Uribe <[email protected]> >>>> To: MyFaces Development <[email protected]>; Mark Struberg >> <[email protected]> >>>> Cc: >>>> Sent: Thursday, November 15, 2012 11:06 PM >>>> Subject: Re: StateSaving review >>>> >>>> Hi >>>> >>>> 2012/11/15 Mark Struberg <[email protected]>: >>>>> >>>>>> You did more than that. You removed the following classes: >>>>>> >>>>>> IntIntSerializedViewKey >>>>>> IntByteArraySerializedViewKey >>>>>> ReferenceSerializedViewKey >>>>> >>>>> Those have been almost 1:1 clones of other classes. Go check >>>> SerializedViewKey, it contains all the merged functionality. Without >> any >>>> performance overhead. >>>>> Look at the code first and then you can complain. You revert code >> done by >>>> other people without even looking at the changes it seems. >>>>> >>>>> >>>> >>>> I have checked the code. I know what SerializedViewKey does. >>>> I'm pretty aware of what it does and its effects. I have to say it, >>>> it is YOU the one who are reverting code done by other people >>>> without even looking at the changes, just because you have >>>> the perception that the changes are wrong, but nothing more. >>>> Without any proof or any deep consideration. >>>> >>>> The changes I have done over the time has follow the same >>>> protocol, create an issue, provide a patch, wait for feedback of >>>> the community, if no objections after a prudent time commit them >>>> and close the issue. In this moment the changes done by me has >>>> the tacit aval of the community. Your changes do not have that, >>>> because they have objections done 72 hours after they were >>>> proposed. This is a meritocracy, and the rules of the community >>>> in this case are on my side. >>>> >>>> I'm sorry to be so direct, but as a member of the community, I >>>> have the right to participate and I'm willing to exercise that >> right. >>>> I will not look to other side and let some untested code go into >>>> myfaces core 2.1.x. I appreciate your efforts to make this part >>>> better, but I encourage you to respect and play with the rules >>>> of the community. >>>> >>>> Look the dates of the improvements: >>>> >>>> MYFACES-3563 >>>> Created: 10/Jun/12 11:33 >>>> Resolved: 25/Jun/12 09:05 >>>> >>>> MYFACES-3117 >>>> Created: 26/Apr/11 19:08 >>>> Resolved: 14/May/11 02:27 >>>> >>>> That code has been there for years!!!!!! , the latest changes were done >>>> 5 months ago !!!. Nobody complained at that time. Now you came here, >>>> complaining about the code and you want to fix it without any interest >>>> of what others think about that. That's not cool. >>>> >>>>> >>>>>> and on top of that you removed JspStateManagerImpl, which is a >> class >>>>>> that is there for compatibility with old state managers that >> relies on >>>>>> MyFaces 1.2.x. >>>>> >>>>> This is not JSF-1.x since a long time. APIs in the StateManager >> got changed >>>> for AJAX support, etc. >>>>> >>>>> But I'm all fine with going back to the JspStateManagerImpl >> again as I >>>> already said. >>>>> >>>>> Btw, may I quote your answer to my question if I can delete it? >>>>> LU> "Remove that code is a valid option." >>>>> >>>> >>>> The concrete case is spring webflow does not support PSS algorithm, so >>>> we need to switch back and enable compatibility mode with JSF 1.2. >>>> JspStateManagerImpl is there because the state manager in that library >>>> relies on the old behavior. I think there are other cases too. But I >> hope to >>>> remove it in the future (JSF 2.2). >>>> >>>> regards, >>>> >>>> Leonardo Uribe >>>> >>>>> LieGrue, >>>>> strub >>>>> >>>>> >>>>> >>>>> >>>>> ----- Original Message ----- >>>>>> From: Leonardo Uribe <[email protected]> >>>>>> To: MyFaces Development <[email protected]>; Mark >> Struberg >>>> <[email protected]> >>>>>> Cc: >>>>>> Sent: Thursday, November 15, 2012 10:20 PM >>>>>> Subject: Re: StateSaving review >>>>>> >>>>>> Hi >>>>>> >>>>>> 2012/11/15 Mark Struberg <[email protected]>: >>>>>>> Leo, the stuff which currently got changed is nothing >> more than >>>> just your >>>>>> code. I only refactored it into an own package and moved the >> inner >>>> classes to >>>>>> toplevel to show up the complexity of the solution. There was >> no code >>>> change >>>>>> _yet_ other than the viewId. We can change back the viewId to >> the hash >>>> if you >>>>>> feel better. It makes no difference anyway. >>>>>>> >>>>>> >>>>>> You did more than that. You removed the following classes: >>>>>> >>>>>> IntIntSerializedViewKey >>>>>> IntByteArraySerializedViewKey >>>>>> ReferenceSerializedViewKey >>>>>> >>>>>> also you created this one (it was an abstract class before) >>>>>> >>>>>> SerializedViewKey >>>>>> >>>>>> and on top of that you removed JspStateManagerImpl, which is a >> class >>>>>> that is there for compatibility with old state managers that >> relies on >>>>>> MyFaces 1.2.x. Obviously you have not considered those cases, >> but I >>>> have >>>>>> done that long time ago. >>>>>> >>>>>> Without ask, without propose a patch, without let the people >> who care >>>> about >>>>>> to participate. Just do it and let others do the clean up. >>>>>> >>>>>> Mark, I have came here and proposed to you many possible >> alternatives >>>>>> and you're letting me without choices. I beg you to move >> your >>>> changes to >>>>>> 2.1.x-client-window branch, and when we have certain about >> which >>>> changes >>>>>> needs to be done, backport them to 2.1.x. In just one step, >> without >>>> this >>>>>> pain and stress you are causing me. Please. The reason why it >> was >>>> created >>>>>> 2.1.x-client-window was precisely to try changes for client >> window / >>>> state >>>>>> manager and flash scope. >>>>>> >>>>>> In this moment I can't do any release. You have tied my >> hands. >>>> I'm >>>>>> trying to be >>>>>> kind, please. >>>>>> >>>>>>> In the long run this stuff needs _heavy_ cleanup. >>>>>>> Again: Extracting out an abstraction over Server vs >> Client >>>> stateSaving is >>>>>> perfectly fine, but the code is now way more complicated than >> it ever >>>> has been. >>>>>> Premature abstraction is way worse than premature >> optimisation. >>>>>>> >>>>>> >>>>>> It is a work in progress, I already told you many times. I >> know the >>>> abstraction >>>>>> is not the best, but it works, and has helped a lot to me to >> understand >>>> how >>>>>> this algorithm should work. It is like a worm that became >> chrysalis and >>>> in the >>>>>> future will become a butterfly. "... the morphing step is >> not nice >>>>>> ..." >>>>>> >>>>>> regards, >>>>>> >>>>>> Leonardo >>>>>> >>>>>>> LieGrue, >>>>>>> strub >>>>>>> >>>>>>> >>>>>>> >>>>>>> >>>>>>> ----- Original Message ----- >>>>>>>> From: Leonardo Uribe <[email protected]> >>>>>>>> To: MyFaces Development >> <[email protected]>; Mark >>>> Struberg >>>>>> <[email protected]> >>>>>>>> Cc: >>>>>>>> Sent: Thursday, November 15, 2012 9:30 PM >>>>>>>> Subject: Re: StateSaving review >>>>>>>> >>>>>>>> Hi >>>>>>>> >>>>>>>> 2012/11/15 Mark Struberg <[email protected]>: >>>>>>>>> Leo, I'm ok with a revert. But not to your >> version >>>> but to the >>>>>> original >>>>>>>> JspStateManagerImpl! >>>>>>>>> >>>>>>>>> Really, this currently feels so overcomplicated >> without >>>> giving >>>>>> much >>>>>>>> benefit. It would have been perfectly enough to >> remove the >>>> viewId >>>>>> String and >>>>>>>> replace it with a XORShift random value if you >> don't trust >>>> the >>>>>> sequencer. >>>>>>>>> >>>>>>>>> I know one goal was to abstract out the state >> between >>>> client and >>>>>> server. >>>>>>>> But that doesn't mean that we end up with Object, >> Object >>>> and do >>>>>> hardcoded >>>>>>>> upcasts as done right now in the code. This is no >> improvement >>>> over the >>>>>> original >>>>>>>> code. The windowId change would have been 30 LOC btw. >>>>>>>>> >>>>>>>> >>>>>>>> It is fine if you want to change the implementation >> to make >>>> something >>>>>>>> better. What's not fine, is that you want to work >> in 2.1.x >>>> branch >>>>>>>> directly, that is right now in maintenance stage. So, >> you are >>>> doing >>>>>>>> all kind of refactorings, and that makes me crazy. >> The last >>>> release >>>>>>>> was done on 23/Sep/12 and I'm doing 1 release >> each 2 >>>> months. Your >>>>>>>> changes will take longer and I need time to review >> them, >>>> because I >>>>>>>> will not do a release over changes that I have not >> seen. >>>>>>>> >>>>>>>> My idea is start a release process next week. Why >> don't >>>> you take >>>>>> your >>>>>>>> time?, instead you try to do changes without any >> clear >>>> direction. >>>>>>>> >>>>>>>> In this moments, we are starting the necessary steps >> to work >>>> on 2.2.x. >>>>>>>> You should be working in 2.1.x-client-window branch >> !!!!! >>>>>>>> >>>>>>>> regards, >>>>>>>> >>>>>>>> Leonardo Uribe >>>>>>>> >>>>>>>>> LieGrue, >>>>>>>>> strub >>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> ----- Original Message ----- >>>>>>>>>> From: Leonardo Uribe >> <[email protected]> >>>>>>>>>> To: MyFaces Development >>>> <[email protected]>; Mark >>>>>> Struberg >>>>>>>> <[email protected]> >>>>>>>>>> Cc: >>>>>>>>>> Sent: Thursday, November 15, 2012 8:44 PM >>>>>>>>>> Subject: Re: StateSaving review >>>>>>>>>> >>>>>>>>>> Hi >>>>>>>>>> >>>>>>>>>> 2012/11/15 Mark Struberg >> <[email protected]>: >>>>>>>>>>> Btw, I still do not see where the trick >> for >>>> storing a >>>>>> separate >>>>>>>> state list >>>>>>>>>> for each browser tab is hidden. >>>>>>>>>>> That was the goal of all the >> refactoring >>>> initially. Where >>>>>> is that >>>>>>>> done? >>>>>>>>>>> >>>>>>>>>> >>>>>>>>>> Mark, months ago we created >> 2.1.x-client-window >>>> branch: >>>>>>>>>> >>>>>>>>>> >>>>>>>> >>>>>> >>>> >> http://svn.apache.org/repos/asf/myfaces/core/branches/2.1.x-client-window/ >>>>>>>>>> >>>>>>>>>> This branch is the same as 2.1.x, but only >> with the >>>> changes >>>>>> proposed >>>>>>>>>> long time ago for JSF 2.2 client window. >>>>>>>>>> >>>>>>>>>> It has what you need to have a state list >> for each >>>> browser >>>>>> tab. >>>>>>>>>> The only thing you need to provide is a >> ClientWindow >>>>>> implementation >>>>>>>>>> and that's it. The state saving code >> already has >>>> included >>>>>> the >>>>>>>> client-window >>>>>>>>>> concept. I also fixed Flash scope to use >> client >>>> window. >>>>>>>>>> It is also available in a maver repo: >>>>>>>>>> >>>>>>>>>> >>>>>>>> >>>>>> >>>> >> https://repository.apache.org/content/repositories/snapshots/org/apache/myfaces/core/myfaces-bundle/ >>>>>>>>>> >>>>>>>>>> Let's make a deal. Move your changes to >> that >>>> branch, let >>>>>> me apply >>>>>>>> my fix >>>>>>>>>> in trunk and do a release. When your changes >> are >>>> ready, we can >>>>>> discuss >>>>>>>>>> them and backport from 2.1.x-client-window >> to trunk. >>>> Does that >>>>>> sound >>>>>>>>>> good for you? >>>>>>>>>> >>>>>>>>>> regards, >>>>>>>>>> >>>>>>>>>> Leonardo Uribe >>>>>>>>>> >>>>>>>>>> >>>>>>>>>>> LieGrue, >>>>>>>>>>> strub >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> ----- Original Message ----- >>>>>>>>>>>> From: Mark Struberg >>>> <[email protected]> >>>>>>>>>>>> To: MyFaces Development >>>>>> <[email protected]> >>>>>>>>>>>> Cc: >>>>>>>>>>>> Sent: Thursday, November 15, 2012 >> 6:32 PM >>>>>>>>>>>> Subject: Re: StateSaving review >>>>>>>>>>>> >>>>>>>>>>>> I did further reviews: >>>>>>>>>>>> >>>>>>>>>>>> Currently the CounterKeyFactory >> needs some >>>> random to >>>>>> be unique >>>>>>>>>> (according to >>>>>>>>>>>> Leo) and the RandomKeyFactory needs >> a >>>> counter to be >>>>>> unique. >>>>>>>>>>>> >>>>>>>>>>>> Does that ring a bell? That stuff >> is >>>> completely >>>>>> useless! >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> Foooooolks wake up, let's >> provide ONE >>>> factory >>>>>> which is >>>>>>>> waterproof. >>>>>>>>>> That >>>>>>>>>>>> would be much better than having >> 15++ >>>> classes full of >>>>>> half >>>>>>>> baken/broken >>>>>>>>>>>> algorithms. >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> Proposal: Get rid of all that >> KeyFactory >>>> stuff again >>>>>> and have >>>>>>>> one GOOD >>>>>>>>>>>> algorithm: Counter + XORShift >> Random. >>>>>>>>>>>> >>>>>>>>>>>> The KeyFactory stuff got only >> introduced in >>>> 2.1.9 and >>>>>> is >>>>>>>> crashing in >>>>>>>>>> production. >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> LieGrue, >>>>>>>>>>>> strub >>>>>>>>>>>> >>>>>>>>>>>> PS: don't take it personal, but >> from >>>> looking at >>>>>> the code I >>>>>>>> see >>>>>>>>>> clearly what >>>>>>>>>>>> happens if someone works on a stuff >> alone >>>> without >>>>>> reviews. >>>>>>>> This always >>>>>>>>>> leads to >>>>>>>>>>>> deficits, regardless how good one >> is. >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> ----- Original Message ----- >>>>>>>>>>>>> From: Leonardo Uribe >>>> <[email protected]> >>>>>>>>>>>>> To: MyFaces Development >>>>>> <[email protected]>; >>>>>>>> Mark >>>>>>>>>> Struberg >>>>>>>>>>>> <[email protected]> >>>>>>>>>>>>> Cc: >>>>>>>>>>>>> Sent: Thursday, November 15, >> 2012 6:20 >>>> PM >>>>>>>>>>>>> Subject: Re: StateSaving >> review >>>>>>>>>>>>> >>>>>>>>>>>>> Hi >>>>>>>>>>>>> >>>>>>>>>>>>> 2012/11/15 Mark Struberg >>>>>> <[email protected]>: >>>>>>>>>>>>>> I'm currently >> reviewing all >>>> this area. >>>>>> It seems >>>>>>>> that we >>>>>>>>>> have quite >>>>>>>>>>>> some >>>>>>>>>>>>> stuff to improve. >>>>>>>>>>>>>> >>>>>>>>>>>>>> a.) just a gut feeling >> yet, but my >>>> tummy >>>>>> tells me >>>>>>>> that we >>>>>>>>>> have to >>>>>>>>>>>> review >>>>>>>>>>>>> our key generator/storage >> strategies. >>>> Too >>>>>> complicated or >>>>>>>> too badly >>>>>>>>>>>> documented. >>>>>>>>>>>>> At least they are not self >> describing. >>>>>>>>>>>>> >>>>>>>>>>>>> It's a complex algorithm. >> No >>>> documentation >>>>>> so far, >>>>>>>> because it >>>>>>>>>> is still >>>>>>>>>>>>> work in progress. >>>>>>>>>>>>> >>>>>>>>>>>>>> >>>>>>>>>>>>>> b.) candidate 1: >>>> CounterKeyFactory. If we >>>>>> like to >>>>>>>> prevent >>>>>>>>>> reboot >>>>>>>>>>>> clashes >>>>>>>>>>>>> then we might add another int >> which >>>> contains a >>>>>> random >>>>>>>> value. Think >>>>>>>>>> about >>>>>>>>>>>> having >>>>>>>>>>>>> a Server with a single page >> right now. >>>> Click on >>>>>> it a few >>>>>>>> times, >>>>>>>>>> then >>>>>>>>>>>> restart >>>>>>>>>>>>> myfaces and issue a few >> requests to the >>>> same >>>>>> page and go >>>>>>>> back in >>>>>>>>>> your >>>>>>>>>>>> browser >>>>>>>>>>>>> history. Proposal: instead of >> the >>>> viewId we >>>>>> should add a >>>>>>>> random >>>>>>>>>> number. >>>>>>>>>>>>>> >>>>>>>>>>>>> >>>>>>>>>>>>> Since the counter is also >> stored into >>>> session, >>>>>> the last >>>>>>>> counter >>>>>>>>>> values >>>>>>>>>>>>> is not lost. >>>>>>>>>>>>> >>>>>>>>>>>>>> c.) a general one. We >> might >>>> introduce an >>>>>> own Random >>>>>>>> which >>>>>>>>>> either uses >>>>>>>>>>>>> >> java.util.concurrent.ThreadLocalRandom >>>> for java >>>>>> 7++ or >>>>>>>> the old >>>>>>>>>> Random impl. >>>>>>>>>>>>>> ThreadLocalRandom has a >> _much_ >>>> better >>>>>> performance >>>>>>>> on >>>>>>>>>> servers! Or we >>>>>>>>>>>> just >>>>>>>>>>>>> use a simple XORShift which is >> surely >>>> good >>>>>> enough for >>>>>>>> most cases >>>>>>>>>> and >>>>>>>>>>>> performs >>>>>>>>>>>>> like hell. The spreading of >> XORShift is >>>> better >>>>>> than the >>>>>>>> standard >>>>>>>>>> Java >>>>>>>>>>>> algorithm >>>>>>>>>>>>> even ... >>>>>>>>>>>>>> >>>>>>>>>>>>> >>>>>>>>>>>>> It could work. >>>>>>>>>>>>> >>>>>>>>>>>>>> d.) KeyFactory looks a >> bit >>>> overengineered. >>>>>> The >>>>>>>> return type is >>>>>>>>>> either >>>>>>>>>>>>> Integer or byte [] but the >> encoded >>>> value is >>>>>> always >>>>>>>> represented as >>>>>>>>>> String. >>>>>>>>>>>>>> >>>>>>>>>>>>> >>>>>>>>>>>>> The key is encrypted and >> base64 >>>> encoded, but >>>>>> that's >>>>>>>> done in >>>>>>>>>>>>> >>>> HtmlResponseStateManager.writeViewStateField. >>>>>> The change >>>>>>>> only has >>>>>>>>>>>>> sense if we move the >> responsibility of >>>> encrypt >>>>>> to >>>>>>>>>>>>> >>>>>> ServerSideStateCacheImpl/ClientSideStateCacheImpl. My >>>>>>>> opinion is >>>>>>>>>> it is >>>>>>>>>>>>> better to keep it as is. >>>>>>>>>>>>> >>>>>>>>>>>>>> e.) Instead of trashing >> the >>>> Session with >>>>>>>> setAttribute and >>>>>>>>>> synchronized >>>>>>>>>>>> >>>>>>>>>>>>> blocks we should rather store >> an >>>> AtomicInteger. >>>>>> This is >>>>>>>> perfectly >>>>>>>>>> fine now >>>>>>>>>>>> as we >>>>>>>>>>>>> do not support java 1.4 any >> longer, >>>> right? >>>>>>>>>>>>>> >>>>>>>>>>>>> >>>>>>>>>>>>> The synchronized blocks are >> over the >>>>>>>> SerializedViewCollection, >>>>>>>>>> which >>>>>>>>>>>>> is unique per session, so it >> does not >>>> suppose >>>>>> any >>>>>>>> overhead (tested >>>>>>>>>>>>> with YourKit profiler). But >> performance >>>> tests >>>>>> shows that >>>>>>>> the >>>>>>>>>>>>> additional calls to session >> map impose >>>> an small >>>>>> overhead >>>>>>>> (the >>>>>>>>>> ideal is >>>>>>>>>>>>> minimize those calls). Since >> the >>>> counter is >>>>>> stored per >>>>>>>> session, >>>>>>>>>> there >>>>>>>>>>>>> is no chance of key clashing. >> One >>>> option could >>>>>> be >>>>>>>> something like >>>>>>>>>> the >>>>>>>>>>>>> trick used in Flash Scope. An >>>> AtomicLong from a >>>>>> random >>>>>>>> seed, but >>>>>>>>>>>>> anyway it is necessary to >> change the >>>> algorithm >>>>>> for check >>>>>>>> if there >>>>>>>>>> is a >>>>>>>>>>>>> key clashing, generates >> another key. >>>>>>>>>>>>> >>>>>>>>>>>>>> Just a few random >> ideas... >>>>>>>>>>>>>> >>>>>>>>>>>>> >>>>>>>>>>>>> MS>> another one: >>>>>>>>>>>>> MS>> All that stuff has >> nothing >>>> to do with >>>>>> the >>>>>>>> RenderKit and >>>>>>>>>> should >>>>>>>>>>>> go >>>>>>>>>>>>> into >>>> org.apache.myfaces.application.viewstate. >>>>>>>>>>>>> MS>> wdyt? >>>>>>>>>>>>> >>>>>>>>>>>>> That stuff is used by >>>>>>>>>>>>> >>>>>>>> >> org.apache.myfaces.renderkit.html.HtmlResponseStateManager, so >>>> in >>>>>>>>>>>>> theory is related to the >> renderkit >>>> (because >>>>>>>> ResponseStateManager >>>>>>>>>> is >>>>>>>>>>>>> renderkit dependant), but it >> is an >>>> application >>>>>> scope >>>>>>>> concept. >>>>>>>>>> Maybe >>>>>>>>>>>>> >>>>>> org.apache.myfaces.renderkit.application.viewstate. has >>>>>>>> more sense >>>>>>>>>> to >>>>>>>>>>>>> me. >>>>>>>>>>>>> >>>>>>>>>>>>> MS>> Also the following >> classes >>>> are >>>>>> related and >>>>>>>> should imo >>>>>>>>>> get moved >>>>>>>>>>>>> to this new package: >>>>>>>>>>>>> MS>> * StateCache >>>>>>>>>>>>> MS>> * StateCacheFactory >>>>>>>>>>>>> >>>>>>>>>>>>> Yes, a better name >>>>>>>>>>>>> >>>>>>>>>>>>> MS>> * StateManagerImpl >>>>>>>>>>>>> >>>>>>>>>>>>> No, StateManagerImpl deals >> with the >>>> logic >>>>>> related to >>>>>>>> calculate the >>>>>>>>>>>>> state to a view, StateCache >> deals with >>>> the >>>>>> storage of the >>>>>>>> view >>>>>>>>>> (maybe >>>>>>>>>>>>> a better name is StateStorage, >> for >>>> example >>>>>>>>>>>>> >>>>>> ServerSideStateStorageImpl/ClientSideStateStorageImpl? >>>>>>>>>>>>> >>>>>>>>>>>>> regards, >>>>>>>>>>>>> >>>>>>>>>>>>> Leonardo >>>>>>>>>>>>> >>>>>>>>>>>>>> LieGrue, >>>>>>>>>>>>>> strub >>>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>> >>>>>>>> >>>>>> >>>> >>
