Hey Ed -
On 3/13/13 6:53 PM, Edward Burns wrote:
>>>>>> 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.
Thanks for following up on my comments. Glad to see this was not
pointless after all. :-)
> 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>.
>
Okay.
FWIW, this one worries me, since failing to address these cases means
that our window management solution is unreliable, which in some ways is
worse than non-existent. :-/
I would be interested to hear thoughts on this from Leonardo or other
folks who were involved in the original proposal.
> 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.
Okay, I am confused again. :-)
Why can't we assert that the Map is immutable? If third party
frameworks want to provide additional parameters, the proper extension
point is the (new) ClientWindowFactory. Allowing any old code to mess
with the ClientWindow parameter maps opens us up to the potential for
bizarre results. Rather than facilitating this, we should promote
extension via the usual means (ie. provide your own factory that adds
your desired behavior).
The ClientWindow's parameter map should be immutable.
> Also, for performance I think
> checking for a null return is quicker than an empty map.
This is not a good approach to performance optimization. The JVM is
amazingly capable of optimizing away simple code. After the JIT kicks
in, a call to isEmpty() on the Collections.emptyMap() is going to be so
fast that there is no point in attempting to optimize this away manually.
I highly recommend reviewing what Joshua Bloch's Effective Java has to
say on these topics.
In particular, it is worth reading "Item 55: Optimize Judiciously",
which makes the point that:
> It is a very bad idea to warp an API to achieve good performance
And "Item 43: Return empty arrays or collections, not nulls", which
concludes:
> there is no reason ever to return null from an array- or
> collection-valued method instead of returning an empty array or
> collection.
Not that we should blindly follow the wisdom of Mr Bloch, but we should
have compelling reasons for deviating from this very relevant advice.
Optimizing isEmpty() calls by replacing these with null checks is not a
good reason to go our own way here.
> 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!
>
Haha. I completely agree with that sentiment!
When it comes to API design, I am a minimalist. I also prefer to avoid
redundancy. Clients of the ClientWindow API only need to know whether
there are parameters or not, and don't need to be able to distinguish
between the various cases that might lead to an empty parameter set. As
such, getQueryURLParameters().isEmpty() is the minimally sufficient
solution. isClientWidnowRenderModeEnabled() is redundant and doesn't
add any especially useful information. As a minimalist, I take pleasure
in killing off seemingly pointless APIs. :-)
In any case, whether or not we kill this particular method is a minor
issue. Perhaps when 2.3 kicks up we should have more discussion of our
design principles. I understand that starting this discussion a couple
of days before the spec is finalized is bit ridiculous.
> Andy, please review these changes, committed as r11735 on Mojarra trunk.
>
I will take a look tomorrow. Thank you again for tackling this!
Andy