[gwt-contrib] Re: Add SafeUri type, similar to SafeHtml but for values in a URL attribute context. (issue1447812)

2011-06-06 Thread xtof 

http://gwt-code-reviews.appspot.com/1447812/

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: Add SafeUri type, similar to SafeHtml but for values in a URL attribute context. (issue1447812)

2011-06-03 Thread xtof 


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/03 17:00:04, jlabanca wrote:

On 2011/06/03 07:39:23, xtof wrote:
> 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
>  "{0}"
> 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.



I don't think its even possible. The check in emitParameterExpression

ensures

that SafeUri is only used in a URL_ATTRIBUTE_ENTIRE context.


Right. Sorry that's what I meant to say, but clearly didn't state it
very well :)

http://gwt-code-reviews.appspot.com/1447812/diff/1003/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/1447812/diff/1003/user/test/com/google/gwt/safehtml/client/SafeHtmlTemplatesTest.java#newcode58
user/test/com/google/gwt/safehtml/client/SafeHtmlTemplatesTest.java:58:
@Template("{0}{1}")
On 2011/06/03 17:00:04, jlabanca wrote:

Missing a closing span.  If you aren't testing something specific to

the

malformed HTML, I suggest you add the closing span back on.


Done.

http://gwt-code-reviews.appspot.com/1447812/

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: Add SafeUri type, similar to SafeHtml but for values in a URL attribute context. (issue1447812)

2011-06-03 Thread jlabanca 

LGTM


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/03 07:39:23, xtof wrote:

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
  "{0}"
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.


I don't think its even possible. The check in emitParameterExpression
ensures that SafeUri is only used in a URL_ATTRIBUTE_ENTIRE context.

http://gwt-code-reviews.appspot.com/1447812/diff/1003/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/1447812/diff/1003/user/test/com/google/gwt/safehtml/client/SafeHtmlTemplatesTest.java#newcode58
user/test/com/google/gwt/safehtml/client/SafeHtmlTemplatesTest.java:58:
@Template("{0}{1}")
Missing a closing span.  If you aren't testing something specific to the
malformed HTML, I suggest you add the closing span back on.

http://gwt-code-reviews.appspot.com/1447812/

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: Add SafeUri type, similar to SafeHtml but for values in a URL attribute context. (issue1447812)

2011-06-03 Thread jlabanca 

LGTM

http://gwt-code-reviews.appspot.com/1447812/

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: Add SafeUri type, similar to SafeHtml but for values in a URL attribute context. (issue1447812)

2011-06-03 Thread xtof 

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;)
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
 "{0}"
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
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!
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:4

[gwt-contrib] Re: Add SafeUri type, similar to SafeHtml but for values in a URL attribute context. (issue1447812)

2011-06-03 Thread xtof 

http://gwt-code-reviews.appspot.com/1447812/

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: Add SafeUri type, similar to SafeHtml but for values in a URL attribute context. (issue1447812)

2011-06-02 Thread jlabanca 

On 2011/06/02 17:59:40, xtof wrote:

On Thu, Jun 2, 2011 at 10:51,  wrote:



>
>
>


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!
> unsafeCastFromUntrustedString() is better.  Can we also deprecate

the

> method?
>
Sounds good.  Should I also deprecate methods that call this method

(e.g.

Image(String))? That would seem to make sense...

Agreed.



>
> 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://gwt-code-reviews.appspot.com/1447812/

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: Add SafeUri type, similar to SafeHtml but for values in a URL attribute context. (issue1447812)

2011-06-02 Thread Christoph Kern 
On Thu, Jun 2, 2011 at 10:51,  wrote:

>
>
> 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!
> unsafeCastFromUntrustedString() is better.  Can we also deprecate the
> method?
>
Sounds good.  Should I also deprecate methods that call this method (e.g.
Image(String))? That would seem to make sense...


>
> 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

[gwt-contrib] Re: Add SafeUri type, similar to SafeHtml but for values in a URL attribute context. (issue1447812)

