I've gotten through the AutobBean changes and additions. No blockers so far.
As we discussed, I want to get out of your way. If you TBR this and the follow up CLs, I'll catch up on Monday. I think we should set up a VC on Monday, maybe screen share, and walk through the rest together. http://gwt-code-reviews.appspot.com/1062801/diff/1/2 File samples/dynatablerf/src/com/google/gwt/sample/dynatablerf/console/Console.java (right): http://gwt-code-reviews.appspot.com/1062801/diff/1/2#newcode16 samples/dynatablerf/src/com/google/gwt/sample/dynatablerf/console/Console.java:16: package com.google.gwt.sample.dynatablerf.console; Should this be under server? http://gwt-code-reviews.appspot.com/1062801/diff/1/10 File user/src/com/google/gwt/autobean/rebind/model/AutoBeanMethod.java (right): http://gwt-code-reviews.appspot.com/1062801/diff/1/10#newcode21 user/src/com/google/gwt/autobean/rebind/model/AutoBeanMethod.java:21: import com.google.gwt.editor.rebind.model.ModelUtils; Do you really want autobean to depend upon editor? http://gwt-code-reviews.appspot.com/1062801/diff/1/12 File user/src/com/google/gwt/autobean/server/AutoBeanFactoryMagic.java (right): http://gwt-code-reviews.appspot.com/1062801/diff/1/12#newcode37 user/src/com/google/gwt/autobean/server/AutoBeanFactoryMagic.java:37: * AutoBeanFactoyModel. typo: Factoy I don't quite parse the disclaimer. Basically "is totally undefensive, assumes it's being asked to do reasonable things"? http://gwt-code-reviews.appspot.com/1062801/diff/1/12#newcode39 user/src/com/google/gwt/autobean/server/AutoBeanFactoryMagic.java:39: public class AutoBeanFactoryMagic { AutoBeanFactoryJvmBuilder? Magic is cute and all, but for a public class... http://gwt-code-reviews.appspot.com/1062801/diff/1/13 File user/src/com/google/gwt/autobean/server/BeanMethod.java (right): http://gwt-code-reviews.appspot.com/1062801/diff/1/13#newcode63 user/src/com/google/gwt/autobean/server/BeanMethod.java:63: return method.getName().startsWith("get") Here and in the RF validator, we should probably be using org.apache.commons.beanutils.BeanUtils to decide what's a getter and what's not, s.t. people can use their is* and has* methods. Ditto for setters, and sooner or later for indexed properties. Not worth derailing for, but worth a todo. http://gwt-code-reviews.appspot.com/1062801/diff/1/13#newcode88 user/src/com/google/gwt/autobean/server/BeanMethod.java:88: CALL { This is strictly about category methods, right? Name it to give a clue? http://gwt-code-reviews.appspot.com/1062801/diff/1/13#newcode116 user/src/com/google/gwt/autobean/server/BeanMethod.java:116: static Method findMethod(SimpleBeanHandler<?> handler, Method method) { findCategoryMethod http://gwt-code-reviews.appspot.com/1062801/diff/1/14 File user/src/com/google/gwt/autobean/server/Configuration.java (right): http://gwt-code-reviews.appspot.com/1062801/diff/1/14#newcode16 user/src/com/google/gwt/autobean/server/Configuration.java:16: package com.google.gwt.autobean.server; Should move to server.impl? http://gwt-code-reviews.appspot.com/1062801/diff/1/14#newcode30 user/src/com/google/gwt/autobean/server/Configuration.java:30: public class Configuration { Does this need to be public? If so, should it move to server.impl? http://gwt-code-reviews.appspot.com/1062801/diff/1/14#newcode40 user/src/com/google/gwt/autobean/server/Configuration.java:40: try { Sicko. Is this really clearer than Configuration rtn = toReturn; toReturn = null; return rtn; Esp. if you rename toReturn to toBuild? http://gwt-code-reviews.appspot.com/1062801/diff/1/18 File user/src/com/google/gwt/autobean/server/ProxyAutoBean.java (right): http://gwt-code-reviews.appspot.com/1062801/diff/1/18#newcode16 user/src/com/google/gwt/autobean/server/ProxyAutoBean.java:16: package com.google.gwt.autobean.server; server.impl http://gwt-code-reviews.appspot.com/1062801/diff/1/19 File user/src/com/google/gwt/autobean/server/ShimHandler.java (right): http://gwt-code-reviews.appspot.com/1062801/diff/1/19#newcode78 user/src/com/google/gwt/autobean/server/ShimHandler.java:78: method.setAccessible(true); Why twice? http://gwt-code-reviews.appspot.com/1062801/diff/1/19#newcode81 user/src/com/google/gwt/autobean/server/ShimHandler.java:81: } else if (BeanMethod.GET.matches(method)) { I always find else after a return a bit unreadable, a bit harder to notice that this is an exit point. http://gwt-code-reviews.appspot.com/1062801/diff/1/23 File user/src/com/google/gwt/autobean/shared/AutoBean.java (right): http://gwt-code-reviews.appspot.com/1062801/diff/1/23#newcode55 user/src/com/google/gwt/autobean/shared/AutoBean.java:55: EncodedAutoBean<T> encode(); Feels funny, inflexible, to have this built into the AutoBean interface. I'd expect to instantiate a visitor that produced this when handed a bean, say EncodingVisitor, something like that. http://gwt-code-reviews.appspot.com/1062801/diff/1/24 File user/src/com/google/gwt/autobean/shared/AutoBeanCodex.java (right): http://gwt-code-reviews.appspot.com/1062801/diff/1/24#newcode81 user/src/com/google/gwt/autobean/shared/AutoBeanCodex.java:81: boolean isEncoded = EncodedAutoBean.class.equals(ctx.getElementType()); Gah! We *must* implement isAssignable and isInstance, this is nuts. http://gwt-code-reviews.appspot.com/1062801/diff/1/24#newcode253 user/src/com/google/gwt/autobean/shared/AutoBeanCodex.java:253: */ Noise http://gwt-code-reviews.appspot.com/1062801/diff/1/28 File user/src/com/google/gwt/autobean/shared/EncodedAutoBean.java (right): http://gwt-code-reviews.appspot.com/1062801/diff/1/28#newcode20 user/src/com/google/gwt/autobean/shared/EncodedAutoBean.java:20: * {...@link AutoBeanCodex#encodeForJsonPayload(AutoBean)}. This allows AutoBeans method name http://gwt-code-reviews.appspot.com/1062801/diff/1/28#newcode22 user/src/com/google/gwt/autobean/shared/EncodedAutoBean.java:22: * single payload. Be good to note where this is an issue. "For example, in RequestFactory messages and EntityProxies are backed by different AutoBeanFactories" http://gwt-code-reviews.appspot.com/1062801/diff/1/35 File user/src/com/google/gwt/core/client/impl/WeakMapping.java (right): http://gwt-code-reviews.appspot.com/1062801/diff/1/35#newcode26 user/src/com/google/gwt/core/client/impl/WeakMapping.java:26: * (except for Strings). This implementation is used in hosted mode. s/hosted/development Sounds like you need to update this comment anyway, since you're using it on the server now? http://gwt-code-reviews.appspot.com/1062801/diff/1/35#newcode31 user/src/com/google/gwt/core/client/impl/WeakMapping.java:31: * This implementation is used in hosted mode only. It uses a HashMap to still true? http://gwt-code-reviews.appspot.com/1062801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
