Thanks for the review, John. I have some questions for you below.
http://gwt-code-reviews.appspot.com/1472801/diff/1/user/src/com/google/gwt/uibinder/rebind/FieldWriterType.java File user/src/com/google/gwt/uibinder/rebind/FieldWriterType.java (right): http://gwt-code-reviews.appspot.com/1472801/diff/1/user/src/com/google/gwt/uibinder/rebind/FieldWriterType.java#newcode28 user/src/com/google/gwt/uibinder/rebind/FieldWriterType.java:28: RENDERABLE_STAMPER(2), On 2011/07/06 13:29:23, jlabanca wrote:
Should this have the same precedence as DOM_ID_HOLDER?
In principle that's ok, but you're right, it's best to be more explicit on these things. 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 13:29:23, jlabanca wrote:
getToken() is deprecated, use: builder.append(stamper.stamp(TEMPLATE.render()));
Explained on other comment. 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 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. 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#newcode25 user/src/com/google/gwt/user/client/ui/RenderableStamper.java:25: * On 2011/07/06 13:29:23, jlabanca wrote:
overly minimalistic javadoc
Oops, forgot to go back and fix this. Done. http://gwt-code-reviews.appspot.com/1472801/diff/1/user/src/com/google/gwt/user/client/ui/RenderableStamper.java#newcode40 user/src/com/google/gwt/user/client/ui/RenderableStamper.java:40: public String getToken() { On 2011/07/06 13:29:23, jlabanca wrote:
Add @Deprecated annotation, and @deprecated javadoc tag
Done. http://gwt-code-reviews.appspot.com/1472801/diff/1/user/src/com/google/gwt/user/client/ui/RenderableStamper.java#newcode48 user/src/com/google/gwt/user/client/ui/RenderableStamper.java:48: return Document.get().getElementById(token).cast(); On 2011/07/06 13:29:23, jlabanca wrote:
The .cast() call will trigger an NPE if the token is not found. Can
you add a
check, and add a javadoc comment that describes the expected behavior
if the
element is not attached?
Also, this would be a good place for a dev mode only runtime check to
verify
that the element is attached. A TODO would be fine for now.
Done. 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 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? 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 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? http://gwt-code-reviews.appspot.com/1472801/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
