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 17:08:42 GMT
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

Mime
View raw message