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) {
On 2011/03/02 23:27:35, rjrjr wrote:
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
Done.
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) {
On 2011/03/02 23:27:35, rjrjr wrote:
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
While I understand what you're getting at, how do I distinguish when to
call tokenForInlineStringExpression vs. tokenForStringInHtmlTemplate?
The issue is that declareDomField would need to call either of these
depending on the context, but all that is passed to declareDomField is
the field name. There is not enough information to determine which of
the two to call.
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;
On 2011/03/02 23:27:35, rjrjr wrote:
private final HtmlTemplates templates;
Done.
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,
On 2011/03/02 23:27:35, rjrjr wrote:
Please don't use one character param names.
Done.
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");
On 2011/03/02 23:27:35, rjrjr wrote:
assert, no logging
I disagree that an assertion is appropriate here since assertions can be
disabled. I modified the code to throw an IllegalArgumentException
instead.
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:
On 2011/03/02 23:27:35, rjrjr wrote:
templates = h;
Done.
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(")) {
On 2011/03/02 23:27:35, rjrjr wrote:
if (templates.isSafeConstant(arg)) {
Done.
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;
On 2011/03/02 23:27:35, rjrjr wrote:
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");
}
Done.
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) {
On 2011/03/02 23:27:35, rjrjr wrote:
please reverse these params, to be more consistent w/Java declarations
Done.
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>();
On 2011/03/02 23:27:35, rjrjr wrote:
private final Set<String> safeConstantTokens = new HashSet<String>();
Done.
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");
On 2011/03/02 23:27:35, rjrjr wrote:
Logger is for user errors. Just throw RuntimeException here, or use
asserts,
this would be a straight up coding bug.
Done.
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: }
On 2011/03/02 23:27:35, rjrjr wrote:
public void noteSafeConstant(String token) {
safeConstantTokens.add(token);
}
public boolean isSafeConstant(String token) {
return safeConstantTokens.contains(token);
}
Done.
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
On 2011/03/02 23:27:35, rjrjr wrote:
doesn't accumulate
Done.
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.
On 2011/03/02 23:27:35, rjrjr wrote:
Does this provide any coverage that HtmlTemplatesTest doesn't?
It covers the case of a null HtmlTemplates being passed to the
constructor; otherwise the tests overlap. I can remove the
HtmlTemplateTest class if you think it's unnecessary.
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"));
On 2011/03/02 23:27:35, rjrjr wrote:
Can a user action lead to this? If not, it should not be logged, and
probably
not worth testing.
Done.
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) {
On 2011/03/02 23:27:35, rjrjr wrote:
Should be a failed assert, not logged for the user. Not worth testing.
Done.
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() {
On 2011/03/02 23:27:35, rjrjr wrote:
ditto, shouldn't log, not a useful test.
Done.
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#newcode99
user/test/com/google/gwt/uibinder/rebind/model/HtmlTemplatesTest.java:99:
} catch (UnableToCompleteException e) {
On 2011/03/02 23:27:35, rjrjr wrote:
as in HtmlTemplateTest
Done.
http://gwt-code-reviews.appspot.com/1305801/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
