Thanks for the review! Please take another look...
--xtof
http://gwt-code-reviews.appspot.com/1447812/diff/1/tools/api-checker/config/gwt23_24userApi.conf
File tools/api-checker/config/gwt23_24userApi.conf (right):
http://gwt-code-reviews.appspot.com/1447812/diff/1/tools/api-checker/config/gwt23_24userApi.conf#newcode155
tools/api-checker/config/gwt23_24userApi.conf:155:
com.google.gwt.user.client.ui.impl.ClippedImageImpl::adjust(Lcom/google/gwt/dom/client/Element;Ljava/lang/String;IIII)
MISSING
On 2011/06/02 13:47:05, jlabanca wrote:
Instead of all of these exceptions for ClippedImageImpl, you can
exclude
com.google.gwt.user.client.ui.impl from api checks. We don't make any
guarantees
about the API of impl classes.
Done.
http://gwt-code-reviews.appspot.com/1447812/diff/1/user/src/com/google/gwt/resources/client/DataResource.java
File user/src/com/google/gwt/resources/client/DataResource.java (right):
http://gwt-code-reviews.appspot.com/1447812/diff/1/user/src/com/google/gwt/resources/client/DataResource.java#newcode66
user/src/com/google/gwt/resources/client/DataResource.java:66: * will be
an absolute URL.
On 2011/06/02 13:47:05, jlabanca wrote:
Should this be deprecated like ImageResource?
Done.
http://gwt-code-reviews.appspot.com/1447812/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/1447812/diff/1/user/src/com/google/gwt/safehtml/rebind/SafeHtmlTemplatesImplMethodCreator.java#newcode432
user/src/com/google/gwt/safehtml/rebind/SafeHtmlTemplatesImplMethodCreator.java:432:
if (isSafeUri(parameterType)) {
On 2011/06/02 13:47:05, jlabanca wrote:
Is it safe to use safeUri in a text context? Seems like a mistake at
the least.
It would be safe here, since it's going to be HTML escaped just like any
other string. I can't think of too many reasons anyone would
legitimately do this. Perhaps in a template used to linkify URLs, as in
"<a href='{0}'>{0}</a>"
where {0} is a SafeUri.
Seems like a pretty unlikely scenario, and I think I'll remove this
special case here in the interest of simplicity. In any case, per your
comment above I've made the change so that this would throw an error.
http://gwt-code-reviews.appspot.com/1447812/diff/1/user/src/com/google/gwt/safehtml/shared/SafeUriHostedModeUtils.java
File user/src/com/google/gwt/safehtml/shared/SafeUriHostedModeUtils.java
(right):
http://gwt-code-reviews.appspot.com/1447812/diff/1/user/src/com/google/gwt/safehtml/shared/SafeUriHostedModeUtils.java#newcode32
user/src/com/google/gwt/safehtml/shared/SafeUriHostedModeUtils.java:32:
public class SafeUriHostedModeUtils {
On 2011/06/02 13:47:05, jlabanca wrote:
Should this class be package protected? It looks like an impl class,
but its
public in a non-impl package, which makes it look like anyone can use
it.
That doesn't seem to work; if I make it package private I get
java.lang.IllegalAccessError:
com/google/gwt/safehtml/shared/SafeUriHostedModeUtils
at
com.google.gwt.safehtml.shared.UriUtils.fromTrustedString(UriUtils.java:199
all over the unit tests. I'm guessing that super-sourced classes need
to be public.
http://gwt-code-reviews.appspot.com/1447812/diff/1/user/src/com/google/gwt/safehtml/shared/SafeUriHostedModeUtils.java#newcode39
user/src/com/google/gwt/safehtml/shared/SafeUriHostedModeUtils.java:39:
* >RFC 3987bis Web Addresses</a>
On 2011/06/02 13:47:05, jlabanca wrote:
Will this look correct in javadoc? You might need to exceed the 100
line limit
here.
Done.
http://gwt-code-reviews.appspot.com/1447812/diff/1/user/src/com/google/gwt/safehtml/shared/SafeUriHostedModeUtils.java#newcode54
user/src/com/google/gwt/safehtml/shared/SafeUriHostedModeUtils.java:54:
public static final String FORCE_CHECK_VALID_URI =
"com.google.gwt.safehtml.ForceCheckValidUri";
On 2011/06/02 13:47:05, jlabanca wrote:
Can you add a comment explaining how this is set?
Done.
http://gwt-code-reviews.appspot.com/1447812/diff/1/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/1447812/diff/1/user/src/com/google/gwt/safehtml/shared/UriUtils.java#newcode209
user/src/com/google/gwt/safehtml/shared/UriUtils.java:209: *
safe!</strong>
On 2011/06/02 17:51:54, jlabanca wrote:
unsafeCastFromUntrustedString() is better. Can we also deprecate the
method?
Done. I'm having second thoughts about deprecating its callers though
(a bunch of methods of Image). We haven't deprecated "plain string"
methods for SafeHtml versions elsewhere either, so this would be
somewhat inconsistent.
I think I'll leave the Image(String) methods alone for now?
Use code should always be able to use one of the other methods. Only
library
code (GWT and libraries based on GWT) have the legacy support problem.
On 2011/06/02 17:45:16, xtof wrote:
> On 2011/06/02 13:47:05, jlabanca wrote:
> > This method worries me. When I saw the name, I assumed it was the
equivalent
> of
> > fromString(). Anyone who looks at the method name without reading
the
JavaDoc
> > might assume the same.
> >
> > I suggest we remove the method and let users manage unsafe URIs.
That
forces
> > the user to make the tough decisions, whether they sanitize the
URI, or call
> > fromTrustedString() even if the URI isn't trusted.
>
> This method is intended for use in places where a string we don't
know
anything
> about needs to be turned into a SafeUri in a legacy-API situation.
For
instance
> in this CL, the Image class has been refactored to use SafeUri
internally.
> However, it retains the Image(String uri) constructor, which uses
this method
to
> turn the string into a SafeUri to call the Image(SafeUri uri)
constructor
with.
>
> I'd prefer that we don't use the fromTrustedString method in those
situations:
> Use of that method is essentially an assertion by the programmer
that they can
> ensure from context that the argument satisfies the SafeUri
contract. In the
> Image(String) case, this is not so.
>
> I agree that the name isn't scary enough though.
>
> Perhaps, "unsafeCastFromUntrustedString" or something like that?
http://gwt-code-reviews.appspot.com/1447812/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
