>>>>> On Mon, 11 Mar 2013 15:25:39 -0400, Andy Schwartz <andy.schwartz_at_oracle.com> said:
AS> Gang -
AS> A likely pointless review of ClientWindow, given how late this is
AS> arriving. Nonetheless, here goes:
AS> Index: javax/faces/lifecycle/ClientWindow.java
AS> ===================================================================
AS> --- javax/faces/lifecycle/ClientWindow.java (revision 0)
AS> +++ javax/faces/lifecycle/ClientWindow.java (revision 11719)
AS> + * <p>To accomadate the widest possible range of implementation choices
AS> + * to support this feature, explicit names for modes other than "none"
AS> + * are not specified.
AS> Below we state that an "url" mode is supported. Which is correct?
Some volunteers were pushing for just "none". Some were pushing for
just "none" and "url". At this point, I'm going to choose "none" and
"url". I've updated the docs.
AS> + * <p>In addition to the hidden field already described. The runtime
AS> + * must ensure that any component that renders a hyperlink that causes
AS> + * the user agent to send a GET request to the Faces server when it is
AS> + * clicked has a query parameter with a name and value specified in
AS> + * {_at_link ResponseStateManager#CLIENT_WINDOW_URL_PARAM}. This
AS> + * requirement is met by several of the "encode" methods on {_at_link
AS> + * javax.faces.context.ExternalContext}. See {_at_link
AS> + * javax.faces.context.ExternalContext#encodeActionURL(java.lang.String)
AS> + * } for details.</p>
AS> What is the expected behavior when the end user creates a second browser
AS> tab/window with an existing window id? For example, this can happen by:
AS> - Bookmarking an url with a window id and opening that URL in multiple tabs.
AS> - Copying and pasting an url from one tab into a second tab.
AS> - (On some browsers) Using the browser's "New Window" action.
AS> For each of the above cases, each window/tab should be assigned its own
AS> unique id, even though it would require additional work to determine
AS> when a client window id has been copied across windows/tabs. Will JSF's
AS> client window implementation handle this? If not, we run the risk of
AS> severely hosing our end users.
This is something we'll have to clarify in the next release. I have
opened <
http://java.net/jira/browse/JAVASERVERFACES_SPEC_PUBLIC-1172>.
AS> + /**
AS> + * <p class="changed_added_2_2">The context-param that controls the
AS> operation
AS> + * of the <code>ClientWindow</code> feature. The runtime must support
AS> + * the values "none" and "url", without the quotes, but other values
AS> + * are possible. If not specified, "none" is assumed.</p>
AS> + *
AS> + * @since 2.2
AS> + */
AS> + public static final String CLIENT_WINDOW_MODE_PARAM_NAME =
AS> What should the JSF implementation do with a value that it does not
AS> recognize?
I have modified the text to say that "none" is assumed in that case.
AS> + /**
AS> + * <p class="changed_added_2_2">This method will be called whenever
AS> a URL
AS> + * is generated by the runtime where client window related
AS> parameters need
AS> + * to be inserted into the URL. This guarantees custom {_at_code
AS> ClientWindow} implementations
AS> + * that they will have the opportunity to insert any additional
AS> client window specific
AS> + * information in any case where a URL is generated, such as the
AS> rendering
AS> + * of hyperlinks. The default implementation of this method returns
AS> + * the {_at_code null}.</p>
AS> + *
AS> + *
AS> + * @since 2.2
AS> + * @param context the {_at_code FacesContext} for this request.
AS> + * @return {_at_code null} or a map of parameters to insert into the
AS> URL query string.
AS> + */
AS> +
AS> + public abstract Map<String, String>
AS> getQueryURLParameters(FacesContext context);
AS> Is the returned Map mutable? My thinking is that:
AS> - The returned Map should be immutable. And...
AS> - The method should be spec'ed to return a non-null value.
AS> An alternate approach would be to avoid returning a Map altogether and
AS> instead ask the ClientWindow to tack its parameters onto a provided base
AS> URL, similar to our other encode*URL APIs. This would simplify life for
AS> client code (eg. no need to create an Iterator, iterate over parameter
AS> names, check for already existing parameters, etc...)
This was the API suggested by Leonardo as an extension mechanism. The
idea was to provide the method and specify the places where it must be
called so that custom ClientWindow implementations can rest assured that
the method will be called. Because we can't assert that the map is
immutable, I don't want to require it. Also, for performance I think
checking for a null return is quicker than an empty map.
AS> + /**
AS> + * <p class="changed_added_2_2">The implementation is responsible
AS> + * for examining the incoming request and extracting the value that
AS> must
AS> + * be returned from the {_at_link #getId} method. If {_at_link
AS> #CLIENT_WINDOW_MODE_PARAM_NAME}
AS> + * is "none" this method must not be invoked. If {_at_link
AS> #CLIENT_WINDOW_MODE_PARAM_NAME}
AS> + * is "url" the implementation must first look for a request parameter
AS> + * under the name given by the value of {_at_link
AS> javax.faces.render.ResponseStateManager#CLIENT_WINDOW_PARAM}.
AS> + * If no value is found, look for a request parameter under the
AS> name given
AS> + * by the value of {_at_link
AS> javax.faces.render.ResponseStateManager#CLIENT_WINDOW_URL_PARAM}.
AS> + * If no value is found, fabricate an id that uniquely identifies this
AS> + * <code>ClientWindow</code> within the scope of the current
AS> session. This
AS> + * value must be encrypted with a key stored in the http session
AS> and made
AS> + * available to return from the {_at_link #getId} method. The value
AS> must be
AS> + * suitable for inclusion as a hidden field or query parameter.
AS> + * If a value is found, decrypt it using the key from the session and
AS> + * make it available for return from {_at_link #getId}.</p>
AS> + *
AS> + * @param context the {_at_link FacesContext} for this request.
AS> + *
AS> + * @since 2.2
AS> + */
AS> +
AS> + public abstract void decode(FacesContext context);
AS> Currently ClientWindow creation/initialization is buried in
AS> LifecycleImpl.attachWindow(). This makes it difficult to provide custom
AS> ClientWindow implementations, as it requires that you replace or wrap
AS> the Lifecycle. It also assumes that you are only creating ClientWindows
AS> from within the JSF lifecycle. Based on our experience with Trinidad's
AS> window manager, we are likely going to want to create the ClientWindow
AS> earlier (eg. from a filter).
AS> Fortunately, there is a simple solution, which is to follow the typical
AS> JSF factory pattern. We should introduce a ClientWindowFactory that is
AS> responsible for vending ClientWindow instances. The factory can return
AS> the appropriate type of ClientWindow implementation based on the client
AS> window mode (eg. return a stub instance for "none", an url-based
AS> instance for "url", or a 3rd party instance, eg. in the case of Trinidad.)
Done.
AS> This would allow us to kill off some unneeded complexity - eg. we could
AS> kill ClientWindow.decode() - the factory would handle this instead.
Done.
AS> While we're at it, we could kill Lifecycle.attachWindow(), and instead
AS> spec that the ClientWindow is set up during FacesContext creation - eg.
AS> the FacesContextFacotry could set up the ClientWindow on the
AS> ExternalContext before creating the FacesContext instance.
Done.
AS> + /**
AS> + * <p class="changed_added_2_2">Components that permit per-use
AS> disabling
AS> + * of the appending of the ClientWindow in generated URLs must call
AS> this method
AS> + * first before rendering those URLs. The caller must call {_at_link
AS> #enableClientWindowRenderMode(javax.faces.context.FacesContext)}
AS> + * from a <code>finally</code> block after rendering the URL. If
AS> + * {_at_link #CLIENT_WINDOW_MODE_PARAM_NAME} is "url" without the
AS> quotes, all generated
AS> + * URLs that cause a GET request must append the ClientWindow by
AS> default.
AS> + * This is specified as a static method because callsites need to
AS> access it
AS> + * without having access to an actual {_at_code ClientWindow}
AS> instance.</p>
AS> + *
AS> + * @param context the {_at_link FacesContext} for this request.
AS> + *
AS> + * @since 2.2
AS> + */
AS> +
AS> + public static void disableClientWindowRenderMode(FacesContext
AS> context) {
AS> I don't understand why these are static. This explanation:
AS> + * This is specified as a static method because callsites need to
AS> access it
AS> + * without having access to an actual {_at_code ClientWindow}
AS> instance.</p>
AS> Is confusing.
AS> The ClientWindow is currently initialized in FacesServlet.service():
>> if (handler.isResourceRequest(context)) {
>> handler.handleResourceRequest(context);
>> } else {
>> lifecycle.attachWindow(context);
>> lifecycle.execute(context);
>> lifecycle.render(context);
>> }
AS> As such, if you've got access to a FacesContext (as is the case when
AS> disableClientWindowRenderMode is called), you've got access to a
AS> ClientWindow.
AS> If for some reason the ClientWindow is not available (eg. if
AS> facesContext.getExternalContext().getClientWindow() reutrns null), well,
AS> then... there is nothing to disable anyway, right?
AS> At the moment the only place that Mojarra calls
AS> ClientWindow.disableClientWindowRenderMode() is in
AS> OutcomeTargetRenderer.getEncodedTargetURL(), which is called during
AS> encoding. We clearly have access to a ClientWindow here.
AS> What's the deal with making these methods static? From what I can tell
AS> they should be instance methods.
Ok, I can get behind that. Done.
AS> Also, should we spec that when disabled,
AS> ClientWindow.getQueryURLParameters() returns the empty collection? If
AS> we did that, do we even need an isClientWindowRenderModeEnabled() API?
I'd rather keep the method since it was requested by the EG. Remember,
this isn't the U.S. Congress. Compromise is not heresy!
Andy, please review these changes, committed as r11735 on Mojarra trunk.
Ed
--