Thanks! just a few more comments, otherwise pretty much LGTM.
http://gwt-code-reviews.appspot.com/1380806/diff/25001/user/src/com/google/gwt/safehtml/rebind/SafeHtmlTemplatesImplMethodCreator.java File user/src/com/google/gwt/safehtml/rebind/SafeHtmlTemplatesImplMethodCreator.java (right): http://gwt-code-reviews.appspot.com/1380806/diff/25001/user/src/com/google/gwt/safehtml/rebind/SafeHtmlTemplatesImplMethodCreator.java#newcode302 user/src/com/google/gwt/safehtml/rebind/SafeHtmlTemplatesImplMethodCreator.java:302: if (HtmlContext.Type.URL_ATTRIBUTE_START == contextType) { Hmm, I just realized there's another case: When a parameter occurs in a URI attribute but not at the beginning (e.g., <img src='{http://foo.com/{0}'>). In that case we get contextType==ATTRIBUTE_VALUE, and would show the warning from the else branch, which wouldn't be exactly accurate. Fixing this unfortunately isn't trivial, because that case (in a URI attribute but not at the beginning) isn't exposed in HtmlContext. Perhaps take a TODO? I'm starting to think that this code needs some refactoring... http://gwt-code-reviews.appspot.com/1380806/diff/25001/user/src/com/google/gwt/safehtml/shared/UriUtils.java File user/src/com/google/gwt/safehtml/shared/UriUtils.java (right): http://gwt-code-reviews.appspot.com/1380806/diff/25001/user/src/com/google/gwt/safehtml/shared/UriUtils.java#newcode65 user/src/com/google/gwt/safehtml/shared/UriUtils.java:65: StringBuilder sb = new StringBuilder(); Maybe write this as the else branch, it'll be a little easier to read I think. http://gwt-code-reviews.appspot.com/1380806/diff/25001/user/src/com/google/gwt/safehtml/shared/UriUtils.java#newcode186 user/src/com/google/gwt/safehtml/shared/UriUtils.java:186: public static SafeUri fromUntrustedString(String s) { It might make sense to call maybeCheckValidUri here too? http://gwt-code-reviews.appspot.com/1380806/diff/25001/user/src/com/google/gwt/safehtml/shared/UriUtils.java#newcode236 user/src/com/google/gwt/safehtml/shared/UriUtils.java:236: if (isSafeUri(uri)) { Since you've created SafeUriHostedModeUtils.maybeCheckValidUri(s), we perhaps should call this here too (since it's a stricter check of URI well formedness), i.e. if (SafeUriHostedModeUtils.maybeCheckValidUri(uri) && isSafeUri(uri)) ? http://gwt-code-reviews.appspot.com/1380806/diff/25001/user/src/com/google/gwt/user/client/ui/Image.java File user/src/com/google/gwt/user/client/ui/Image.java (right): http://gwt-code-reviews.appspot.com/1380806/diff/25001/user/src/com/google/gwt/user/client/ui/Image.java#newcode144 user/src/com/google/gwt/user/client/ui/Image.java:144: private String url = null; I'm still wondering about the change we discussed earlier, of making this a SafeUri, and using UriUtils#fromUntrustedString in the Image(String url) constructor. You'd have to add getSafeUri to ClippedState (or just change getUrl to return the SafeUri -- after all this is a private class so there should be only internal uses). http://gwt-code-reviews.appspot.com/1380806/diff/25001/user/test/com/google/gwt/safehtml/shared/GwtUriUtilsTest.java File user/test/com/google/gwt/safehtml/shared/GwtUriUtilsTest.java (right): http://gwt-code-reviews.appspot.com/1380806/diff/25001/user/test/com/google/gwt/safehtml/shared/GwtUriUtilsTest.java#newcode67 user/test/com/google/gwt/safehtml/shared/GwtUriUtilsTest.java:67: public void testEncode_withEscapesInvalidEscapes() { Maybe add a test for a URL containing utf-8 characters? http://gwt-code-reviews.appspot.com/1380806/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
