dev@javaserverfaces.java.net

Re: Seeking retroactive review [419-ImplicitNavigation]

From: Ed Burns <Ed.Burns_at_Sun.COM>
Date: Wed, 21 Jan 2009 08:18:44 -0800

I've filed impl task
<https://javaserverfaces.dev.java.net/issues/show_bug.cgi?id=937> on
this, assigned to me.

Ryan, can I do it after the PFD handoff?


EB> SECTION: Modified Files
EB> ----------------------------
EB> M jsf-api/src/javax/faces/application/ViewHandler.java
EB>
EB> - declare new method deriveViewId()
EB>
RL> Why no implementaion of this method in the API proper?

I'll add one that just returns the argument input.

EB> M jsf-ri/src/com/sun/faces/application/view/MultiViewHandler.java

EB> - implement deriveViewId(), but this calls through to Util.

RL> See above.

RL> Also you have:

RL> +
RL> + public void setRestoreViewPhase(RestoreViewPhase restoreViewPhase) {
RL> +
RL> }

RL> What is that?

Leftover IDE cruft. I'll remove it, sorry.
 
EB> M jsf-ri/src/com/sun/faces/config/WebConfiguration.java
EB>
EB> - getter for "list like" context-param values. Initialize a cache of such
EB> things in the ctor
EB>

RL> + private Map<WebContextInitParameter, String []> cachedListParams;
RL> +
RL> + public String [] getOptionValue(WebContextInitParameter param, String sep) {
RL> + String [] result = null;
RL> +
RL> + assert(null != cachedListParams);
RL> + if (null == (result = cachedListParams.get(param))) {
RL> + String value = getOptionValue(param);
RL> + if (null == value) {
RL> + result = new String[0];
RL> + } else {
RL> + result = value.split(sep);
RL> + }
RL> + cachedListParams.put(param, result);
RL> + }
RL> +
RL> + return result;
RL> + }

RL> Use Util.split() not String.split.

Ok, I'll do that.

RL> Also, minor nit, but it will save me going back and doing it is keep
RL> the structure of the class consistent. Private members are declared
RL> at the top, not inline.

Yes, I'll fix that.

EB> A jsf-ri/systest-per-webapp/implicit-navigation/src/java/com/sun/faces/systest/ImplicitNavigationTestCase.java

EB> - Execute the new code PENDING(edburns) need automated test

RL> I would much rather see this in the NavigationHandler cactus test
RL> case unless there is a reason for having a separate webapp for this.
RL> Each new systest-per-webapp slows down the testing cycle, so I would
RL> rather we were conservative on adding these.

Ok, I'll fix that.

Ed

-- 
| ed.burns_at_sun.com  | office: 408 884 9519 OR x31640
| homepage:         | http://ridingthecrest.com/