[
https://issues.apache.org/jira/browse/MYFACES-2785?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12885810#action_12885810
]
Leonardo Uribe commented on MYFACES-2785:
-----------------------------------------
I have some comments to do about how to enhance the proposed patch.
- In the patch there is an static method added called:
public static FacesInitializer createFacesInitializer(ServletContext
context)
Really this is an utility method called from other places to retrieve the
"default" FacesInitializer to be used. Maybe we could call it
createDefaultFacesInitializer. In theory we should allow custom initialization
of myfaces, but the strategy we are using right now is not very "friendly". In
fact, StartupServletContextListener call initialization code but there is no
way to plug the FacesInitializer to be used. For example, there is no way to
force myfaces to use Jsp20FacesInitializer, instead we had to include some
special code to detect Google Application Engine usage.
A "factory" o "builder" method should be on a separate class, or even better,
if we have multiple FacesInitializer instances we should provide a factory for
that, like we have in
org.apache.myfaces.config.annotation.LifecycleProviderFactory.
I think we should move some code from commons-discovery to myfaces, because
commons-discovery dependency cause commons-logging to be added as a transitive
dependency, and the objective in myfaces 2.0.x is use java util logging.
- about UIViewRoot creation on StartupFacesContext: I think it is better the
way we had before. The reason is UIViewRoot is to FacesContext as a Person is
to a Car, not as Wheel to a Car. A wheel is part of a car but a UIViewRoot is
not a part of a FacesContext. Obviously, the proposal about create it
automatically does not harm in that specific case, but note it is not a good
OO design. There is not a big reason to "lazy load" a UIViewRoot instance on
startup.
The testing code looks good.
> Clean up initialization code and add tests for StartupServletContextListener
> and MyFacesServlet
> -----------------------------------------------------------------------------------------------
>
> Key: MYFACES-2785
> URL: https://issues.apache.org/jira/browse/MYFACES-2785
> Project: MyFaces Core
> Issue Type: Task
> Components: JSR-314
> Affects Versions: 2.0.1-SNAPSHOT
> Reporter: Jakob Korherr
> Assignee: Jakob Korherr
> Attachments: MYFACES-2785.patch
>
>
> Some major code clean up on the initialization of MyFaces:
> - The solution for startup and shutdown FacesContext implementations
> (MYFACES-2730) introduced some duplicate code on StartupFacesContextImpl and
> FacesContextImpl. This can be solved by providing a base implementation class
> (like the one in for StartupExternalContext). This will make maintaining the
> two FacesContext implementation very easy, because there are no duplicate
> methods (except for getViewRoot() on StartupFacesContextImpl).
> - JUnit tests are needed to verify the behavior of
> StartupServletContextListener and MyFacesServlet and to check if the
> FacesContext is available on startup and shutdown
> - AbstractFacesInitializer should provide a static method to get the right
> FacesInitializer impl instead of having several duplicate methods in
> StartupServletContextListener and MyFacesServlet that do nothing but getting
> the right impl and invoking some method on it.
> - AbstractFacesInitializer.dispatchInitDestroyEvent should use the
> application object from the StartupFacesContextImpl and not directly from the
> factory
> - AbstractFacesInitializer.getLifecycleId() is unused because of MYFACES-2730
> - initStartupFacesContext() and initShutdownFacesContext() should set the
> field startup correctly (true or false) and should not create the UIViewRoot
> directly (it should be created in StartupFacesContextImpl at first access)
> - minor javadoc copy and paste error on FacesInitializer
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
