dev@javaserverfaces.java.net

Re: Request for code review for issue 1696

From: Ed Burns <ed.burns_at_sun.com>
Date: Wed, 8 Sep 2010 18:50:00 -0700

>>>>> On Wed, 08 Sep 2010 11:12:28 -0700, Sheetal Vartak <sheetal.vartak_at_oracle.com> said:

SV> If the action on the button is not specified correctly i.e. in our
SV> case the wrong action is "/submit.xhtml". This does not get resolved
SV> to anything during the Invoke App phase's handling of the
SV> navigation. In this case, we need to see a good error message rather
SV> than the NPE which is currently being thrown.

SV> Index: jsf-ri/src/main/java/com/sun/faces/facelets/tag/jsf/ComponentTagHandlerDelegateImpl.java
SV> ===================================================================
SV> --- jsf-ri/src/main/java/com/sun/faces/facelets/tag/jsf/ComponentTagHandlerDelegateImpl.java (revision 8594)
SV> +++ jsf-ri/src/main/java/com/sun/faces/facelets/tag/jsf/ComponentTagHandlerDelegateImpl.java (working copy)
SV> @@ -174,6 +174,8 @@
SV> if (c instanceof NamingContainer) {
SV> oldUnique = ComponentSupport.setNeedUniqueIds(ctx, false);
SV> setUniqueIds = true;
SV> + if (createCompositeComponentDelegate != null)
SV> + createCompositeComponentDelegate.setCompositeComponent(c);

First, our coding style (even though some find it it unreadable)
dictates we always use curly braces with an if/else statement.

Second, it's not appropriate to assume that just because c is a
NamingContainer it is a composite component. Please add an additional
operand to your conditional UIComponent.isCompositeComponent(c).

Like this:

           if (createCompositeComponentDelegate != null &&
               UIComponent.isCompositeComponent(c)) {
               createCompositeComponentDelegate.setCompositeComponent(c);
           }

Otherwise, looks good. r=edburns


Ed

-- 
| ed.burns_at_sun.com | office: +1 407 458 0017
| homepage:               | http://ridingthecrest.com/
|  0 work days until JSF 2.1 Milestone 5