On 9/29/06, Mario Ivankovits <[EMAIL PROTECTED]> wrote:
Hi! > Yes, I did. > > Its a very simple one. > > > No, I totally disagree. > > There is *zero* documentation on what the changes attempt to accomplish. There is a one-liner above the code, maybe not the best english, but hey .... its not zero ;-)
Looking from the perspective of someone reviewing the commit notification related to this patch (if it were applied), this is *effectively* zero because it does not establish any motivation for why the change should be made in the first place.
Besides that, there seem to be a large number of changes that are > (from the > viewpoint of an external person) totally gratuitous -- such as > changing data > types from List<...> to Collection<...>. This was required to being able to use a TreeSet instead of a list to avoid duplicate adding of jars.
Totally unnecessary, since the outer loop is iterating over the jar files that exist in WEB-INF/lib and will never process the same jar twice.
As far as I understood your patch, you were planning to remove the > condition > about jars that had a META-INF/faces-conifg.xml file. Is that not the > acase? I didnt remove anything, I just *added* a functionality. Nothing changed to the previous behaviour. This patch and the discussion "[tiger] speedup of startup and extension to view annotation" are two completely different topics.
Then it should be posted on a completely separate JIRA issue. I am complaining about the fact that SHALE-295 makes no attempt to explain *why* the proposed patch should be applied, or *what* the proposed patch is supposed to accomplish. Wihtout those two things, there is no way I'm going to apply it. Now to the patch: We heavily use the javax.faces.CONFIG_FILES context
parameter to configure which faces-config.xml to load. As it is currently, shale-tiger will ignore our jars as we do not have a META-INF/faces-config.xml, instead we placed ours e.g. in content/APP/webconf/faces-config.xml Now by checking also the jars containing faces-config.xml configured by CONFIG_FILES we can make even such a configuration work with shale-tiger. Using your explicitResources() method will not increase or decrease speedup. This is not what the patch tried to address. I hope now its clear what I mean and what this patch do.
The javax.faces.CONFIG_FILES context init parameter is defined to be a comma-separated list of faces-config.xml resoures. You seem to be proposing to overload it as also pointing at jar files to be processed, or package name wildcards to be matched. Those uses are contrary to the documented behavior of this context init paramater (in the JSF Specification). Therefore, at a minimum, a different paramter name MUST be used. Even if a different parameter name is used to specify package names to look at, you still haven't convinced me that the suggested changes will not *hurt* startup performance, rather than *improve* it. Ciao,
Mario
Craig
