It took me some time, and I haven't thoroughly reviewed all the code, neither did I tried it yet against my own code base.
I really like the move to APT, but to replace the RFIV, there would need to be a DomainToClientMapper, probably generated in a similar way as the TypeTokenResolver.Builder. In the small tests I did, the interfaces validated with RfValidator but later failed in RFIV (I did something similar to issue 5926 http://code.google.com/p/google-web-toolkit/issues/detail?id=5926 i.e. with bridge methods; in my case, using generics because that's the issue we're facing in our app, which shouldn't require a @SkipInterfaceValidation). I did the review in several short periods, jumping from one class to the other, so maybe the comments don't line up very well. http://gwt-code-reviews.appspot.com/1467804/diff/1/user/build.xml File user/build.xml (right): http://gwt-code-reviews.appspot.com/1467804/diff/1/user/build.xml#newcode155 user/build.xml:155: <exclude name="com/google/web/bindery/requestfactory/apt/**"/> Why is this needed? I can understand that it makes iterating on the APT code easier (changes you make to it aren't taken into account when determining whether to precompile modules –which is slow and resource consuming–, when launching, e.g., unit tests) but you probably meant to only have it a s a temporary measure? (I mean, same would be true of many **/server/** classes) http://gwt-code-reviews.appspot.com/1467804/diff/1/user/src/com/google/web/bindery/requestfactory/apt/DomainChecker.java File user/src/com/google/web/bindery/requestfactory/apt/DomainChecker.java (right): http://gwt-code-reviews.appspot.com/1467804/diff/1/user/src/com/google/web/bindery/requestfactory/apt/DomainChecker.java#newcode50 user/src/com/google/web/bindery/requestfactory/apt/DomainChecker.java:50: private ExecutableElement found; shouldn't they all be 'final' ? http://gwt-code-reviews.appspot.com/1467804/diff/1/user/src/com/google/web/bindery/requestfactory/apt/DomainChecker.java#newcode83 user/src/com/google/web/bindery/requestfactory/apt/DomainChecker.java:83: TypeMirror paramType = maybeBox(paramElement.asType(), state); Correct me if I'm wrong, but this is a change from RFIV, where argument types weren't "boxed" before being compared. If this is a willful change, then ReflectiveServiceLayer would have to be updated as well when looking up methods. Similarly, return type is only boxed in RFIV for service methods, not properties. http://gwt-code-reviews.appspot.com/1467804/diff/1/user/src/com/google/web/bindery/requestfactory/apt/DomainChecker.java#newcode96 user/src/com/google/web/bindery/requestfactory/apt/DomainChecker.java:96: || state.types.isSubsignature((ExecutableType) x.asType(), (ExecutableType) found That's great, as it seems to fix issue 5926. (unfortunately, because RFIV is still used, an interface that validates with RfValidator at compile-time can fail with RFIV at runtime) http://code.google.com/p/google-web-toolkit/issues/detail?id=5926 http://gwt-code-reviews.appspot.com/1467804/diff/1/user/src/com/google/web/bindery/requestfactory/apt/DomainChecker.java#newcode111 user/src/com/google/web/bindery/requestfactory/apt/DomainChecker.java:111: returnType = maybeBox(returnType, state); Why isn't this done in the constructor? http://gwt-code-reviews.appspot.com/1467804/diff/1/user/src/com/google/web/bindery/requestfactory/apt/DomainChecker.java#newcode115 user/src/com/google/web/bindery/requestfactory/apt/DomainChecker.java:115: return scanAllInheritedMethods(x, state); Isn't that more or less the default behavior? (default behavior would visit fields and nested types too) http://gwt-code-reviews.appspot.com/1467804/diff/1/user/src/com/google/web/bindery/requestfactory/apt/DomainChecker.java#newcode149 user/src/com/google/web/bindery/requestfactory/apt/DomainChecker.java:149: TypeMirror returnType = x.getReturnType().accept(new ClientToDomainMapper(), state); x should probably be turned into an ExecutableType using state.types.asMemberOf(domainType.asType(), x) to make sure we preserve "actual type variables" in the hierarchy (and similarly in MethodFinder). I mean, if we have: interface BaseFooProxy<T> { T getT(); void setT(T); } and interface FooProxy extends BaseFooProxy<BarProxy> { } the ExecutableElement will have a "formal type variable" T and we only know about its bounds. What we'd like to check here is that the domain object has "BarProxy getT()" and "void setT(BarProxy)" methods, and only ExecutableType would give us the information. There might be other places where that'd be beneficial. ...and that'd help fix issue 5926 in an elegant way: http://code.google.com/p/google-web-toolkit/issues/detail?id=5926 http://gwt-code-reviews.appspot.com/1467804/diff/1/user/src/com/google/web/bindery/requestfactory/apt/Finder.java File user/src/com/google/web/bindery/requestfactory/apt/Finder.java (right): http://gwt-code-reviews.appspot.com/1467804/diff/1/user/src/com/google/web/bindery/requestfactory/apt/Finder.java#newcode34 user/src/com/google/web/bindery/requestfactory/apt/Finder.java:34: * Make this externally-configurable? That'd be easy, using ProcessingEnvironment#getOptions. http://gwt-code-reviews.appspot.com/1467804/diff/1/user/src/com/google/web/bindery/requestfactory/apt/Finder.java#newcode46 user/src/com/google/web/bindery/requestfactory/apt/Finder.java:46: // Ignore anything other than interfaces Shouldn't that comment be *above* the 'if' rather than within? http://gwt-code-reviews.appspot.com/1467804/diff/1/user/src/com/google/web/bindery/requestfactory/apt/ProxyScanner.java File user/src/com/google/web/bindery/requestfactory/apt/ProxyScanner.java (right): http://gwt-code-reviews.appspot.com/1467804/diff/1/user/src/com/google/web/bindery/requestfactory/apt/ProxyScanner.java#newcode40 user/src/com/google/web/bindery/requestfactory/apt/ProxyScanner.java:40: if (x.getSimpleName().contentEquals("stableId") && x.getParameters().isEmpty()) { How about overriding shouldIgnore and put this check in there? http://gwt-code-reviews.appspot.com/1467804/diff/1/user/src/com/google/web/bindery/requestfactory/apt/ProxyScanner.java#newcode51 user/src/com/google/web/bindery/requestfactory/apt/ProxyScanner.java:51: // Parameters checked by visitVariable Should that comment be outside the 'if' ? http://gwt-code-reviews.appspot.com/1467804/diff/1/user/src/com/google/web/bindery/requestfactory/apt/ProxyScanner.java#newcode62 user/src/com/google/web/bindery/requestfactory/apt/ProxyScanner.java:62: state.poison(x, "A proxy must be annotated with %s, %s, or %s", ProxyFor.class Isn't it a breaking change? That is, in 2.3 (and 2.4) I can declare a "BaseFooProxy extends EntityProxy" without ProxyFor/etc. annotation, and then define sub-interfaces with the annotation. With this check, I have to move the "extends EntityProxy" to the sub-interfaces (as you did in the unit test). I don't really mind, but it'll probably break a few people's code (starting with mine) Otherwise, maybe the check could be moved to places referencing proxy types, i.e. places where ProxyScanner is called except RfValidator (and similarly for request context's @Service/ServiceName vs. RequestContextScanner). http://gwt-code-reviews.appspot.com/1467804/diff/1/user/src/com/google/web/bindery/requestfactory/apt/ProxyScanner.java#newcode68 user/src/com/google/web/bindery/requestfactory/apt/ProxyScanner.java:68: // See javadoc on Element.getAnnotation() for why it works this way Just wondering, why isn't State#checkExtraTypes using this method? or why isn't this code using the same algorithm as State#checkExtraTypes? (in other words: why two ways of doing the same thing, in the same codebase?) http://gwt-code-reviews.appspot.com/1467804/diff/1/user/src/com/google/web/bindery/requestfactory/apt/RequestContextScanner.java File user/src/com/google/web/bindery/requestfactory/apt/RequestContextScanner.java (right): http://gwt-code-reviews.appspot.com/1467804/diff/1/user/src/com/google/web/bindery/requestfactory/apt/RequestContextScanner.java#newcode71 user/src/com/google/web/bindery/requestfactory/apt/RequestContextScanner.java:71: } else if (isSetter(x, state)) { Setters in a RequestContext? Did I miss some earlier change? http://gwt-code-reviews.appspot.com/1467804/diff/1/user/src/com/google/web/bindery/requestfactory/apt/RfApt.java File user/src/com/google/web/bindery/requestfactory/apt/RfApt.java (right): http://gwt-code-reviews.appspot.com/1467804/diff/1/user/src/com/google/web/bindery/requestfactory/apt/RfApt.java#newcode149 user/src/com/google/web/bindery/requestfactory/apt/RfApt.java:149: + BinaryName.toSourceName(entry.getValue()) + ".class.getName());"); Now there's another issue when applied to the gwt-user unit tests: many test cases declare non-public inner interfaces (generally package-protected) and the generated build cannot compile due to type visibility issues. Looks like Class.forName is the only real fix. Maybe that should be done in TypeTokenResolver.Builder#addTypeToken (so it'll also be applied to tokens loaded from properties files), and this code simply generate string literals? http://gwt-code-reviews.appspot.com/1467804/diff/1/user/src/com/google/web/bindery/requestfactory/apt/ScannerBase.java File user/src/com/google/web/bindery/requestfactory/apt/ScannerBase.java (right): http://gwt-code-reviews.appspot.com/1467804/diff/1/user/src/com/google/web/bindery/requestfactory/apt/ScannerBase.java#newcode116 user/src/com/google/web/bindery/requestfactory/apt/ScannerBase.java:116: protected void poisonIfRedundant(State state, TypeElement x, Annotation... annotations) { That method deserves a javadoc! I thought it was misnamed as it wasn't checking much things, but reading ProxyScanner I found that one annotation has already be "asserted" and the passed annotations are other possible annotations on the same element (a type, generally), that have been "resolved" and should then be 'null' (otherwise, it means they're present, thus redundant). The javadoc may be just some sample code showing how it's supposed to be used. http://gwt-code-reviews.appspot.com/1467804/diff/1/user/src/com/google/web/bindery/requestfactory/apt/State.java File user/src/com/google/web/bindery/requestfactory/apt/State.java (right): http://gwt-code-reviews.appspot.com/1467804/diff/1/user/src/com/google/web/bindery/requestfactory/apt/State.java#newcode161 user/src/com/google/web/bindery/requestfactory/apt/State.java:161: return clientToDomainMain; Collections.unmodifiableMap() ? (as a safeguard) http://gwt-code-reviews.appspot.com/1467804/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
