Generally looks good, just a couple of suggestions. Let's go one more
round on this.


http://gwt-code-reviews.appspot.com/1601806/diff/1015/user/src/com/google/web/bindery/requestfactory/shared/impl/AbstractRequestContext.java
File
user/src/com/google/web/bindery/requestfactory/shared/impl/AbstractRequestContext.java
(right):

http://gwt-code-reviews.appspot.com/1601806/diff/1015/user/src/com/google/web/bindery/requestfactory/shared/impl/AbstractRequestContext.java#newcode624
user/src/com/google/web/bindery/requestfactory/shared/impl/AbstractRequestContext.java:624:
* resolving property values, won't automatically edit proxies that
weren't
I'm an autobean newbie, but shouldn't we change the diffing algorithm so
that it doesn't actually cause edits to proxies? I mean, when you a do a
diff, that should never happen, right?

I don't think that possible modification should block that change
though; we can do it in a follow-up. Let me know what you think.

http://gwt-code-reviews.appspot.com/1601806/diff/1015/user/src/com/google/web/bindery/requestfactory/shared/impl/AbstractRequestContext.java#newcode1206
user/src/com/google/web/bindery/requestfactory/shared/impl/AbstractRequestContext.java:1206:
* break backwards compatibility for those edge-cases).
Maybe move this up into the method javadoc.

http://gwt-code-reviews.appspot.com/1601806/diff/1015/user/src/com/google/web/bindery/requestfactory/shared/impl/AbstractRequestContext.java#newcode1209
user/src/com/google/web/bindery/requestfactory/shared/impl/AbstractRequestContext.java:1209:
/*
This code is nearly identical to the code that you changed above. Can
you refactor it?

http://gwt-code-reviews.appspot.com/1601806/diff/1015/user/test/com/google/web/bindery/requestfactory/gwt/client/RequestFactoryTest.java
File
user/test/com/google/web/bindery/requestfactory/gwt/client/RequestFactoryTest.java
(right):

http://gwt-code-reviews.appspot.com/1601806/diff/1015/user/test/com/google/web/bindery/requestfactory/gwt/client/RequestFactoryTest.java#newcode376
user/test/com/google/web/bindery/requestfactory/gwt/client/RequestFactoryTest.java:376:
assertTrue(context.isChanged());
Great job with this test code.

http://gwt-code-reviews.appspot.com/1601806/diff/1015/user/test/com/google/web/bindery/requestfactory/server/RequestPayloadJreTest.java
File
user/test/com/google/web/bindery/requestfactory/server/RequestPayloadJreTest.java
(right):

http://gwt-code-reviews.appspot.com/1601806/diff/1015/user/test/com/google/web/bindery/requestfactory/server/RequestPayloadJreTest.java#newcode1
user/test/com/google/web/bindery/requestfactory/server/RequestPayloadJreTest.java:1:
package com.google.web.bindery.requestfactory.server;
Copyright header.

http://gwt-code-reviews.appspot.com/1601806/diff/1015/user/test/com/google/web/bindery/requestfactory/shared/impl/RequestPayloadTest.java
File
user/test/com/google/web/bindery/requestfactory/shared/impl/RequestPayloadTest.java
(right):

http://gwt-code-reviews.appspot.com/1601806/diff/1015/user/test/com/google/web/bindery/requestfactory/shared/impl/RequestPayloadTest.java#newcode2
user/test/com/google/web/bindery/requestfactory/shared/impl/RequestPayloadTest.java:2:

Copyright header.

http://gwt-code-reviews.appspot.com/1601806/diff/1015/user/test/com/google/web/bindery/requestfactory/shared/impl/RequestPayloadTest.java#newcode24
user/test/com/google/web/bindery/requestfactory/shared/impl/RequestPayloadTest.java:24:
* too much things to the server.
too much --> to many

http://gwt-code-reviews.appspot.com/1601806/diff/1015/user/test/com/google/web/bindery/requestfactory/shared/impl/RequestPayloadTest.java#newcode51
user/test/com/google/web/bindery/requestfactory/shared/impl/RequestPayloadTest.java:51:
public void testOperationPayload() throws Exception {
Add a bit of doc here indicating what you're going to test.

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

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

Reply via email to