incubator-adffaces-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Adam Winer" <awi...@gmail.com>
Subject Re: Re: [PORTAL] Changes and API review?
Date Fri, 15 Dec 2006 19:07:52 GMT
On 12/15/06, Scott O'Bryan <darkarena@gmail.com> wrote:
> 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.

I don't see why our "global" configurator will have to
comply with this API.  It's our own thing.  We don't
need to comply or not comply with any external API.

So, nope, please kill it.

> > - 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).

NONE of it should be public unless it is 100%, no two ways
about it, absolutely required.

> > - 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.

Then set a request attribute, whatever, to turn off our wrappers.
Any behind-the-scenes fiddling is WAAAY better than all of
this extra code.

-- Adam


> > 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