http://gwt-code-reviews.appspot.com/1305801/diff/48001/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/1305801/diff/48001/user/src/com/google/gwt/uibinder/rebind/UiBinderWriter.java#newcode686 user/src/com/google/gwt/uibinder/rebind/UiBinderWriter.java:686: public String tokenForSafeHtmlExpression(String expression) { String token = tokenForStringExpression("SafeHtmlUtils.fromSafeConstant(" + expression + ")"); htmlTemplates.noteSafeConstant(token); return token; Are these ever used in a context where the \" + stuff is needed? If not, no need to call tokenForStringExpression, and no need to remove it later in HtmlTemplate#populateArgMap http://gwt-code-reviews.appspot.com/1305801/diff/48001/user/src/com/google/gwt/uibinder/rebind/UiBinderWriter.java#newcode701 user/src/com/google/gwt/uibinder/rebind/UiBinderWriter.java:701: public String tokenForStringExpression(String expression) { Is it ever the case that the same token will be reach both HtmlTemplate#populateArgMap, and a spot where \" + stuff is needed? If not, should rename this tokenForInlineStringExpression(), and add a new method without the quotes and a name like tokenForStringInHtmlTemplate. And rename tokenForSafeHtmlExpression to tokenForTrustedHtmlInHtmlTemplate. This would allow you to get rid of the other substring(4 calls in HtmlTemplate#populateArgMap http://gwt-code-reviews.appspot.com/1305801/diff/48001/user/src/com/google/gwt/uibinder/rebind/model/HtmlTemplate.java File user/src/com/google/gwt/uibinder/rebind/model/HtmlTemplate.java (right): http://gwt-code-reviews.appspot.com/1305801/diff/48001/user/src/com/google/gwt/uibinder/rebind/model/HtmlTemplate.java#newcode33 user/src/com/google/gwt/uibinder/rebind/model/HtmlTemplate.java:33: private final String methodName; private final HtmlTemplates templates; http://gwt-code-reviews.appspot.com/1305801/diff/48001/user/src/com/google/gwt/uibinder/rebind/model/HtmlTemplate.java#newcode37 user/src/com/google/gwt/uibinder/rebind/model/HtmlTemplate.java:37: public HtmlTemplate(String html, Tokenator t, HtmlTemplates h, Please don't use one character param names. http://gwt-code-reviews.appspot.com/1305801/diff/48001/user/src/com/google/gwt/uibinder/rebind/model/HtmlTemplate.java#newcode40 user/src/com/google/gwt/uibinder/rebind/model/HtmlTemplate.java:40: logger.die("Template html cannot be null"); assert, no logging http://gwt-code-reviews.appspot.com/1305801/diff/48001/user/src/com/google/gwt/uibinder/rebind/model/HtmlTemplate.java#newcode48 user/src/com/google/gwt/uibinder/rebind/model/HtmlTemplate.java:48: templates = h; http://gwt-code-reviews.appspot.com/1305801/diff/48001/user/src/com/google/gwt/uibinder/rebind/model/HtmlTemplate.java#newcode138 user/src/com/google/gwt/uibinder/rebind/model/HtmlTemplate.java:138: if (arg.startsWith("\" + SafeHtmlUtils.fromSafeConstant(")) { if (templates.isSafeConstant(arg)) { http://gwt-code-reviews.appspot.com/1305801/diff/48001/user/src/com/google/gwt/uibinder/rebind/model/HtmlTemplateArgument.java File user/src/com/google/gwt/uibinder/rebind/model/HtmlTemplateArgument.java (right): http://gwt-code-reviews.appspot.com/1305801/diff/48001/user/src/com/google/gwt/uibinder/rebind/model/HtmlTemplateArgument.java#newcode24 user/src/com/google/gwt/uibinder/rebind/model/HtmlTemplateArgument.java:24: private final String type; Please make the constructor private and use two factory methods: static HtmlTemplateArgument forHtml(String arg) { return new HtmlTemplateArgument(arg, "SafeHtml"); } static HtmlTemplateArgument forString(String arg) { return new HtmlTemplateArgument(arg, "String"); } http://gwt-code-reviews.appspot.com/1305801/diff/48001/user/src/com/google/gwt/uibinder/rebind/model/HtmlTemplateArgument.java#newcode26 user/src/com/google/gwt/uibinder/rebind/model/HtmlTemplateArgument.java:26: public HtmlTemplateArgument(String arg, String type) { please reverse these params, to be more consistent w/Java declarations http://gwt-code-reviews.appspot.com/1305801/diff/48001/user/src/com/google/gwt/uibinder/rebind/model/HtmlTemplates.java File user/src/com/google/gwt/uibinder/rebind/model/HtmlTemplates.java (right): http://gwt-code-reviews.appspot.com/1305801/diff/48001/user/src/com/google/gwt/uibinder/rebind/model/HtmlTemplates.java#newcode33 user/src/com/google/gwt/uibinder/rebind/model/HtmlTemplates.java:33: private final List<HtmlTemplate> htmlTemplates = new ArrayList<HtmlTemplate>(); private final Set<String> safeConstantTokens = new HashSet<String>(); http://gwt-code-reviews.appspot.com/1305801/diff/48001/user/src/com/google/gwt/uibinder/rebind/model/HtmlTemplates.java#newcode55 user/src/com/google/gwt/uibinder/rebind/model/HtmlTemplates.java:55: logger.die("Template tokenator cannot be null"); Logger is for user errors. Just throw RuntimeException here, or use asserts, this would be a straight up coding bug. http://gwt-code-reviews.appspot.com/1305801/diff/48001/user/src/com/google/gwt/uibinder/rebind/model/HtmlTemplates.java#newcode80 user/src/com/google/gwt/uibinder/rebind/model/HtmlTemplates.java:80: } public void noteSafeConstant(String token) { safeConstantTokens.add(token); } public boolean isSafeConstant(String token) { return safeConstantTokens.contains(token); } http://gwt-code-reviews.appspot.com/1305801/diff/48001/user/test/com/google/gwt/uibinder/elementparsers/MockUiBinderWriter.java File user/test/com/google/gwt/uibinder/elementparsers/MockUiBinderWriter.java (right): http://gwt-code-reviews.appspot.com/1305801/diff/48001/user/test/com/google/gwt/uibinder/elementparsers/MockUiBinderWriter.java#newcode51 user/test/com/google/gwt/uibinder/elementparsers/MockUiBinderWriter.java:51: * Mocked out to accumulate the declared templates. Returns doesn't accumulate http://gwt-code-reviews.appspot.com/1305801/diff/48001/user/test/com/google/gwt/uibinder/rebind/model/HtmlTemplateTest.java File user/test/com/google/gwt/uibinder/rebind/model/HtmlTemplateTest.java (right): http://gwt-code-reviews.appspot.com/1305801/diff/48001/user/test/com/google/gwt/uibinder/rebind/model/HtmlTemplateTest.java#newcode28 user/test/com/google/gwt/uibinder/rebind/model/HtmlTemplateTest.java:28: * HtmlTemplate unit tests. Does this provide any coverage that HtmlTemplatesTest doesn't? http://gwt-code-reviews.appspot.com/1305801/diff/48001/user/test/com/google/gwt/uibinder/rebind/model/HtmlTemplateTest.java#newcode66 user/test/com/google/gwt/uibinder/rebind/model/HtmlTemplateTest.java:66: logger.died.contains("Template html cannot be null")); Can a user action lead to this? If not, it should not be logged, and probably not worth testing. http://gwt-code-reviews.appspot.com/1305801/diff/48001/user/test/com/google/gwt/uibinder/rebind/model/HtmlTemplateTest.java#newcode77 user/test/com/google/gwt/uibinder/rebind/model/HtmlTemplateTest.java:77: } catch (UnableToCompleteException e) { Should be a failed assert, not logged for the user. Not worth testing. http://gwt-code-reviews.appspot.com/1305801/diff/48001/user/test/com/google/gwt/uibinder/rebind/model/HtmlTemplateTest.java#newcode83 user/test/com/google/gwt/uibinder/rebind/model/HtmlTemplateTest.java:83: public void testNullHtmlTemplates() { ditto, shouldn't log, not a useful test. http://gwt-code-reviews.appspot.com/1305801/diff/48001/user/test/com/google/gwt/uibinder/rebind/model/HtmlTemplatesTest.java File user/test/com/google/gwt/uibinder/rebind/model/HtmlTemplatesTest.java (right): http://gwt-code-reviews.appspot.com/1305801/diff/48001/user/test/com/google/gwt/uibinder/rebind/model/HtmlTemplatesTest.java#newcode36 user/test/com/google/gwt/uibinder/rebind/model/HtmlTemplatesTest.java:36: public class HtmlTemplatesTest extends TestCase { nice http://gwt-code-reviews.appspot.com/1305801/diff/48001/user/test/com/google/gwt/uibinder/rebind/model/HtmlTemplatesTest.java#newcode99 user/test/com/google/gwt/uibinder/rebind/model/HtmlTemplatesTest.java:99: } catch (UnableToCompleteException e) { as in HtmlTemplateTest http://gwt-code-reviews.appspot.com/1305801/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
