On Wed, Sep 3, 2008 at 2:38 AM, Simon Kitching <[EMAIL PROTECTED]> wrote:
> Sorry, but I must vote -1 on this. > > Most of the issues I raised earlier about the ExtensionsFilter refactoring > have still not been addressed. > One change has been made: the ExtensionsFilter now does actually do the > work, and the FacesContextFactory does not wrap the object in that case. > I believe that you didn't see the latest code. Please check it. > > But the rest of the concerns I had still exist. In particular, there is a > large amount of new code that exists to parse the web.xml file looking for > config properties - and I don't see why any of this code should exist. I had > thought that an earlier email by you (Leonardo) meant that you agreed and > would change this to just looking for servlet init parameters via the normal > APIs. > Check the code please. > > In particular, I think we should delete the ExtensionsFilterConfig class > completely. > I'm open to suggestions, but no alternative has been put on the table. > > Minor points: > * Why is new class AbstractAttributeMap in the "servlet" package? > * Should AbstractAttributeMap be named "_AbstractAttributeMap" instead, ie > is it part of tomahawk's "stable public api" or just an internal > implementation detail? > * New classes should have @since annotations > * etc Any of this points has been proposed before. The problem is if the points are blocker or not for a release. > > Regards, > Simon > >
