LGTM with nits.

http://gwt-code-reviews.appspot.com/96804/diff/1/2
File
user/src/com/google/gwt/uibinder/rebind/GwtResourceEntityResolver.java
(right):

http://gwt-code-reviews.appspot.com/96804/diff/1/2#newcode49
Line 49: "https://dl-ssl.google.com/download/gwt/DTD/"}));
Should it include the version number?

Do you want to allow people to use http as well as https?  If so, both
should probably be included.

http://gwt-code-reviews.appspot.com/96804/diff/1/4
File user/src/com/google/gwt/uibinder/rebind/W3cDomHelper.java (right):

http://gwt-code-reviews.appspot.com/96804/diff/1/4#newcode44
Line 44: public W3cDomHelper(final TreeLogger logger) {
Javadoc, mentioning that logger can be null since it is called that way?

Speaking of which, it looks like you would get NPE below.  Either this
should handle a null logger, or the callers should supply a dummy
logger.

http://gwt-code-reviews.appspot.com/96804/diff/1/4#newcode54
Line 54: builder.setErrorHandler(new ErrorHandler() {
Is this error handler needed from the new DTD, or an unrelated
improvement?

http://gwt-code-reviews.appspot.com/96804/diff/1/9
File user/test/com/google/gwt/uibinder/test/client/WidgetBasedUi.ui.xml
(right):

http://gwt-code-reviews.appspot.com/96804/diff/1/9#newcode31
Line 31: allows you to use familiar HTML entities like %nbsp; and
•,
Should this be   ?

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

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

Reply via email to