http://gwt-code-reviews.appspot.com/1472801/diff/1/user/src/com/google/gwt/user/client/ui/Composite.java
File user/src/com/google/gwt/user/client/ui/Composite.java (right):
http://gwt-code-reviews.appspot.com/1472801/diff/1/user/src/com/google/gwt/user/client/ui/Composite.java#newcode111
user/src/com/google/gwt/user/client/ui/Composite.java:111:
builder.append(TEMPLATE.renderWithId(stamper.getToken()));
On 2011/07/06 14:09:08, jlabanca wrote:
On 2011/07/06 13:53:47, rdcastro wrote:
> On 2011/07/06 13:29:23, jlabanca wrote:
> > getToken() is deprecated, use:
> > builder.append(stamper.stamp(TEMPLATE.render()));
>
> Explained on other comment.
@SuppressWarning("deprecation")
Done.
http://gwt-code-reviews.appspot.com/1472801/diff/1/user/src/com/google/gwt/user/client/ui/RenderablePanel.java
File user/src/com/google/gwt/user/client/ui/RenderablePanel.java
(right):
http://gwt-code-reviews.appspot.com/1472801/diff/1/user/src/com/google/gwt/user/client/ui/RenderablePanel.java#newcode198
user/src/com/google/gwt/user/client/ui/RenderablePanel.java:198: String
id = stamper.getToken();
On 2011/07/06 14:09:08, jlabanca wrote:
On 2011/07/06 13:53:47, rdcastro wrote:
> On 2011/07/06 13:29:23, jlabanca wrote:
> > Can you stamp the SafeHtml returned from the TEMPLATE instead of
using the
ID
> > directly? Removing getToken() from our code will help confirm
that its safe
> to
> > hide the implementation.
>
> I can, but I didn't want to do that in this CL because I know
there'll be
_some_
> performance regression, but I don't if it'll be measurable. I'd like
to get
this
> checked in and then I can work separately on orkut measuring and
comparing
both
> approaches and decide which works best (and if the difference is
actually
> noticeable). I think I'll end up converting everything to
ElementBuilder,
which
> will be a biggish CL, but centered on orkut code.
>
> In any case, I'll have a follow-up to this CL that hides the
getToken method
at
> most within a week.
SGTM - but can you add a TODO explaining that you are going to test
performance
so the next person doesn't try to fix it.
Also, you'll have to add a @SuppressWarning("deprecation") annotation
now that
getToken() is deprecated.
Done.
http://gwt-code-reviews.appspot.com/1472801/diff/1/user/src/com/google/gwt/user/client/ui/RenderableStamper.java
File user/src/com/google/gwt/user/client/ui/RenderableStamper.java
(right):
http://gwt-code-reviews.appspot.com/1472801/diff/1/user/src/com/google/gwt/user/client/ui/RenderableStamper.java#newcode58
user/src/com/google/gwt/user/client/ui/RenderableStamper.java:58: int
endOfFirstTag = html.indexOf('>');
On 2011/07/06 14:09:08, jlabanca wrote:
It would still break with "<div/>".
1-) trim() and verify that the first character is a '<'. If not,
return.
2-) Find the first occurrence of either ' ' or '/>' or '>'; if none is
found,
return
(this can't happen, I think, because then the SafeHtml would be
malformed)
3-) Inject the ID
On 2011/07/06 13:53:47, rdcastro wrote:
> On 2011/07/06 13:29:23, jlabanca wrote:
> > This doesn't handle all forms of SafeHtml. For example, a simple
string
> "hello
> > world" is considered safe html, as is a self closing tag "<div
> > style='color:red;' />"
>
> Good point. I'm inclined to change this to:
>
> 1-) Find the first occurrence of '<'; if none is found, return;
> 2-) Find the first occurrence of either ' ' or '>'; if none is
found, return
> (this can't happen, I think, because then the SafeHtml would be
malformed)
> 3-) Inject the ID
>
> how does that sound?
Actually, come to think of it, the ID doesn't really have to be the
first attribute, so we can do something slightly simpler. How does this
look?
http://gwt-code-reviews.appspot.com/1472801/diff/1/user/src/com/google/gwt/user/client/ui/RenderableStamper.java#newcode62
user/src/com/google/gwt/user/client/ui/RenderableStamper.java:62:
.append(token)
On 2011/07/06 14:09:08, jlabanca wrote:
On 2011/07/06 13:53:47, rdcastro wrote:
> On 2011/07/06 13:29:23, jlabanca wrote:
> > Are we sure that the token is always safe? We should escape in
> > RenderableStamper's constructor, or at least add a comment
explaining that
we
> > assume it is safe.
>
> Escaping sounds like the right solution. What do you guys usually
use for
> escaping?
SafeHtmlUtils.htmlEscape(String)
By default, you generate the ID using alphanumeric characters, so it
will be
unchanged.
Done.
http://gwt-code-reviews.appspot.com/1472801/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
