Hi Leonardo,
Leonardo Uribe schrieb:
On Wed, Sep 3, 2008 at 2:38 AM, Simon Kitching <[EMAIL PROTECTED]
<mailto:[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.
Sorry, I don't understand what you mean.
To rephrase what I said above:
* I suggested a change to re-enable the ExtensionsFilter
* The change was made
* This is good
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.
I did, right before writing the email :-)
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.
I did make a suggestion. And I thought you agreed.
The suggestion was for the TomahawkFacesContext* classes to simply use
ServletContext.getInitParameter -- for servlet config
PortletContext.getInitParameter -- for portlet config
As I pointed out earlier, we *do* need to be backwards-compatible for
code that still uses ExtensionsFilter. But because the old filter code
has been restored, this continues to work as before.
For new code, the config params can just be configured as global options
rather than as options within a filter-mapping. Because the ability to
use tomahawk "extensions" functionality without a filter-mapping is new,
there is no backwards-compatibility requirement.
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.
Another interesting question would be: what other new classes have been
added?
A clirr report would be useful here...
Regards,
Simon
