Re: [gwt-contrib] Re: GWT 2.2 ?
On Fri, Jan 14, 2011 at 5:56 PM, Thomas Broyer t.bro...@gmail.com wrote: (well, now I have 5 open issues with RequestFactory assigned to me by my team, let's see if they're RF issues of –much more likely– issues with either user code or my heavy use of ServiceLayerDecorator) Feedback is the breakfast of champions. -- Bob Vawter Google Web Toolkit Team -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Fixing HasWidgetsTester#testAll to actually call testDoDetachChildrenWithError. Currently, it c... (issue1300801)
Reviewers: pdr,
Description:
Fixing HasWidgetsTester#testAll to actually call
testDoDetachChildrenWithError. Currently, it calls
testDoAttachChildrenWithError twice. HTMLTableTestBase needs to be
modified to work with the test.
Please review this at http://gwt-code-reviews.appspot.com/1300801/show
Affected files:
M user/test/com/google/gwt/user/client/ui/HTMLTableTestBase.java
M user/test/com/google/gwt/user/client/ui/HasWidgetsTester.java
Index: user/test/com/google/gwt/user/client/ui/HTMLTableTestBase.java
===
--- user/test/com/google/gwt/user/client/ui/HTMLTableTestBase.java
(revision 9561)
+++ user/test/com/google/gwt/user/client/ui/HTMLTableTestBase.java (working
copy)
@@ -33,8 +33,10 @@
*/
public abstract class HTMLTableTestBase extends GWTTestCase {
static class Adder implements HasWidgetsTester.WidgetAdder {
+private int row = -1;
+
public void addChild(HasWidgets container, Widget child) {
- ((HTMLTable) container).setWidget(0, 0, child);
+ ((HTMLTable) container).setWidget(++row, 0, child);
}
}
@@ -67,7 +69,7 @@
public abstract HTMLTable getTable(int row, int column);
public void testAttachDetachOrder() {
-HasWidgetsTester.testAll(getTable(1, 1), new Adder(), true);
+HasWidgetsTester.testAll(getTable(25, 1), new Adder(), true);
}
public void testBoundsOnEmptyTable() {
Index: user/test/com/google/gwt/user/client/ui/HasWidgetsTester.java
===
--- user/test/com/google/gwt/user/client/ui/HasWidgetsTester.java (revision
9561)
+++ user/test/com/google/gwt/user/client/ui/HasWidgetsTester.java (working
copy)
@@ -112,7 +112,7 @@
testAttachDetachOrder(container, adder);
testRemovalOfNonExistantChild(container);
testDoAttachChildrenWithError(container, adder,
supportsMultipleWidgets);
-testDoAttachChildrenWithError(container, adder,
supportsMultipleWidgets);
+testDoDetachChildrenWithError(container, adder,
supportsMultipleWidgets);
}
/**
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Adding a new widget ResizeLayoutPanel that can trigger a resize event when it changes size. The ... (issue1301801)
Reviewers: pdr, Description: Adding a new widget ResizeLayoutPanel that can trigger a resize event when it changes size. The widget uses hidden scrollable divs to detect resize on non-IE browsers. This allow users to embed widgets that RequireResize in their app without having an unbroken chain of ProvidesResize up to the RootLayoutPanel. Also adding a new widget HeaderPanel that uses the same resize implementation to layout content between naturally sized headers and footers. This is similar to a DockLayoutPanel (north/south/center only), but it doesn't require users to specify a specific height for the header and footer. Please review this at http://gwt-code-reviews.appspot.com/1301801/show Affected files: A user/src/com/google/gwt/user/ResizeLayoutPanel.gwt.xml M user/src/com/google/gwt/user/User.gwt.xml A user/src/com/google/gwt/user/client/ui/HeaderPanel.java A user/src/com/google/gwt/user/client/ui/ResizeLayoutPanel.java M user/test/com/google/gwt/user/UISuite.java A user/test/com/google/gwt/user/client/ui/HeaderPanelTest.java A user/test/com/google/gwt/user/client/ui/ResizeLayoutPanelTest.java -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: First pass at Issue 1405 (Dialog Box header fix) (issue1149803)
@jeff - Can you upload a patch relative to trunk/ instead of trunk/user? I have some comments, but I'm getting a 500 error when I try to publish them. I think its because the patch isn't relative to trunk/. http://gwt-code-reviews.appspot.com/1149803/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: First pass at Issue 1405 (Dialog Box header fix) (issue1149803)
Great work. I found a few nitpicks that should be easy to fix.
http://gwt-code-reviews.appspot.com/1149803/diff/22001/23001
File user/src/com/google/gwt/user/client/ui/DialogBox.java (right):
http://gwt-code-reviews.appspot.com/1149803/diff/22001/23001#newcode113
user/src/com/google/gwt/user/client/ui/DialogBox.java:113: public
interface Caption extends HasAllMouseHandlers, HasHTML, IsWidget {
Replace HasHTML with HasSafeHtml. We're moving away from String html
methods.
http://gwt-code-reviews.appspot.com/1149803/diff/22001/23001#newcode184
user/src/com/google/gwt/user/client/ui/DialogBox.java:184: * auto-hide
is set to false and It should
and IT = and it
http://gwt-code-reviews.appspot.com/1149803/diff/22001/23001#newcode210
user/src/com/google/gwt/user/client/ui/DialogBox.java:210: * Creates an
empty dialog box specifying its auto-hide property and an
implementation of the {@link Caption}. It should
long line
http://gwt-code-reviews.appspot.com/1149803/diff/22001/23001#newcode214
user/src/com/google/gwt/user/client/ui/DialogBox.java:214: * @param
autoHide
copy param descriptions from other constructors.
http://gwt-code-reviews.appspot.com/1149803/diff/22001/23001#newcode221
user/src/com/google/gwt/user/client/ui/DialogBox.java:221: assert
captionWidget != null : The caption must not be null;
Since caption can now come from the outside, we need to call:
caption.asWidget().removeFromParent()
http://gwt-code-reviews.appspot.com/1149803/diff/22001/23001#newcode298
user/src/com/google/gwt/user/client/ui/DialogBox.java:298: // not fire
its click event for example
For multiline comments, use /* comments:
/*
* line 0
* line 1
*/
http://gwt-code-reviews.appspot.com/1149803/diff/22001/23001#newcode351
user/src/com/google/gwt/user/client/ui/DialogBox.java:351: * {@link
#setHTML(SafeHTML)} method..
extra .
http://gwt-code-reviews.appspot.com/1149803/diff/22001/23001#newcode359
user/src/com/google/gwt/user/client/ui/DialogBox.java:359:
setHTML(html.asString());
assuming HasSafeHtml interface:
caption.setHTML(html);
http://gwt-code-reviews.appspot.com/1149803/diff/22001/23001#newcode372
user/src/com/google/gwt/user/client/ui/DialogBox.java:372:
caption.setHTML(html);
caption.setHTML(SafeHtmlUtils.fromTrustedString(html))
http://gwt-code-reviews.appspot.com/1149803/diff/22001/23002
File user/test/com/google/gwt/user/client/ui/DialogBoxTest.java (right):
http://gwt-code-reviews.appspot.com/1149803/diff/22001/23002#newcode97
user/test/com/google/gwt/user/client/ui/DialogBoxTest.java:97:
assertEquals(dialogBox.getHTML(), btext/b);
Some browser capitalize HTML automatically. Use:
dialogBox.getHTML().toLowercase();
http://gwt-code-reviews.appspot.com/1149803/diff/22001/23002#newcode100
user/test/com/google/gwt/user/client/ui/DialogBoxTest.java:100:
assertTrue(caption.asWidget().getElement() == DOM.getChild(td, 0));
cleanup after the test:
dialogBox.hide();
http://gwt-code-reviews.appspot.com/1149803/diff/22001/23002#newcode103
user/test/com/google/gwt/user/client/ui/DialogBoxTest.java:103: private
class CaptionForTesting extends Composite implements
Move class definition to the top of the test class.
http://gwt-code-reviews.appspot.com/1149803/show
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: First pass at Issue 1405 (Dialog Box header fix) (issue1149803)
Also, can you sign a CLA so we can accept patch. If you scroll down to the bottom of the link below, you can sign it electronically. http://code.google.com/legal/individual-cla-v1.0.html http://gwt-code-reviews.appspot.com/1149803/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
Re: [gwt-contrib] Re: GWT 2.2 ?
On Tuesday, January 18, 2011 3:40:23 PM UTC+1, Bob Vawter wrote: On Fri, Jan 14, 2011 at 5:56 PM, Thomas Broyer t.br...@gmail.com wrote: (well, now I have 5 open issues with RequestFactory assigned to me by my team, let's see if they're RF issues of –much more likely– issues with either user code or my heavy use of ServiceLayerDecorator) Feedback is the breakfast of champions. Should I understand that you got my HasDataEditor patch in time for you breakfast yesterday? ;-) (I'm a bit lost with timezones) http://gwt-code-reviews.appspot.com/1297801/show And just so you know: all those issues were indeed mistakes in our user code and/or our ServiceLayerDecorator-s; you can sleep in peace. -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Improve canvas for browsers (and permutations) with partial canvas support. (issue1296801)
http://gwt-code-reviews.appspot.com/1296801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Improve canvas for browsers (and permutations) with partial canvas support. (issue1296801)
It looks like you no longer have a public way to check if a canvas is
supported without trying to create one? That seems bad.
http://gwt-code-reviews.appspot.com/1296801/diff/7001/8002
File user/src/com/google/gwt/canvas/client/Canvas.java (right):
http://gwt-code-reviews.appspot.com/1296801/diff/7001/8002#newcode42
user/src/com/google/gwt/canvas/client/Canvas.java:42: public static
Canvas createCanvasIfSupported() {
Naming nit: use of these factory methods might be easier to automate
down the road if they are consistent, so I'd suggest
Canvas#createIfSupported(). DRY and all that.
http://gwt-code-reviews.appspot.com/1296801/show
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: First pass at Issue 1405 (Dialog Box header fix) (issue1149803)
I take your point about UiBinder support. You can have your invariant and bind it too by updating DialogBoxParser (and DialogBoxParserTest) to optionally handle the new constructor argument. On Tue, Jan 18, 2011 at 8:02 AM, jlaba...@google.com wrote: Also, can you sign a CLA so we can accept patch. If you scroll down to the bottom of the link below, you can sign it electronically. http://code.google.com/legal/individual-cla-v1.0.html http://gwt-code-reviews.appspot.com/1149803/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: First pass at Issue 1405 (Dialog Box header fix) (issue1149803)
http://gwt-code-reviews.appspot.com/1149803/diff/22001/23001
File user/src/com/google/gwt/user/client/ui/DialogBox.java (right):
http://gwt-code-reviews.appspot.com/1149803/diff/22001/23001#newcode113
user/src/com/google/gwt/user/client/ui/DialogBox.java:113: public
interface Caption extends HasAllMouseHandlers, HasHTML, IsWidget {
On the other hand, UiBinder knows how to parse things that implement
HasHtml, but it doesn't have a clue yet about HasSafeHtml. I don't think
this is good advice until HasHtml is deprecated, which we haven't
managed to do quite yet.
On 2011/01/18 15:59:56, jlabanca wrote:
Replace HasHTML with HasSafeHtml. We're moving away from String html
methods.
http://gwt-code-reviews.appspot.com/1149803/diff/22001/23001#newcode372
user/src/com/google/gwt/user/client/ui/DialogBox.java:372:
caption.setHTML(html);
If this makes sense here, shouldn't we be doing it in every
setHTML(String) method? Doing it some places and not others seems pretty
confusing.
On 2011/01/18 15:59:56, jlabanca wrote:
caption.setHTML(SafeHtmlUtils.fromTrustedString(html))
http://gwt-code-reviews.appspot.com/1149803/show
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Mark some test as Failing. Note these test only fail on IE. (issue1303801)
Reviewers: rchandia,
Description:
Mark some test as Failing. Note these test only fail on IE.
See
http://code.google.com/p/google-web-toolkit/issues/detail?id=5882
Please review this at http://gwt-code-reviews.appspot.com/1303801/show
Affected files:
M
user/test/org/hibernate/jsr303/tck/tests/constraints/constraintcomposition/ConstraintCompositionGwtTest.java
M
user/test/org/hibernate/jsr303/tck/tests/constraints/customconstraint/CustomConstraintValidatorGwtTest.java
M
user/test/org/hibernate/jsr303/tck/tests/constraints/validatorresolution/ValidatorResolutionGwtTest.java
M
user/test/org/hibernate/jsr303/tck/tests/validation/ValidatePropertyGwtTest.java
Index:
user/test/org/hibernate/jsr303/tck/tests/constraints/constraintcomposition/ConstraintCompositionGwtTest.java
===
---
user/test/org/hibernate/jsr303/tck/tests/constraints/constraintcomposition/ConstraintCompositionGwtTest.java
(revision 9561)
+++
user/test/org/hibernate/jsr303/tck/tests/constraints/constraintcomposition/ConstraintCompositionGwtTest.java
(working copy)
@@ -40,10 +40,12 @@
delegate.testAttributesDefinedOnComposingConstraints();
}
+ @Failing(issue = 5882)
public void testComposedConstraints() {
delegate.testComposedConstraints();
}
+ @Failing(issue = 5882)
public void testComposedConstraintsAreRecursive() {
delegate.testComposedConstraintsAreRecursive();
}
Index:
user/test/org/hibernate/jsr303/tck/tests/constraints/customconstraint/CustomConstraintValidatorGwtTest.java
===
---
user/test/org/hibernate/jsr303/tck/tests/constraints/customconstraint/CustomConstraintValidatorGwtTest.java
(revision 9561)
+++
user/test/org/hibernate/jsr303/tck/tests/constraints/customconstraint/CustomConstraintValidatorGwtTest.java
(working copy)
@@ -30,6 +30,7 @@
return org.hibernate.jsr303.tck.tests.constraints.customconstraint.TckTest;
}
+ @Failing(issue = 5882)
public void testDefaultPropertyPath() {
delegate.testDefaultPropertyPath();
}
Index:
user/test/org/hibernate/jsr303/tck/tests/constraints/validatorresolution/ValidatorResolutionGwtTest.java
===
---
user/test/org/hibernate/jsr303/tck/tests/constraints/validatorresolution/ValidatorResolutionGwtTest.java
(revision 9561)
+++
user/test/org/hibernate/jsr303/tck/tests/constraints/validatorresolution/ValidatorResolutionGwtTest.java
(working copy)
@@ -34,6 +34,7 @@
delegate.testAmbiguousValidatorResolution();
}
+ @Failing(issue = 5882)
public void testResolutionOfMinMaxForDifferentTypes() {
delegate.testResolutionOfMinMaxForDifferentTypes();
}
Index:
user/test/org/hibernate/jsr303/tck/tests/validation/ValidatePropertyGwtTest.java
===
---
user/test/org/hibernate/jsr303/tck/tests/validation/ValidatePropertyGwtTest.java
(revision 9561)
+++
user/test/org/hibernate/jsr303/tck/tests/validation/ValidatePropertyGwtTest.java
(working copy)
@@ -45,6 +45,7 @@
}
}
+ @Failing(issue = 5882)
public void testValidateProperty() {
delegate.testValidateProperty();
}
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Mark some test as Failing. Note these test only fail on IE. (issue1303801)
LGTM On 2011/01/18 18:35:52, Nick Chalko wrote: http://gwt-code-reviews.appspot.com/1303801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: First pass at Issue 1405 (Dialog Box header fix) (issue1149803)
On Tue, Jan 18, 2011 at 10:29 AM, larse...@gmail.com wrote: http://gwt-code-reviews.appspot.com/1149803/show Thanks so much for reviewing this guys. @Ray, Do you want me to go back to allowing a setter for the caption? I certainly wouldn't want to see both the setter and the constructor. If you want to go for the setter after all, John will need to scrutinize the code that orphans the previous header, and support for setHeader(null), including unit tests — I'll screw it up. And you'll want to give it an @UiChild annotation. If you want to stick with the constructor, the changes to DialogBoxParser and DialogBoxParserTest to make it UiBinder friendly are pretty simple. And if I get to pick, yes, a full blown setter-based implementation is preferable. I'm just trying not to ask for the moon. -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Improve canvas for browsers (and permutations) with partial canvas support. (issue1296801)
http://gwt-code-reviews.appspot.com/1296801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Add support for mapping ConstraintViolation objects into SimpleBeanEditor. (issue1260801)
Updated this patch with a hack to ApiChecker that allows it to find the external javax.validation source jars. http://gwt-code-reviews.appspot.com/1260801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Add support for mapping ConstraintViolation objects into SimpleBeanEditor. (issue1260801)
lgtm http://gwt-code-reviews.appspot.com/1260801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Improve canvas for browsers (and permutations) with partial canvas support. (issue1296801)
http://gwt-code-reviews.appspot.com/1296801/diff/13001/14002
File user/src/com/google/gwt/canvas/client/Canvas.java (right):
http://gwt-code-reviews.appspot.com/1296801/diff/13001/14002#newcode45
user/src/com/google/gwt/canvas/client/Canvas.java:45:
CanvasElementSupportDetector detector =
GWT.create(CanvasElementSupportDetector.class);
Duplicate code, should call isSupported instead.
http://gwt-code-reviews.appspot.com/1296801/diff/13001/14002#newcode174
user/src/com/google/gwt/canvas/client/Canvas.java:174: static class
CanvasElementSupportDetector {
Can this be private?
http://gwt-code-reviews.appspot.com/1296801/diff/13001/14003
File user/src/com/google/gwt/dom/client/PartialSupport.java (right):
http://gwt-code-reviews.appspot.com/1296801/diff/13001/14003#newcode23
user/src/com/google/gwt/dom/client/PartialSupport.java:23: * supported
at runtime.
What about the factory method? Even if it's not universally needed, you
can document how it will appear when it's appropriate.
http://gwt-code-reviews.appspot.com/1296801/diff/13001/14005
File user/test/com/google/gwt/canvas/client/CanvasTest.java (right):
http://gwt-code-reviews.appspot.com/1296801/diff/13001/14005#newcode33
user/test/com/google/gwt/canvas/client/CanvasTest.java:33: public class
CanvasTest extends GWTTestCase {
Doesn't test isSupported.
http://gwt-code-reviews.appspot.com/1296801/show
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Removes a Java 1.6-ism. (issue1304801)
Reviewers: knorton,
Description:
Removes a Java 1.6-ism.
Please review this at http://gwt-code-reviews.appspot.com/1304801/show
Affected files:
M user/test/com/google/gwt/dev/jjs/test/singlejso/TypeHierarchyTest.java
Index:
user/test/com/google/gwt/dev/jjs/test/singlejso/TypeHierarchyTest.java
===
--- user/test/com/google/gwt/dev/jjs/test/singlejso/TypeHierarchyTest.java
(revision 9561)
+++ user/test/com/google/gwt/dev/jjs/test/singlejso/TypeHierarchyTest.java
(working copy)
@@ -127,7 +127,6 @@
}
private static class JvmNode implements Node {
-@Override
public JvmNode appendChild(Node node) {
return (JvmNode)node;
}
@@ -149,7 +148,6 @@
}
private static class JsNode extends JavaScriptObject implements Node {
-@Override
public final native JsNode appendChild(Node node) /*-{
return node;
}-*/;
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Removes a Java 1.6-ism. (issue1304801)
lgtm. thanks. http://gwt-code-reviews.appspot.com/1304801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
Re: [gwt-contrib] Re: typo in documentation?
Good catch Thomas, didn't see the a.. but yeah, it isn't very clear. -- Arthur Kalmenson On Fri, Jan 14, 2011 at 3:42 AM, Thomas Broyer t.bro...@gmail.com wrote: I guess that's why it says *a* BasicPlace rather than just BasicPlace. Yes, it's up to you to define such a class. David Chandler (or was it Chris Ramsdale?) once answered about this paragraph and acknowledged it wasn't very clear… -- http://groups.google.com/group/Google-Web-Toolkit-Contributors -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Optimizations for server-side invocations of CustomFieldSerializers. (issue1273801)
Mostly LGTM.
http://gwt-code-reviews.appspot.com/1273801/diff/1/6
File
user/src/com/google/gwt/user/client/rpc/core/java/lang/Double_CustomFieldSerializer.java
(right):
http://gwt-code-reviews.appspot.com/1273801/diff/1/6#newcode45
user/src/com/google/gwt/user/client/rpc/core/java/lang/Double_CustomFieldSerializer.java:45:
throws SerializationException {
On 2011/01/17 02:05:46, Miles Chaston wrote:
On 2011/01/10 20:12:52, jat wrote:
Continuation lines should be indented +4 columns. You might want to
look in
eclipse/README.txt to setup eclipse so formatting/etc matches our
style guide.
I will do my best, but I use IntelliJ rather than eclipse.
Is there a way of me testing the formatting by a command-line that
understands
the eclipse formatting?
ant checkstyle will catch some but not all of these.
I asked tobyr to email you settings files for using IntelliJ with GWT.
http://gwt-code-reviews.appspot.com/1273801/diff/1/33
File
user/src/com/google/gwt/user/server/rpc/impl/ServerSerializationStreamWriter.java
(right):
http://gwt-code-reviews.appspot.com/1273801/diff/1/33#newcode288
user/src/com/google/gwt/user/server/rpc/impl/ServerSerializationStreamWriter.java:288:
private static class CustomFieldSerializerHandle {
An alternative to wrapping everything would be to have a single value,
like you do for EMPTY_HANDLE, to indicate that it is known not to extend
CustomFieldSerializer. It could just be a private implementation of
CustomFieldSerializer that does nothing and is compared against.
That would keep single lookups in the map, while also avoiding wrapper
objects.
http://gwt-code-reviews.appspot.com/1273801/diff/8001/9034#newcode728
user/src/com/google/gwt/user/server/rpc/impl/ServerSerializationStreamWriter.java:728:
continue;
Won't this give a raw type warning? If so, it should be
CustomFieldSerializer? instead.
http://gwt-code-reviews.appspot.com/1273801/show
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: First pass at Issue 1405 (Dialog Box header fix) (issue1149803)
I'll hopefully have a chance to poke around at that code tonight and make a decision then as to what I have time to implement. On Tue, Jan 18, 2011 at 12:41 PM, Ray Ryan rj...@google.com wrote: On Tue, Jan 18, 2011 at 10:29 AM, larse...@gmail.com wrote: http://gwt-code-reviews.appspot.com/1149803/show Thanks so much for reviewing this guys. @Ray, Do you want me to go back to allowing a setter for the caption? I certainly wouldn't want to see both the setter and the constructor. If you want to go for the setter after all, John will need to scrutinize the code that orphans the previous header, and support for setHeader(null), including unit tests — I'll screw it up. And you'll want to give it an @UiChild annotation. If you want to stick with the constructor, the changes to DialogBoxParser and DialogBoxParserTest to make it UiBinder friendly are pretty simple. And if I get to pick, yes, a full blown setter-based implementation is preferable. I'm just trying not to ask for the moon. -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] [google-web-toolkit] r9562 committed - Add support for mapping ConstraintViolation objects into SimpleBeanEdi...
Revision: 9562
Author: b...@google.com
Date: Tue Jan 18 08:32:58 2011
Log: Add support for mapping ConstraintViolation objects into
SimpleBeanEditor.
Patch by: bobv
Review by: rjrjr
Review at http://gwt-code-reviews.appspot.com/1260801
http://code.google.com/p/google-web-toolkit/source/detail?r=9562
Added:
/trunk/user/src/com/google/gwt/editor/client/impl/SimpleViolation.java
Modified:
/trunk/build.xml
/trunk/tools/api-checker/src/com/google/gwt/tools/apichecker/ApiCompatibilityChecker.java
/trunk/user/src/com/google/gwt/editor/Editor.gwt.xml
/trunk/user/src/com/google/gwt/editor/client/SimpleBeanEditorDriver.java
/trunk/user/src/com/google/gwt/editor/client/impl/AbstractSimpleBeanEditorDriver.java
/trunk/user/src/com/google/gwt/editor/client/testing/MockSimpleBeanEditorDriver.java
/trunk/user/src/com/google/gwt/requestfactory/client/RequestFactoryEditorDriver.java
/trunk/user/src/com/google/gwt/requestfactory/client/impl/AbstractRequestFactoryEditorDriver.java
/trunk/user/src/com/google/gwt/requestfactory/client/testing/MockRequestFactoryEditorDriver.java
/trunk/user/test/com/google/gwt/editor/rebind/model/EditorModelTest.java
===
--- /dev/null
+++ /trunk/user/src/com/google/gwt/editor/client/impl/SimpleViolation.java
Tue Jan 18 08:32:58 2011
@@ -0,0 +1,162 @@
+/*
+ * Copyright 2011 Google Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the License); you may
not
+ * use this file except in compliance with the License. You may obtain a
copy of
+ * the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an AS IS BASIS,
WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+ * License for the specific language governing permissions and limitations
under
+ * the License.
+ */
+package com.google.gwt.editor.client.impl;
+
+import java.util.Iterator;
+import java.util.List;
+
+import javax.validation.ConstraintViolation;
+
+/**
+ * Abstraction of a ConstraintViolation or a RequestFactory Violation
object.
+ * Also contains a factory method to create SimpleViolation instances from
+ * {@link ConstraintViolation} objects.
+ */
+public abstract class SimpleViolation {
+ /**
+ * Provides a source of SimpleViolation objects based on
ConstraintViolations.
+ * This is re-used by the RequestFactoryEditorDriver implementation,
which
+ * does not share a type hierarchy with the SimpleBeanEditorDriver.
+ */
+ static class ConstraintViolationIterable implements
IterableSimpleViolation {
+
+private final IterableConstraintViolation? violations;
+
+public ConstraintViolationIterable(
+IterableConstraintViolation? violations) {
+ this.violations = violations;
+}
+
+public IteratorSimpleViolation iterator() {
+ // Use a fresh source iterator each time
+ final IteratorConstraintViolation? source =
violations.iterator();
+ return new IteratorSimpleViolation() {
+public boolean hasNext() {
+ return source.hasNext();
+}
+
+public SimpleViolation next() {
+ return new SimpleViolationAdapter(source.next());
+}
+
+public void remove() {
+ source.remove();
+}
+ };
+}
+ }
+
+ /**
+ * Adapts the ConstraintViolation interface to the SimpleViolation
interface.
+ */
+ static class SimpleViolationAdapter extends SimpleViolation {
+private final ConstraintViolation? v;
+
+public SimpleViolationAdapter(ConstraintViolation? v) {
+ this.v = v;
+}
+
+public Object getKey() {
+ return v.getLeafBean();
+}
+
+public String getMessage() {
+ return v.getMessage();
+}
+
+public String getPath() {
+ /*
+ * TODO(bobv,nchalko): Determine the correct way to extract this
+ * information from the ConstraintViolation.
+ */
+ return v.getPropertyPath().toString();
+}
+
+public Object getUserDataObject() {
+ return v;
+}
+ }
+
+ public static IterableSimpleViolation iterableFromConstrantViolations(
+ IterableConstraintViolation? violations) {
+return new ConstraintViolationIterable(violations);
+ }
+
+ /**
+ * Maps an abstract representation of a violation into the appropriate
+ * EditorDelegate.
+ */
+ public static void pushViolations(IterableSimpleViolation violations,
+ DelegateMap delegateMap) {
+// For each violation
+for (SimpleViolation error : violations) {
+ Object key = error.getKey();
+ ListAbstractEditorDelegate?, ? delegateList =
delegateMap.get(key);
+ if (delegateList != null) {
+
+// For each delegate editing some record...
+for (AbstractEditorDelegate?, ? baseDelegate : delegateList) {
+
+ // compute its base path in the hierarchy...
+ String
[gwt-contrib] Re: Improve canvas for browsers (and permutations) with partial canvas support. (issue1296801)
http://gwt-code-reviews.appspot.com/1296801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Improve canvas for browsers (and permutations) with partial canvas support. (issue1296801)
http://gwt-code-reviews.appspot.com/1296801/diff/7001/8002
File user/src/com/google/gwt/canvas/client/Canvas.java (right):
http://gwt-code-reviews.appspot.com/1296801/diff/7001/8002#newcode42
user/src/com/google/gwt/canvas/client/Canvas.java:42: public static
Canvas createCanvasIfSupported() {
On 2011/01/18 17:46:05, rjrjr wrote:
Naming nit: use of these factory methods might be easier to automate
down the
road if they are consistent, so I'd suggest
Canvas#createIfSupported(). DRY and
all that.
Done.
http://gwt-code-reviews.appspot.com/1296801/diff/13001/14002#newcode45
user/src/com/google/gwt/canvas/client/Canvas.java:45: return null;
On 2011/01/18 18:55:17, rjrjr wrote:
Duplicate code, should call isSupported instead.
I was trying to provide a way for developers to avoid creating two
Canvas elements by re-using the one created during the check for the
actual Canvas creation (calling isSupported() requires creating a Canvas
element). Think that's too hacky?
http://gwt-code-reviews.appspot.com/1296801/diff/13001/14002#newcode174
user/src/com/google/gwt/canvas/client/Canvas.java:174: }
On 2011/01/18 18:55:17, rjrjr wrote:
Can this be private?
Done.
http://gwt-code-reviews.appspot.com/1296801/diff/13001/14003
File user/src/com/google/gwt/dom/client/PartialSupport.java (right):
http://gwt-code-reviews.appspot.com/1296801/diff/13001/14003#newcode23
user/src/com/google/gwt/dom/client/PartialSupport.java:23: * supported
at runtime.
On 2011/01/18 18:55:17, rjrjr wrote:
What about the factory method? Even if it's not universally needed,
you can
document how it will appear when it's appropriate.
Improved the javadoc two include both methods. Does the new wording
LGTY?
http://gwt-code-reviews.appspot.com/1296801/diff/13001/14005
File user/test/com/google/gwt/canvas/client/CanvasTest.java (right):
http://gwt-code-reviews.appspot.com/1296801/diff/13001/14005#newcode33
user/test/com/google/gwt/canvas/client/CanvasTest.java:33: public class
CanvasTest extends GWTTestCase {
On 2011/01/18 18:55:17, rjrjr wrote:
Doesn't test isSupported.
Done.
http://gwt-code-reviews.appspot.com/1296801/show
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Removes a Java 1.6-ism. (issue1304801)
Submitted as of r9563 On 2011/01/18 19:23:21, knorton wrote: lgtm. thanks. http://gwt-code-reviews.appspot.com/1304801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] [google-web-toolkit] r9563 committed - Removes a Java 1.6-ism....
Revision: 9563
Author: gwt.mirror...@gmail.com
Date: Tue Jan 18 08:45:31 2011
Log: Removes a Java 1.6-ism.
Review at http://gwt-code-reviews.appspot.com/1304801
Review by: knor...@google.com
http://code.google.com/p/google-web-toolkit/source/detail?r=9563
Modified:
/trunk/user/test/com/google/gwt/dev/jjs/test/singlejso/TypeHierarchyTest.java
===
---
/trunk/user/test/com/google/gwt/dev/jjs/test/singlejso/TypeHierarchyTest.java
Thu Jan 13 15:08:15 2011
+++
/trunk/user/test/com/google/gwt/dev/jjs/test/singlejso/TypeHierarchyTest.java
Tue Jan 18 08:45:31 2011
@@ -127,7 +127,6 @@
}
private static class JvmNode implements Node {
-@Override
public JvmNode appendChild(Node node) {
return (JvmNode)node;
}
@@ -149,7 +148,6 @@
}
private static class JsNode extends JavaScriptObject implements Node {
-@Override
public final native JsNode appendChild(Node node) /*-{
return node;
}-*/;
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Make production mode stack traces match JRE spec more closely (issue1295802)
Reviewers: scottb,
Description:
Make production mode stack traces match JRE spec more closely
- Fix StackTraceElement#getFileName(), so that it returns null instead
of Unknown Source
- Fix StackTraceElement#getLineNumber(), so that it returns -1 instead
of 0
- Fix StackTraceElement#toString(), so that lineNumber is omitted when
it is unknown, so that
Unknown.foo(Unknown Source:0) becomes Unknown.foo(Unknown Source)
Review by: sco...@google.com
Please review this at http://gwt-code-reviews.appspot.com/1295802/show
Affected files:
M user/src/com/google/gwt/core/client/impl/StackTraceCreator.java
M user/super/com/google/gwt/emul/java/lang/StackTraceElement.java
Index: user/src/com/google/gwt/core/client/impl/StackTraceCreator.java
===
--- user/src/com/google/gwt/core/client/impl/StackTraceCreator.java
(revision 9493)
+++ user/src/com/google/gwt/core/client/impl/StackTraceCreator.java
(working copy)
@@ -67,8 +67,8 @@
StackTraceElement[] stackTrace = new
StackTraceElement[stack.length()];
for (int i = 0, j = stackTrace.length; i j; i++) {
-stackTrace[i] = new StackTraceElement(Unknown, stack.get(i),
-Unknown source, 0);
+stackTrace[i] = new StackTraceElement(Unknown, stack.get(i),
null,
+LINE_NUMBER_UNKNOWN);
}
e.setStackTrace(stackTrace);
}
@@ -77,8 +77,8 @@
JsArrayString stack = StackTraceCreator.createStackTrace();
StackTraceElement[] stackTrace = new
StackTraceElement[stack.length()];
for (int i = 0, j = stackTrace.length; i j; i++) {
-stackTrace[i] = new StackTraceElement(Unknown, stack.get(i),
-Unknown source, 0);
+stackTrace[i] = new StackTraceElement(Unknown, stack.get(i),
null,
+LINE_NUMBER_UNKNOWN);
}
t.setStackTrace(stackTrace);
}
@@ -145,8 +145,8 @@
for (int i = 0, j = stackTrace.length; i j; i++) {
// Locations is also backwards
String location = locations.get(j - i - 1);
-String fileName = Unknown source;
-int lineNumber = 0;
+String fileName = null;
+int lineNumber = LINE_NUMBER_UNKNOWN;
if (location != null) {
int idx = location.indexOf(':');
if (idx != -1) {
@@ -361,6 +361,8 @@
}
}
+ private static final int LINE_NUMBER_UNKNOWN = -1;
+
/**
* Create a stack trace based on a JavaScriptException. This method
should
* only be called in Production Mode.
Index: user/super/com/google/gwt/emul/java/lang/StackTraceElement.java
===
--- user/super/com/google/gwt/emul/java/lang/StackTraceElement.java
(revision 9493)
+++ user/super/com/google/gwt/emul/java/lang/StackTraceElement.java
(working copy)
@@ -60,7 +60,8 @@
}
public String toString() {
-return className + . + methodName + ( + fileName + : + lineNumber
-+ );
+return className + . + methodName + (
++ (fileName != null ? fileName : Unknown Source)
++ (lineNumber 0 ? : + lineNumber : ) + );
}
}
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] [google-web-toolkit] r9564 committed - Mark some test as Failing. Note these test only fail on IE....
Revision: 9564
Author: ncha...@google.com
Date: Tue Jan 18 08:56:33 2011
Log: Mark some test as Failing. Note these test only fail on IE.
See
http://code.google.com/p/google-web-toolkit/issues/detail?id=5882
Review at http://gwt-code-reviews.appspot.com/1303801
Review by: rchan...@google.com
http://code.google.com/p/google-web-toolkit/source/detail?r=9564
Modified:
/trunk/user/test/org/hibernate/jsr303/tck/tests/constraints/constraintcomposition/ConstraintCompositionGwtTest.java
/trunk/user/test/org/hibernate/jsr303/tck/tests/constraints/customconstraint/CustomConstraintValidatorGwtTest.java
/trunk/user/test/org/hibernate/jsr303/tck/tests/constraints/validatorresolution/ValidatorResolutionGwtTest.java
/trunk/user/test/org/hibernate/jsr303/tck/tests/validation/ValidatePropertyGwtTest.java
===
---
/trunk/user/test/org/hibernate/jsr303/tck/tests/constraints/constraintcomposition/ConstraintCompositionGwtTest.java
Mon Jan 17 11:36:20 2011
+++
/trunk/user/test/org/hibernate/jsr303/tck/tests/constraints/constraintcomposition/ConstraintCompositionGwtTest.java
Tue Jan 18 08:56:33 2011
@@ -40,10 +40,12 @@
delegate.testAttributesDefinedOnComposingConstraints();
}
+ @Failing(issue = 5882)
public void testComposedConstraints() {
delegate.testComposedConstraints();
}
+ @Failing(issue = 5882)
public void testComposedConstraintsAreRecursive() {
delegate.testComposedConstraintsAreRecursive();
}
===
---
/trunk/user/test/org/hibernate/jsr303/tck/tests/constraints/customconstraint/CustomConstraintValidatorGwtTest.java
Mon Jan 17 11:36:20 2011
+++
/trunk/user/test/org/hibernate/jsr303/tck/tests/constraints/customconstraint/CustomConstraintValidatorGwtTest.java
Tue Jan 18 08:56:33 2011
@@ -30,6 +30,7 @@
return org.hibernate.jsr303.tck.tests.constraints.customconstraint.TckTest;
}
+ @Failing(issue = 5882)
public void testDefaultPropertyPath() {
delegate.testDefaultPropertyPath();
}
===
---
/trunk/user/test/org/hibernate/jsr303/tck/tests/constraints/validatorresolution/ValidatorResolutionGwtTest.java
Mon Jan 17 11:36:20 2011
+++
/trunk/user/test/org/hibernate/jsr303/tck/tests/constraints/validatorresolution/ValidatorResolutionGwtTest.java
Tue Jan 18 08:56:33 2011
@@ -34,6 +34,7 @@
delegate.testAmbiguousValidatorResolution();
}
+ @Failing(issue = 5882)
public void testResolutionOfMinMaxForDifferentTypes() {
delegate.testResolutionOfMinMaxForDifferentTypes();
}
===
---
/trunk/user/test/org/hibernate/jsr303/tck/tests/validation/ValidatePropertyGwtTest.java
Mon Jan 17 11:36:20 2011
+++
/trunk/user/test/org/hibernate/jsr303/tck/tests/validation/ValidatePropertyGwtTest.java
Tue Jan 18 08:56:33 2011
@@ -45,6 +45,7 @@
}
}
+ @Failing(issue = 5882)
public void testValidateProperty() {
delegate.testValidateProperty();
}
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Fix bug on IE where onload events don't fire for IFrames. (issue1294801)
http://gwt-code-reviews.appspot.com/1294801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Improve canvas for browsers (and permutations) with partial canvas support. (issue1296801)
LGTM with nits.
http://gwt-code-reviews.appspot.com/1296801/diff/21001/22002
File user/src/com/google/gwt/canvas/client/Canvas.java (right):
http://gwt-code-reviews.appspot.com/1296801/diff/21001/22002#newcode61
user/src/com/google/gwt/canvas/client/Canvas.java:61: public static
final boolean isSupported() {
final is meaningless on static methods, right?
http://gwt-code-reviews.appspot.com/1296801/diff/21001/22002#newcode74
user/src/com/google/gwt/canvas/client/Canvas.java:74: * Protected
constructor. Use {@link #createIfSupported()} to create a Canvas.
If this is protected, you should also talk about how subclasses should
use it. For example, you might want to say that they should never have
a public constructor, or else you need to specify that this can throw
exceptions if called when not supported.
If you don't expect subclasses, it might be better to make this private
for now, and then you can define what subclasses do when it becomes
useful to subclass.
http://gwt-code-reviews.appspot.com/1296801/diff/21001/22003
File user/src/com/google/gwt/dom/client/PartialSupport.java (right):
http://gwt-code-reviews.appspot.com/1296801/diff/21001/22003#newcode22
user/src/com/google/gwt/dom/client/PartialSupport.java:22: * By
convention, classes annotated with PartialSupport will provide two
static methods:
80 chars, throughout the change.
http://gwt-code-reviews.appspot.com/1296801/diff/21001/22003#newcode26
user/src/com/google/gwt/dom/client/PartialSupport.java:26: * li
codestatic createIfSupported()/code factory method that returns a
new feature
Should this mention the return type?
http://gwt-code-reviews.appspot.com/1296801/show
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Fix bug on IE where onload events don't fire for IFrames. (issue1294801)
http://gwt-code-reviews.appspot.com/1294801/diff/3001/4002
File user/test/com/google/gwt/dom/client/FrameTests.java (right):
http://gwt-code-reviews.appspot.com/1294801/diff/3001/4002#newcode49
user/test/com/google/gwt/dom/client/FrameTests.java:49: final Timer
timer = new Timer() {
On 2011/01/14 19:14:48, jlabanca wrote:
Do you need this timer? The test will time out if onload is never
triggered.
Done.
http://gwt-code-reviews.appspot.com/1294801/diff/3001/4002#newcode75
user/test/com/google/gwt/dom/client/FrameTests.java:75:
delayTestFinish(2 * delayMillis);
On 2011/01/14 19:14:48, jlabanca wrote:
Move this above the line where you create the iframe to avoid timing
issues,
especially in HtmlUnit. If the iframe happens to load synchronously
or very
quickly, the onload event might fire before delayTestFinish. In
general, always
call delayTestFinish() before calling the thing that will trigger an
asynchronous event.
Done.
http://gwt-code-reviews.appspot.com/1294801/show
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Fix bug on IE where onload events don't fire for IFrames. (issue1294801)
LGTM http://gwt-code-reviews.appspot.com/1294801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Improve canvas for browsers (and permutations) with partial canvas support. (issue1296801)
http://gwt-code-reviews.appspot.com/1296801/diff/21001/22004
File user/src/com/google/gwt/user/client/IsSupported.java (left):
http://gwt-code-reviews.appspot.com/1296801/diff/21001/22004#oldcode32
user/src/com/google/gwt/user/client/IsSupported.java:32: public
interface IsSupported {
Is this already in GWT 2.1.1? If so, we probably don't want to pull it,
especially if we are just replacing it with another empty interface.
I don't think we need PartialSupport, but if we do, can it just extend
IsSupported?
http://gwt-code-reviews.appspot.com/1296801/show
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] [google-web-toolkit] r9565 committed - Optimize redundant 'switch' statements...
Revision: 9565
Author: r...@google.com
Date: Tue Jan 18 10:26:55 2011
Log: Optimize redundant 'switch' statements
Review at http://gwt-code-reviews.appspot.com/1286801
Review by: sco...@google.com
http://code.google.com/p/google-web-toolkit/source/detail?r=9565
Added:
/trunk/dev/core/src/com/google/gwt/dev/js/JsDuplicateCaseFolder.java
/trunk/dev/core/test/com/google/gwt/dev/js/JsDuplicateCaseFolderTest.java
Modified:
/trunk/dev/core/src/com/google/gwt/dev/jjs/JavaToJavaScriptCompiler.java
/trunk/dev/core/src/com/google/gwt/dev/js/ast/JsBlock.java
/trunk/dev/core/src/com/google/gwt/dev/js/ast/JsBreak.java
/trunk/dev/core/src/com/google/gwt/dev/js/ast/JsIf.java
===
--- /dev/null
+++ /trunk/dev/core/src/com/google/gwt/dev/js/JsDuplicateCaseFolder.java
Tue Jan 18 10:26:55 2011
@@ -0,0 +1,174 @@
+/*
+ * Copyright 2011 Google Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the License); you may
not
+ * use this file except in compliance with the License. You may obtain a
copy of
+ * the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an AS IS BASIS,
WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+ * License for the specific language governing permissions and limitations
under
+ * the License.
+ */
+package com.google.gwt.dev.js;
+
+import com.google.gwt.dev.js.ast.JsBlock;
+import com.google.gwt.dev.js.ast.JsContext;
+import com.google.gwt.dev.js.ast.JsModVisitor;
+import com.google.gwt.dev.js.ast.JsProgram;
+import com.google.gwt.dev.js.ast.JsStatement;
+import com.google.gwt.dev.js.ast.JsSwitch;
+import com.google.gwt.dev.js.ast.JsSwitchMember;
+
+import java.util.HashMap;
+import java.util.LinkedList;
+import java.util.List;
+import java.util.Map;
+
+/**
+ * Combine case labels with identical bodies. Case bodies that may fall
through
+ * to the following case label and case bodies following a possible
fallthrough
+ * are left undisturbed.
+ *
+ * For example, consider the following input:
+ *
+ * pre
+ * switch (x) {
+ * case 0: y = 17; break;
+ * case 1: if (z == 0) { y = 18; break; } else { y = 19 } // fallthrough
else
+ * case 2: return 22;
+ * case 3: if (z == 0) { y = 18; break; } else { y = 19 } // fallthrough
else
+ * case 4: y = 17; break;
+ * case 5: y = 17; break;
+ * case 6: return 22;
+ * }
+ * /pre
+ *
+ * This will be transformed into:
+ *
+ * pre
+ * switch (x) {
+ * case 0: y = 17; break;
+ * case 1: if (z == 0) { y = 18; break; } else { y = 19 }
+ * case 6: case 2: return 22;
+ * case 3: if (z == 0) { y = 18; break; } else { y = 19 }
+ * case 5: case 4: y = 17; break;
+ * }
+ *
+ * pre
+ *
+ * Cases (2, 6) and (4, 5) have been coalesced. Note that case 0 has not
been
+ * combined with cases 4 and 5 since case 4 cannot be moved due to the
potential
+ * fallthrough from case 3, and we currently only coalesce a given cases
with a
+ * preceding case and so cannot move case 0 downward.
+ *
+ * Although this pattern is unlikely to occur frequently in hand-written
code,
+ * it can account for a significant amount of space in generated code.
+ */
+public class JsDuplicateCaseFolder {
+
+ private class DuplicateCaseFolder extends JsModVisitor {
+
+public DuplicateCaseFolder() {
+}
+
+@Override
+public boolean visit(JsSwitch x, JsContextJsStatement ctx) {
+ boolean modified = false;
+
+ // A map from case body source code to the original case label
+ // in which they appeared
+ MapString, JsSwitchMember seen = new HashMapString,
JsSwitchMember();
+
+ // Original list of members
+ ListJsSwitchMember cases = x.getCases();
+ // Coalesced list of members
+ ListJsSwitchMember newCases = new LinkedListJsSwitchMember();
+
+ // Keep track of whether the previous case can fall through
+ // to the current case
+ boolean hasPreviousFallthrough = false;
+
+ // Iterate over members and locate ones with bodies identical to
+ // previous members
+ for (JsSwitchMember member : cases) {
+ListJsStatement stmts = member.getStmts();
+
+// Don't rewrite any cases that might fall through
+if (!unconditionalControlBreak(stmts)) {
+ hasPreviousFallthrough = true;
+ // copy the case into the output
+ newCases.add(member);
+ continue;
+}
+
+String body = toSource(stmts);
+JsSwitchMember previousCase = seen.get(body);
+if (previousCase == null || hasPreviousFallthrough) {
+ // Don't coalesce a case that can be reached via fallthrough
+ // from the previous case
+ newCases.add(member);
+ seen.put(body, member);
+} else {
+ // Locate the position of the case that this case is to be
+ //
Re: [gwt-contrib] Re: Add optimizations for redundant and trivial 'switch' statements (issue1286801)
I'm curious, has there been any measurement/data as to the effect this
has on output size?
On Mon, Jan 17, 2011 at 1:18 PM, sco...@google.com wrote:
LGTM, just nits. No need to re-review.
http://gwt-code-reviews.appspot.com/1286801/diff/5002/6002
File dev/core/src/com/google/gwt/dev/js/JsDuplicateCaseFolder.java
(right):
http://gwt-code-reviews.appspot.com/1286801/diff/5002/6002#newcode143
dev/core/src/com/google/gwt/dev/js/JsDuplicateCaseFolder.java:143:
sb.append(\0); // separate statements with an otherwise illegal char
I would actually use '\n' here so that the string is easily readable in
a debugger.
http://gwt-code-reviews.appspot.com/1286801/diff/5002/6003
File dev/core/src/com/google/gwt/dev/js/ast/JsBlock.java (right):
http://gwt-code-reviews.appspot.com/1286801/diff/5002/6003#newcode50
dev/core/src/com/google/gwt/dev/js/ast/JsBlock.java:50: public boolean
unconditionalControlBreak() {
This work correctly because of the implementation of
JBreakStatement.unconditionalControlBreak(), which never counts labelled
break statements as breaks. Could you add a comment here or there (or
both?) about this dependency between the two?
http://gwt-code-reviews.appspot.com/1286801/diff/5002/6004
File dev/core/src/com/google/gwt/dev/js/ast/JsIf.java (right):
http://gwt-code-reviews.appspot.com/1286801/diff/5002/6004#newcode86
dev/core/src/com/google/gwt/dev/js/ast/JsIf.java:86: }
This is handy, but DeadCodeElimination should have already taken care of
these cases. I would argue for going with the computationally cheaper
(and simpler) implementation here if it doesn't actually improve the
output.
http://gwt-code-reviews.appspot.com/1286801/diff/5002/6005
File dev/core/test/com/google/gwt/dev/js/JsDuplicateCaseFolderTest.java
(right):
http://gwt-code-reviews.appspot.com/1286801/diff/5002/6005#newcode85
dev/core/test/com/google/gwt/dev/js/JsDuplicateCaseFolderTest.java:85:
private void check(String input, String expected) throws Exception {
Total nit, but I would reverse the argument order to match the order of
assertEquals().
http://gwt-code-reviews.appspot.com/1286801/diff/5002/6005#newcode87
dev/core/test/com/google/gwt/dev/js/JsDuplicateCaseFolderTest.java:87:
String optimized = optimize(expected);
You probably don't actually want to optimize this, right? Suggest:
// Generate canonical expected output (no optimizers)
String expectedOutput = super.optimize(expected, new Class[0]);
http://gwt-code-reviews.appspot.com/1286801/show
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Improve canvas for browsers (and permutations) with partial canvas support. (issue1296801)
On Tue, Jan 18, 2011 at 11:53 AM, p...@google.com wrote:
http://gwt-code-reviews.appspot.com/1296801/diff/7001/8002
File user/src/com/google/gwt/canvas/client/Canvas.java (right):
http://gwt-code-reviews.appspot.com/1296801/diff/7001/8002#newcode42
user/src/com/google/gwt/canvas/client/Canvas.java:42: public static
Canvas createCanvasIfSupported() {
On 2011/01/18 17:46:05, rjrjr wrote:
Naming nit: use of these factory methods might be easier to automate
down the
road if they are consistent, so I'd suggest
Canvas#createIfSupported(). DRY and
all that.
Done.
http://gwt-code-reviews.appspot.com/1296801/diff/13001/14002#newcode45
user/src/com/google/gwt/canvas/client/Canvas.java:45: return null;
On 2011/01/18 18:55:17, rjrjr wrote:
Duplicate code, should call isSupported instead.
I was trying to provide a way for developers to avoid creating two
Canvas elements by re-using the one created during the check for the
actual Canvas creation (calling isSupported() requires creating a Canvas
element). Think that's too hacky?
Oh, I didn't notice that. Objection withdrawn.
http://gwt-code-reviews.appspot.com/1296801/diff/13001/14002#newcode174
user/src/com/google/gwt/canvas/client/Canvas.java:174: }
On 2011/01/18 18:55:17, rjrjr wrote:
Can this be private?
Done.
http://gwt-code-reviews.appspot.com/1296801/diff/13001/14003
File user/src/com/google/gwt/dom/client/PartialSupport.java (right):
http://gwt-code-reviews.appspot.com/1296801/diff/13001/14003#newcode23
user/src/com/google/gwt/dom/client/PartialSupport.java:23: * supported
at runtime.
On 2011/01/18 18:55:17, rjrjr wrote:
What about the factory method? Even if it's not universally needed,
you can
document how it will appear when it's appropriate.
Improved the javadoc two include both methods. Does the new wording
LGTY?
http://gwt-code-reviews.appspot.com/1296801/diff/13001/14005
File user/test/com/google/gwt/canvas/client/CanvasTest.java (right):
http://gwt-code-reviews.appspot.com/1296801/diff/13001/14005#newcode33
user/test/com/google/gwt/canvas/client/CanvasTest.java:33: public class
CanvasTest extends GWTTestCase {
On 2011/01/18 18:55:17, rjrjr wrote:
Doesn't test isSupported.
Done.
http://gwt-code-reviews.appspot.com/1296801/show
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
Re: [gwt-contrib] Re: Add optimizations for redundant and trivial 'switch' statements (issue1286801)
I saw 4.5% on the uncompressed size.
Dan
On Tue, Jan 18, 2011 at 4:39 PM, Ray Cromwell cromwell...@google.com wrote:
I'm curious, has there been any measurement/data as to the effect this
has on output size?
On Mon, Jan 17, 2011 at 1:18 PM, sco...@google.com wrote:
LGTM, just nits. No need to re-review.
http://gwt-code-reviews.appspot.com/1286801/diff/5002/6002
File dev/core/src/com/google/gwt/dev/js/JsDuplicateCaseFolder.java
(right):
http://gwt-code-reviews.appspot.com/1286801/diff/5002/6002#newcode143
dev/core/src/com/google/gwt/dev/js/JsDuplicateCaseFolder.java:143:
sb.append(\0); // separate statements with an otherwise illegal char
I would actually use '\n' here so that the string is easily readable in
a debugger.
http://gwt-code-reviews.appspot.com/1286801/diff/5002/6003
File dev/core/src/com/google/gwt/dev/js/ast/JsBlock.java (right):
http://gwt-code-reviews.appspot.com/1286801/diff/5002/6003#newcode50
dev/core/src/com/google/gwt/dev/js/ast/JsBlock.java:50: public boolean
unconditionalControlBreak() {
This work correctly because of the implementation of
JBreakStatement.unconditionalControlBreak(), which never counts labelled
break statements as breaks. Could you add a comment here or there (or
both?) about this dependency between the two?
http://gwt-code-reviews.appspot.com/1286801/diff/5002/6004
File dev/core/src/com/google/gwt/dev/js/ast/JsIf.java (right):
http://gwt-code-reviews.appspot.com/1286801/diff/5002/6004#newcode86
dev/core/src/com/google/gwt/dev/js/ast/JsIf.java:86: }
This is handy, but DeadCodeElimination should have already taken care of
these cases. I would argue for going with the computationally cheaper
(and simpler) implementation here if it doesn't actually improve the
output.
http://gwt-code-reviews.appspot.com/1286801/diff/5002/6005
File dev/core/test/com/google/gwt/dev/js/JsDuplicateCaseFolderTest.java
(right):
http://gwt-code-reviews.appspot.com/1286801/diff/5002/6005#newcode85
dev/core/test/com/google/gwt/dev/js/JsDuplicateCaseFolderTest.java:85:
private void check(String input, String expected) throws Exception {
Total nit, but I would reverse the argument order to match the order of
assertEquals().
http://gwt-code-reviews.appspot.com/1286801/diff/5002/6005#newcode87
dev/core/test/com/google/gwt/dev/js/JsDuplicateCaseFolderTest.java:87:
String optimized = optimize(expected);
You probably don't actually want to optimize this, right? Suggest:
// Generate canonical expected output (no optimizers)
String expectedOutput = super.optimize(expected, new Class[0]);
http://gwt-code-reviews.appspot.com/1286801/show
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Improve canvas for browsers (and permutations) with partial canvas support. (issue1296801)
http://gwt-code-reviews.appspot.com/1296801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Improve canvas for browsers (and permutations) with partial canvas support. (issue1296801)
http://gwt-code-reviews.appspot.com/1296801/diff/21001/22002
File user/src/com/google/gwt/canvas/client/Canvas.java (right):
http://gwt-code-reviews.appspot.com/1296801/diff/21001/22002#newcode61
user/src/com/google/gwt/canvas/client/Canvas.java:61: public static
final boolean isSupported() {
On 2011/01/18 20:30:22, jat wrote:
final is meaningless on static methods, right?
Yes. Removed.
http://gwt-code-reviews.appspot.com/1296801/diff/21001/22002#newcode74
user/src/com/google/gwt/canvas/client/Canvas.java:74: * Protected
constructor. Use {@link #createIfSupported()} to create a Canvas.
On 2011/01/18 20:30:22, jat wrote:
If this is protected, you should also talk about how subclasses should
use it.
For example, you might want to say that they should never have a
public
constructor, or else you need to specify that this can throw
exceptions if
called when not supported.
If you don't expect subclasses, it might be better to make this
private for now,
and then you can define what subclasses do when it becomes useful to
subclass.
Decided to put it as private. We discussed leaving it as protected but
the subclass route opens up a can of worms factory that I think would be
more harm than good.
http://gwt-code-reviews.appspot.com/1296801/diff/21001/22003
File user/src/com/google/gwt/dom/client/PartialSupport.java (right):
http://gwt-code-reviews.appspot.com/1296801/diff/21001/22003#newcode22
user/src/com/google/gwt/dom/client/PartialSupport.java:22: * By
convention, classes annotated with PartialSupport will provide two
static methods:
On 2011/01/18 20:30:22, jat wrote:
80 chars, throughout the change.
Done.
http://gwt-code-reviews.appspot.com/1296801/diff/21001/22003#newcode26
user/src/com/google/gwt/dom/client/PartialSupport.java:26: * li
codestatic createIfSupported()/code factory method that returns a
new feature
On 2011/01/18 20:30:22, jat wrote:
Should this mention the return type?
Done.
http://gwt-code-reviews.appspot.com/1296801/diff/21001/22004
File user/src/com/google/gwt/user/client/IsSupported.java (left):
http://gwt-code-reviews.appspot.com/1296801/diff/21001/22004#oldcode32
user/src/com/google/gwt/user/client/IsSupported.java:32: public
interface IsSupported {
On 2011/01/18 21:14:25, jlabanca wrote:
Is this already in GWT 2.1.1? If so, we probably don't want to pull
it,
especially if we are just replacing it with another empty interface.
I don't think we need PartialSupport, but if we do, can it just extend
IsSupported?
IsSupported isn't in 2.1.1 so we're free to change it as needed. I think
some sort of annotation/marker is good since this is such a large break
from GWT's norm.
http://gwt-code-reviews.appspot.com/1296801/show
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Implement an isDirty() method for Editor framework drivers. (issue1306801)
Reviewers: rjrjr, Description: Implement an isDirty() method for Editor framework drivers. Issue 5881. Patch by: bobv Review by: rjrjr Please review this at http://gwt-code-reviews.appspot.com/1306801/show Affected files: M user/src/com/google/gwt/editor/client/EditorDelegate.java M user/src/com/google/gwt/editor/client/SimpleBeanEditorDriver.java M user/src/com/google/gwt/editor/client/impl/AbstractEditorDelegate.java M user/src/com/google/gwt/editor/client/impl/AbstractSimpleBeanEditorDriver.java M user/src/com/google/gwt/editor/client/impl/DelegateMap.java M user/src/com/google/gwt/editor/client/testing/MockEditorDelegate.java M user/src/com/google/gwt/editor/client/testing/MockSimpleBeanEditorDriver.java M user/src/com/google/gwt/editor/rebind/AbstractEditorDriverGenerator.java M user/src/com/google/gwt/requestfactory/client/RequestFactoryEditorDriver.java M user/src/com/google/gwt/requestfactory/client/impl/AbstractRequestFactoryEditorDriver.java M user/src/com/google/gwt/requestfactory/client/testing/MockRequestFactoryEditorDriver.java M user/test/com/google/gwt/editor/client/SimpleBeanEditorTest.java -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Improve canvas for browsers (and permutations) with partial canvas support. (issue1296801)
LGTM http://gwt-code-reviews.appspot.com/1296801/diff/28001/29002 File user/src/com/google/gwt/canvas/client/Canvas.java (right): http://gwt-code-reviews.appspot.com/1296801/diff/28001/29002#newcode64 user/src/com/google/gwt/canvas/client/Canvas.java:64: CanvasElementSupportDetector.class); You shouldn't GWT.create() on every call to isSupported. Create a static variable and do a null check here, GWT.creating only the first time. In fact, you might just cache the return value after its called once. Sorry I missed this before. http://gwt-code-reviews.appspot.com/1296801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Implement spec section 2.2. Applying multiple constraints of the same type. (issue1288802)
Reviewers: rchandia, Description: Implement spec section 2.2. Applying multiple constraints of the same type. [JSR 303 TCK Result] 63 of 257 (24.51%) Pass with 26 Failures and 5 Errors. Please review this at http://gwt-code-reviews.appspot.com/1288802/show Affected files: M user/src/com/google/gwt/validation/rebind/GwtSpecificValidatorCreator.java M user/test/org/hibernate/jsr303/tck/tests/validation/PropertyPathGwtTest.java M user/test/org/hibernate/jsr303/tck/tests/validation/TckTestValidatorFactory.java -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Make production mode stack traces match JRE spec more closely (issue1295802)
LGTM w/ nit. http://gwt-code-reviews.appspot.com/1295802/diff/1/3 File user/super/com/google/gwt/emul/java/lang/StackTraceElement.java (right): http://gwt-code-reviews.appspot.com/1295802/diff/1/3#newcode65 user/super/com/google/gwt/emul/java/lang/StackTraceElement.java:65: + (lineNumber 0 ? : + lineNumber : ) + ); Should be = since 0 is valid. http://gwt-code-reviews.appspot.com/1295802/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] [google-web-toolkit] r9566 committed - Fix MIME type and EOL style on javadocs.
Revision: 9566 Author: j...@google.com Date: Tue Jan 18 20:55:47 2011 Log: Fix MIME type and EOL style on javadocs. http://code.google.com/p/google-web-toolkit/source/detail?r=9566 Modified: /javadoc/2.1/com/google/gwt/autobean/server/AutoBeanFactoryMagic.html /javadoc/2.1/com/google/gwt/autobean/server/Configuration.Builder.html /javadoc/2.1/com/google/gwt/autobean/server/Configuration.html /javadoc/2.1/com/google/gwt/autobean/server/class-use/AutoBeanFactoryMagic.html /javadoc/2.1/com/google/gwt/autobean/server/class-use/Configuration.Builder.html /javadoc/2.1/com/google/gwt/autobean/server/class-use/Configuration.html /javadoc/2.1/com/google/gwt/autobean/server/package-frame.html /javadoc/2.1/com/google/gwt/autobean/server/package-summary.html /javadoc/2.1/com/google/gwt/autobean/server/package-tree.html /javadoc/2.1/com/google/gwt/autobean/server/package-use.html /javadoc/2.1/com/google/gwt/autobean/shared/AutoBean.PropertyName.html /javadoc/2.1/com/google/gwt/autobean/shared/AutoBean.html /javadoc/2.1/com/google/gwt/autobean/shared/AutoBeanCodex.html /javadoc/2.1/com/google/gwt/autobean/shared/AutoBeanFactory.Category.html /javadoc/2.1/com/google/gwt/autobean/shared/AutoBeanFactory.NoWrap.html /javadoc/2.1/com/google/gwt/autobean/shared/AutoBeanFactory.html /javadoc/2.1/com/google/gwt/autobean/shared/AutoBeanUtils.html /javadoc/2.1/com/google/gwt/autobean/shared/AutoBeanVisitor.CollectionPropertyContext.html /javadoc/2.1/com/google/gwt/autobean/shared/AutoBeanVisitor.Context.html /javadoc/2.1/com/google/gwt/autobean/shared/AutoBeanVisitor.MapPropertyContext.html /javadoc/2.1/com/google/gwt/autobean/shared/AutoBeanVisitor.PropertyContext.html /javadoc/2.1/com/google/gwt/autobean/shared/AutoBeanVisitor.html /javadoc/2.1/com/google/gwt/autobean/shared/Splittable.html /javadoc/2.1/com/google/gwt/autobean/shared/ValueCodex.html /javadoc/2.1/com/google/gwt/autobean/shared/class-use/AutoBean.PropertyName.html /javadoc/2.1/com/google/gwt/autobean/shared/class-use/AutoBean.html /javadoc/2.1/com/google/gwt/autobean/shared/class-use/AutoBeanCodex.html /javadoc/2.1/com/google/gwt/autobean/shared/class-use/AutoBeanFactory.Category.html /javadoc/2.1/com/google/gwt/autobean/shared/class-use/AutoBeanFactory.NoWrap.html /javadoc/2.1/com/google/gwt/autobean/shared/class-use/AutoBeanFactory.html /javadoc/2.1/com/google/gwt/autobean/shared/class-use/AutoBeanUtils.html /javadoc/2.1/com/google/gwt/autobean/shared/class-use/AutoBeanVisitor.CollectionPropertyContext.html /javadoc/2.1/com/google/gwt/autobean/shared/class-use/AutoBeanVisitor.Context.html /javadoc/2.1/com/google/gwt/autobean/shared/class-use/AutoBeanVisitor.MapPropertyContext.html /javadoc/2.1/com/google/gwt/autobean/shared/class-use/AutoBeanVisitor.PropertyContext.html /javadoc/2.1/com/google/gwt/autobean/shared/class-use/AutoBeanVisitor.html /javadoc/2.1/com/google/gwt/autobean/shared/class-use/Splittable.html /javadoc/2.1/com/google/gwt/autobean/shared/class-use/ValueCodex.html /javadoc/2.1/com/google/gwt/autobean/shared/package-frame.html /javadoc/2.1/com/google/gwt/autobean/shared/package-summary.html /javadoc/2.1/com/google/gwt/autobean/shared/package-tree.html /javadoc/2.1/com/google/gwt/autobean/shared/package-use.html /javadoc/2.1/com/google/gwt/cell/client/Cell.Context.html /javadoc/2.1/com/google/gwt/cell/client/class-use/Cell.Context.html /javadoc/2.1/com/google/gwt/i18n/client/NumberFormat.html /javadoc/2.1/com/google/gwt/requestfactory/server/ServiceLayer.html /javadoc/2.1/com/google/gwt/requestfactory/server/ServiceLayerDecorator.html /javadoc/2.1/com/google/gwt/requestfactory/server/SimpleRequestProcessor.html /javadoc/2.1/com/google/gwt/requestfactory/server/class-use/ServiceLayer.html /javadoc/2.1/com/google/gwt/requestfactory/server/class-use/ServiceLayerDecorator.html /javadoc/2.1/com/google/gwt/requestfactory/server/class-use/SimpleRequestProcessor.html /javadoc/2.1/com/google/gwt/requestfactory/server/testing/InProcessRequestTransport.html /javadoc/2.1/com/google/gwt/requestfactory/server/testing/RequestFactoryMagic.html /javadoc/2.1/com/google/gwt/requestfactory/server/testing/class-use/InProcessRequestTransport.html /javadoc/2.1/com/google/gwt/requestfactory/server/testing/class-use/RequestFactoryMagic.html /javadoc/2.1/com/google/gwt/requestfactory/server/testing/package-frame.html /javadoc/2.1/com/google/gwt/requestfactory/server/testing/package-summary.html /javadoc/2.1/com/google/gwt/requestfactory/server/testing/package-tree.html /javadoc/2.1/com/google/gwt/requestfactory/server/testing/package-use.html /javadoc/2.1/com/google/gwt/requestfactory/shared/BaseProxy.html /javadoc/2.1/com/google/gwt/requestfactory/shared/DefaultProxyStore.html /javadoc/2.1/com/google/gwt/requestfactory/shared/Locator.html /javadoc/2.1/com/google/gwt/requestfactory/shared/ProxySerializer.html
[gwt-contrib] Re: Implement spec section 2.2. Applying multiple constraints of the same type. (issue1288802)
http://gwt-code-reviews.appspot.com/1288802/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
