Adam Winer wrote:
Scott,
My big concern is with the sheer quantity of new public APIs
(that is, public classes in trinidad-api). We should be avoiding making
anything public unless it is absolutely, critically necessary.
Configurator APIs: I'm not completely sold on the name, but anyway,
I think we should:
Got another name you'd rather use?
- Make Configurator an abstract class, not an interface. Make most
of the methods empty, not abstract.
Should be a simple fix
- Add getGlobalConfigurator() to the Configurator API
I'm not a big fan of this. The reason is that I keep running into
instances in the Portal where I may have to add a per "physical" request
type API. Current talk in JSR-301 is that the system will hopefully
provide us with before and after request hooks which are much cleaner
then phase listeners because we can do stuff before and after the
lifecycle executes. The main question still in everyone's mind at this
juncture is whether these will be run at the beginning and end of each
"portal" request or the beginning and end of each physical request.
Current leanings are at the beginning and end of each portal request and
so there is a very real possibility that people might want to store
attributes or whatnot that have the lifetime of the physical request.
Therefore if these or other things arise, having an API for a global
cofigurator will be more flexible in the future because we'll be able to
add to this API would breaking binary compatibility. If we start
returning normal Configurators form these API's it will force us to
"cast" the object or add the methods to the base configurator object.
In short, I think there are (or could be) sufficient difference between
the GlobalConfigurator and the normal Configurator to justify the extra
class even though the API's are not currently present.
- Eliminate GlobalConfigurator and GenericConfigurator classes
I can get rid of Generic Configurator, see above for Global Configurator
TrinidadListener:
- *Definitely* should be in impl, not in api. Register it automatically
by including it in tr.tld.
I was mimicking what we were doing with the Trinidad filter. But moving
it to the TLD is definatly cleaner. I'll do this.
All of the classes in webapp/wrappers and context/external:
- Why aren't these in impl?
- I don't understand at all why we could or should be implementing
ServletExternalContext... that's provided by the impl. And
PortletExternalContext should be provided by the bridge,
or the impl as well if it supports portlets. What am I missing?
I suspect these come from adding TrinidadFacesContext, so...
These are valid questions and I went back and forth on this myself. The
main issue is that the Configurators rely on someone being able to
override the ExternalContext. And while a decorator may be sufficient
for the overriding part, it's kind of silly (in my opinion) to force
everyone to re-implement the pieces of the external context (such as,
say, the RequestParameterMap) which is why those are public. As for the
ServletExternalContext and the PortletExternalContext, if you look at
the API for configurators again, they require us to supply an external
context. In order to maintain compatibility with the servlet usecases
that previous versions of Trinidad supported, we essentially need to
construct a valid ExternalContext within the filter (Sevlet or future
Portlet) and supply it to the configurators. It's better to provide
implementations of these rather then rely on having to make them yourself.
Now that being said, I moved the external context classes over to public
as a while. While I believe we need the decorators and some of the map
objects public, we can probably move the full ExternalContext
implementations in impl (or in the case of the Portal one which was
provided for completeness, remove it since I don't think it's used
anywhere currently (I'll check).
- TrinidadFacesContext: why can't you just use the regular
FacesContextFactory, as we're doing today? Almost any
solution is better than duplicating large amounts of impl code.
This is a very good question and one that I thought a lot about. First
off, this class is used within the resource servlets. Faces itself is
designed with the idea of allowing renderkits to extend the framework as
they need to. In theory (and I know the reality is somewhat different),
but someone could add two renderkits to a particular web-app and use
them. The problem is that the FacesContextFactory takes all of these
entensions into account when returning the context. From a renderkit
perspective, this is good because you hopefully have any functionality
that your renderkit provides, somewhere in the Chain of FacesContexts.
The problem, however, is that our Resource servlets belong to Trinidad
only. This means that there is no reason for us to have to go through
all the external-context wrapping that other renderkits might tack on
through their own custom factories. This adds bloat when there really
doesn't need to be.
From a Portal perspectivethis is even more important. Trinidad, up
until this project, has been able to limit exposure to the "wrapping"
problem by relying on the fact that the filters are run only before and
after a call to the "faces servlet" essentially. Being that there is no
equivalent in the Portal environment, modifications have had to be made
to allow Configurators to be run through a custom FacesContextFactory.
This means that even within our own renderkit, things like skinning,
file upload handling, and RequestContext initialization (as well as the
running of additional services) all happen when these external resources
retrieve the FacesContext from the factory.
As far as I'm aware, there is no sure-fire way in Faces to say, "give me
a FacesContext, but ignore all other FacesContext wrappers from everyone
else". If there was a way to get the "default" FacesContext then I
would agree with you.
ExternalContextUtils:
- To what extent does this really need to be in API?
- In particular, I'd rather not expose any of the methods
that are getting added to ExternalContext in 1.2:
- getRequestCharacterEncoding()
- getRequestContentType()
... but in general, I'd rather not expose anything here
as a public API unless absolutely necessary.
LOL Yeah, let me check what the exposure is. When I wrote the utility,
I was using it from something inside of the API package, but I think
that this can be solved by using RequestContext. Everything else that
needs to use this should be inside impl. I'll take a look at why it's
still public and get back to you.
- A Coding surprise: you may not call
request.getClass().getMethod(). Doesn't work, sadly, because
the defining class might be package-private. You have to
get the API directly from ServletRequest.class, PortletRequest.class,
etc.
Good point. Since I'm going to have to so a series of instanceof's
however, I may as well just cast them. I was hoping to make this faster
rather then bogging the system down with casts. But in light of this I
suppose I'll just have to bite the bullet and do it. Thanks for taking
a look at this Adam. I look forward to your reply.
Scott
