Further investigation:
PortletExternalContext can be removed. There is a possibility that it
will need to be added again for JSR-301 but that depends on interfaces
which aren't defined yet so I'm cool on getting rid of it till we need it.
The ServletExternalContext is used in the ResourceServlet (which is in
the API package) and the TrinidadFilterImpl (which is in the Impl
package). Should I do some research on how to move this to the Impl
package and still allow it to be called from ResourceServlet? I know
we're still discussing the need to use a custom FacesContext, but if it
turns out we should keep it, we're going to need to do something for the
ExternalContext.
One more point about the TrinidadFacesContext. It replaces the
PseudoFacesContext that is already in Trinidad. The PseudoFacesContext
didn't implement the base FacesContext stuff, but most of the external
context was already handled in that class. The TrinidadFacesContext is
an enhanced version of the PseudoFacesContext only it's not as hokey
because it's a full impl instead of a partial impl AND it can be used
from the resources.
Finally, I looked and there is no reason that ExternalContextUtils
cannot be put into the impl package. It's mostly used for setup of the
RequestContext and some other logic which happens outside of the Faces
scope. So good call.
Scott
Scott O'Bryan wrote:
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
