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))); On 2011/03/23 20:42:52, xtof wrote:
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>?
That sure would be far better! Note however that with the removal of the warning for using String parameter in URL context, we can just revert that change. (Done.)
Would presumably be a breaking API change though.
Maybe add a new Cell then? 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()); On 2011/03/23 20:42:52, xtof wrote:
It would be nice if there was a getSafeHtml, to avoid the need for a fromTrustedString here (see also comment in ClippedImagePrototype).
Done. 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 On 2011/03/23 20:42:52, xtof wrote:
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).
Done. 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())) { On 2011/03/23 20:42:52, xtof wrote:
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.
Done. 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. On 2011/03/23 20:42:52, xtof wrote:
This should be "[...] in a URI context within an HTML document, for
example in a
URI-valued attribute", or some such...
Done. 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)) { On 2011/03/23 20:42:52, xtof wrote:
instanceof SafeUri
Done. 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()); On 2011/03/23 20:42:52, xtof wrote:
SafeUri
Done. 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); On 2011/03/23 20:42:52, xtof wrote:
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?
Done. 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()); On 2011/03/23 20:42:52, xtof wrote:
100 chars, here and line 114
The files need to be reformatted using the new formatting rules anyway, so I didn't even try to fit in the 100-chars line length (to not mix formatting changes with other "code changes" and make it easier to review) http://gwt-code-reviews.appspot.com/1380806/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
