[gwt-contrib] Re: SafeHtmlRenderer code gen for UiBinder. Picking up issue 1426803 (issue1442804)
Submitted as of r10210 On 2011/05/23 20:00:39, rjrjr wrote: LGTM Woot! http://gwt-code-reviews.appspot.com/1442804/diff/10001/user/test/com/google/gwt/uibinder/elementparsers/DialogBoxParserTest.java File user/test/com/google/gwt/uibinder/elementparsers/DialogBoxParserTest.java (right): http://gwt-code-reviews.appspot.com/1442804/diff/10001/user/test/com/google/gwt/uibinder/elementparsers/DialogBoxParserTest.java#newcode80 user/test/com/google/gwt/uibinder/elementparsers/DialogBoxParserTest.java:80: fieldName.setHTML(\@mockToken-fieldName-Hello, I bcaption/byou.\);, Absolutely, and sorry for the mess. On 2011/05/23 19:52:21, rchandia wrote: On 2011/05/23 18:25:23, rjrjr wrote: is the fieldName string coming from a constant in ElementParserTester or something? If so, can you open up its visibility and use it in these tests, rather having it inlined all over the place? Done for the lines changed in this Issue. The string comes from ElementParserTester#FIELD_NAME. It is hard-coded all over the place, though (at least 45/278 tests in UiBinderJreSuite). May I finish surfacing it in a separate Issue? http://gwt-code-reviews.appspot.com/1442804/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: SafeHtmlRenderer code gen for UiBinder. Picking up issue 1426803 (issue1442804)
Rolled back at r10213 On 2011/05/24 15:31:52, rchandia wrote: Submitted as of r10210 On 2011/05/23 20:00:39, rjrjr wrote: LGTM Woot! http://gwt-code-reviews.appspot.com/1442804/diff/10001/user/test/com/google/gwt/uibinder/elementparsers/DialogBoxParserTest.java File user/test/com/google/gwt/uibinder/elementparsers/DialogBoxParserTest.java (right): http://gwt-code-reviews.appspot.com/1442804/diff/10001/user/test/com/google/gwt/uibinder/elementparsers/DialogBoxParserTest.java#newcode80 user/test/com/google/gwt/uibinder/elementparsers/DialogBoxParserTest.java:80: fieldName.setHTML(\@mockToken-fieldName-Hello, I bcaption/byou.\);, Absolutely, and sorry for the mess. On 2011/05/23 19:52:21, rchandia wrote: On 2011/05/23 18:25:23, rjrjr wrote: is the fieldName string coming from a constant in ElementParserTester or something? If so, can you open up its visibility and use it in these tests, rather having it inlined all over the place? Done for the lines changed in this Issue. The string comes from ElementParserTester#FIELD_NAME. It is hard-coded all over the place, though (at least 45/278 tests in UiBinderJreSuite). May I finish surfacing it in a separate Issue? http://gwt-code-reviews.appspot.com/1442804/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: SafeHtmlRenderer code gen for UiBinder. Picking up issue 1426803 (issue1442804)
PTAL
http://gwt-code-reviews.appspot.com/1442804/diff/7001/user/src/com/google/gwt/uibinder/rebind/AbstractFieldWriter.java
File user/src/com/google/gwt/uibinder/rebind/AbstractFieldWriter.java
(right):
http://gwt-code-reviews.appspot.com/1442804/diff/7001/user/src/com/google/gwt/uibinder/rebind/AbstractFieldWriter.java#newcode119
user/src/com/google/gwt/uibinder/rebind/AbstractFieldWriter.java:119:
public void resetWritten() {
On 2011/05/16 19:37:47, rjrjr wrote:
No.
Done.
http://gwt-code-reviews.appspot.com/1442804/diff/7001/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/1442804/diff/7001/user/src/com/google/gwt/uibinder/rebind/UiBinderWriter.java#newcode1360
user/src/com/google/gwt/uibinder/rebind/UiBinderWriter.java:1360:
fieldManager.rewriteGwtFieldsDeclaration(niceWriter,
uiOwnerType.getName());
On 2011/05/16 19:37:47, rjrjr wrote:
I called this out in the first review. What's up with this rewriting
stuff, and
resetting the written status in the field writers? It seems very
wrong.
Done.
http://gwt-code-reviews.appspot.com/1442804/diff/7001/user/src/com/google/gwt/uibinder/rebind/UiBinderWriter.java#newcode1469
user/src/com/google/gwt/uibinder/rebind/UiBinderWriter.java:1469: //
TODO(sbrubaker): Fix method signature
On 2011/05/16 19:37:47, rjrjr wrote:
Stephanie won't be doing these. Shouldn't these issues be fixed now?
Done.
http://gwt-code-reviews.appspot.com/1442804/diff/7001/user/src/com/google/gwt/uibinder/rebind/UiBinderWriter.java#newcode1479
user/src/com/google/gwt/uibinder/rebind/UiBinderWriter.java:1479: //
TODO(sbrubaker): Find a better way to remove asString
On 2011/05/16 19:37:47, rjrjr wrote:
ditto
Done.
http://gwt-code-reviews.appspot.com/1442804/diff/7001/user/test/com/google/gwt/uibinder/elementparsers/DialogBoxParserTest.java
File
user/test/com/google/gwt/uibinder/elementparsers/DialogBoxParserTest.java
(right):
http://gwt-code-reviews.appspot.com/1442804/diff/7001/user/test/com/google/gwt/uibinder/elementparsers/DialogBoxParserTest.java#newcode80
user/test/com/google/gwt/uibinder/elementparsers/DialogBoxParserTest.java:80:
fieldName.setHTML(template.html1().asString());,
On 2011/05/16 19:37:47, rjrjr wrote:
This is a very fundamental change of the test. You're no longer
verifying the
output of the parser.
Done. Tests fixed by modifying MockUiBinderWriter instead.
http://gwt-code-reviews.appspot.com/1442804/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: SafeHtmlRenderer code gen for UiBinder. Picking up issue 1426803 (issue1442804)
http://gwt-code-reviews.appspot.com/1442804/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: SafeHtmlRenderer code gen for UiBinder. Picking up issue 1426803 (issue1442804)
Nearly there, just a couple of nits. Thanks!
After this patch lands we're going to need to think harder about the
testing. The coverage introduced here is pretty light. E.g., we're not
reproducing any of the escaping checks (like the various
test*FunnyChars tests in UiBinderTest. I don't have an immediate idea,
but we should certainly avoid mass copy and paste of the tests and the
appropriate template fragments.
Could you post a sample of the generated code?
http://gwt-code-reviews.appspot.com/1442804/diff/10001/user/src/com/google/gwt/uibinder/rebind/AbstractFieldWriter.java
File user/src/com/google/gwt/uibinder/rebind/AbstractFieldWriter.java
(right):
http://gwt-code-reviews.appspot.com/1442804/diff/10001/user/src/com/google/gwt/uibinder/rebind/AbstractFieldWriter.java#newcode123
user/src/com/google/gwt/uibinder/rebind/AbstractFieldWriter.java:123:
public void resetWritten() {
uncalled, right?
http://gwt-code-reviews.appspot.com/1442804/diff/10001/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/1442804/diff/10001/user/src/com/google/gwt/uibinder/rebind/UiBinderWriter.java#newcode235
user/src/com/google/gwt/uibinder/rebind/UiBinderWriter.java:235: private
boolean isRenderer;
Should be final
http://gwt-code-reviews.appspot.com/1442804/diff/10001/user/src/com/google/gwt/uibinder/rebind/UiBinderWriter.java#newcode861
user/src/com/google/gwt/uibinder/rebind/UiBinderWriter.java:861:
die(baseClass.getName() + must implement UiBinder or
SafeHtmlRenderer);
Are we making a comparable check for implements SafeHtmlRenderer?
http://gwt-code-reviews.appspot.com/1442804/diff/10001/user/src/com/google/gwt/uibinder/rebind/UiBinderWriter.java#newcode1464
user/src/com/google/gwt/uibinder/rebind/UiBinderWriter.java:1464: //
TODO(sbrubaker): Fix method signature
Is this TODO still valid? I've lost track of what it refers to. If it
is, can you make it a bit more informative?
http://gwt-code-reviews.appspot.com/1442804/diff/10001/user/test/com/google/gwt/uibinder/elementparsers/DialogBoxParserTest.java
File
user/test/com/google/gwt/uibinder/elementparsers/DialogBoxParserTest.java
(right):
http://gwt-code-reviews.appspot.com/1442804/diff/10001/user/test/com/google/gwt/uibinder/elementparsers/DialogBoxParserTest.java#newcode80
user/test/com/google/gwt/uibinder/elementparsers/DialogBoxParserTest.java:80:
fieldName.setHTML(\@mockToken-fieldName-Hello, I
bcaption/byou.\);,
is the fieldName string coming from a constant in ElementParserTester
or something? If so, can you open up its visibility and use it in these
tests, rather having it inlined all over the place?
http://gwt-code-reviews.appspot.com/1442804/diff/10001/user/test/com/google/gwt/uibinder/test/client/UiBinderTest.java
File user/test/com/google/gwt/uibinder/test/client/UiBinderTest.java
(right):
http://gwt-code-reviews.appspot.com/1442804/diff/10001/user/test/com/google/gwt/uibinder/test/client/UiBinderTest.java#newcode73
user/test/com/google/gwt/uibinder/test/client/UiBinderTest.java:73:
public void testSafeHtmlRendererText() {
Doesn't need to block this patch, but as a follow up could you break
this out into its own test class, perhaps SafeHtmlRendererTest?
SafeHtmlAsComponentsTest should serve as an example.
http://gwt-code-reviews.appspot.com/1442804/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: SafeHtmlRenderer code gen for UiBinder. Picking up issue 1426803 (issue1442804)
http://gwt-code-reviews.appspot.com/1442804/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: SafeHtmlRenderer code gen for UiBinder. Picking up issue 1426803 (issue1442804)
http://gwt-code-reviews.appspot.com/1442804/diff/10001/user/src/com/google/gwt/uibinder/rebind/AbstractFieldWriter.java
File user/src/com/google/gwt/uibinder/rebind/AbstractFieldWriter.java
(right):
http://gwt-code-reviews.appspot.com/1442804/diff/10001/user/src/com/google/gwt/uibinder/rebind/AbstractFieldWriter.java#newcode123
user/src/com/google/gwt/uibinder/rebind/AbstractFieldWriter.java:123:
public void resetWritten() {
On 2011/05/23 18:25:23, rjrjr wrote:
uncalled, right?
Done.
http://gwt-code-reviews.appspot.com/1442804/diff/10001/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/1442804/diff/10001/user/src/com/google/gwt/uibinder/rebind/UiBinderWriter.java#newcode235
user/src/com/google/gwt/uibinder/rebind/UiBinderWriter.java:235: private
boolean isRenderer;
On 2011/05/23 18:25:23, rjrjr wrote:
Should be final
Done.
http://gwt-code-reviews.appspot.com/1442804/diff/10001/user/src/com/google/gwt/uibinder/rebind/UiBinderWriter.java#newcode861
user/src/com/google/gwt/uibinder/rebind/UiBinderWriter.java:861:
die(baseClass.getName() + must implement UiBinder or
SafeHtmlRenderer);
On 2011/05/23 18:25:23, rjrjr wrote:
Are we making a comparable check for implements SafeHtmlRenderer?
Actually, this is the wrong place for this check. It should happen in
the constructor, where isRenderer is determined.
http://gwt-code-reviews.appspot.com/1442804/diff/10001/user/src/com/google/gwt/uibinder/rebind/UiBinderWriter.java#newcode1464
user/src/com/google/gwt/uibinder/rebind/UiBinderWriter.java:1464: //
TODO(sbrubaker): Fix method signature
On 2011/05/23 18:25:23, rjrjr wrote:
Is this TODO still valid? I've lost track of what it refers to. If it
is, can
you make it a bit more informative?
Done. Confirmed with sbrubaker that the TODO was stale.
http://gwt-code-reviews.appspot.com/1442804/diff/10001/user/test/com/google/gwt/uibinder/elementparsers/DialogBoxParserTest.java
File
user/test/com/google/gwt/uibinder/elementparsers/DialogBoxParserTest.java
(right):
http://gwt-code-reviews.appspot.com/1442804/diff/10001/user/test/com/google/gwt/uibinder/elementparsers/DialogBoxParserTest.java#newcode80
user/test/com/google/gwt/uibinder/elementparsers/DialogBoxParserTest.java:80:
fieldName.setHTML(\@mockToken-fieldName-Hello, I
bcaption/byou.\);,
On 2011/05/23 18:25:23, rjrjr wrote:
is the fieldName string coming from a constant in
ElementParserTester or
something? If so, can you open up its visibility and use it in these
tests,
rather having it inlined all over the place?
Done for the lines changed in this Issue.
The string comes from ElementParserTester#FIELD_NAME. It is hard-coded
all over the place, though (at least 45/278 tests in UiBinderJreSuite).
May I finish surfacing it in a separate Issue?
http://gwt-code-reviews.appspot.com/1442804/diff/10001/user/test/com/google/gwt/uibinder/test/client/UiBinderTest.java
File user/test/com/google/gwt/uibinder/test/client/UiBinderTest.java
(right):
http://gwt-code-reviews.appspot.com/1442804/diff/10001/user/test/com/google/gwt/uibinder/test/client/UiBinderTest.java#newcode73
user/test/com/google/gwt/uibinder/test/client/UiBinderTest.java:73:
public void testSafeHtmlRendererText() {
On 2011/05/23 18:25:23, rjrjr wrote:
Doesn't need to block this patch, but as a follow up could you break
this out
into its own test class, perhaps SafeHtmlRendererTest?
SafeHtmlAsComponentsTest
should serve as an example.
SGTM.
http://gwt-code-reviews.appspot.com/1442804/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: SafeHtmlRenderer code gen for UiBinder. Picking up issue 1426803 (issue1442804)
LGTM Woot! http://gwt-code-reviews.appspot.com/1442804/diff/10001/user/test/com/google/gwt/uibinder/elementparsers/DialogBoxParserTest.java File user/test/com/google/gwt/uibinder/elementparsers/DialogBoxParserTest.java (right): http://gwt-code-reviews.appspot.com/1442804/diff/10001/user/test/com/google/gwt/uibinder/elementparsers/DialogBoxParserTest.java#newcode80 user/test/com/google/gwt/uibinder/elementparsers/DialogBoxParserTest.java:80: fieldName.setHTML(\@mockToken-fieldName-Hello, I bcaption/byou.\);, Absolutely, and sorry for the mess. On 2011/05/23 19:52:21, rchandia wrote: On 2011/05/23 18:25:23, rjrjr wrote: is the fieldName string coming from a constant in ElementParserTester or something? If so, can you open up its visibility and use it in these tests, rather having it inlined all over the place? Done for the lines changed in this Issue. The string comes from ElementParserTester#FIELD_NAME. It is hard-coded all over the place, though (at least 45/278 tests in UiBinderJreSuite). May I finish surfacing it in a separate Issue? http://gwt-code-reviews.appspot.com/1442804/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: SafeHtmlRenderer code gen for UiBinder. Picking up issue 1426803 (issue1442804)
http://gwt-code-reviews.appspot.com/1442804/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: SafeHtmlRenderer code gen for UiBinder. Picking up issue 1426803 (issue1442804)
http://gwt-code-reviews.appspot.com/1442804/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: SafeHtmlRenderer code gen for UiBinder. Picking up issue 1426803 (issue1442804)
PTAL. Smoke tests pass. On 2011/05/13 19:16:42, rchandia wrote: http://gwt-code-reviews.appspot.com/1442804/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: SafeHtmlRenderer code gen for UiBinder. Picking up issue 1426803 (issue1442804)
http://gwt-code-reviews.appspot.com/1442804/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
