Hi Andy
2013/3/14 Andy Schwartz <andy.schwartz_at_oracle.com>
> 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 isClientWindowRenderModeEnable**d().
>>
>> 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).
>
>
If ClientWindowFactory is more meaningful I have no objections to include
it.
Just remember that ClientWindowFactory in some cases requires to check
agent capabilities, so it requires FacesContext/ExternalContext to
instantiate
a 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?
>
>
Originally, the proposal supposed add some encodeXXX methods
into ClientWindow. But in this case do you really need to have
such flexibility? really the only thing you really need in this part
is add some query parameters. Use encodeXXX approach requires
copy and paste the same code that check the passed url, remove
duplicates, use '?' or '&' when necessary and that's not nice.
If you need something else you can always override ExternalContext,
so for those very rare cases (I can't see anyone) there is an alternative.
I consider the performance bonus is important, because generate
urls is an intensive task.
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.**isClientWindowRenderModeEnable**d()
> - 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#**isClientWindowRenderModeEnable**d(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.
>
>
+1
> 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.
The extra iterator is not a problem compared with the objects
required to scan an url and create a new one with the params
(on encodeXXX() case)
>
>
> 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.
>
>
Ok.
>
> 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.
>
>
Ok.
>
> 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 isClientWindowRenderModeEnable**d(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.
>
>
I can understand the simplicity behind decode() and encode() model, but the
map idea has its value too, and in this case it does exactly what we need
without require to write the same logic over and over.
>
> 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.
>
>
Good idea.
>
> 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 isClientWindowRenderModeEnable**d()
>> API?
>>
>> We need the methods, because not all client window modes requires
>> query params to work.
>>
>>
>
> Okay. At the moment we need isClientWindowRenderModeEnable**d() 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 isClientWindowRenderModeEnable**d()
> 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.
>
>
Yes, there are non-url values that really need
isClientWindowRenderModeEnabled()
The problem implementing these models is that requires some changes in the
renderer classes. But the flexibility in this part really worth it.
>
> 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. isClientWidnowRenderModeEnable**d()
>> 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 isClientWindowRenderModeEnable**d() during url rewriting. Though it
> sounds like you have other ideas on when this method might need to be
> called.
>
>
In this moment I don't see any other operation to do inside encode()
methods more
than add query parameters.
>
> 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! :-)
>
>
regards,
Leonardo Uribe
> Andy
>
> regards,
>>
>> Leonardo Uribe
>>
>>
>
>