LGTM On Tue, May 3, 2011 at 9:54 AM, Ray Ryan <rj...@google.com> wrote:
> > > On Tue, May 3, 2011 at 9:49 AM, <her...@google.com> wrote: > >> >> >> http://gwt-code-reviews.appspot.com/1427809/diff/1/user/src/com/google/gwt/uibinder/client/LazyDomElement.java >> File user/src/com/google/gwt/uibinder/client/LazyDomElement.java >> (right): >> >> >> http://gwt-code-reviews.appspot.com/1427809/diff/1/user/src/com/google/gwt/uibinder/client/LazyDomElement.java#newcode33 >> user/src/com/google/gwt/uibinder/client/LazyDomElement.java:33: * >> <div ui:field="myDiv" class="{anyClass}"/> >> On 2011/05/02 16:51:54, rjrjr wrote: >> >>> The class bit is noise, can you snip it? >>> >> >> Done. >> >> >> http://gwt-code-reviews.appspot.com/1427809/diff/1/user/src/com/google/gwt/uibinder/client/LazyDomElement.java#newcode45 >> user/src/com/google/gwt/uibinder/client/LazyDomElement.java:45: */ >> >> On 2011/05/02 16:51:54, rjrjr wrote: >> >>> /** >>> * Creates an instance to fetch the element with the given id. >>> */ >>> >> >> Done. >> >> >> http://gwt-code-reviews.appspot.com/1427809/diff/1/user/src/com/google/gwt/uibinder/client/LazyDomElement.java#newcode54 >> user/src/com/google/gwt/uibinder/client/LazyDomElement.java:54: >> >> On 2011/05/02 16:51:54, rjrjr wrote: >> >>> /** >>> * Returns the dom element >>> * @return the dom element >>> * @throws RuntimeException is the element cannot be found >>> */ >>> >> >> Done. >> >> >> >> http://gwt-code-reviews.appspot.com/1427809/diff/1/user/src/com/google/gwt/uibinder/client/LazyDomElement.java#newcode59 >> user/src/com/google/gwt/uibinder/client/LazyDomElement.java:59: throw >> new RuntimeException("Element is not attached."); >> On 2011/05/02 16:51:54, rjrjr wrote: >> >>> "Cannot find element with id \"" + domId + "\". Perhaps it is not >>> >> attached to >> >>> the document body?" >>> >> >> Done. >> >> >> http://gwt-code-reviews.appspot.com/1427809/diff/1/user/src/com/google/gwt/uibinder/rebind/FieldWriterOfLazyDomElement.java >> File >> user/src/com/google/gwt/uibinder/rebind/FieldWriterOfLazyDomElement.java >> (right): >> >> >> http://gwt-code-reviews.appspot.com/1427809/diff/1/user/src/com/google/gwt/uibinder/rebind/FieldWriterOfLazyDomElement.java#newcode46 >> >> user/src/com/google/gwt/uibinder/rebind/FieldWriterOfLazyDomElement.java:46: >> if (!templateFieldType.isAssignableTo(parameterType)) { >> On 2011/05/02 16:51:54, rjrjr wrote: >> >>> Needs a unit test. See FieldWriterOfGeneratedCssResourceTest for >>> >> example. >> >> Done. >> >> >> http://gwt-code-reviews.appspot.com/1427809/diff/1/user/src/com/google/gwt/uibinder/rebind/UiBinderWriter.java >> File user/src/com/google/gwt/uibinder/rebind/UiBinderWriter.java >> (right): >> >> >> http://gwt-code-reviews.appspot.com/1427809/diff/1/user/src/com/google/gwt/uibinder/rebind/UiBinderWriter.java#newcode380 >> user/src/com/google/gwt/uibinder/rebind/UiBinderWriter.java:380: // But >> if the owner field is an instance of LazyDomElement then the code >> On 2011/05/02 16:51:54, rjrjr wrote: >> >>> /* for long comments >>> >> >> Done. >> >> >> >> http://gwt-code-reviews.appspot.com/1427809/diff/1/user/src/com/google/gwt/uibinder/rebind/UiBinderWriter.java#newcode463 >> user/src/com/google/gwt/uibinder/rebind/UiBinderWriter.java:463: // We >> can switch types if useLazyWidgetBuilders is enabled and the >> On 2011/05/02 16:51:54, rjrjr wrote: >> >>> /* >>> >> >> Done. >> >> >> >> http://gwt-code-reviews.appspot.com/1427809/diff/1/user/src/com/google/gwt/uibinder/rebind/UiBinderWriter.java#newcode710 >> user/src/com/google/gwt/uibinder/rebind/UiBinderWriter.java:710: return >> lazyDomElementClass.isAssignableFrom(ownerField.getType().getRawType()); >> On 2011/05/02 16:51:54, rjrjr wrote: >> >>> Can getRawType can return null? Is isAssignableFrom null safe? >>> >> >> Nop, OwnerField dies if it can't resolve the type. >> >> >> >> http://gwt-code-reviews.appspot.com/1427809/diff/1010/user/src/com/google/gwt/uibinder/rebind/model/OwnerField.java >> File user/src/com/google/gwt/uibinder/rebind/model/OwnerField.java >> (right): >> >> >> http://gwt-code-reviews.appspot.com/1427809/diff/1010/user/src/com/google/gwt/uibinder/rebind/model/OwnerField.java#newcode85 >> user/src/com/google/gwt/uibinder/rebind/model/OwnerField.java:85: public >> JClassType getRawType() { >> On 2011/05/03 16:37:02, rjrjr wrote: >> >>> Please inline this. >>> >> >> You meant remove this method? This is very handy for tests. > > > Oh, I see. Could you put a comment in the method body to that effect? > >> >> >> >> http://gwt-code-reviews.appspot.com/1427809/diff/1010/user/test/com/google/gwt/uibinder/rebind/FieldWriterOfExistingTypeTest.java >> File >> >> user/test/com/google/gwt/uibinder/rebind/FieldWriterOfExistingTypeTest.java >> (right): >> >> >> http://gwt-code-reviews.appspot.com/1427809/diff/1010/user/test/com/google/gwt/uibinder/rebind/FieldWriterOfExistingTypeTest.java#newcode22 >> >> user/test/com/google/gwt/uibinder/rebind/FieldWriterOfExistingTypeTest.java:22: >> public class FieldWriterOfExistingTypeTest extends TestCase { >> On 2011/05/03 16:37:02, rjrjr wrote: >> >>> I forgot how handy EasyMock is >>> >> >> True. >> >> >> http://gwt-code-reviews.appspot.com/1427809/ >> > > -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
