I'll leave the final word to Brian. Some minor comments below, but otherwise looks good.
http://gwt-code-reviews.appspot.com/1727807/diff/14001/user/src/com/google/gwt/editor/client/impl/SimpleViolation.java File user/src/com/google/gwt/editor/client/impl/SimpleViolation.java (right): http://gwt-code-reviews.appspot.com/1727807/diff/14001/user/src/com/google/gwt/editor/client/impl/SimpleViolation.java#newcode141 user/src/com/google/gwt/editor/client/impl/SimpleViolation.java:141: // a delegate. If for some reason it doesn't match, then "If for some reason it doesn't match, and we enter findParent with an empty absolutePath, it will throw an exception." I wonder if that case should be moved out of findParent though, so it's close to that comment. Or we could simply inline 'findParent' here. http://gwt-code-reviews.appspot.com/1727807/diff/14001/user/src/com/google/gwt/editor/client/impl/SimpleViolation.java#newcode176 user/src/com/google/gwt/editor/client/impl/SimpleViolation.java:176: originalAbsolutePath.substring(absolutePath.length()); Could we compute this only once outside the loop? Maybe even making 2 loops: if (originalAbsolutePath.equals(absolutePath)) { for (...) { delegate.recordError(...); } } else { String addlPath = ...; for (...) { delegate.recordError(...); } } http://gwt-code-reviews.appspot.com/1727807/diff/14001/user/src/com/google/gwt/editor/client/impl/SimpleViolation.java#newcode210 user/src/com/google/gwt/editor/client/impl/SimpleViolation.java:210: private static String findParent(String originalAbsolutePath, Because it doesn't really "find" anything, I'd rather name it 'getParentPath'. http://gwt-code-reviews.appspot.com/1727807/diff/14001/user/src/com/google/gwt/editor/client/impl/SimpleViolation.java#newcode214 user/src/com/google/gwt/editor/client/impl/SimpleViolation.java:214: "no delegates or editors found for path: " + originalAbsolutePath); Not sure we want this message in the compiled JS, given it's something that should really never happen. Maybe throw an exception without message, but add an assert just before the throw, for when assertions are enabled (DevMode / SuperDevMode, or "debug builds"): assert false : "no delegates or editors found... throw new IllegalStateException(); Please ignore if the compiler is smart enough to prune it already. http://gwt-code-reviews.appspot.com/1727807/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
