http://gwt-code-reviews.appspot.com/1446811/diff/1/user/src/com/google/gwt/user/client/ui/IsRenderable.java File user/src/com/google/gwt/user/client/ui/IsRenderable.java (right):
http://gwt-code-reviews.appspot.com/1446811/diff/1/user/src/com/google/gwt/user/client/ui/IsRenderable.java#newcode29 user/src/com/google/gwt/user/client/ui/IsRenderable.java:29: public interface IsRenderable extends SafeHtmlRenderer<String> { Let's drop the extends SafeHtmlRenderer. Should document that it's up to the implementation to choose how to use the id (unless we pass in a helper, see below). Perhaps change its name to token to drive home the point. http://gwt-code-reviews.appspot.com/1446811/diff/1/user/src/com/google/gwt/user/client/ui/IsRenderable.java#newcode33 user/src/com/google/gwt/user/client/ui/IsRenderable.java:33: * the DOM tree. The id is assumed to be the same passed in at render time. I think it's time to implement a helper class, to be used for both rendering and lookup. If we don't have one soon retrofitting will be a nightmare. Document here that the receiver cannot assume that the element is attached to the dom, and @see to the helper. The helper could be something like: SafeHtml stampElement(SafeHtml mustBeOpeningTag, String token) { String tag = mustBeOpeningTag().asString(); assert tag.endsWith(">"); return SafeHtmlUtil.whatever(tag.substring(tag.length()-1) + ">", "id='" + token + "'>")); } Element find(String token, Element parentElement) { ensureAttached(parentElement); return Document.get().getElementById(token); } And I think the thing to do is pass an instance of the helper into the render() and wrap() calls. The alternative is to make some hacky static lookup scheme, but I think we'll regret that pretty quickly. You buying it? http://gwt-code-reviews.appspot.com/1446811/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/1446811/diff/1/user/src/com/google/gwt/user/client/ui/RenderablePanel.java#newcode61 user/src/com/google/gwt/user/client/ui/RenderablePanel.java:61: BUILT, RENDERED, WRAPPED, DONE I'm not liking the phases. I think we can simplify. http://gwt-code-reviews.appspot.com/1446811/diff/1/user/src/com/google/gwt/user/client/ui/RenderablePanel.java#newcode65 user/src/com/google/gwt/user/client/ui/RenderablePanel.java:65: // single callback. Time to take care of this. But that said, I don't like our plan from the other day. :-) We earlier talked about having binder generate subclasses of RenderablePanel that override no-op default implementations, but I'm having second thoughts about that. My rational was that it avoids unnecessary class definitions, but that's silly — a subclass of RenderablePanel is a new class just like a command object is — and it will make it trickier to allow people to use their own subclasses of RenderablePanel (which they will do, trust me). Instead, let's introduce a null command object and use it here, so that you can always call the thing without having to think abou it: public Command wrapInitializationCallback = NullCommand.INSTANCE; public Command detachedInitializationCallback = NullCommand.INSTANCE; That said, I think you're right to dislike the magical phase check in getElement(). Can you go ahead and rework this to give the wrap callback an argument? So this becomes: public WrapCallback wrapInitializationCallback = WrapCallback.INSTANCE; public Command detachedInitializationCallback = NullCommand.INSTANCE; But *that* said, see the comment on line 182. Maybe we dont need these commands at all. http://gwt-code-reviews.appspot.com/1446811/diff/1/user/src/com/google/gwt/user/client/ui/RenderablePanel.java#newcode70 user/src/com/google/gwt/user/client/ui/RenderablePanel.java:70: protected SafeHtml html = null; I dislike protected fields. Can you methods around these? Should also ensure that the code here uses the methods rather than poking the fields directly, in case subclasses get tricky. http://gwt-code-reviews.appspot.com/1446811/diff/1/user/src/com/google/gwt/user/client/ui/RenderablePanel.java#newcode72 user/src/com/google/gwt/user/client/ui/RenderablePanel.java:72: private String styleName = null; TODO need a more general mechanism for caching attributes while unwrapped http://gwt-code-reviews.appspot.com/1446811/diff/1/user/src/com/google/gwt/user/client/ui/RenderablePanel.java#newcode182 user/src/com/google/gwt/user/client/ui/RenderablePanel.java:182: public void performDetachedInitialization() { If we go ahead and add the helper object to the wrap and render calls, can it be in charge of accumulating the callbacks as well, and get these command objects out of here? http://gwt-code-reviews.appspot.com/1446811/diff/1/user/src/com/google/gwt/user/client/ui/RenderablePanel.java#newcode230 user/src/com/google/gwt/user/client/ui/RenderablePanel.java:230: if (currentPhase != Phase.RENDERED) { You're forcing the same instance to be responsible for both rendering and wrapping, which I expect we'll regret. I still don't see a reason to forbid redundant calls here. The second call just drops the current element and replaces it with the new one, no? http://gwt-code-reviews.appspot.com/1446811/diff/1/user/src/com/google/gwt/user/client/ui/RenderablePanel.java#newcode258 user/src/com/google/gwt/user/client/ui/RenderablePanel.java:258: protected SafeHtml getInnerHtml() { This method should be something like getPrerenderedHtml http://gwt-code-reviews.appspot.com/1446811/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