2011-06-02 Thread jlabanca 


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!
unsafeCastFromUntrustedString() is better.  Can we also deprecate the
method?

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

[gwt-contrib] Re: Add SafeUri type, similar to SafeHtml but for values in a URL attribute context. (issue1447812)

2011-06-02 Thread xtof 


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#newcode305
user/src/com/google/gwt/safehtml/rebind/SafeHtmlTemplatesImplMethodCreator.java:305:
// not a fatal error.
On 2011/06/02 13:47:05, jlabanca wrote:

Why isn't this a fatal error?  Using a SafeUri in the middle of a URL

attribute

seems just as bad as the above case, and using it outside of the URL

context

seems like a dev mistake.

I think I agree; we should treat this analogous to the SafeStyles case
above.  Thomas, ok with you to make that change?

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!
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

[gwt-contrib] Re: Add SafeUri type, similar to SafeHtml but for values in a URL attribute context. (issue1447812)

2011-06-02 Thread jlabanca 


http://gwt-code-reviews.appspot.com/1447812/diff/1/user/test/com/google/gwt/user/client/ui/impl/ClippedImagePrototypeTest.java
File
user/test/com/google/gwt/user/client/ui/impl/ClippedImagePrototypeTest.java
(left):

http://gwt-code-reviews.appspot.com/1447812/diff/1/user/test/com/google/gwt/user/client/ui/impl/ClippedImagePrototypeTest.java#oldcode42
user/test/com/google/gwt/user/client/ui/impl/ClippedImagePrototypeTest.java:42:
private static class TestLoadListener implements LoadListener {
On 2011/06/02 13:47:05, jlabanca wrote:

Don't remove this.  Its actually used in a commented out test.  In

theory the

issue has been fixed, so I'll take a look at re-enabling the test.


Ignore this comment.  TestLoadListener isn't used in the commented out
method either.  You can remove it.

http://gwt-code-reviews.appspot.com/1447812/

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: Add SafeUri type, similar to SafeHtml but for values in a URL attribute context. (issue1447812)

2011-06-02 Thread jlabanca 


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;)
MISSING
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.

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.
Should this be deprecated like ImageResource?

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#newcode305
user/src/com/google/gwt/safehtml/rebind/SafeHtmlTemplatesImplMethodCreator.java:305:
// not a fatal error.
Why isn't this a fatal error?  Using a SafeUri in the middle of a URL
attribute seems just as bad as the above case, and using it outside of
the URL context seems like a dev mistake.

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)) {
Is it safe to use safeUri in a text context?  Seems like a mistake at
the least.

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 {
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.

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
Will this look correct in javadoc?  You might need to exceed the 100
line limit here.

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";
Can you add a comment explaining how this is set?

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!
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.

http://gwt-code-reviews.appspot.com/1447812/diff/1/user/test/com/google/gwt/user/client/ui/impl/ClippedImagePrototypeTest.java
File
user/test/com/google/gwt/user/client/ui/impl/ClippedImagePrototypeTest.java
(left):

http://gwt-code-reviews.appspot.com/1447812/diff/1/user/test/com/google/gwt/user/client/ui/impl/ClippedImagePrototypeTest.java#oldcode42
user/test/com/google/gwt/user/client/ui/impl/ClippedImagePrototypeTest.java:42:
private static class TestLoadListener implements LoadListener {
Don't remove this.  Its actually used in a commented out test.  In
theory the issue has been fixed, so I'll take a look at re-enabling the
test.

http://gwt-code-reviews.appspot.com/1447812/

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: Add SafeUri type, similar to SafeHtml but for values in a URL attribute context. (issue1447812)

2011-06-01 Thread xtof 

This is a continuation of the review of
http://gwt-code-reviews.appspot.com/1380806, authored by
http://gwt-code-reviews.appspot.com/user/tbroyer, to capture minor
changes made by the submitter.

For the main code review discussion, see issue 1380806.


http://gwt-code-reviews.appspot.com/1447812/

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors