Hey Ben,
First of all, this looks great! Good work. I have a couple of
comments/questions:
1. The old (now deprecated) constructor. While I don't think we
should remove it in the 1.4.3 release....it should be totally safe to
just delete it in the trunk. There are no calls to it in Stripes, and
since it's protected and there's no way to easily use a subclass of
FlashScope, nobody else should be using it either. Do you agree?
2. At FlashScope:350 you syncrhonize on a public static final String
in StripesConstants. I've never done this myself, but I'm wondering
if it's safe? The compiler has a tendency to inline references to
public static final Strings, and I wonder if that might end up
negating the sync'ing, or if the compiler is smart enough to not
inline (or intern the String) in cases like this. Might just be
safer to sync on FlashScope.class instead?
3. I'm still a little leery of the use of a static Random for
generation of the keys, that then has to be synchronized over.
Admittedly the contents of the sync {} are short and quick, and this
isn't something that should be called that much (one per request
maximum), but it still concerns me. I'm probably worried about
nothing, but I just wonder if there isn't a strategy that would be as
good and not involve JVM-wide syncing.
4. You're gonna merge this to the 1.4.x branch right? ;)
-t
On Mar 30, 2007, at 12:09 AM, [EMAIL PROTECTED] wrote:
> Revision: 500
> http://svn.sourceforge.net/stripes/?rev=500&view=rev
> Author: bengunter
> Date: 2007-03-29 21:09:09 -0700 (Thu, 29 Mar 2007)
>
> Log Message:
> -----------
> Resolved STS-319: Request and response objects should be released
> when using flash scope.
> At the end of each request, any ActionBeans in the current flash
> scope have their
> contexts' request and response objects replaced with objects that
> are safe to use on the
> next request.
>
> - Added FlashRequest and FlashResponseInvocationHandler classes
>
> - Deprecated FlashScope(HttpServletRequest) in favor of
> FlashScope(HttpServletRequest, Integer)
>
> - The key for the current flash scope for a request is now stored
> as a request attribute
>
> - Some cleanup code was moved from StripesFilter.flashOutbound() to
> FlashScope.requestComplete()
>
> - Added additional flash scope tests, including tests for the new
> request and response
>
> Modified Paths:
> --------------
> trunk/stripes/src/net/sourceforge/stripes/controller/
> FlashScope.java
> trunk/stripes/src/net/sourceforge/stripes/controller/
> StripesConstants.java
> trunk/stripes/src/net/sourceforge/stripes/controller/
> StripesFilter.java
> trunk/tests/src/net/sourceforge/stripes/controller/
> FlashScopeTests.java
>
> Added Paths:
> -----------
> trunk/stripes/src/net/sourceforge/stripes/controller/
> FlashRequest.java
> trunk/stripes/src/net/sourceforge/stripes/controller/
> FlashResponseInvocationHandler.java
-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys-and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV
_______________________________________________
Stripes-development mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/stripes-development
