On 9 August 2010 20:51, <[email protected]> wrote: > Mostly LGTM. This looks like a nice cleanup and should remove a lot of > potential for lock contention. I like that a lot of the code gets a lot > simpler too. SerializabilityUtil.java is some particularly embarrassing > code that I helped write. This patch should allow that code to be > refactored to be simpler in the future, I'd think. > > > > http://gwt-code-reviews.appspot.com/716801/diff/14001/15001 > File dev/core/src/com/google/gwt/dev/util/HttpHeaders.java (right): > > http://gwt-code-reviews.appspot.com/716801/diff/14001/15001#newcode65 > dev/core/src/com/google/gwt/dev/util/HttpHeaders.java:65: * The Internet > date format for HTTP. > It would be great to add a comment here as to why we have to go through > all this trouble in the first place. Just something like "The doc for > DataFormat states: Date formats are not synchronized. It is recommended > to create separate format instances for each thread. If multiple threads > access a format concurrently, it must be synchronized externally." > > http://gwt-code-reviews.appspot.com/716801/diff/14001/15004 > File > user/src/com/google/gwt/user/server/rpc/AbstractRemoteServiceServlet.java > (right): > > http://gwt-code-reviews.appspot.com/716801/diff/14001/15004#newcode33 > > user/src/com/google/gwt/user/server/rpc/AbstractRemoteServiceServlet.java:33: > protected static final transient ThreadLocal<HttpServletRequest> > perThreadRequest = > Remove the "transient" keyword here and below. > Agree, i forgot to remove it.
> http://gwt-code-reviews.appspot.com/716801/diff/14001/15007 > File > > user/src/com/google/gwt/user/server/rpc/impl/ConcurrentReferenceHashMap.java > (right): > > http://gwt-code-reviews.appspot.com/716801/diff/14001/15007#newcode1025 > > user/src/com/google/gwt/user/server/rpc/impl/ConcurrentReferenceHashMap.java:1025: > public ConcurrentReferenceHashMap(int initialCapacity, boolean userefid) > { > We need to be really clear about where this class came from, what > version, what license it's under, etc. Are we sure we can just rebrand > it as Apache? Is there any reason to move it from the jsr166y package > to our own? Did we modify this? For example, I cannot find this > particular constructor in the public versions (or any constructor > overloads that take a boolean argument). > > I added that constructor. I have no clue regarding licenses and branding issues, i wanted to preserve functionality with this patch so i just took a technically suitable data structure that i used before in other projects. If we must keep the Reference functionality and this class license does not cut it(?), do you know of any alternative concurrent reference map implementation with a suitable license ?. http://gwt-code-reviews.appspot.com/716801/show > > -- > http://groups.google.com/group/Google-Web-Toolkit-Contributors -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
