Hi Leonardo -
Thanks for reviewing my review!
On 3/14/13 12:08 AM, Leonardo Uribe wrote:
> Hi
>
> -------------------------------------------------------------------------
> Please read this mail, specially the part when I explain the difference
> between getQueryURLParameters() and isClientWindowRenderModeEnabled().
>
> Don't remove these methods!!!
>
Don't worry - Ed is not planning to remove either of these.
> -------------------------------------------------------------------------
>
> AS>> Below we state that an "url" mode is supported. Which is correct?
>
> The idea is different implementation can provide multiple modes. For example,
> in MyFaces we have "url" and "client" mode, which is a variant of the hack
> developed in MyFaces CODI. In that sense, if the param is not set or it has
> "none" as value, ClientWindow stuff is disabled.
>
> I suppose the "url" mode is the most common one (used in MyFaces Orchestra
> as "conversationContext"), but by default ClientWindow stuff is disabled.
>
> The problem here is each mode has its advantages/disadvantages. There is no
> mode that can be perfect, so the idea in this API is cover a wide range of
> possible algorithms.
>
Got it.
> EB>> Some volunteers were pushing for just "none". Some were pushing for
> EB>> just "none" and "url". At this point, I'm going to choose "none" and
> EB>> "url". I've updated the docs.
>
> Ok.
>
> 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
> AS>> own 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
>
> It depends on the mode selected.
>
Right. I was wondering how the single standard mode ("url" mode) would
handle these cases. Seems that the answer is: not well, but that's okay
because other implementations are possible.
> AS>> What should the JSF implementation do with a value that it does not
> AS>> recognize?
>
> It assumes there is an attached ClientWindow implementation with that value
> as identifier. In other words, Lifecycle.attachWindow() method will check
> the param, take the value and create a proper ClientWindow instance.
>
Okay. I still like the idea of exposing a ClientWindowFactory that can
examine the param and pick the appropriate ClientWindow implementation
(including possibly a custom ClientWindow implementation).
> AS>> + + public abstract Map<String, String>
> 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.
>
> It is inmutable. I don't have any problem with return a non-null value.
>
Great.
> The idea of the method is allow ClientWindow implementations to hook
> query params into generated links. ClientWindow implementation can
> choose between use it or not. For example, a ClientWindow implementation
> could use an css class to recognize links and add the query param
> on the client using javascript instead generate it on the server
> (Mark Struberg has a project working under that idea).
>
Interesting.
> 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
> AS>> base URL, similar to our other encode*URL APIs. This would simplify
> AS>> life for client code (eg. no need to create an Iterator, iterate
> AS>> over parameter names, check for already existing parameters, etc...)
>
> The calculation for query params requires some string concatenation that is
> best done in one place (inside ExternalContext). In this way, a lot less
> objects needs to be created when calculating the link, so the extra work
> will definitively pay off.
>
I've been discussing the ClientWindow API with two of my colleagues
here: Blake Sullivan and Max Starets. Both were involved in
implementing the Trinidad + ADF Faces window management API.
Interestingly, they both independently made the same recommendation:
that we should follow the typical JSF decode/encode model. That is,
they would like to see methods on ClientWindow to:
1. Decode parameters from the incoming request. And...
2. Encode parameters into urls (as necessary).
This provides the most implementation flexibility and aligns well with
the other JSF APIs.
I mentioned your concerns about #2 regarding bonus string
concatenation/object creation, but both felt that this overhead would be
nominal. I share that opinion.
Sticking with the advice that I gave Ed last night, I would like to
recommend that we go with this simple approach rather than gearing our
API to address a performance problem that might not be a problem.
Leonardo - I know that you are happy with #1 above. Would you be
willing to live with #2 as well?
If not, some more comments on the Map-based below...
> In that sense, the default implementation of ExternalContext.encodeXXX
> check if client window API is enabled and if that so it scan this map
> for query params and add them.
>
I think I figured out one reason why we weren't seeing eye to eye on
whether these two calls:
- ClientWindow.isClientWindowRenderModeEnabled()
- ClientWindow.getQueryURLParameters().isEmpty()
Are redundant.
I missed one important detail, described in the
ExternalContext.encodeActionURL() API doc:
> * <p class="changed_added_2_2">Call {_at_link
> javax.faces.lifecycle.ClientWindow#isClientWindowRenderModeEnabled(javax.faces.context.FacesContext)
> }.
> * If the result is <code>false</code> take no further action and
> return
> * the rewritten URL. If the result is <code>true</code>, call
> {_at_link #getClientWindow()}.
> * If the result is non-<code>null</code>, call {_at_link
> javax.faces.lifecycle.ClientWindow#getId()}
> * and append the id to the query string of the URL, making the
> necessary
> * allowances for a pre-existing query string or no query-string.</p>
> *
> * <p>Call {_at_link
> javax.faces.lifecycle.ClientWindow#getQueryURLParameters}.
> * If the result is non-{_at_code null}, for each parameter in the map,
> * unconditionally add that parameter to the URL.</p>
I had been assuming that the ClientWindow's parameter map would include
the parameter corresponding to the ClientWindow id - ie. that if
ClientWindow.getQueryURLParameters() returned the empty map, the url
would not be modified.
Based on the above documentation, this is not the case.
I am not happy with this.
The ClientWindow should have complete control over what state gets
encoded into the url - eg. a ClientWindow implementation should be able
to pick its own query parameter name, and
ExternalContext.encodeActionURL() should not care. This is not the case
with the current design.
My preference would be to just let the ClientWindow completely handle
encoding (and decoding) its state - ie. add an encode method (and
restore the decode method).
However, if we are not willing to do this, let's at least spec that
getQueryURLParameters() is responsible for returning a Map that includes
all state that is needed by decode(), including the window id parameter.
This does result in one extra object allocation per-encoded url (an
Iterator), but, again the overhead of this should be nominal within the
scope of a typical JSF request.
> EB>> This was the API suggested by Leonardo as an extension mechanism. The
> EB>> idea was to provide the method and specify the places where it must be
> EB>> called so that custom ClientWindow implementations can rest assured that
> EB>> the method will be called. Because we can't assert that the map is
> EB>> immutable, I don't want to require it. Also, for performance I think
> EB>> checking for a null return is quicker than an empty map.
>
> The only one who can change that map is the ClientWindow implementation.
> From the point of the caller (ExternalContext implementation) the map is
> just for take the query params and add them to the generated url. No
> add/remove/update operations are executed from that 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).
>
> In this version, LifecycleWrapper interface was added. I think the method
> is ok, because it is the context where JSF runs the one who define how
> ClientWindow api works. In that sense, ClientWindow implementations are just
> a wrapper that provides the necessary info to JSF runtime about how to deal
> with client windows, but you can still have something before to do the
> real work.
>
Even if we leave Lifecycle.attachWindow(), I would still prefer that
this use a factory for instantiating the ClientWindow object.
> 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 instance
> AS>> for "url", or a 3rd party instance, eg. in the case of Trinidad.)
>
> The same can be done with LifecycleWrapper/LifecycleFactory. The difference
> is the logic related to enable/disable an specific client window mode can
> be overriden using LifecycleWrapper. For example, you could have a
> "client-url-mixed" mode that works different according to the browser
> capabilities and so on.
>
Sure. This could also be easily implemented inside of a
ClientWindowFactory.
> AS>> This would allow us to kill off some unneeded complexity - eg. we could
> AS>> kill ClientWindow.decode() - the factory would handle this instead.
>
> I feel Client Window.decode() straightforward. This part is the central point
> each ClientWindow implementation should override, so the factory cannot
> really do it.
>
The feedback that I got from my colleagues here is that they agree.
Though they would also like to see the corresponding encode() method. :-)
> AS>> While we're at it, we could kill Lifecycle.attachWindow(), and instead
> AS>> spec that the ClientWindow is set up during FacesContext creation -
> AS>> eg. the FacesContextFacotry could set up the ClientWindow on the
> AS>> ExternalContext before creating the FacesContext instance.
>
> It is important to have a full FacesContext/ExternalContext instance before
> attachWindow(). These two steps are different, because in a resource request,
> you have FacesContext/ExternalContext, but you don't need to call
> attachWindow(), because the served resource "usually" does not have state.
>
Ah, I see. So if we init the ClientWindow in the FacesContextFactory,
we'll be doing this for all requests, not just for view-related
requests. Good point.
FacesServlet currently does this:
> // Acquire the FacesContext instance for this request
> FacesContext context = facesContextFactory.getFacesContext
> (servletConfig.getServletContext(), request, response,
> lifecycle);
>
> // Execute the request processing lifecycle for this request
> try {
> ResourceHandler handler =
> context.getApplication().getResourceHandler();
> if (handler.isResourceRequest(context)) {
> handler.handleResourceRequest(context);
> } else {
> lifecycle.attachWindow(context);
> lifecycle.execute(context);
> lifecycle.render(context);
> }
The timing of the the ClientWindow initialization - after the
FacesContext has been created but just before Lifecycle.execute() (and
only for non-resource requests) seems reasonable. As long as we've got
a ClientWindowFactory, I don't especially care whether the FacesServlet
calls this directly, or it delegates this work to Lifecycle.attachWindow.
> In MyFaces Orchestra, the code is inside FacesContextFactory, and in my
> opinion that code doesn't look nice.
>
Okay. I'll need to take a look at Ed's latest code to see how this
turned out.
> AS>> + + public static void
> disableClientWindowRenderMode(FacesContext context) {
> AS>>
> AS>> I don't understand why these are static. This explanation:
> AS>>
> AS>> + * This is specified as a static method because callsites
> need to access it
> AS>> + * without having access to an actual {_at_code ClientWindow}
> instance.</p>
> AS>>
> AS>> Is confusing.
>
> There are three methods:
>
> public static void disableClientWindowRenderMode(FacesContext context)
> public static void enableClientWindowRenderMode(FacesContext context)
> public static boolean isClientWindowRenderModeEnabled(FacesContext context)
>
> The idea is have a general form to say ClientWindow implementation when
> a fragment needs to include client window logic or not. For example, if
> url mode is used, the query param is skipped from link generation in
> ExternalContext.encodeXXX.
>
If we expose a ClientWindow.encode() method, this method can do the
right thing without requiring clients to manually check the disabled state.
So, for example, ExternalContext.encodeActionURL would simply call
external.getClientWindow().encode() and not have to worry about any of
the details.
This seems like a win to me.
> h:link and h:button has an attribute called disableClientWindow that call
> these methods. The idea is component authors can control this flag and
> deal with the necessary logic inside renderer classes.
>
> The methods are static, because they are not supposed to change, after
> all, those methods are just for control a flag.
>
Kind of sounds like these methods should have been marked final rather
than static.
> AS>> If for some reason the ClientWindow is not available (eg. if
> facesContext.getExternalContext().getClientWindow() reutrns null),
> AS>> well, 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
> AS>> during encoding. We clearly have access to a ClientWindow here.
>
> AS>> What's the deal with making these methods static? From what I
> AS>> can tell they should be instance methods.
>
> There is no problem for me if they are not static and are inside
> ClientWindow as normal methods.
>
Cool.
> AS>> Also, should we spec that when disabled,
> AS>> ClientWindow.getQueryURLParameters() returns the empty collection?
> AS>> If we did that, do we even need an isClientWindowRenderModeEnabled() API?
>
> We need the methods, because not all client window modes requires
> query params to work.
>
Okay. At the moment we need isClientWindowRenderModeEnabled() even in
the url-mode case, because without this the window id will be encoded
into the URL.
If we implement either of my suggestions:
- Replace getQueryURLParameters() with an encode() method. Or...
- Spec getQueryURLParameters() such that the window id parameter is
included in the returned Map.
The JSF implementation never need to call
isClientWindowRenderModeEnabled() in the url-mode case.
However, perhaps there are non-url mode cases where the ClientWindow
disabled state would come in handy. None of these cases currently exist
in Mojarra, so not sure what these are exactly. But it's not like I
convinced Ed to remove this method, so it'll be there if any such cases
arise.
> AS>> When it comes to API design, I am a minimalist. I also prefer
> AS>> to avoid redundancy. Clients of the ClientWindow API only need to
> AS>> know whether there are parameters or not, and don't need to be able
> AS>> to distinguish between the various cases that might lead to an empty
> AS>> parameter set. As such, getQueryURLParameters().isEmpty() is the
> AS>> minimally sufficient solution. isClientWidnowRenderModeEnabled() is
> AS>> redundant and doesn't add any especially useful information. As a
> AS>> minimalist, I take pleasure in killing off seemingly pointless APIs. :-)
>
> EB>> Again, I'm going to push back on that one.
>
> There is no redundancy here. Whether use or not query params into its
> logic is responsibility of the ClientWindow implementation, but it is not
> a requeriment. So there are two different concepts here.
>
My thinking that this was redundant was based on the assumption that
getQueryURLParameters() returned *all* of the parameters. If we go this
route (or if we add an encode() method), then there would be no need to
call isClientWindowRenderModeEnabled() during url rewriting. Though it
sounds like you have other ideas on when this method might need to be
called.
> Please do not remove disableClientWindowRenderMode() related methods,
> otherwise the trick used by Mark in real life cannot be ported to this
> API.
>
I definitely don't want to remove disableClientWindowRenderMode() - we
clearly need this. It's really just the disabled test that bugs me.
Ah, well, hopefully that explains my thinking a bit better. And, more
importantly, hopefully we can come to a conclusion very quickly! :-)
Andy
> regards,
>
> Leonardo Uribe
>