On 3/14/13 3:11 PM, Edward Burns wrote:
> At this point, I'm sorry to say that I'm lost about what Leonardo and
> Andy want to do here. ACTION: Andy, please confer with Leonardo *off
> list* and come back with your recommendations.
>
I wasn't able to reach Leonardo this afternoon.
Our email exchange basically covered three questions:
1. When should the ClientWindow be created?
2. Which type should be responsible for decoding?
3. Which type should be responsible for encoding the window id into the
url?
For #1, Leonardo argued that the ClientWindow should not be created by
the FacesContextFactory, since this would result in unnecessary
ClientWindow creation during resource requests.
I missed this consideration in my original review and agree with
Leonardo that we want to avoid this.
For #2, Leonardo (and my colleagues here) argued that the ClientWindow
instance should itself be responsible for decoding. Although I argued
against this in my original review, I conceded the point to Leonardo and
company.
For #3, my feeling is that the cleanest solution is to add a parallel
ClientWindow.encode(FacesContext, String url) to match the decode().
Leonardo is opposed to this due to performance concerns. Although I
don't think that this would lead to a performance problems, I am not
going to be able to convince Leonardo of that without time to do
performance testing, which we don't have now. So, this is out.
A fallback that we discussed would be to spec
ClientWindow.getQueryURLParameters() such that it returns a Map
containing not just bonus query parameters, but the client window id
parameter as well. This gives ClientWindow implementations more control
over how its state is represented in the url, which I prefer.
Leonardo is happy with this alternative.
I am attaching a Mojarra patch which does the following:
To address #1, the patch restores Mojarra's original
Lifecycle.attachWindow() API and implementation and reverts the
FacesContextFactory changes. One difference between this new version of
attachWindow() and the prior version is that attachWindow() now uses the
ClientWindowFactory to instantiate ClientWindow instances. Other than
that, this is the same as the original Mojarra implementation.
To address #2, the patch restores Mojarra's original decode() API and
implementation.
To address #3, I attempted to implement my (alternate) desired behavior
of embedding the client window id inside of the getQueryURLParameters()
map. I was able to implement this for
ExternalContextImpl.encodeActionURL(), but then grew frustrated when I
saw that a variation of this code is also present in Mojarra's internal
UrlBuilder class. I did a quick attempt at refactoring this code, but I
wasn't able to get my head around some of the complexities of this class.
And, so, I give up.
We can punt on #3 and chalk it up as the price I pay for not paying
attention when these APIs were being designed.
One remaining loose end is whether we should document the limitations of
the url-mode based ClientWindow implementations. Application developers
that decide to enable this mode should be aware of the fact that it does
not provide any guarantee that each browser window/tab will in fact have
its own separate client window id. This seems like important
information to communicate, since these folks will need to be prepared
to deal with the potential confusion that may result.
Andy
Index: jsf-ri/src/main/java/com/sun/faces/context/FacesContextFactoryImpl.java
===================================================================
--- jsf-ri/src/main/java/com/sun/faces/context/FacesContextFactoryImpl.java (revision 11747)
+++ jsf-ri/src/main/java/com/sun/faces/context/FacesContextFactoryImpl.java (working copy)
@@ -41,7 +41,6 @@
package com.sun.faces.context;
-import com.sun.faces.lifecycle.ClientWindowFactoryImpl;
import com.sun.faces.util.Util;
import javax.faces.FacesException;
@@ -50,8 +49,6 @@
import javax.faces.context.ExternalContextFactory;
import javax.faces.context.FacesContext;
import javax.faces.context.FacesContextFactory;
-import javax.faces.lifecycle.ClientWindow;
-import javax.faces.lifecycle.ClientWindowFactory;
import javax.faces.lifecycle.Lifecycle;
public class FacesContextFactoryImpl extends FacesContextFactory {
@@ -60,7 +57,6 @@
private ExceptionHandlerFactory exceptionHandlerFactory;
private ExternalContextFactory externalContextFactory;
- private ClientWindowFactory clientWindowFactory;
// ------------------------------------------------------------ Constructors
@@ -72,14 +68,6 @@
FactoryFinder.getFactory(FactoryFinder.EXCEPTION_HANDLER_FACTORY);
externalContextFactory = (ExternalContextFactory)
FactoryFinder.getFactory(FactoryFinder.EXTERNAL_CONTEXT_FACTORY);
- clientWindowFactory = null;
-
- if (Util.isUnitTestModeEnabled()) {
- clientWindowFactory = new ClientWindowFactoryImpl(false);
- } else {
- clientWindowFactory = (ClientWindowFactory)
- FactoryFinder.getFactory(FactoryFinder.CLIENT_WINDOW_FACTORY);
- }
}
@@ -105,8 +93,6 @@
lifecycle);
ctx.setExceptionHandler(exceptionHandlerFactory.getExceptionHandler());
- ClientWindow cw = clientWindowFactory.getClientWindow(ctx);
- ctx.getExternalContext().setClientWindow(cw);
return ctx;
Index: jsf-ri/src/main/java/com/sun/faces/lifecycle/ClientWindowFactoryImpl.java
===================================================================
--- jsf-ri/src/main/java/com/sun/faces/lifecycle/ClientWindowFactoryImpl.java (revision 11747)
+++ jsf-ri/src/main/java/com/sun/faces/lifecycle/ClientWindowFactoryImpl.java (working copy)
@@ -43,7 +43,6 @@
import com.sun.faces.config.WebConfiguration;
import java.util.Map;
import javax.faces.application.Application;
-import javax.faces.component.UINamingContainer;
import javax.faces.context.ExternalContext;
import javax.faces.context.FacesContext;
import javax.faces.event.AbortProcessingException;
@@ -52,7 +51,6 @@
import javax.faces.event.SystemEventListener;
import javax.faces.lifecycle.ClientWindow;
import javax.faces.lifecycle.ClientWindowFactory;
-import javax.faces.render.ResponseStateManager;
public class ClientWindowFactoryImpl extends ClientWindowFactory {
@@ -98,38 +96,6 @@
return null;
}
- String id = null;
- Map<String, String> requestParamMap = context.getExternalContext().getRequestParameterMap();
- id = requestParamMap.get(ResponseStateManager.CLIENT_WINDOW_URL_PARAM);
- // The hidden field always takes precedence, if present.
- if (requestParamMap.containsKey(ResponseStateManager.CLIENT_WINDOW_PARAM)) {
- id = requestParamMap.get(ResponseStateManager.CLIENT_WINDOW_PARAM);
- }
- if (null == id) {
- id = calculateClientWindow(context);
- }
- ClientWindow result = new ClientWindowImpl(id);
- return result;
+ return new ClientWindowImpl();
}
-
- private String calculateClientWindow(FacesContext context) {
- String id = null;
- final String clientWindowCounterKey = "com.sun.faces.lifecycle.ClientWindowCounterKey";
- ExternalContext extContext = context.getExternalContext();
- Map<String, Object> sessionAttrs = extContext.getSessionMap();
- Integer counter = (Integer) sessionAttrs.get(clientWindowCounterKey);
- if (null == counter) {
- counter = Integer.valueOf(0);
- }
- char sep = UINamingContainer.getSeparatorChar(context);
- id = extContext.getSessionId(true) + sep +
- + counter;
-
- sessionAttrs.put(clientWindowCounterKey, ++counter);
-
- return id;
- }
-
-
-
}
Index: jsf-ri/src/main/java/com/sun/faces/lifecycle/LifecycleImpl.java
===================================================================
--- jsf-ri/src/main/java/com/sun/faces/lifecycle/LifecycleImpl.java (revision 11747)
+++ jsf-ri/src/main/java/com/sun/faces/lifecycle/LifecycleImpl.java (working copy)
@@ -40,6 +40,8 @@
package com.sun.faces.lifecycle;
+import com.sun.faces.util.Util;
+
import com.sun.faces.config.WebConfiguration;
import java.util.List;
import java.util.concurrent.CopyOnWriteArrayList;
@@ -47,6 +49,7 @@
import java.util.logging.Logger;
import javax.faces.FacesException;
+import javax.faces.FactoryFinder;
import javax.faces.context.FacesContext;
import javax.faces.event.PhaseListener;
import javax.faces.lifecycle.Lifecycle;
@@ -60,8 +63,8 @@
import javax.faces.event.SystemEvent;
import javax.faces.event.SystemEventListener;
import javax.faces.lifecycle.ClientWindow;
+import javax.faces.lifecycle.ClientWindowFactory;
-
/**
* <p><b>LifecycleImpl</b> is the stock implementation of the standard
* Lifecycle in the JavaServer Faces RI.</p>
@@ -131,6 +134,47 @@
// ------------------------------------------------------- Lifecycle Methods
+ @Override
+ public void attachWindow(FacesContext context) {
+ if (!isClientWindowEnabled) {
+ return;
+ }
+ if (context == null) {
+ throw new NullPointerException
+ (MessageUtils.getExceptionMessageString
+ (MessageUtils.NULL_PARAMETERS_ERROR_MESSAGE_ID, "context"));
+ }
+
+ ExternalContext extContext = context.getExternalContext();
+ ClientWindow myWindow = extContext.getClientWindow();
+ if (null == myWindow) {
+ myWindow = createClientWindow(context);
+ if (null != myWindow) {
+ myWindow.decode(context);
+ extContext.setClientWindow(myWindow);
+ }
+ }
+
+
+ // If you need to do the "send down the HTML" trick, be sure to
+ // mark responseComplete true after doing so. That way
+ // the remaining lifecycle methods will not execute.
+
+ }
+
+ private ClientWindow createClientWindow(FacesContext context) {
+ ClientWindowFactory clientWindowFactory = null;
+
+ if (Util.isUnitTestModeEnabled()) {
+ clientWindowFactory = new ClientWindowFactoryImpl(false);
+ } else {
+ clientWindowFactory = (ClientWindowFactory)
+ FactoryFinder.getFactory(FactoryFinder.CLIENT_WINDOW_FACTORY);
+ }
+
+ return clientWindowFactory.getClientWindow(context);
+ }
+
// Execute the phases up to but not including Render Response
public void execute(FacesContext context) throws FacesException {
Index: jsf-ri/src/main/java/com/sun/faces/lifecycle/ClientWindowImpl.java
===================================================================
--- jsf-ri/src/main/java/com/sun/faces/lifecycle/ClientWindowImpl.java (revision 11747)
+++ jsf-ri/src/main/java/com/sun/faces/lifecycle/ClientWindowImpl.java (working copy)
@@ -40,27 +40,61 @@
*/
package com.sun.faces.lifecycle;
-import java.util.Collections;
import java.util.Map;
+import javax.faces.component.UINamingContainer;
+import javax.faces.context.ExternalContext;
import javax.faces.context.FacesContext;
import javax.faces.lifecycle.ClientWindow;
+import javax.faces.render.ResponseStateManager;
public class ClientWindowImpl extends ClientWindow {
String id;
- ClientWindowImpl(String id) {
- this.id = id;
+ public ClientWindowImpl() {
}
@Override
public Map<String, String> getQueryURLParameters(FacesContext context) {
- return Collections.emptyMap();
+ return null;
}
+
+
@Override
+ public void decode(FacesContext context) {
+ Map<String, String> requestParamMap = context.getExternalContext().getRequestParameterMap();
+ if (isClientWindowRenderModeEnabled(context)) {
+ id = requestParamMap.get(ResponseStateManager.CLIENT_WINDOW_URL_PARAM);
+ }
+ // The hidden field always takes precedence, if present.
+ if (requestParamMap.containsKey(ResponseStateManager.CLIENT_WINDOW_PARAM)) {
+ id = requestParamMap.get(ResponseStateManager.CLIENT_WINDOW_PARAM);
+ }
+ if (null == id) {
+ id = calculateClientWindow(context);
+ }
+ }
+
+ private String calculateClientWindow(FacesContext context) {
+ final String clientWindowCounterKey = "com.sun.faces.lifecycle.ClientWindowCounterKey";
+ ExternalContext extContext = context.getExternalContext();
+ Map<String, Object> sessionAttrs = extContext.getSessionMap();
+ Integer counter = (Integer) sessionAttrs.get(clientWindowCounterKey);
+ if (null == counter) {
+ counter = Integer.valueOf(0);
+ }
+ char sep = UINamingContainer.getSeparatorChar(context);
+ id = extContext.getSessionId(true) + sep +
+ + counter;
+
+ sessionAttrs.put(clientWindowCounterKey, ++counter);
+
+ return id;
+ }
+
+ @Override
public String getId() {
return id;
}
-
}
Index: jsf-api/src/main/java/javax/faces/lifecycle/Lifecycle.java
===================================================================
--- jsf-api/src/main/java/javax/faces/lifecycle/Lifecycle.java (revision 11747)
+++ jsf-api/src/main/java/javax/faces/lifecycle/Lifecycle.java (working copy)
@@ -97,8 +97,31 @@
* is <code>null</code>
*/
public abstract void execute(FacesContext context) throws FacesException;
-
+
+
/**
+ * <p class="changed_added_2_2">Create or restore the {_at_link
+ * ClientWindow} to be used to display the {_at_link
+ * javax.faces.component.UIViewRoot} for this run through the
+ * lifecycle. See the class documentation for {_at_link ClientWindow}
+ * for an overview of the feature.
+ *
+ * If {_at_link javax.faces.context.ExternalContext#getClientWindow()} returns
+ * null, create a new instance of <code>ClientWindow</code> using the
+ * {_at_link ClientWindowFactory}. If the result is non-null, call
+ * {_at_link ClientWindow#decode(javax.faces.context.FacesContext)} on it.
+ * Store the new <code>ClientWindow</code> by calling
+ * {_at_link javax.faces.context.ExternalContext#setClientWindow(javax.faces.lifecycle.ClientWindow)}.</p>
+ *
+ *
+ * @since 2.2
+ */
+
+ public void attachWindow(FacesContext context) {
+ }
+
+
+ /**
* <p>Return the set of registered {_at_link PhaseListener}s for this
* {_at_link Lifecycle} instance. If there are no registered listeners,
* a zero-length array is returned.</p>
Index: jsf-api/src/main/java/javax/faces/lifecycle/ClientWindow.java
===================================================================
--- jsf-api/src/main/java/javax/faces/lifecycle/ClientWindow.java (revision 11747)
+++ jsf-api/src/main/java/javax/faces/lifecycle/ClientWindow.java (working copy)
@@ -81,10 +81,9 @@
* lifetime.</p>
* <p>The <code>ClientWindow</code> instance is associated with the
- * incoming request by virtue of action taken in {_at_link
- * javax.faces.context.FacesContextFactory#getFacesContext}. This
- * method will cause a new instance of <code>ClientWindow</code> to be
- * created, assigned an id, and passed to {_at_link
+ * incoming request during the {_at_link Lifecycle#attachWindow} method.
+ * This method will cause a new instance of <code>ClientWindow</code> to
+ * be created, assigned an id, and passed to {_at_link
* javax.faces.context.ExternalContext#setClientWindow}.</p>
* <p>During state saving, regardless of the window id mode, or state
@@ -141,18 +140,42 @@
*/
public abstract Map<String, String> getQueryURLParameters(FacesContext context);
-
+
/**
- * <p class="changed_added_2_2">Return a String value that uniquely
- * identifies this <code>ClientWindow</code> within the scope of the
- * current session. See {_at_link ClientWindowFactory#getClientWindow}
- * for the specification of how to derive this value.</p>
+ * <p class="changed_added_2_2">Return a String value that uniquely
+ * identifies this <code>ClientWindow</code>
+ * within the scope of the current session. See {_at_link #decode} for the
+ * specification of how to derive this value.</p>
*
* @since 2.2
*/
public abstract String getId();
+ /**
+ * <p class="changed_added_2_2">The implementation is responsible
+ * for examining the incoming request and extracting the value that must
+ * be returned from the {_at_link #getId} method. If {_at_link #CLIENT_WINDOW_MODE_PARAM_NAME}
+ * is "none" this method must not be invoked. If {_at_link #CLIENT_WINDOW_MODE_PARAM_NAME}
+ * is "url" the implementation must first look for a request parameter
+ * under the name given by the value of {_at_link javax.faces.render.ResponseStateManager#CLIENT_WINDOW_PARAM}.
+ * If no value is found, look for a request parameter under the name given
+ * by the value of {_at_link javax.faces.render.ResponseStateManager#CLIENT_WINDOW_URL_PARAM}.
+ * If no value is found, fabricate an id that uniquely identifies this
+ * <code>ClientWindow</code> within the scope of the current session. This
+ * value must be encrypted with a key stored in the http session and made
+ * available to return from the {_at_link #getId} method. The value must be
+ * suitable for inclusion as a hidden field or query parameter.
+ * If a value is found, decrypt it using the key from the session and
+ * make it available for return from {_at_link #getId}.</p>
+ *
+ * @param context the {_at_link FacesContext} for this request.
+ *
+ * @since 2.2
+ */
+
+ public abstract void decode(FacesContext context);
+
private static final String PER_USE_CLIENT_WINDOW_URL_QUERY_PARAMETER_DISABLED_KEY =
"javax.faces.lifecycle.ClientWindowRenderModeEnablement";
Index: jsf-api/src/main/java/javax/faces/lifecycle/ClientWindowWrapper.java
===================================================================
--- jsf-api/src/main/java/javax/faces/lifecycle/ClientWindowWrapper.java (revision 11747)
+++ jsf-api/src/main/java/javax/faces/lifecycle/ClientWindowWrapper.java (working copy)
@@ -79,6 +79,10 @@
return getWrapped().isClientWindowRenderModeEnabled(context);
}
+ @Override
+ public void decode(FacesContext context) {
+ getWrapped().decode(context);
+ }
+
-
}
Index: jsf-api/src/main/java/javax/faces/lifecycle/ClientWindowFactory.java
===================================================================
--- jsf-api/src/main/java/javax/faces/lifecycle/ClientWindowFactory.java (revision 11747)
+++ jsf-api/src/main/java/javax/faces/lifecycle/ClientWindowFactory.java (working copy)
@@ -69,20 +69,13 @@
/**
* <p class="changed_added_2_2">The implementation is responsible
- * for examining the incoming request and extracting the value that must
- * be returned from the {_at_link ClientWindow#getId} method. If {_at_link ClientWindow#CLIENT_WINDOW_MODE_PARAM_NAME}
- * is "none" this method must return {_at_code null}. If {_at_link ClientWindow#CLIENT_WINDOW_MODE_PARAM_NAME}
- * is "url" the implementation must first look for a request parameter
- * under the name given by the value of {_at_link javax.faces.render.ResponseStateManager#CLIENT_WINDOW_PARAM}.
- * If no value is found, look for a request parameter under the name given
- * by the value of {_at_link javax.faces.render.ResponseStateManager#CLIENT_WINDOW_URL_PARAM}.
- * If no value is found, fabricate an id that uniquely identifies this
- * <code>ClientWindow</code> within the scope of the current session. This
- * value must be encrypted with a key stored in the http session and made
- * available to return from the {_at_link ClientWindow#getId} method. The value must be
- * suitable for inclusion as a hidden field or query parameter.
- * If a value is found, decrypt it using the key from the session and
- * make it available for return from {_at_link ClientWindow#getId}.</p>
+ * for creating the {_at_link ClientWindow} instance for this request.
+ * If {_at_link ClientWindow#CLIENT_WINDOW_MODE_PARAM_NAME}
+ * is "none" or unspecified, this method must return {_at_code null}.
+ * If {_at_link ClientWindow#CLIENT_WINDOW_MODE_PARAM_NAME}
+ * is "url" the implementation must return a <code>ClientWindow</code>
+ * instance that implements the url-mode semantics described in
+ * {_at_link ClientWindow}.
*
* @param context the {_at_link FacesContext} for this request.
* @return the {_at_link ClientWindow} for this request, or {_at_code null}
Index: jsf-api/src/main/java/javax/faces/webapp/FacesServlet.java
===================================================================
--- jsf-api/src/main/java/javax/faces/webapp/FacesServlet.java (revision 11747)
+++ jsf-api/src/main/java/javax/faces/webapp/FacesServlet.java (working copy)
@@ -556,7 +556,9 @@
* javax.faces.application.ResourceHandler#isResourceRequest}. If
* this returns <code>true</code> call {_at_link
* javax.faces.application.ResourceHandler#handleResourceRequest}.
- * If this returns <code>false</code>,
+ * If this returns <code>false</code>, <span
+ * class="changed_added_2_2">call {_at_link
+ * javax.faces.lifecycle.Lifecycle#attachWindow} followed by </span>
* {_at_link javax.faces.lifecycle.Lifecycle#execute} followed by
* {_at_link javax.faces.lifecycle.Lifecycle#render}. If a {_at_link
* javax.faces.FacesException} is thrown in either case, extract the
@@ -640,6 +642,7 @@
if (handler.isResourceRequest(context)) {
handler.handleResourceRequest(context);
} else {
+ lifecycle.attachWindow(context);
lifecycle.execute(context);
lifecycle.render(context);
}