myfaces-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Leonardo Uribe (JIRA)" <...@myfaces.apache.org>
Subject [jira] Commented: (MYFACES-2785) Clean up initialization code and add tests for StartupServletContextListener and MyFacesServlet
Date Wed, 07 Jul 2010 03:51:58 GMT

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


Mime
View raw message