Flushing comments accumulated so far. Still reviewing.
http://gwt-code-reviews.appspot.com/924801/diff/2001/3010 File user/src/com/google/gwt/editor/client/AutoBean.java (right): http://gwt-code-reviews.appspot.com/924801/diff/2001/3010#newcode21 user/src/com/google/gwt/editor/client/AutoBean.java:21: * PFM. ahem http://gwt-code-reviews.appspot.com/924801/diff/2001/3010#newcode87 user/src/com/google/gwt/editor/client/AutoBean.java:87: * A clone operation only ever produces a shallow copy of its tags, since the Having trouble squaring this sentence with the deep flag. http://gwt-code-reviews.appspot.com/924801/diff/2001/3010#newcode98 user/src/com/google/gwt/editor/client/AutoBean.java:98: List<Operation> getOperations(); Document that abuse of this can destroy dead code elimination? You really want this to be public? If you're not using it for the immediate change, take it out of the interface? Should autobean be an impl class, so that we can pretend that people won't use it directly? Or could we actually make it non public? http://gwt-code-reviews.appspot.com/924801/diff/2001/3012 File user/src/com/google/gwt/editor/client/AutoBeanUtils.java (right): http://gwt-code-reviews.appspot.com/924801/diff/2001/3012#newcode39 user/src/com/google/gwt/editor/client/AutoBeanUtils.java:39: public static Map<String, Object> diff(AutoBean<?> a, AutoBean<?> b) { Do you actually use the diff anywhere, or just the fact that things are different? Is this another spot to fret about unoptimizable string based property games? E.g. I can use this to get the full property list by comparing a bean to an empty one. Really, fundamentally, can unused auto bean properties ever be dead stripped? Not saying it's a deal breaker if they can't, but we should be conscious of the choice and document it. http://gwt-code-reviews.appspot.com/924801/diff/2001/3016 File user/src/com/google/gwt/editor/rebind/AutoBeanFactoryGenerator.java (right): http://gwt-code-reviews.appspot.com/924801/diff/2001/3016#newcode92 user/src/com/google/gwt/editor/rebind/AutoBeanFactoryGenerator.java:92: { You're not using these braces for anything, it's all in the for loop http://gwt-code-reviews.appspot.com/924801/diff/2001/3023 File user/src/com/google/gwt/editor/rebind/model/ModelUtils.java (right): http://gwt-code-reviews.appspot.com/924801/diff/2001/3023#newcode32 user/src/com/google/gwt/editor/rebind/model/ModelUtils.java:32: public class ModelUtils { public? http://gwt-code-reviews.appspot.com/924801/diff/2001/3043 File user/src/com/google/gwt/requestfactory/client/impl/AbstractRequest.java (right): http://gwt-code-reviews.appspot.com/924801/diff/2001/3043#newcode93 user/src/com/google/gwt/requestfactory/client/impl/AbstractRequest.java:93: System.out.println(responseText); oops http://gwt-code-reviews.appspot.com/924801/diff/2001/3043#newcode161 user/src/com/google/gwt/requestfactory/client/impl/AbstractRequest.java:161: // Really want Class.isInstance() Hear hear. And isAssignableFrom() while we're at it http://gwt-code-reviews.appspot.com/924801/diff/2001/3043#newcode200 user/src/com/google/gwt/requestfactory/client/impl/AbstractRequest.java:200: if (receiver != null) { Should we have a default receiver that throws an RTE on unhandled violations or failures? It's awfully easy to forget to provide a callback and then miss what's going on when things fail silently. That's why we previously required the receiver arg in fire, to make that mistake much more difficult to make. I don't think I want to bring that back, but it seems like we should do something. http://gwt-code-reviews.appspot.com/924801/diff/2001/3044 File user/src/com/google/gwt/requestfactory/client/impl/AbstractRequestContext.java (right): http://gwt-code-reviews.appspot.com/924801/diff/2001/3044#newcode51 user/src/com/google/gwt/requestfactory/client/impl/AbstractRequestContext.java:51: private List<AbstractRequest<?>> invocations = new ArrayList<AbstractRequest<?>>(); Tastey http://gwt-code-reviews.appspot.com/924801/diff/2001/3044#newcode85 user/src/com/google/gwt/requestfactory/client/impl/AbstractRequestContext.java:85: checkLocked(); Really do this before checking the seen set? Seems like you'll get nondeterministic fails. http://gwt-code-reviews.appspot.com/924801/diff/2001/3044#newcode94 user/src/com/google/gwt/requestfactory/client/impl/AbstractRequestContext.java:94: if (!bean.isFrozen() && context != null) { If the bean is frozen and it's a foreigner you're screwed, no? http://gwt-code-reviews.appspot.com/924801/diff/2001/3044#newcode145 user/src/com/google/gwt/requestfactory/client/impl/AbstractRequestContext.java:145: receiver.onSuccess(null); NPE on null receiver? (Which problem goes away if you take my default receiver suggestion.) http://gwt-code-reviews.appspot.com/924801/diff/2001/3044#newcode148 user/src/com/google/gwt/requestfactory/client/impl/AbstractRequestContext.java:148: locked = true; Redundant, you locked on fire, but maybe that's fine. http://gwt-code-reviews.appspot.com/924801/diff/2001/3044#newcode152 user/src/com/google/gwt/requestfactory/client/impl/AbstractRequestContext.java:152: receiver.onViolation(errors); When do you unlock? I thought there was a test to check that I can re-edit on violation or failure. http://gwt-code-reviews.appspot.com/924801/diff/2001/3044#newcode170 user/src/com/google/gwt/requestfactory/client/impl/AbstractRequestContext.java:170: return true; Actually, no please. I used to be able to tell that a newly created proxy had no real info added to it, and then let the user cancel without being nagged. Don't want to lose that. http://gwt-code-reviews.appspot.com/924801/diff/2001/3044#newcode468 user/src/com/google/gwt/requestfactory/client/impl/AbstractRequestContext.java:468: * Process an arroy of ReturnRecords, delegating to arroyo http://gwt-code-reviews.appspot.com/924801/diff/2001/3044#newcode472 user/src/com/google/gwt/requestfactory/client/impl/AbstractRequestContext.java:472: WriteOperation... operations) { I don't think I've seen you pass more than one operation at a time http://gwt-code-reviews.appspot.com/924801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
