[google-guice] dhanji commented on revision r733.
Details are at http://code.google.com/p/google-guice/source/detail?r=733
Score: Neutral
General Comment:
Thanks Jesse. Lots of good points.
Line-by-line comments:
File: /trunk/servlet/src/com/google/inject/servlet/FilterPipeline.java
(r733)
===============================================================================
Line 39: */
-------------------------------------------------------------------------------
Done
Line 47: }
-------------------------------------------------------------------------------
Trying "defaultFilterChain", i.e. web.xml default.
File: /trunk/servlet/src/com/google/inject/servlet/GuiceFilter.java (r733)
===============================================================================
Line 2: * Copyright (C) 2006-2008 Google Inc.
-------------------------------------------------------------------------------
Done.
Line 51: * This filter should appear above every filter that makes use of
Guice injection or servlet
-------------------------------------------------------------------------------
Done. picky! ;)
Line 52: * scopes functionality. Ideally, you want to register ONLY this
filter in web.xml and register
-------------------------------------------------------------------------------
How about "Typically."
Line 53: * any other filters using {...@link Servlets#configure()}. But this
is not strictly necessary.
-------------------------------------------------------------------------------
Done.
Line 56: * You will generally want to place sitemesh and similar (purely
decorative) filters above
-------------------------------------------------------------------------------
Come to think, there's no real need to do this. Everything can be
dispatched by GuiceFilter.
Line 67: static volatile WeakReference<ServletContext> servletContext =
-------------------------------------------------------------------------------
Done.
Line 76: throw new RuntimeException(
-------------------------------------------------------------------------------
Do you have an example?
Line 88: GuiceFilter.pipeline = new
WeakReference<FilterPipeline>(pipeline);
-------------------------------------------------------------------------------
The injector could be gc'ed before the webapp is unloaded and I didn't want
to interfere with memory profiling by holding on to it if that is the case.
Line 91: //VisibleForTesting (only)
-------------------------------------------------------------------------------
Me neither. I thought it would be useful for spis, but OK good call.
Line 103: //not even a default pipeline was available--bad!
-------------------------------------------------------------------------------
Fixed. We'll depend on the default pipeline directly (i.e. without any
injector). If an injector with a ServletModule pops up, we'll replace with
the managed pipeline and continue as per normal. Fortunately for us, the
servlet specification allows lazy initialization. =)
Line 104: if (null == filterPipeline)
-------------------------------------------------------------------------------
Done.
Line 106: + "setup the servlet module by
using Servlets.configure()."
-------------------------------------------------------------------------------
Gotcha.
Line 130: public static ServletContext getServletContext() {
-------------------------------------------------------------------------------
typo. Fixed.
Line 174: if (null != filterPipeline)
-------------------------------------------------------------------------------
Done.
File: /trunk/servlet/src/com/google/inject/servlet/ServletModule.java (r733)
===============================================================================
Line 53: @Deprecated
-------------------------------------------------------------------------------
OK as part of the switch to the subclassed binding api.
File: /trunk/servlet/test/com/google/inject/servlet/EdslTest.java (r733)
===============================================================================
Line
43: .filterRegex("/person/[0-9]*").through(DummyFilterImpl.class)
-------------------------------------------------------------------------------
Done.
Respond to these comments at
http://code.google.com/p/google-guice/source/detail?r=733
--
You received this message because you starred this review, or because
your project has directed all notifications to a mailing list that you
subscribe to.
You may adjust your review notification preferences at:
http://code.google.com/hosting/settings
--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups
"google-guice-dev" group.
To post to this group, send email to [email protected]
To unsubscribe from this group, send email to
[email protected]
For more options, visit this group at
http://groups.google.com/group/google-guice-dev?hl=en
-~----------~----~----~----~------~----~------~--~---
