On Tue, Oct 5, 2010 at 1:26 PM, <[email protected]> wrote: > > http://gwt-code-reviews.appspot.com/959801/diff/4001/5001 > File > > user/src/com/google/gwt/requestfactory/server/JsonRequestProcessor.java > (right): > > http://gwt-code-reviews.appspot.com/959801/diff/4001/5001#newcode78 > user/src/com/google/gwt/requestfactory/server/JsonRequestProcessor.java:78: > this.version = givenVersion; > version = jsonObject.has(Constants.ENCODED_VERSION_PROPERTY) ? > jsonObject.getString(Constants.ENCODED_VERSION_PROPERTY) : null; > > For that matter, do you even need the has() call? Will getString return > null if it isn't set? >
If the property is absent, getString(..) returns an JSONException. > > http://gwt-code-reviews.appspot.com/959801/diff/4001/5001#newcode1371 > > user/src/com/google/gwt/requestfactory/server/JsonRequestProcessor.java:1371: > // handle DELETE > What are you fixing by getting rid of the write operation check? > > The objective was not to get rid of the write operation check, but just to refactor the code so that the flow is cleaner. After refactoring, a boolean was all that was necessary. > http://gwt-code-reviews.appspot.com/959801/diff/4001/5002 > File > > > user/test/com/google/gwt/requestfactory/server/JsonRequestProcessorTest.java > (right): > > http://gwt-code-reviews.appspot.com/959801/diff/4001/5002#newcode150 > > user/test/com/google/gwt/requestfactory/server/JsonRequestProcessorTest.java:150: > public void testEndToEndSmartDiff_NoChange() throws Exception { > It looks like you're testing the version code path, but no longer > testing the non-version code path? Until we delete the latter, we should > maintain its coverage. > > Fixed in the followup patch. > http://gwt-code-reviews.appspot.com/959801/diff/4001/5003 > File user/test/com/google/gwt/requestfactory/server/SimpleFoo.java > (right): > > http://gwt-code-reviews.appspot.com/959801/diff/4001/5003#newcode200 > user/test/com/google/gwt/requestfactory/server/SimpleFoo.java:200: > s1.isChanged = false; > This is pointless, it defaults to false. > > http://gwt-code-reviews.appspot.com/959801/diff/4001/5003#newcode206 > user/test/com/google/gwt/requestfactory/server/SimpleFoo.java:206: > s2.isChanged = false; > ditto > > Done. > http://gwt-code-reviews.appspot.com/959801/diff/4001/5003#newcode283 > user/test/com/google/gwt/requestfactory/server/SimpleFoo.java:283: * > increment version numbers. > Won't be able to use autobean, this is a JRE class. Wasn't thinking. > > Document that it is only set by setUserName and the other one. > > Done. > > http://gwt-code-reviews.appspot.com/959801/show > -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
