http://gwt-code-reviews.appspot.com/1380806/diff/1/user/src/com/google/gwt/cell/client/ImageCell.java File user/src/com/google/gwt/cell/client/ImageCell.java (right):
http://gwt-code-reviews.appspot.com/1380806/diff/1/user/src/com/google/gwt/cell/client/ImageCell.java#newcode58 user/src/com/google/gwt/cell/client/ImageCell.java:58: sb.append(template.img(UriUtils.fromString(value))); It seems that in this case (and similar ones down the line), changing the template signature to img(SafeUri) results in unnecessary verbosity -- when the template took a string, the template generator inserts the sanitizeUri call automatically, to the same effect. Would it make sense to take a SafeUri and expose that in the render signature, i.e make this an AbstractCell<SafeUri>? Would presumably be a breaking API change though. http://gwt-code-reviews.appspot.com/1380806/diff/1/user/src/com/google/gwt/cell/client/ImageResourceCell.java File user/src/com/google/gwt/cell/client/ImageResourceCell.java (right): http://gwt-code-reviews.appspot.com/1380806/diff/1/user/src/com/google/gwt/cell/client/ImageResourceCell.java#newcode39 user/src/com/google/gwt/cell/client/ImageResourceCell.java:39: value).getHTML()); It would be nice if there was a getSafeHtml, to avoid the need for a fromTrustedString here (see also comment in ClippedImagePrototype). http://gwt-code-reviews.appspot.com/1380806/diff/1/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/1/user/src/com/google/gwt/safehtml/rebind/SafeHtmlTemplatesImplMethodCreator.java#newcode117 user/src/com/google/gwt/safehtml/rebind/SafeHtmlTemplatesImplMethodCreator.java:117: * <li>If the template parameter occurs at the start of a URI-valued The comment isn't quite correct as written, because as is it implies that processing for start-of-URI happens even if it was a SafeUri (but it is actually omitted). http://gwt-code-reviews.appspot.com/1380806/diff/1/user/src/com/google/gwt/safehtml/rebind/SafeHtmlTemplatesImplMethodCreator.java#newcode268 user/src/com/google/gwt/safehtml/rebind/SafeHtmlTemplatesImplMethodCreator.java:268: if (!SAFE_URI_FQCN.equals(parameterType.getQualifiedSourceName())) { I'm not sure we want to print a warning here. I think it's quite legitimate for a template to have a String-valued variable in URL_START context, and just let the template take care of sanitization (see also comment in ImageCell.java). This is analogous to using String vs SafeHtml in "inner text" context. In a lot of cases it makes sense for the argument be String valued and let the template take care of escaping; only if I don't want the template to touch it I need to supply a SafeHtml. http://gwt-code-reviews.appspot.com/1380806/diff/1/user/src/com/google/gwt/safehtml/rebind/SafeHtmlTemplatesImplMethodCreator.java#newcode340 user/src/com/google/gwt/safehtml/rebind/SafeHtmlTemplatesImplMethodCreator.java:340: expression = expression + ".asString()"; This is one of those places where it's annoying that we decided to use asString() instead of just letting toString() return the wrapped contents of the SafeXyz. Too late now... http://gwt-code-reviews.appspot.com/1380806/diff/1/user/src/com/google/gwt/safehtml/shared/SafeUri.java File user/src/com/google/gwt/safehtml/shared/SafeUri.java (right): http://gwt-code-reviews.appspot.com/1380806/diff/1/user/src/com/google/gwt/safehtml/shared/SafeUri.java#newcode21 user/src/com/google/gwt/safehtml/shared/SafeUri.java:21: * vulnerabilities) in an HTML context. This should be "[...] in a URI context within an HTML document, for example in a URI-valued attribute", or some such... http://gwt-code-reviews.appspot.com/1380806/diff/1/user/src/com/google/gwt/safehtml/shared/SafeUriString.java File user/src/com/google/gwt/safehtml/shared/SafeUriString.java (right): http://gwt-code-reviews.appspot.com/1380806/diff/1/user/src/com/google/gwt/safehtml/shared/SafeUriString.java#newcode65 user/src/com/google/gwt/safehtml/shared/SafeUriString.java:65: if (!(obj instanceof SafeHtml)) { instanceof SafeUri http://gwt-code-reviews.appspot.com/1380806/diff/1/user/src/com/google/gwt/safehtml/shared/SafeUriString.java#newcode68 user/src/com/google/gwt/safehtml/shared/SafeUriString.java:68: return uri.equals(((SafeHtml) obj).asString()); SafeUri http://gwt-code-reviews.appspot.com/1380806/diff/1/user/src/com/google/gwt/user/client/ui/impl/ClippedImagePrototype.java File user/src/com/google/gwt/user/client/ui/impl/ClippedImagePrototype.java (right): http://gwt-code-reviews.appspot.com/1380806/diff/1/user/src/com/google/gwt/user/client/ui/impl/ClippedImagePrototype.java#newcode68 user/src/com/google/gwt/user/client/ui/impl/ClippedImagePrototype.java:68: return impl.getHTML(url.asString(), left, top, width, height); Would it make sense to provide a getSafeHtml() method here that returns SafeHtml and wraps getHTML() with SafeHtmlUtils.fromTrustedString? This would push the fromTrusted closer to where the HTML is assembled in a supposedly trusted manner, making it more easily verified as correct. Perhaps SafeHtml could even be pushed into the impls, and use SafeHtmlTemplates within them? http://gwt-code-reviews.appspot.com/1380806/diff/1/user/test/com/google/gwt/safehtml/client/SafeHtmlTemplatesTest.java File user/test/com/google/gwt/safehtml/client/SafeHtmlTemplatesTest.java (right): http://gwt-code-reviews.appspot.com/1380806/diff/1/user/test/com/google/gwt/safehtml/client/SafeHtmlTemplatesTest.java#newcode110 user/test/com/google/gwt/safehtml/client/SafeHtmlTemplatesTest.java:110: UriUtils.fromTrustedString(GOOD_URL), SafeHtmlUtils.fromSafeConstant(HTML_MARKUP)).asString());
100 chars, here and line 114
http://gwt-code-reviews.appspot.com/1380806/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
