http://gwt-code-reviews.appspot.com/893801/diff/1/6 File user/src/com/google/gwt/requestfactory/client/impl/JsoCollection.java (right):
http://gwt-code-reviews.appspot.com/893801/diff/1/6#newcode25 user/src/com/google/gwt/requestfactory/client/impl/JsoCollection.java:25: void setDependencies(DeltaValueStoreJsonImpl dvs, Javadoc http://gwt-code-reviews.appspot.com/893801/diff/1/7 File user/src/com/google/gwt/requestfactory/client/impl/JsoList.java (right): http://gwt-code-reviews.appspot.com/893801/diff/1/7#newcode38 user/src/com/google/gwt/requestfactory/client/impl/JsoList.java:38: return @java.util.Date::createFrom(D)(millis); Formatting. http://gwt-code-reviews.appspot.com/893801/diff/1/8 File user/src/com/google/gwt/requestfactory/client/impl/JsoSet.java (right): http://gwt-code-reviews.appspot.com/893801/diff/1/8#newcode91 user/src/com/google/gwt/requestfactory/client/impl/JsoSet.java:91: return ((JsArrayString) array).length(); Shouldn't this return list.length()? It doesn't look like the array field is ever mutated by this class. http://gwt-code-reviews.appspot.com/893801/diff/1/9 File user/src/com/google/gwt/requestfactory/client/impl/ProxyImpl.java (right): http://gwt-code-reviews.appspot.com/893801/diff/1/9#newcode74 user/src/com/google/gwt/requestfactory/client/impl/ProxyImpl.java:74: public boolean equals(Object o) { This is the wrong semantic. EntityProxy objects represent immutable snapshots of an entity at a certain point in time. http://gwt-code-reviews.appspot.com/893801/diff/1/9#newcode125 user/src/com/google/gwt/requestfactory/client/impl/ProxyImpl.java:125: public int hashCode() { Remove. http://gwt-code-reviews.appspot.com/893801/diff/1/10 File user/src/com/google/gwt/requestfactory/client/impl/ProxyJsoImpl.java (right): http://gwt-code-reviews.appspot.com/893801/diff/1/10#newcode373 user/src/com/google/gwt/requestfactory/client/impl/ProxyJsoImpl.java:373: // todo Is there any reason not to implement this? http://gwt-code-reviews.appspot.com/893801/diff/1/11 File user/src/com/google/gwt/requestfactory/rebind/RequestFactoryGenerator.java (right): http://gwt-code-reviews.appspot.com/893801/diff/1/11#newcode645 user/src/com/google/gwt/requestfactory/rebind/RequestFactoryGenerator.java:645: requestClassName = AbstractJsonValueListRequest.class.getName(); Add a comment demonstrating the code (fragment) being generated. http://gwt-code-reviews.appspot.com/893801/diff/1/12 File user/src/com/google/gwt/requestfactory/server/JsonRequestProcessor.java (right): http://gwt-code-reviews.appspot.com/893801/diff/1/12#newcode354 user/src/com/google/gwt/requestfactory/server/JsonRequestProcessor.java:354: Class<?> propertyType = property instanceof CollectionProperty ? ((CollectionProperty) property).getLeafType() : property.getType(); Call this variable elementType http://gwt-code-reviews.appspot.com/893801/diff/1/12#newcode645 user/src/com/google/gwt/requestfactory/server/JsonRequestProcessor.java:645: public Map<String, Property<?>> getPropertiesFromRecord( getPropertiesFromEntity http://gwt-code-reviews.appspot.com/893801/diff/1/12#newcode880 user/src/com/google/gwt/requestfactory/server/JsonRequestProcessor.java:880: if (aVal == bVal || aVal.equals(bVal)) { NPE if aVal=null and bVal=Object http://gwt-code-reviews.appspot.com/893801/diff/1/12#newcode1159 user/src/com/google/gwt/requestfactory/server/JsonRequestProcessor.java:1159: if (cachedEntityLookup.containsKey(propKey)) { Is this containsKey() here to account for cached null values? If so, add a comment to that effect. Actually, this whole block of if statements is hard to follow without some kind of explanatory text to accompany it. http://gwt-code-reviews.appspot.com/893801/diff/1/12#newcode1188 user/src/com/google/gwt/requestfactory/server/JsonRequestProcessor.java:1188: leafType); idem http://gwt-code-reviews.appspot.com/893801/diff/1/12#newcode1394 user/src/com/google/gwt/requestfactory/server/JsonRequestProcessor.java:1394: toReturn.put(field.getName(), fieldType); Looking how the else block below checks the ProxyFor field, should this block do the same? http://gwt-code-reviews.appspot.com/893801/diff/1/13 File user/src/com/google/gwt/requestfactory/server/ReflectionBasedOperationRegistry.java (right): http://gwt-code-reviews.appspot.com/893801/diff/1/13#newcode150 user/src/com/google/gwt/requestfactory/server/ReflectionBasedOperationRegistry.java:150: return (Class<?>) ((ParameterizedType) params[0]).getRawType(); Comment why. http://gwt-code-reviews.appspot.com/893801/diff/1/14 File user/src/com/google/gwt/requestfactory/shared/impl/CollectionProperty.java (right): http://gwt-code-reviews.appspot.com/893801/diff/1/14#newcode27 user/src/com/google/gwt/requestfactory/shared/impl/CollectionProperty.java:27: * @param <C> the type of the element the container contains The meanings of the type params look backwards. How about C = Collection and E == element (or L == leaf)? http://gwt-code-reviews.appspot.com/893801/diff/1/16 File user/src/com/google/gwt/user/client/ui/SplitLayoutPanel.java (right): http://gwt-code-reviews.appspot.com/893801/diff/1/16#newcode55 user/src/com/google/gwt/user/client/ui/SplitLayoutPanel.java:55: SplitLayoutPanel extends DockLayoutPanel { Unrelated change? http://gwt-code-reviews.appspot.com/893801/diff/1/17 File user/test/com/google/gwt/requestfactory/client/RequestFactoryTest.java (right): http://gwt-code-reviews.appspot.com/893801/diff/1/17#newcode791 user/test/com/google/gwt/requestfactory/client/RequestFactoryTest.java:791: assertTrue(!response.getOneToManySetField().contains( assertFalse http://gwt-code-reviews.appspot.com/893801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
