dev@javaserverfaces.java.net

Re: Seeking retroactive review [419-ImplicitNavigation]

From: Ryan Lubke <Ryan.Lubke_at_Sun.COM>
Date: Wed, 21 Jan 2009 08:40:57 -0800

Ed Burns wrote:
> 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?
>
That's fine.
>
> 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
>
>