Re: Added new public static method to Window.Location: reloadParameterMap. (issue1859804)

[skyb...@google.com]

http://gwt-code-reviews.appspot.com/1859804/diff/1/user/src/com/google/gwt/user/client/Window.java
File user/src/com/google/gwt/user/client/Window.java (right):

http://gwt-code-reviews.appspot.com/1859804/diff/1/user/src/com/google/gwt/user/client/Window.java#newcode238
user/src/com/google/gwt/user/client/Window.java:238: public static void
reloadParameterMap() {
If we have an explicit reloadParameters method, it should probably clear
both paramMap and listParamMap.

However, I think it's better not to make GWT apps have to call this new
method; instead we should automatically sync. But doing it in popstate
seems insufficent? Apparently we'd have to handle changes via pushstate
and replacestate as well? [1]

A more robust approach would be to cache the query string used to
populate paramMap and listParamMap and automatically clear both if
getQueryString() != cachedQueryString. This would mean calling
getQueryString() every time we retrieve a parameter via getParameter()
or getParameterMap(); this is cheap in the default implementation but
looks a bit more expensive in WindowImplIE.java. It seems unlikely this
is performance-critical code, though.

By the way we should also have an external bug for this change in the
issue tracker.

[1] https://developer.mozilla.org/en-US/docs/DOM/window.onpopstate

http://gwt-code-reviews.appspot.com/1859804/

[cromw...@google.com]

You could also return a "Live" Map implementation so that the Map always
reflects the most up to date values even if getParameterMap() isn't
called. The same caching logic would apply.

On 2012/10/31 20:19:01, perneto wrote:

http://gwt-code-reviews.appspot.com/1859804/diff/1/user/src/com/google/gwt/user/client/Window.java
> File user/src/com/google/gwt/user/client/Window.java (right):


http://gwt-code-reviews.appspot.com/1859804/diff/1/user/src/com/google/gwt/user/client/Window.java#newcode238
> user/src/com/google/gwt/user/client/Window.java:238: public static
void
> reloadParameterMap() {

> On 2012/10/31 19:28:36, skybrian wrote:
> > If we have an explicit reloadParameters method, it should probably
clear both
> > paramMap and listParamMap.

> Oh right, missed that.

> > However, I think it's better not to make GWT apps have to call this
new
> method;
> > instead we should automatically sync. But doing it in popstate seems
> > insufficent? Apparently we'd have to handle changes via pushstate
and
> > replacestate as well? [1]

> Interesting. I experimented with Chrome before sending this CL; Chrome
does
> trigger a popstate event when pushState is called (didn't try
replaceState but I
> guess it's the same).

> I think I've heard somewhere that Chrome didn't use to do that, but it
> definitely does at the moment.

> > A more robust approach would be to cache the query string used to
populate
> > paramMap and listParamMap and automatically clear both if
getQueryString() !=
> > cachedQueryString. This would mean calling getQueryString() every
time we
> > retrieve a parameter via getParameter() or getParameterMap(); this
is cheap in
> > the default implementation but looks a bit more expensive in
> WindowImplIE.java.
> > It seems unlikely this is performance-critical code, though.

> Yes, this sounds better to me.

[skyb...@google.com]

On 2012/11/05 16:34:46, perneto wrote:
> FYI: I added http://code.google.com/p/rietveld/issues/detail?id=402 to
> track this change, as requested by Brian.

Oops, that's actually the rietveld issue tracker, not the GWT issue
tracker. The one you want is at:
http://code.google.com/p/google-web-toolkit/issues/list

I don't think we should make the returned Map a live map, since
currently it's immutable and there's probably code that relies on it not
changing. It seems less surprising to call getParameterMap() to get the
new version.


http://gwt-code-reviews.appspot.com/1859804/