incubator-adffaces-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Scott O'Bryan" <darkar...@gmail.com>
Subject Re: [PORTAL] Changes and API review?
Date Fri, 15 Dec 2006 19:59:02 GMT
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
>


Mime
View raw message