[gwt-contrib] Re: Fixing setInnerHTML calls on attach/detach sections. (issue1422811)
LGTM http://gwt-code-reviews.appspot.com/1422811/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Using the Editor framework to edit tasks in the MobileWebApp sample. The DateButton widget is li... (issue1425808)
LGTM http://gwt-code-reviews.appspot.com/1425808/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Adds cache of CollectClassData to make refresh faster. (issue1420809)
http://gwt-code-reviews.appspot.com/1420809/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Adds cache of CollectClassData to make refresh faster. (issue1420809)
http://gwt-code-reviews.appspot.com/1420809/diff/4006/dev/core/src/com/google/gwt/dev/javac/CompiledClass.java File dev/core/src/com/google/gwt/dev/javac/CompiledClass.java (right): http://gwt-code-reviews.appspot.com/1420809/diff/4006/dev/core/src/com/google/gwt/dev/javac/CompiledClass.java#newcode168 dev/core/src/com/google/gwt/dev/javac/CompiledClass.java:168: remove extra n/l http://gwt-code-reviews.appspot.com/1420809/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Adds cache of CollectClassData to make refresh faster. (issue1420809)
http://gwt-code-reviews.appspot.com/1420809/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] [google-web-toolkit] r10086 committed - Fixing setInnerHTML calls on attach/detach sections....
Revision: 10086
Author: her...@google.com
Date: Wed Apr 27 03:02:03 2011
Log: Fixing setInnerHTML calls on attach/detach sections.
Review at http://gwt-code-reviews.appspot.com/1422811
http://code.google.com/p/google-web-toolkit/source/detail?r=10086
Modified:
/trunk/user/src/com/google/gwt/uibinder/elementparsers/WidgetPlaceholderInterpreter.java
/trunk/user/src/com/google/gwt/uibinder/rebind/AbstractFieldWriter.java
===
---
/trunk/user/src/com/google/gwt/uibinder/elementparsers/WidgetPlaceholderInterpreter.java
Tue Apr 26 11:22:45 2011
+++
/trunk/user/src/com/google/gwt/uibinder/elementparsers/WidgetPlaceholderInterpreter.java
Wed Apr 27 03:02:03 2011
@@ -182,14 +182,16 @@
if (uiWriter.useLazyWidgetBuilders()) {
if (idIsHasText.contains(idHolder)) {
-fieldManager.require(childField).addAttachStatement(
-%s.setText(%s.getElementById(%s).getInnerText());,
childField,
-fieldManager.convertFieldToGetter(fieldName),
+fieldManager.require(fieldName).addAttachStatement(
+%s.setText(%s.getElementById(%s).getInnerText());,
+fieldManager.convertFieldToGetter(childField),
+fieldName,
fieldManager.convertFieldToGetter(idHolder));
} else if (idIsHasHTML.contains(idHolder)) {
-fieldManager.require(childField).addAttachStatement(
-%s.setHTML(%s.getElementById(%s).getInnerHTML());,
childField,
-fieldManager.convertFieldToGetter(fieldName),
+fieldManager.require(fieldName).addAttachStatement(
+%s.setHTML(%s.getElementById(%s).getInnerHTML());,
+fieldManager.convertFieldToGetter(childField),
+fieldName,
fieldManager.convertFieldToGetter(idHolder));
}
} else {
===
--- /trunk/user/src/com/google/gwt/uibinder/rebind/AbstractFieldWriter.java
Tue Apr 26 11:22:45 2011
+++ /trunk/user/src/com/google/gwt/uibinder/rebind/AbstractFieldWriter.java
Wed Apr 27 03:02:03 2011
@@ -274,6 +274,13 @@
}
w.newline();
+// If we forced an attach, we should always detach, regardless of
whether
+// there are any detach statements.
+if (attachedVar != null) {
+ w.write(// Detach section.);
+ w.write(%s.detach();, attachedVar);
+}
+
if (detachStatements.size() 0) {
if (isAttachable) {
w.write(%s.detachedInitializationCallback = , getName());
@@ -283,14 +290,12 @@
w.outdent();
w.write(@Override public void execute() {);
w.indent();
- } else if (attachedVar != null) {
-w.write(// Detach section.);
-w.write(%s.detach();, attachedVar);
}
for (String s : detachStatements) {
w.write(s);
}
+
if (isAttachable) {
w.outdent();
w.write(});
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: EnumOrdinalizer cleanup (issue1426804)
I am noticing that while unnecessary casts of an EnumType to (Enum) no longer happen, there is still a cast generated, for the EnumType itself. Is that necessary? E.g., I'm seeing code like this: Fruit fruit = Fruit.APPLE; int i = fruit.ordinal(); ending up in the AST like this (from an AST dump): EntryPoint$Fruit fruit = EntryPoint$Fruit.APPLE; int i = ((EntryPoint$Fruit) fruit).ordinal; Is that extra cast necessary still? I don't think it affects correctness, and I assume it gets optimized away later, but I wonder if we can prevent it from getting inserted at all here. http://gwt-code-reviews.appspot.com/1426804/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Handle SafeHtml as return type in ui:text (issue1409802)
http://gwt-code-reviews.appspot.com/1409802/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Adds some diagnostics to an exception thrown in CompiledClass. (issue1425810)
http://gwt-code-reviews.appspot.com/1425810/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Adds some diagnostics to an exception thrown in CompiledClass. (issue1425810)
http://gwt-code-reviews.appspot.com/1425810/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Handle SafeHtml as return type in ui:text (issue1409802)
http://gwt-code-reviews.appspot.com/1409802/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Adds some diagnostics to an exception thrown in CompiledClass. (issue1425810)
LGTM http://gwt-code-reviews.appspot.com/1425810/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Adds cache of CollectClassData to make refresh faster. (issue1420809)
LGTM http://gwt-code-reviews.appspot.com/1420809/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Add SafeUri type, similar to SafeHtml but for values in a URL attribute context. (issue1380806)
LGTM
http://gwt-code-reviews.appspot.com/1380806/diff/25001/user/src/com/google/gwt/safehtml/shared/UriUtils.java
File user/src/com/google/gwt/safehtml/shared/UriUtils.java (right):
http://gwt-code-reviews.appspot.com/1380806/diff/25001/user/src/com/google/gwt/safehtml/shared/UriUtils.java#newcode172
user/src/com/google/gwt/safehtml/shared/UriUtils.java:172: public static
SafeUri fromTrustedString(String s) {
On 2011/04/18 18:14:21, skybrian wrote:
Could you put some examples of safe and unsafe URLs in the javadoc?
This should
make it more obvious to reviewers of calls to this method what they
should be
looking for.
I think it's a bit more complicated than that. In particular, it
matters where the value came from. For example, we need to consider
data: URIs as inherently dangerous. _however_ a data: URI that's fully
program controlled (e.g., a resource) is considered inherently safe.
I.e. the provenance of the value matters more than what the actual value
looks like.
Which is also why the SafeUri contract (as well as the SafeHtml) has
this vague language about safe from cross site scripting. In
principle, the contract could actually be written to state that
evaluating the URI must not result in execution of script, unless the
script is fully under program control.
I wonder if we should make that change; for instance one might create a
SafeUri object for 'javascript:void(0);'. Which does execute script,
but is harmless because the script is fully program controlled. Which
means it's not cross site scripting.
I wonder if we should specify this in the SafeUri contract at this level
of detail?
http://gwt-code-reviews.appspot.com/1380806/diff/25001/user/src/com/google/gwt/user/client/ui/Image.java
File user/src/com/google/gwt/user/client/ui/Image.java (right):
http://gwt-code-reviews.appspot.com/1380806/diff/25001/user/src/com/google/gwt/user/client/ui/Image.java#newcode144
user/src/com/google/gwt/user/client/ui/Image.java:144: private String
url = null;
On 2011/04/23 14:36:22, tbroyer wrote:
On 2011/04/18 16:22:32, xtof wrote:
I'm still wondering about the change we discussed earlier, of making
this a
SafeUri, and using UriUtils#fromUntrustedString in the Image(String
url)
constructor. You'd have to add getSafeUri to ClippedState (or just
change
getUrl to return the SafeUri -- after all this is a private class so
there
should be only internal uses).
Done.
Changed everything to SafeUri internally, with String-based public
APIs
delegating to SafeUri-based ones, wrapping the String with
fromUntrustedString.
Nice! I'm glad this worked out :)
http://gwt-code-reviews.appspot.com/1380806/diff/29005/user/src/com/google/gwt/safehtml/rebind/SafeHtmlTemplatesImplMethodCreator.java
File
user/src/com/google/gwt/safehtml/rebind/SafeHtmlTemplatesImplMethodCreator.java
(right):
http://gwt-code-reviews.appspot.com/1380806/diff/29005/user/src/com/google/gwt/safehtml/rebind/SafeHtmlTemplatesImplMethodCreator.java#newcode302
user/src/com/google/gwt/safehtml/rebind/SafeHtmlTemplatesImplMethodCreator.java:302:
// TODO(xtof): refactor HtmlContext with isStart/isEnd/isEntire
accessors an simplified type.
an[d] simplified?
http://gwt-code-reviews.appspot.com/1380806/diff/29005/user/src/com/google/gwt/user/client/ui/Image.java
File user/src/com/google/gwt/user/client/ui/Image.java (right):
http://gwt-code-reviews.appspot.com/1380806/diff/29005/user/src/com/google/gwt/user/client/ui/Image.java#newcode301
user/src/com/google/gwt/user/client/ui/Image.java:301: public abstract
void setVisibleRect(Image image, int left, int top, int width, int
height);
Maybe undo this line wrap?
http://gwt-code-reviews.appspot.com/1380806/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: First step of isolating a bunch of code that is used for generating (issue1422807)
LGTM http://gwt-code-reviews.appspot.com/1422807/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] [google-web-toolkit] r10088 committed - Removed unecessary @Override
Revision: 10088
Author: zun...@google.com
Date: Wed Apr 27 07:16:38 2011
Log: Removed unecessary @Override
http://code.google.com/p/google-web-toolkit/source/detail?r=10088
Modified:
/trunk/dev/core/src/com/google/gwt/dev/javac/testing/JavaSource.java
===
--- /trunk/dev/core/src/com/google/gwt/dev/javac/testing/JavaSource.java
Thu Apr 21 07:48:58 2011
+++ /trunk/dev/core/src/com/google/gwt/dev/javac/testing/JavaSource.java
Wed Apr 27 07:16:38 2011
@@ -33,7 +33,6 @@
this.path = Shared.toPath(typeName);
}
- @Override
public String getPath() {
return path;
}
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Using the Editor framework to edit tasks in the MobileWebApp sample. The DateButton widget is li... (issue1425808)
committed as r10087 http://gwt-code-reviews.appspot.com/1425808/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Add SafeUri type, similar to SafeHtml but for values in a URL attribute context. (issue1380806)
LGTM with nits.
http://gwt-code-reviews.appspot.com/1380806/diff/29005/user/src/com/google/gwt/resources/client/ImageResource.java
File user/src/com/google/gwt/resources/client/ImageResource.java
(right):
http://gwt-code-reviews.appspot.com/1380806/diff/29005/user/src/com/google/gwt/resources/client/ImageResource.java#newcode127
user/src/com/google/gwt/resources/client/ImageResource.java:127: String
getURL();
Should this be deprecated?
http://gwt-code-reviews.appspot.com/1380806/diff/29005/user/src/com/google/gwt/resources/rg/DataResourceGenerator.java
File user/src/com/google/gwt/resources/rg/DataResourceGenerator.java
(right):
http://gwt-code-reviews.appspot.com/1380806/diff/29005/user/src/com/google/gwt/resources/rg/DataResourceGenerator.java#newcode68
user/src/com/google/gwt/resources/rg/DataResourceGenerator.java:68:
sw.println(new + DataResourcePrototype.class.getName() + ();
Should this be getCanonicalName()? In this case they are the same, but
in general getName() is not going to return a name you can use in the
source.
If you change, change them all in this change.
http://gwt-code-reviews.appspot.com/1380806/diff/29005/user/src/com/google/gwt/resources/rg/ImageResourceGenerator.java
File user/src/com/google/gwt/resources/rg/ImageResourceGenerator.java
(right):
http://gwt-code-reviews.appspot.com/1380806/diff/29005/user/src/com/google/gwt/resources/rg/ImageResourceGenerator.java#newcode481
user/src/com/google/gwt/resources/rg/ImageResourceGenerator.java:481:
new String[]{bundle.getNormalContentsFieldName(),
bundle.getRtlContentsFieldName()};
space between ] and {
http://gwt-code-reviews.appspot.com/1380806/diff/29005/user/src/com/google/gwt/user/client/ui/Image.java
File user/src/com/google/gwt/user/client/ui/Image.java (right):
http://gwt-code-reviews.appspot.com/1380806/diff/29005/user/src/com/google/gwt/user/client/ui/Image.java#newcode301
user/src/com/google/gwt/user/client/ui/Image.java:301: public abstract
void setVisibleRect(Image image, int left, int top, int width, int
height);
On 2011/04/27 17:18:58, xtof wrote:
Maybe undo this line wrap?
Since we changed to 100 chars, my understanding is each file when edited
should be reformatted, though ideally as a separate change to avoid
obfuscating the real change. In this case, I don't care much either
way.
http://gwt-code-reviews.appspot.com/1380806/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Adding drag and drop support to the mobile web app. The desktop TaskEditView now has a list of t... (issue1420811)
On 2011/04/26 21:10:21, jlabanca wrote: On Tue, Apr 26, 2011 at 4:57 PM, Jeff Larsen mailto:larse...@gmail.com wrote: Drag n Drop doesn't work in ie8 (expected). Perhaps use deferred binding to get rid of the templates portion for all versions of ie9. Otherwise those templates are pretty useless. DragEvent can implement PartialSupport and provide a static isSupported() method so we can check if drag and drop is supported. Awesome. I learn something new every day in this forum. On http://gwt-cloudtasks.appspot.com/ Templates don't show up for Chrome/Firefox 3.5.x. I wish I had time to do a more formal review, but I've got a huge deadline on Tuesday. http://gwt-code-reviews.appspot.com/1420811/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Add ability to include SafeHtml objects in dom based UI's if the laay widget option is being use... (issue1425811)
Reviewers: rjrjr, Description: Add ability to include SafeHtml objects in dom based UI's if the laay widget option is being used (this is the only way that the setters will work correctly) Please review this at http://gwt-code-reviews.appspot.com/1425811/ Affected files: M user/src/com/google/gwt/uibinder/elementparsers/HtmlInterpreter.java M user/src/com/google/gwt/uibinder/rebind/UiBinderWriter.java M user/test/com/google/gwt/uibinder/UiBinderGwtSuite.java M user/test/com/google/gwt/uibinder/elementparsers/AbsolutePanelParserTest.java A user/test/com/google/gwt/uibinder/test/LazyWidgetBuilderTest.gwt.xml A user/test/com/google/gwt/uibinder/test/client/LazyWidgetBuildersTest.DomBasedUi.ui.xml A user/test/com/google/gwt/uibinder/test/client/LazyWidgetBuildersTest.java A user/test/com/google/gwt/uibinder/test/client/SafeHtmlObject.java -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Fix a class of compiler bugs related to staticImpl. (issue1428804)
As long as the JsInliner can still clean it up, I'm fine, otherwise, it
seems like it would introduce some bloat by having trivial delegations. I'm
actually wondering if we should detect the case when a static method ONLY is
referenced by the instance method it was created from via delegation, and
*anti-staticify* it, e.g.
function foo(x) {
foo$(this, x);
}
function foo$(this$static, x) {
this$static.x=x;
}
becomes
function foo(x) {
this.x=x;
}
-Ray
On Mon, Apr 25, 2011 at 6:46 PM, sco...@google.com wrote:
Reviewers: cromwellian, jbrosenberg,
Message:
Ran into this general class of issue... originally I set out to add
staticImpl handling logic to a couple more places, such as
ImplicitUpcastAnalyzer. But the more I thought about it, the more it
struck me as a real wart, and one that's not giving us much value.
Perhaps even negative value by causing code bloat. I think it's much
simpler to just never inline staticImpls into the calling virtual
method.
Please review this at http://gwt-code-reviews.appspot.com/1428804/
Affected files:
M dev/core/src/com/google/gwt/dev/jjs/impl/MethodInliner.java
M dev/core/src/com/google/gwt/dev/jjs/impl/Pruner.java
M
dev/core/src/com/google/gwt/dev/jjs/impl/SameParameterValueOptimizer.java
M dev/core/src/com/google/gwt/dev/jjs/impl/TypeTightener.java
Index: dev/core/src/com/google/gwt/dev/jjs/impl/MethodInliner.java
diff --git a/dev/core/src/com/google/gwt/dev/jjs/impl/MethodInliner.java
b/dev/core/src/com/google/gwt/dev/jjs/impl/MethodInliner.java
index
d7f5f3fecfdb50302c7a723d3d9ead02d30d9e50..92eb2bb156eaee1683049a95472d1d7f9ce09c0d
100644
--- a/dev/core/src/com/google/gwt/dev/jjs/impl/MethodInliner.java
+++ b/dev/core/src/com/google/gwt/dev/jjs/impl/MethodInliner.java
@@ -145,6 +145,20 @@ public class MethodInliner {
@Override
public boolean visit(JMethod x, Context ctx) {
currentMethod = x;
+ if (program.getStaticImpl(x) != null) {
+/*
+ * Never inline a static impl into the calling instance method. We
used
+ * to allow this, and it required all kinds of special logic in
the
+ * optimizers to keep the AST sane. This was because it was
possible to
+ * tighten an instance call to its static impl after the static
impl had
+ * already been inlined, this meant any flow type optimizer
would have
+ * to fake artifical flow from the instance method to the static
impl.
+ *
+ * TODO: allow the inlining if we are the last remaining call
site, and
+ * prune the static impl? But it might tend to generate more code.
+ */
+return false;
+ }
return true;
}
Index: dev/core/src/com/google/gwt/dev/jjs/impl/Pruner.java
diff --git a/dev/core/src/com/google/gwt/dev/jjs/impl/Pruner.java
b/dev/core/src/com/google/gwt/dev/jjs/impl/Pruner.java
index
0ffdcf40faefc2fdc8b3c4ecf293c2ac372d5568..0b67077fe1db9ca11caf48e0fd5be80f15815e0e
100644
--- a/dev/core/src/com/google/gwt/dev/jjs/impl/Pruner.java
+++ b/dev/core/src/com/google/gwt/dev/jjs/impl/Pruner.java
@@ -360,7 +360,8 @@ public class Pruner {
for (int i = 0; i type.getMethods().size(); ++i) {
JMethod method = type.getMethods().get(i);
-if (!methodIsReferenced(method) ||
pruneViaNoninstantiability(isInstantiated, method)) {
+if (!referencedNonTypes.contains(method)
+|| pruneViaNoninstantiability(isInstantiated, method)) {
// Never prune clinit directly out of the class.
if (i 0) {
type.removeMethod(i);
@@ -394,7 +395,7 @@ public class Pruner {
for (int i = 1; i type.getMethods().size(); ++i) {
JMethod method = type.getMethods().get(i);
// all other interface methods are instance and abstract
-if (!isInstantiated || !methodIsReferenced(method)) {
+if (!isInstantiated || !referencedNonTypes.contains(method)) {
type.removeMethod(i);
madeChanges();
--i;
@@ -426,6 +427,8 @@ public class Pruner {
* devirtualizations. If the instance method has been pruned, then
it's
* okay. Also, it's okay on the final pass since no more
* devirtualizations will occur.
+ *
+ * TODO: prune params; MakeCallsStatic smarter to account for it.
*/
JMethod staticImplFor = program.staticImplFor(x);
// Unless the instance method has already been pruned, of course.
@@ -485,31 +488,6 @@ public class Pruner {
return false;
}
-/**
- * Returns codetrue/code if a method is referenced.
- */
-private boolean methodIsReferenced(JMethod method) {
- // Is the method directly referenced?
- if (referencedNonTypes.contains(method)) {
-return true;
- }
-
- /*
- * Special case: if method is the static impl for a live instance
method,
- * don't prune it
[gwt-contrib] Re: Handle SafeHtml as return type in ui:text (issue1409802)
Good news and bad news.
The bad news is that there is much too much copy and paste in this
patch. But the good news is that your task is basically impossible, so
it's not worth trying to fix that. Or is that bad news too? See below.
http://gwt-code-reviews.appspot.com/1409802/diff/12005/user/src/com/google/gwt/uibinder/rebind/FieldReference.java
File user/src/com/google/gwt/uibinder/rebind/FieldReference.java
(right):
http://gwt-code-reviews.appspot.com/1409802/diff/12005/user/src/com/google/gwt/uibinder/rebind/FieldReference.java#newcode51
user/src/com/google/gwt/uibinder/rebind/FieldReference.java:51:
FieldWriter field = fieldManager.lookup(elements[0]);
Oops. This won't work for forward references. That's a problem.
And I've just realized that we're re-implementing
FieldManager.registerFieldReference(String, JType), which is used for
almost exactly this problem: make note of a use of a FieldReference, and
the type that it is expected to be converted to. Later, when all fields
have been seen, they are all checked to see if they actually exist, and
are of the correct type. It is called as a side effect of the parsing
done by calls like consumeStringAttribute, so our effort is very
redundant.
All of this complexity vanishes if we simply stop trying to make ui:text
do double duty and handle both text and html. I am convinced it's just
not worth it. Nor feasible, really.
New proposal:
ui:text accepts String values only. UiTextInterpreter (which I've
realized isn't the hack I said it was) stays almost exactly as it was.
It stays downstream of ComputedAttributeInterpreter (not even sure that
matters, actually), but it starts verifying attribute#hasComputedValue,
as it should have all along.
Introduce ui:safeHtml from=/, which is just like ui:text but calls
consumeSafeHtmlAttribute instead of consumeStringAttribute. Make the
users choose rather than trying to add overloads to the uibinder
language. The new UiSafeHtmlInterpreter is used in addition to
UiTextInterpreter in HtmlInterpreter's pipeline.
I believe most of the other changes in the patch just won't be needed.
http://gwt-code-reviews.appspot.com/1409802/diff/12005/user/src/com/google/gwt/uibinder/rebind/FieldReference.java#newcode52
user/src/com/google/gwt/uibinder/rebind/FieldReference.java:52: if
(field == null) {
Oops. This null check shows us that this won't work for forward
references. That's a problem.
And I've just realized that we're re-implementing
FieldManager.registerFieldReference(String, JType), which is used for
almost exactly this problem: make note of a use of a FieldReference, and
the type that it is expected to be converted to. Later, when all fields
have been seen, they are all checked to see if they actually exist, and
are of the correct type. It is called as a side effect of the parsing
done by calls like consumeStringAttribute, so our effort is very
redundant.
All of this complexity vanishes if we simply stop trying to make ui:text
do double duty and handle both text and html. I am convinced it's just
not worth it. Nor feasible, really.
New proposal:
ui:text accepts String values only. UiTextInterpreter (which I've
realized isn't the hack I said it was) stays almost exactly as it was.
It stays downstream of ComputedAttributeInterpreter (not even sure that
matters, actually), but it starts verifying attribute#hasComputedValue,
as it should have all along.
Introduce ui:safeHtml from=/, which is just like ui:text but calls
consumeSafeHtmlAttribute instead of consumeStringAttribute. Make the
users choose rather than trying to add overloads to the uibinder
language. The new UiSafeHtmlInterpreter is used in addition to
UiTextInterpreter in HtmlInterpreter's pipeline.
I believe ComputedAttributeInterpreter won't need to change at all.
http://gwt-code-reviews.appspot.com/1409802/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Fix a class of compiler bugs related to staticImpl. (issue1428804)
If the Java MethodInliner kept call counts, I would have special-cased it to
allow inlining the static into the instance when it's the only caller. But
since call counts aren't already computed, I decided it would be best to try
it out as is and see if it actually increases code size in practice.
On Wed, Apr 27, 2011 at 2:20 PM, Ray Cromwell cromwell...@google.comwrote:
As long as the JsInliner can still clean it up, I'm fine, otherwise, it
seems like it would introduce some bloat by having trivial delegations. I'm
actually wondering if we should detect the case when a static method ONLY is
referenced by the instance method it was created from via delegation, and
*anti-staticify* it, e.g.
function foo(x) {
foo$(this, x);
}
function foo$(this$static, x) {
this$static.x=x;
}
becomes
function foo(x) {
this.x=x;
}
-Ray
On Mon, Apr 25, 2011 at 6:46 PM, sco...@google.com wrote:
Reviewers: cromwellian, jbrosenberg,
Message:
Ran into this general class of issue... originally I set out to add
staticImpl handling logic to a couple more places, such as
ImplicitUpcastAnalyzer. But the more I thought about it, the more it
struck me as a real wart, and one that's not giving us much value.
Perhaps even negative value by causing code bloat. I think it's much
simpler to just never inline staticImpls into the calling virtual
method.
Please review this at http://gwt-code-reviews.appspot.com/1428804/
Affected files:
M dev/core/src/com/google/gwt/dev/jjs/impl/MethodInliner.java
M dev/core/src/com/google/gwt/dev/jjs/impl/Pruner.java
M
dev/core/src/com/google/gwt/dev/jjs/impl/SameParameterValueOptimizer.java
M dev/core/src/com/google/gwt/dev/jjs/impl/TypeTightener.java
Index: dev/core/src/com/google/gwt/dev/jjs/impl/MethodInliner.java
diff --git a/dev/core/src/com/google/gwt/dev/jjs/impl/MethodInliner.java
b/dev/core/src/com/google/gwt/dev/jjs/impl/MethodInliner.java
index
d7f5f3fecfdb50302c7a723d3d9ead02d30d9e50..92eb2bb156eaee1683049a95472d1d7f9ce09c0d
100644
--- a/dev/core/src/com/google/gwt/dev/jjs/impl/MethodInliner.java
+++ b/dev/core/src/com/google/gwt/dev/jjs/impl/MethodInliner.java
@@ -145,6 +145,20 @@ public class MethodInliner {
@Override
public boolean visit(JMethod x, Context ctx) {
currentMethod = x;
+ if (program.getStaticImpl(x) != null) {
+/*
+ * Never inline a static impl into the calling instance method.
We used
+ * to allow this, and it required all kinds of special logic in
the
+ * optimizers to keep the AST sane. This was because it was
possible to
+ * tighten an instance call to its static impl after the static
impl had
+ * already been inlined, this meant any flow type optimizer
would have
+ * to fake artifical flow from the instance method to the static
impl.
+ *
+ * TODO: allow the inlining if we are the last remaining call
site, and
+ * prune the static impl? But it might tend to generate more
code.
+ */
+return false;
+ }
return true;
}
Index: dev/core/src/com/google/gwt/dev/jjs/impl/Pruner.java
diff --git a/dev/core/src/com/google/gwt/dev/jjs/impl/Pruner.java
b/dev/core/src/com/google/gwt/dev/jjs/impl/Pruner.java
index
0ffdcf40faefc2fdc8b3c4ecf293c2ac372d5568..0b67077fe1db9ca11caf48e0fd5be80f15815e0e
100644
--- a/dev/core/src/com/google/gwt/dev/jjs/impl/Pruner.java
+++ b/dev/core/src/com/google/gwt/dev/jjs/impl/Pruner.java
@@ -360,7 +360,8 @@ public class Pruner {
for (int i = 0; i type.getMethods().size(); ++i) {
JMethod method = type.getMethods().get(i);
-if (!methodIsReferenced(method) ||
pruneViaNoninstantiability(isInstantiated, method)) {
+if (!referencedNonTypes.contains(method)
+|| pruneViaNoninstantiability(isInstantiated, method)) {
// Never prune clinit directly out of the class.
if (i 0) {
type.removeMethod(i);
@@ -394,7 +395,7 @@ public class Pruner {
for (int i = 1; i type.getMethods().size(); ++i) {
JMethod method = type.getMethods().get(i);
// all other interface methods are instance and abstract
-if (!isInstantiated || !methodIsReferenced(method)) {
+if (!isInstantiated || !referencedNonTypes.contains(method)) {
type.removeMethod(i);
madeChanges();
--i;
@@ -426,6 +427,8 @@ public class Pruner {
* devirtualizations. If the instance method has been pruned, then
it's
* okay. Also, it's okay on the final pass since no more
* devirtualizations will occur.
+ *
+ * TODO: prune params; MakeCallsStatic smarter to account for it.
*/
JMethod staticImplFor = program.staticImplFor(x);
// Unless the instance method has already been pruned, of course.
@@ -485,31 +488,6 @@ public class Pruner {
return false;
}
Re: [gwt-contrib] Re: Fix a class of compiler bugs related to staticImpl. (issue1428804)
On Wed, Apr 27, 2011 at 2:57 PM, Scott Blum sco...@google.com wrote: If the Java MethodInliner kept call counts, I would have special-cased it to allow inlining the static into the instance when it's the only caller. But since call counts aren't already computed, I decided it would be best to try it out as is and see if it actually increases code size in practice. Some of the design of the i18n classes assumes that most of these relay methods go away due to inlining (and in particular, how migration of client-shared is done adds lots of single-call relay methods). -- John A. Tamplin Software Engineer (GWT), Google -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Fix a class of compiler bugs related to staticImpl. (issue1428804)
If I understand correctly, part of staticification is that all call-sites to myVar.foo(x) will be replaced by $foo(myVar, x), and thus foo(x) will be unused, and thus pruned. So, the delegation issues goes away, no? http://gwt-code-reviews.appspot.com/1428804/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Handle SafeHtml as return type in ui:text (issue1409802)
This (unfortunately) makes perfect sense:) On the bright side,
implementing a new tag will be much easier than getting ui:text to do
double duty. One note on your comments: UiTextInterpreter can't verify
attribute#hasComputedValue if it runs downstream of
ComputedAttributeInterpreter, because the computed value has already
been replaced at this point and the check will always be false.
Otherwise, this plan works.
On 2011/04/27 18:42:16, rjrjr wrote:
Good news and bad news.
The bad news is that there is much too much copy and paste in this
patch. But
the good news is that your task is basically impossible, so it's not
worth
trying to fix that. Or is that bad news too? See below.
http://gwt-code-reviews.appspot.com/1409802/diff/12005/user/src/com/google/gwt/uibinder/rebind/FieldReference.java
File user/src/com/google/gwt/uibinder/rebind/FieldReference.java
(right):
http://gwt-code-reviews.appspot.com/1409802/diff/12005/user/src/com/google/gwt/uibinder/rebind/FieldReference.java#newcode51
user/src/com/google/gwt/uibinder/rebind/FieldReference.java:51:
FieldWriter
field = fieldManager.lookup(elements[0]);
Oops. This won't work for forward references. That's a problem.
And I've just realized that we're re-implementing
FieldManager.registerFieldReference(String, JType), which is used for
almost
exactly this problem: make note of a use of a FieldReference, and the
type that
it is expected to be converted to. Later, when all fields have been
seen, they
are all checked to see if they actually exist, and are of the correct
type. It
is called as a side effect of the parsing done by calls like
consumeStringAttribute, so our effort is very redundant.
All of this complexity vanishes if we simply stop trying to make
ui:text do
double duty and handle both text and html. I am convinced it's just
not worth
it. Nor feasible, really.
New proposal:
ui:text accepts String values only. UiTextInterpreter (which I've
realized
isn't the hack I said it was) stays almost exactly as it was. It stays
downstream of ComputedAttributeInterpreter (not even sure that
matters,
actually), but it starts verifying attribute#hasComputedValue, as it
should have
all along.
Introduce ui:safeHtml from=/, which is just like ui:text but calls
consumeSafeHtmlAttribute instead of consumeStringAttribute. Make the
users
choose rather than trying to add overloads to the uibinder language.
The new
UiSafeHtmlInterpreter is used in addition to UiTextInterpreter in
HtmlInterpreter's pipeline.
I believe most of the other changes in the patch just won't be needed.
http://gwt-code-reviews.appspot.com/1409802/diff/12005/user/src/com/google/gwt/uibinder/rebind/FieldReference.java#newcode52
user/src/com/google/gwt/uibinder/rebind/FieldReference.java:52: if
(field ==
null) {
Oops. This null check shows us that this won't work for forward
references.
That's a problem.
And I've just realized that we're re-implementing
FieldManager.registerFieldReference(String, JType), which is used for
almost
exactly this problem: make note of a use of a FieldReference, and the
type that
it is expected to be converted to. Later, when all fields have been
seen, they
are all checked to see if they actually exist, and are of the correct
type. It
is called as a side effect of the parsing done by calls like
consumeStringAttribute, so our effort is very redundant.
All of this complexity vanishes if we simply stop trying to make
ui:text do
double duty and handle both text and html. I am convinced it's just
not worth
it. Nor feasible, really.
New proposal:
ui:text accepts String values only. UiTextInterpreter (which I've
realized
isn't the hack I said it was) stays almost exactly as it was. It stays
downstream of ComputedAttributeInterpreter (not even sure that
matters,
actually), but it starts verifying attribute#hasComputedValue, as it
should have
all along.
Introduce ui:safeHtml from=/, which is just like ui:text but calls
consumeSafeHtmlAttribute instead of consumeStringAttribute. Make the
users
choose rather than trying to add overloads to the uibinder language.
The new
UiSafeHtmlInterpreter is used in addition to UiTextInterpreter in
HtmlInterpreter's pipeline.
I believe ComputedAttributeInterpreter won't need to change at all.
http://gwt-code-reviews.appspot.com/1409802/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Add ability to include SafeHtml objects in dom based UI's if the laay widget option is being use... (issue1425811)
http://gwt-code-reviews.appspot.com/1425811/diff/1/user/src/com/google/gwt/uibinder/elementparsers/HtmlInterpreter.java File user/src/com/google/gwt/uibinder/elementparsers/HtmlInterpreter.java (right): http://gwt-code-reviews.appspot.com/1425811/diff/1/user/src/com/google/gwt/uibinder/elementparsers/HtmlInterpreter.java#newcode91 user/src/com/google/gwt/uibinder/elementparsers/HtmlInterpreter.java:91: return writer.tokenForSafeHtmlExpression( I think instead you want to use the tokenForExpression method that Rafa just added. http://gwt-code-reviews.appspot.com/1425811/diff/1/user/test/com/google/gwt/uibinder/elementparsers/AbsolutePanelParserTest.java File user/test/com/google/gwt/uibinder/elementparsers/AbsolutePanelParserTest.java (right): http://gwt-code-reviews.appspot.com/1425811/diff/1/user/test/com/google/gwt/uibinder/elementparsers/AbsolutePanelParserTest.java#newcode98 user/test/com/google/gwt/uibinder/elementparsers/AbsolutePanelParserTest.java:98: b.append( g:Label/); This is a bit disturbing. Why did it fix it? Or is the real question why did it pass in the first place? http://gwt-code-reviews.appspot.com/1425811/diff/1/user/test/com/google/gwt/uibinder/test/LazyWidgetBuilderTest.gwt.xml File user/test/com/google/gwt/uibinder/test/LazyWidgetBuilderTest.gwt.xml (right): http://gwt-code-reviews.appspot.com/1425811/diff/1/user/test/com/google/gwt/uibinder/test/LazyWidgetBuilderTest.gwt.xml#newcode18 user/test/com/google/gwt/uibinder/test/LazyWidgetBuilderTest.gwt.xml:18: entry-point class=com.google.gwt.uibinder.test.client.LazyWidgetBuilderTest / I don't think you need the entry-point http://gwt-code-reviews.appspot.com/1425811/diff/1/user/test/com/google/gwt/uibinder/test/client/LazyWidgetBuildersTest.java File user/test/com/google/gwt/uibinder/test/client/LazyWidgetBuildersTest.java (right): http://gwt-code-reviews.appspot.com/1425811/diff/1/user/test/com/google/gwt/uibinder/test/client/LazyWidgetBuildersTest.java#newcode52 user/test/com/google/gwt/uibinder/test/client/LazyWidgetBuildersTest.java:52: assertEquals(Hello Bob, domUi.div.getInnerHTML()); check for the b, make sure it didn't get escaped. http://gwt-code-reviews.appspot.com/1425811/diff/1/user/test/com/google/gwt/uibinder/test/client/SafeHtmlObject.java File user/test/com/google/gwt/uibinder/test/client/SafeHtmlObject.java (right): http://gwt-code-reviews.appspot.com/1425811/diff/1/user/test/com/google/gwt/uibinder/test/client/SafeHtmlObject.java#newcode28 user/test/com/google/gwt/uibinder/test/client/SafeHtmlObject.java:28: return Hello + name; Should put some markup in here, so we can test for improper escaping. Hello b + name + /b. http://gwt-code-reviews.appspot.com/1425811/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] [google-web-toolkit] r10089 committed - Adds cache of CollectClassData to make refresh faster....
Revision: 10089
Author: scheg...@google.com
Date: Wed Apr 27 09:34:06 2011
Log: Adds cache of CollectClassData to make refresh faster.
This gives about 10% performance gain on big projects.
Review at http://gwt-code-reviews.appspot.com/1420809
Review by: sco...@google.com
http://code.google.com/p/google-web-toolkit/source/detail?r=10089
Modified:
/trunk/dev/core/src/com/google/gwt/dev/javac/CompiledClass.java
/trunk/dev/core/src/com/google/gwt/dev/javac/TypeOracleMediator.java
/trunk/dev/core/src/com/google/gwt/dev/javac/TypeOracleMediatorFromSource.java
===
--- /trunk/dev/core/src/com/google/gwt/dev/javac/CompiledClass.java Tue Apr
26 07:38:15 2011
+++ /trunk/dev/core/src/com/google/gwt/dev/javac/CompiledClass.java Wed Apr
27 09:34:06 2011
@@ -15,6 +15,7 @@
*/
package com.google.gwt.dev.javac;
+import com.google.gwt.dev.javac.TypeOracleMediator.TypeData;
import com.google.gwt.dev.util.DiskCache;
import com.google.gwt.dev.util.StringInterner;
import com.google.gwt.dev.util.Name.InternalName;
@@ -38,6 +39,7 @@
private final CompiledClass enclosingClass;
private final String internalName;
private final boolean isLocal;
+ private transient TypeData typeData;
private CompilationUnit unit;
private String signatureHash;
@@ -109,6 +111,15 @@
public String getSourceName() {
return
StringInterner.get().intern(InternalName.toSourceName(internalName));
}
+
+ public TypeData getTypeData() {
+if (typeData == null) {
+ typeData =
+ new TypeData(getPackageName(), getSourceName(),
getInternalName(), null, getBytes(),
+ getUnit().getLastModified());
+}
+return typeData;
+ }
public CompilationUnit getUnit() {
return unit;
===
--- /trunk/dev/core/src/com/google/gwt/dev/javac/TypeOracleMediator.java
Tue Apr 19 07:37:00 2011
+++ /trunk/dev/core/src/com/google/gwt/dev/javac/TypeOracleMediator.java
Wed Apr 27 09:34:06 2011
@@ -84,6 +84,11 @@
* Bytecode from compiled Java source.
*/
private final byte[] byteCode;
+
+/**
+ * Prepared information about this class.
+ */
+private CollectClassData classData;
/**
* See {@link Type#getInternalName()}.
@@ -125,6 +130,24 @@
this.byteCode = classBytes;
this.lastModifiedTime = lastModifiedTime;
}
+
+/**
+ * Collects data about a class which only needs the bytecode and no
TypeOracle
+ * data structures. This is used to make the initial shallow identity
pass for
+ * creating JRealClassType/JGenericType objects.
+ */
+synchronized CollectClassData getCollectClassData() {
+ if (classData == null) {
+ClassReader reader = new ClassReader(byteCode);
+classData = new CollectClassData();
+ClassVisitor cv = classData;
+if (TRACE_CLASSES) {
+ cv = new TraceClassVisitor(cv, new PrintWriter(System.out));
+}
+reader.accept(cv, 0);
+ }
+ return classData;
+}
}
/**
@@ -364,7 +387,7 @@
TypeOracleBuildContext context = new
TypeOracleBuildContext(argsLookup);
for (TypeData typeData : typeDataList) {
- CollectClassData cv = processClass(typeData);
+ CollectClassData cv = typeData.getCollectClassData();
// skip any classes that can't be referenced by name outside of
// their local scope, such as anonymous classes and method-local
classes
if (!cv.hasNoExternalName()) {
@@ -455,7 +478,8 @@
private Annotation createAnnotation(TreeLogger logger,
Class? extends Annotation annotationClass, AnnotationData
annotData) {
-MapString, Object values = annotData.getValues();
+// Make a copy before we mutate the collection.
+MapString, Object values = new HashMapString,
Object(annotData.getValues());
for (Map.EntryString, Object entry : values.entrySet()) {
Method method = null;
Throwable caught = null;
@@ -611,22 +635,6 @@
}
return output;
}
-
- /**
- * Collects data about a class which only needs the bytecode and no
TypeOracle
- * data structures. This is used to make the initial shallow identity
pass for
- * creating JRealClassType/JGenericType objects.
- */
- private CollectClassData processClass(TypeData typeData) {
-ClassReader reader = new ClassReader(typeData.byteCode);
-CollectClassData mcv = new CollectClassData();
-ClassVisitor cv = mcv;
-if (TRACE_CLASSES) {
- cv = new TraceClassVisitor(cv, new PrintWriter(System.out));
-}
-reader.accept(cv, 0);
-return mcv;
- }
private boolean resolveAnnotation(TreeLogger logger,
CollectAnnotationData annotVisitor,
===
---
/trunk/dev/core/src/com/google/gwt/dev/javac/TypeOracleMediatorFromSource.java
Wed Dec 15 07:54:33 2010
+++
/trunk/dev/core/src/com/google/gwt/dev/javac/TypeOracleMediatorFromSource.java
[gwt-contrib] Remove FieldWriter.setAttachable() and find out automatically whether the callbacks should be cr... (issue1421807)
Reviewers: rjrjr,
Description:
Remove FieldWriter.setAttachable() and find out automatically whether
the callbacks should be created.
Review by: rj...@google.com
Please review this at http://gwt-code-reviews.appspot.com/1421807/
Affected files:
M
user/src/com/google/gwt/uibinder/elementparsers/AttachableHTMLPanelParser.java
M user/src/com/google/gwt/uibinder/rebind/AbstractFieldWriter.java
M user/src/com/google/gwt/uibinder/rebind/FieldManager.java
M user/src/com/google/gwt/uibinder/rebind/FieldWriter.java
Index:
user/src/com/google/gwt/uibinder/elementparsers/AttachableHTMLPanelParser.java
===
---
user/src/com/google/gwt/uibinder/elementparsers/AttachableHTMLPanelParser.java
(revision 10089)
+++
user/src/com/google/gwt/uibinder/elementparsers/AttachableHTMLPanelParser.java
(working copy)
@@ -32,8 +32,6 @@
final UiBinderWriter writer) throws UnableToCompleteException {
assert writer.useLazyWidgetBuilders();
-writer.getFieldManager().lookup(fieldName)
-.setAttachable(true);
/*
* Gathers up elements that indicate nested Attachable objects.
Index: user/src/com/google/gwt/uibinder/rebind/AbstractFieldWriter.java
===
--- user/src/com/google/gwt/uibinder/rebind/AbstractFieldWriter.java
(revision 10089)
+++ user/src/com/google/gwt/uibinder/rebind/AbstractFieldWriter.java
(working copy)
@@ -23,6 +23,7 @@
import com.google.gwt.core.ext.typeinfo.TypeOracle;
import com.google.gwt.dom.client.Element;
import com.google.gwt.uibinder.rebind.model.OwnerField;
+import com.google.gwt.user.client.ui.AttachableHTMLPanel;
import java.util.ArrayList;
import java.util.Arrays;
@@ -58,7 +59,6 @@
private boolean written;
private int buildPrecedence;
private MortalLogger logger;
- private boolean isAttachable;
public AbstractFieldWriter(String name, MortalLogger logger) {
if (name == null) {
@@ -110,11 +110,6 @@
public void needs(FieldWriter f) {
needs.add(f);
- }
-
- @Override
- public void setAttachable(boolean attachable) {
-this.isAttachable = attachable;
}
@Override
@@ -183,8 +178,15 @@
@Override
public void writeFieldDefinition(IndentedWriter w, TypeOracle typeOracle,
- OwnerField ownerField, DesignTimeUtils designTime, int getterCount)
+ OwnerField ownerField, DesignTimeUtils designTime, int getterCount,
+ boolean useLazyWidgetBuilders)
throws UnableToCompleteException {
+
+JClassType attachableHTMLPanelType = typeOracle.findType(
+AttachableHTMLPanel.class.getName());
+boolean outputAttachDetachCallbacks = useLazyWidgetBuilders
+ getAssignableType() != null
+ getAssignableType().isAssignableTo(attachableHTMLPanelType);
// Check initializer.
if (initializer == null) {
@@ -238,7 +240,7 @@
if (attachStatements.size() 0) {
w.newline();
w.write(// Attach section.);
- if (isAttachable) {
+ if (outputAttachDetachCallbacks) {
// TODO(rdcastro): This is too coupled with AttachableHTMLPanel.
// Make this nicer.
w.write(%s.wrapInitializationCallback = , getName());
@@ -265,7 +267,7 @@
w.write(s);
}
- if (isAttachable) {
+ if (outputAttachDetachCallbacks) {
w.outdent();
w.write(});
w.outdent();
@@ -282,7 +284,7 @@
}
if (detachStatements.size() 0) {
- if (isAttachable) {
+ if (outputAttachDetachCallbacks) {
w.write(%s.detachedInitializationCallback = , getName());
w.indent();
w.indent();
@@ -296,7 +298,7 @@
w.write(s);
}
- if (isAttachable) {
+ if (outputAttachDetachCallbacks) {
w.outdent();
w.write(});
w.outdent();
Index: user/src/com/google/gwt/uibinder/rebind/FieldManager.java
===
--- user/src/com/google/gwt/uibinder/rebind/FieldManager.java (revision
10089)
+++ user/src/com/google/gwt/uibinder/rebind/FieldManager.java (working copy)
@@ -287,8 +287,13 @@
CollectionFieldWriter fields = fieldsMap.values();
for (FieldWriter field : fields) {
int counter = getGetterCounter(field.getName());
- field.writeFieldDefinition(writer, typeOracle,
- ownerClass.getUiField(field.getName()), designTime, counter);
+ field.writeFieldDefinition(
+ writer,
+ typeOracle,
+ ownerClass.getUiField(field.getName()),
+ designTime,
+ counter,
+ useLazyWidgetBuilders);
}
}
Index: user/src/com/google/gwt/uibinder/rebind/FieldWriter.java
===
--- user/src/com/google/gwt/uibinder/rebind/FieldWriter.java (revision
10089)
+++ user/src/com/google/gwt/uibinder/rebind/FieldWriter.java(working copy)
@@ -126,9
[gwt-contrib] Re: Add SafeUri type, similar to SafeHtml but for values in a URL attribute context. (issue1380806)
http://gwt-code-reviews.appspot.com/1380806/diff/25001/user/src/com/google/gwt/safehtml/shared/UriUtils.java
File user/src/com/google/gwt/safehtml/shared/UriUtils.java (right):
http://gwt-code-reviews.appspot.com/1380806/diff/25001/user/src/com/google/gwt/safehtml/shared/UriUtils.java#newcode172
user/src/com/google/gwt/safehtml/shared/UriUtils.java:172: public static
SafeUri fromTrustedString(String s) {
On 2011/04/27 17:18:58, xtof wrote:
I think it's a bit more complicated than that. In particular, it
matters where
the value came from. For example, we need to consider data: URIs as
inherently
dangerous. _however_ a data: URI that's fully program controlled
(e.g., a
resource) is considered inherently safe.
I.e. the provenance of the value matters more than what the actual
value looks
like.
Which is also why the SafeUri contract (as well as the SafeHtml) has
this vague
language about safe from cross site scripting. In principle, the
contract
could actually be written to state that evaluating the URI must not
result in
execution of script, unless the script is fully under program control.
I wonder if we should make that change; for instance one might create
a SafeUri
object for 'javascript:void(0);'. Which does execute script, but is
harmless
because the script is fully program controlled. Which means it's not
cross
site scripting.
I wonder if we should specify this in the SafeUri contract at this
level of
detail?
Hmm. We could start by saying in the SafeUri contract what you just
said: provenance matters and SafeUri could contain any type of URL
provided that it's hard-coded by the app. For untrusted strings, we can
refer people to EscapeUtils.isSafeUri, which has a list of safe URL
schemes.
From a reviewer's standpoint, I believe that if some code implementing
the SafeUri interface only constructs URL's for the schemes listed in
isSafeUri() then I know it's okay, and if not, I'd better get a real
security review from someone who knows web security better.
http://gwt-code-reviews.appspot.com/1380806/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Fix a class of compiler bugs related to staticImpl. (issue1428804)
It looks like there might be some logic in ControlFlowAnalyzer.RescueVisitor.Rescue, which can also be removed if inlining staticImpl's is not an issue. http://gwt-code-reviews.appspot.com/1428804/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Discards the jar file name in the resource location. It isn't necessary, and (issue1428805)
Reviewers: tobyr, jbrosenberg,
Description:
Discards the jar file name in the resource location. It isn't
necessary, and
will cause detritus to build up in the cache if a resource changes from
being
in one jar to another or moves in/out of a .jar file.
Please review this at http://gwt-code-reviews.appspot.com/1428805/
Affected files:
M dev/core/src/com/google/gwt/dev/javac/MemoryUnitCache.java
M dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java
Index: dev/core/src/com/google/gwt/dev/javac/MemoryUnitCache.java
===
--- dev/core/src/com/google/gwt/dev/javac/MemoryUnitCache.java (revision
10088)
+++ dev/core/src/com/google/gwt/dev/javac/MemoryUnitCache.java (working
copy)
@@ -20,8 +20,13 @@
import org.apache.commons.collections.map.AbstractReferenceMap;
import org.apache.commons.collections.map.ReferenceMap;
+import java.io.IOException;
+import java.net.JarURLConnection;
+import java.net.MalformedURLException;
+import java.net.URL;
import java.util.Collections;
import java.util.Map;
+import java.util.jar.JarEntry;
/**
* This cache stores {@link CompilationUnit} instances in a Map.
@@ -91,12 +96,12 @@
*/
public void add(CompilationUnit newUnit) {
UnitCacheEntry newEntry = new UnitCacheEntry(newUnit,
UnitOrigin.RUN_TIME);
-String resourceLocation = newUnit.getResourceLocation();
-UnitCacheEntry oldEntry = unitMap.get(resourceLocation);
+String normalizedResourceLocation =
normalizeResourceLocation(newUnit.getResourceLocation());
+UnitCacheEntry oldEntry = unitMap.get(normalizedResourceLocation);
if (oldEntry != null) {
remove(oldEntry.getUnit());
}
-unitMap.put(resourceLocation, newEntry);
+unitMap.put(normalizedResourceLocation, newEntry);
unitMapByContentId.put(newUnit.getContentId(), newEntry);
}
@@ -116,7 +121,7 @@
}
public CompilationUnit find(String resourceLocation) {
-UnitCacheEntry entry = unitMap.get(resourceLocation);
+UnitCacheEntry entry =
unitMap.get(normalizeResourceLocation(resourceLocation));
if (entry != null) {
return entry.getUnit();
}
@@ -124,7 +129,33 @@
}
public void remove(CompilationUnit unit) {
-unitMap.remove(unit.getResourceLocation());
+unitMap.remove(normalizeResourceLocation(unit.getResourceLocation()));
unitMapByContentId.remove(unit.getContentId());
}
+
+ /**
+ * Create a key for the cache by resourceLocation.
+ *
+ * Strip off any jar file prefix from the location before storing as a
key or
+ * querying the cache.
+ *
+ * @param resourceLocation full URL resource location (may include jar:
+ * prefix).
+ * @return resourceLocation stripped of jar: prefix.
+ */
+ protected String normalizeResourceLocation(String resourceLocation) {
+try {
+ URL url = new URL(resourceLocation);
+ if (url.getProtocol().equals(jar)) {
+JarURLConnection connection = (JarURLConnection)
url.openConnection();
+JarEntry entry = connection.getJarEntry();
+if (entry != null) {
+ return entry.getName();
+}
+ }
+} catch (MalformedURLException ignored) {
+} catch (IOException ignored) {
+}
+return resourceLocation;
+ }
}
Index: dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java
===
--- dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java
(revision 10088)
+++ dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java (working
copy)
@@ -346,7 +346,8 @@
unitCacheMapLoader.await();
super.add(newUnit);
addCount.getAndIncrement();
-unitWriteQueue.add(new
UnitWriteMessage(unitMap.get(newUnit.getResourceLocation(;
+String normalizedResourceLocation =
normalizeResourceLocation(newUnit.getResourceLocation());
+unitWriteQueue.add(new
UnitWriteMessage(unitMap.get(normalizedResourceLocation)));
}
/**
@@ -491,13 +492,14 @@
break;
}
UnitCacheEntry entry = new UnitCacheEntry(unit,
UnitOrigin.PERSISTENT);
- UnitCacheEntry oldEntry =
unitMap.get(unit.getResourceLocation());
+ String normalizedResourceLocation =
normalizeResourceLocation(unit.getResourceLocation());
+ UnitCacheEntry oldEntry =
unitMap.get(normalizedResourceLocation);
if (oldEntry != null unit.getLastModified()
oldEntry.getUnit().getLastModified()) {
super.remove(oldEntry.getUnit());
-unitMap.put(unit.getResourceLocation(), entry);
+unitMap.put(normalizedResourceLocation, entry);
unitMapByContentId.put(unit.getContentId(), entry);
} else if (oldEntry == null) {
-unitMap.put(unit.getResourceLocation(), entry);
+unitMap.put(normalizedResourceLocation, entry);
[gwt-contrib] Re: Fix a class of compiler bugs related to staticImpl. (issue1428804)
I see now (looking at MakeCallsStatic.RewriteCallSites) that not all call sites get replaced, there are a few edge cases, relating to split points, etc., where the call sites are not replacedBut I'm guessing it won't be a large number of cases. http://gwt-code-reviews.appspot.com/1428804/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Fix a class of compiler bugs related to staticImpl. (issue1428804)
On Wed, Apr 27, 2011 at 3:20 PM, jbrosenb...@google.com wrote: If I understand correctly, part of staticification is that all call-sites to myVar.foo(x) will be replaced by $foo(myVar, x), and thus foo(x) will be unused, and thus pruned. So, the delegation issues goes away, no? Yes, as long as all call sites can be devirtualized, the instance method goes away. On Wed, Apr 27, 2011 at 3:51 PM, jbrosenb...@google.com wrote: It looks like there might be some logic in ControlFlowAnalyzer.RescueVisitor.Rescue, which can also be removed if inlining staticImpl's is not an issue. Whoops! Yes, I need to remove that, my bad. On Wed, Apr 27, 2011 at 4:34 PM, jbrosenb...@google.com wrote: I see now (looking at MakeCallsStatic.RewriteCallSites) that not all call sites get replaced, there are a few edge cases, relating to split points, etc., where the call sites are not replacedBut I'm guessing it won't be a large number of cases. Yes, there will be cases where both the instance and static method make it into the final output. In some of those cases, JsInliner will actually do the inlining. In others, my theory is that not inlining them won't have a negative effect on code size. -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Remove FieldWriter.setAttachable() and find out automatically whether the callbacks should be cr... (issue1421807)
LGTM http://gwt-code-reviews.appspot.com/1421807/diff/1/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/1421807/diff/1/user/src/com/google/gwt/uibinder/rebind/AbstractFieldWriter.java#newcode189 user/src/com/google/gwt/uibinder/rebind/AbstractFieldWriter.java:189: getAssignableType().isAssignableTo(attachableHTMLPanelType); Ha! http://gwt-code-reviews.appspot.com/1421807/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Fix a class of compiler bugs related to staticImpl. (issue1428804)
New patch, removed the unneeded code from CFA. http://gwt-code-reviews.appspot.com/1428804/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Fix a class of compiler bugs related to staticImpl. (issue1428804)
LGTM http://gwt-code-reviews.appspot.com/1428804/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Add ability to include SafeHtml objects in dom based UI's if the laay widget option is being use... (issue1425811)
http://gwt-code-reviews.appspot.com/1425811/diff/1/user/src/com/google/gwt/uibinder/elementparsers/HtmlInterpreter.java File user/src/com/google/gwt/uibinder/elementparsers/HtmlInterpreter.java (right): http://gwt-code-reviews.appspot.com/1425811/diff/1/user/src/com/google/gwt/uibinder/elementparsers/HtmlInterpreter.java#newcode91 user/src/com/google/gwt/uibinder/elementparsers/HtmlInterpreter.java:91: return writer.tokenForSafeHtmlExpression( On 2011/04/27 19:29:58, rjrjr wrote: I think instead you want to use the tokenForExpression method that Rafa just added. Done. http://gwt-code-reviews.appspot.com/1425811/diff/1/user/test/com/google/gwt/uibinder/elementparsers/AbsolutePanelParserTest.java File user/test/com/google/gwt/uibinder/elementparsers/AbsolutePanelParserTest.java (right): http://gwt-code-reviews.appspot.com/1425811/diff/1/user/test/com/google/gwt/uibinder/elementparsers/AbsolutePanelParserTest.java#newcode98 user/test/com/google/gwt/uibinder/elementparsers/AbsolutePanelParserTest.java:98: b.append( g:Label/); On 2011/04/27 19:29:58, rjrjr wrote: This is a bit disturbing. Why did it fix it? Or is the real question why did it pass in the first place? Sorry - I thought you saw my comment on the other review thread - I have no idea why this didn't work for TextBox and was hoping that perhaps you would have some inkling? http://gwt-code-reviews.appspot.com/1425811/diff/1/user/test/com/google/gwt/uibinder/test/LazyWidgetBuilderTest.gwt.xml File user/test/com/google/gwt/uibinder/test/LazyWidgetBuilderTest.gwt.xml (right): http://gwt-code-reviews.appspot.com/1425811/diff/1/user/test/com/google/gwt/uibinder/test/LazyWidgetBuilderTest.gwt.xml#newcode18 user/test/com/google/gwt/uibinder/test/LazyWidgetBuilderTest.gwt.xml:18: entry-point class=com.google.gwt.uibinder.test.client.LazyWidgetBuilderTest / On 2011/04/27 19:29:58, rjrjr wrote: I don't think you need the entry-point Done. http://gwt-code-reviews.appspot.com/1425811/diff/1/user/test/com/google/gwt/uibinder/test/client/LazyWidgetBuildersTest.java File user/test/com/google/gwt/uibinder/test/client/LazyWidgetBuildersTest.java (right): http://gwt-code-reviews.appspot.com/1425811/diff/1/user/test/com/google/gwt/uibinder/test/client/LazyWidgetBuildersTest.java#newcode52 user/test/com/google/gwt/uibinder/test/client/LazyWidgetBuildersTest.java:52: assertEquals(Hello Bob, domUi.div.getInnerHTML()); On 2011/04/27 19:29:58, rjrjr wrote: check for the b, make sure it didn't get escaped. Done. http://gwt-code-reviews.appspot.com/1425811/diff/1/user/test/com/google/gwt/uibinder/test/client/SafeHtmlObject.java File user/test/com/google/gwt/uibinder/test/client/SafeHtmlObject.java (right): http://gwt-code-reviews.appspot.com/1425811/diff/1/user/test/com/google/gwt/uibinder/test/client/SafeHtmlObject.java#newcode28 user/test/com/google/gwt/uibinder/test/client/SafeHtmlObject.java:28: return Hello + name; On 2011/04/27 19:29:58, rjrjr wrote: Should put some markup in here, so we can test for improper escaping. Hello b + name + /b. Done. http://gwt-code-reviews.appspot.com/1425811/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Adds some diagnostics to an exception thrown in CompiledClass. (issue1425810)
LGTM w/ nits http://gwt-code-reviews.appspot.com/1425810/diff/6/dev/core/src/com/google/gwt/dev/javac/JdtCompiler.java File dev/core/src/com/google/gwt/dev/javac/JdtCompiler.java (right): http://gwt-code-reviews.appspot.com/1425810/diff/6/dev/core/src/com/google/gwt/dev/javac/JdtCompiler.java#newcode3 dev/core/src/com/google/gwt/dev/javac/JdtCompiler.java:3: * Yer format settings are broke again. :P http://gwt-code-reviews.appspot.com/1425810/diff/6/dev/core/src/com/google/gwt/dev/javac/JdtCompiler.java#newcode350 dev/core/src/com/google/gwt/dev/javac/JdtCompiler.java:350: */ This is supposed to work via the call chain: CSB.doBuildFrom - CompileMoreLater.addValidUnit - JdtCompiler.addCompiledUnit - addPackages() http://gwt-code-reviews.appspot.com/1425810/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Adds a ui:safehtml tag to UiBinder (issue1422812)
Reviewers: rjrjr, Description: Adds a ui:safehtml tag to UiBinder Review by: rj...@google.com Please review this at http://gwt-code-reviews.appspot.com/1422812/ Affected files: M user/src/com/google/gwt/uibinder/elementparsers/ComputedAttributeInterpreter.java M user/src/com/google/gwt/uibinder/elementparsers/HtmlInterpreter.java M user/src/com/google/gwt/uibinder/elementparsers/TextInterpreter.java M user/src/com/google/gwt/uibinder/elementparsers/UiTextInterpreter.java M user/src/com/google/gwt/uibinder/rebind/UiBinderWriter.java M user/src/com/google/gwt/uibinder/rebind/XMLAttribute.java M user/src/com/google/gwt/uibinder/rebind/XMLElement.java M user/test/com/google/gwt/uibinder/test/client/Constants.java M user/test/com/google/gwt/uibinder/test/client/UiBinderTest.java M user/test/com/google/gwt/uibinder/test/client/WidgetBasedUi.java M user/test/com/google/gwt/uibinder/test/client/WidgetBasedUi.ui.xml -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] [google-web-toolkit] r10090 committed - Remove FieldWriter.setAttachable() and find out automatically whether ...
Revision: 10090
Author: rdcas...@google.com
Date: Wed Apr 27 11:32:54 2011
Log: Remove FieldWriter.setAttachable() and find out automatically
whether the callbacks should be created.
Review at http://gwt-code-reviews.appspot.com/1421807
Review by: rj...@google.com
http://code.google.com/p/google-web-toolkit/source/detail?r=10090
Modified:
/trunk/user/src/com/google/gwt/uibinder/elementparsers/AttachableHTMLPanelParser.java
/trunk/user/src/com/google/gwt/uibinder/rebind/AbstractFieldWriter.java
/trunk/user/src/com/google/gwt/uibinder/rebind/FieldManager.java
/trunk/user/src/com/google/gwt/uibinder/rebind/FieldWriter.java
===
---
/trunk/user/src/com/google/gwt/uibinder/elementparsers/AttachableHTMLPanelParser.java
Tue Apr 26 11:22:45 2011
+++
/trunk/user/src/com/google/gwt/uibinder/elementparsers/AttachableHTMLPanelParser.java
Wed Apr 27 11:32:54 2011
@@ -32,8 +32,6 @@
final UiBinderWriter writer) throws UnableToCompleteException {
assert writer.useLazyWidgetBuilders();
-writer.getFieldManager().lookup(fieldName)
-.setAttachable(true);
/*
* Gathers up elements that indicate nested Attachable objects.
===
--- /trunk/user/src/com/google/gwt/uibinder/rebind/AbstractFieldWriter.java
Wed Apr 27 03:02:03 2011
+++ /trunk/user/src/com/google/gwt/uibinder/rebind/AbstractFieldWriter.java
Wed Apr 27 11:32:54 2011
@@ -23,6 +23,7 @@
import com.google.gwt.core.ext.typeinfo.TypeOracle;
import com.google.gwt.dom.client.Element;
import com.google.gwt.uibinder.rebind.model.OwnerField;
+import com.google.gwt.user.client.ui.AttachableHTMLPanel;
import java.util.ArrayList;
import java.util.Arrays;
@@ -58,7 +59,6 @@
private boolean written;
private int buildPrecedence;
private MortalLogger logger;
- private boolean isAttachable;
public AbstractFieldWriter(String name, MortalLogger logger) {
if (name == null) {
@@ -111,11 +111,6 @@
public void needs(FieldWriter f) {
needs.add(f);
}
-
- @Override
- public void setAttachable(boolean attachable) {
-this.isAttachable = attachable;
- }
@Override
public void setBuildPrecendence(int precedence) {
@@ -183,9 +178,16 @@
@Override
public void writeFieldDefinition(IndentedWriter w, TypeOracle typeOracle,
- OwnerField ownerField, DesignTimeUtils designTime, int getterCount)
+ OwnerField ownerField, DesignTimeUtils designTime, int getterCount,
+ boolean useLazyWidgetBuilders)
throws UnableToCompleteException {
+JClassType attachableHTMLPanelType = typeOracle.findType(
+AttachableHTMLPanel.class.getName());
+boolean outputAttachDetachCallbacks = useLazyWidgetBuilders
+ getAssignableType() != null
+ getAssignableType().isAssignableTo(attachableHTMLPanelType);
+
// Check initializer.
if (initializer == null) {
if (ownerField != null ownerField.isProvided()) {
@@ -238,7 +240,7 @@
if (attachStatements.size() 0) {
w.newline();
w.write(// Attach section.);
- if (isAttachable) {
+ if (outputAttachDetachCallbacks) {
// TODO(rdcastro): This is too coupled with AttachableHTMLPanel.
// Make this nicer.
w.write(%s.wrapInitializationCallback = , getName());
@@ -265,7 +267,7 @@
w.write(s);
}
- if (isAttachable) {
+ if (outputAttachDetachCallbacks) {
w.outdent();
w.write(});
w.outdent();
@@ -282,7 +284,7 @@
}
if (detachStatements.size() 0) {
- if (isAttachable) {
+ if (outputAttachDetachCallbacks) {
w.write(%s.detachedInitializationCallback = , getName());
w.indent();
w.indent();
@@ -296,7 +298,7 @@
w.write(s);
}
- if (isAttachable) {
+ if (outputAttachDetachCallbacks) {
w.outdent();
w.write(});
w.outdent();
===
--- /trunk/user/src/com/google/gwt/uibinder/rebind/FieldManager.java Tue
Apr 26 08:02:24 2011
+++ /trunk/user/src/com/google/gwt/uibinder/rebind/FieldManager.java Wed
Apr 27 11:32:54 2011
@@ -287,8 +287,13 @@
CollectionFieldWriter fields = fieldsMap.values();
for (FieldWriter field : fields) {
int counter = getGetterCounter(field.getName());
- field.writeFieldDefinition(writer, typeOracle,
- ownerClass.getUiField(field.getName()), designTime, counter);
+ field.writeFieldDefinition(
+ writer,
+ typeOracle,
+ ownerClass.getUiField(field.getName()),
+ designTime,
+ counter,
+ useLazyWidgetBuilders);
}
}
===
--- /trunk/user/src/com/google/gwt/uibinder/rebind/FieldWriter.java Tue Apr
26 11:22:45 2011
+++ /trunk/user/src/com/google/gwt/uibinder/rebind/FieldWriter.java Wed Apr
27 11:32:54 2011
@@ -126,9 +126,6 @@
*/
void needs(FieldWriter
[gwt-contrib] Re: Add ability to include SafeHtml objects in dom based UI's if the laay widget option is being use... (issue1425811)
http://gwt-code-reviews.appspot.com/1425811/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: EnumOrdinalizer cleanup (issue1426804)
On 2011/04/27 15:12:57, jbrosenberg wrote:
I am noticing that while unnecessary casts of an EnumType to (Enum) no
longer
happen, there is still a cast generated, for the EnumType itself. Is
that
necessary?
E.g., I'm seeing code like this:
Fruit fruit = Fruit.APPLE;
int i = fruit.ordinal();
ending up in the AST like this (from an AST dump):
EntryPoint$Fruit fruit = EntryPoint$Fruit.APPLE;
int i = ((EntryPoint$Fruit) fruit).ordinal;
Is that extra cast necessary still? I don't think it affects
correctness, and I
assume it gets optimized away later, but I wonder if we can prevent it
from
getting inserted at all here.
That cast is the result of the new 'merge' operation, basically. It's
not clear from the AST dump, but the cast is actually a non-null cast.
'fruit' is a nullable Fruit, but 'this$static' which it is replacing is
a non-null Enum. The merge operation turns the cast into a non-null
Fruit. Arguably, that's an irrelevant detail that we should forget
about. Also arguably, MakeCallStatic might be doing the wrong thing
anyway in making this$static non-null; the implicit guarantee of
non-nullity gets lost once you make the call static. For now, I think
it's ok.
http://gwt-code-reviews.appspot.com/1426804/diff/3002/dev/core/src/com/google/gwt/dev/jjs/impl/EnumOrdinalizer.java
File dev/core/src/com/google/gwt/dev/jjs/impl/EnumOrdinalizer.java
(right):
http://gwt-code-reviews.appspot.com/1426804/diff/3002/dev/core/src/com/google/gwt/dev/jjs/impl/EnumOrdinalizer.java#newcode646
dev/core/src/com/google/gwt/dev/jjs/impl/EnumOrdinalizer.java:646: *
qualifying instance.
Actually, crap, I'm going to have to revert this. Because there is no
relationship between when a JVariableRef and JVariable (for example) get
visited, we can't do it in one pass. We can't rely on instance type
being updated already, and we can't rely on it NOT being update, either,
since it will be computed dynamically from the referenced Variable,
which may or may not have been updated.
I need to revert my change that merges them.
http://gwt-code-reviews.appspot.com/1426804/diff/3002/dev/core/src/com/google/gwt/dev/jjs/impl/EnumOrdinalizer.java#newcode653
dev/core/src/com/google/gwt/dev/jjs/impl/EnumOrdinalizer.java:653:
On 2011/04/27 04:01:55, jbrosenberg wrote:
Can this comment be worded a bit more clearly? How about:
Replace any references to an enum ordinal field with the qualifying
instance,
if that instance has been ordinalized (e.g. change
code4.ordinal/code with
code4/code).
Done.
http://gwt-code-reviews.appspot.com/1426804/diff/3002/dev/core/src/com/google/gwt/dev/jjs/impl/EnumOrdinalizer.java#newcode659
dev/core/src/com/google/gwt/dev/jjs/impl/EnumOrdinalizer.java:659:
public void endVisit(JFieldRef x, Context ctx) {
Yes, in a later patch, TypeRemapper does need to implement
endVisit(JMethodCall).
http://gwt-code-reviews.appspot.com/1426804/diff/3002/dev/core/src/com/google/gwt/dev/jjs/impl/EnumOrdinalizer.java#newcode670
dev/core/src/com/google/gwt/dev/jjs/impl/EnumOrdinalizer.java:670:
On 2011/04/27 04:01:55, jbrosenberg wrote:
ditto
Done.
http://gwt-code-reviews.appspot.com/1426804/diff/3002/dev/core/src/com/google/gwt/dev/jjs/impl/EnumOrdinalizer.java#newcode793
dev/core/src/com/google/gwt/dev/jjs/impl/EnumOrdinalizer.java:793:
++removeIndex;
On 2011/04/27 04:01:55, jbrosenberg wrote:
Feel free, looks good. Maybe also add in the tests I had for testing
an
empty/unused enum.
Done.
http://gwt-code-reviews.appspot.com/1426804/diff/3002/dev/core/src/com/google/gwt/dev/jjs/impl/TypeRemapper.java
File dev/core/src/com/google/gwt/dev/jjs/impl/TypeRemapper.java (right):
http://gwt-code-reviews.appspot.com/1426804/diff/3002/dev/core/src/com/google/gwt/dev/jjs/impl/TypeRemapper.java#newcode69
dev/core/src/com/google/gwt/dev/jjs/impl/TypeRemapper.java:69:
x.setType(modRemap(x.getType()));
I see what you're saying, I'll change the comment to reflect this.
Actually, JGwtCreate's sourceType and resultTypes should really be in
terms of String rather than JType, for all it's used for. The
underlying instantiation expressions get mapped ok. I may roll another
CL for this.
http://gwt-code-reviews.appspot.com/1426804/diff/3002/dev/core/test/com/google/gwt/dev/jjs/JavaAstConstructor.java
File dev/core/test/com/google/gwt/dev/jjs/JavaAstConstructor.java
(right):
http://gwt-code-reviews.appspot.com/1426804/diff/3002/dev/core/test/com/google/gwt/dev/jjs/JavaAstConstructor.java#newcode86
dev/core/test/com/google/gwt/dev/jjs/JavaAstConstructor.java:86: public
CharSequence getContent() {
On 2011/04/27 04:01:55, jbrosenberg wrote:
This comment no longer applies?
Done.
http://gwt-code-reviews.appspot.com/1426804/diff/3002/dev/core/test/com/google/gwt/dev/jjs/impl/EnumOrdinalizerTest.java
File dev/core/test/com/google/gwt/dev/jjs/impl/EnumOrdinalizerTest.java
(right):
[gwt-contrib] Re: EnumOrdinalizer cleanup (issue1426804)
New (hopefully final?) patch. http://gwt-code-reviews.appspot.com/1426804/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Model JGwtCreate/JReboundEntryPoint request/result types as strings (issue1427808)
Reviewers: jbrosenberg, cromwellian, Message: The actual types aren't important, the rebind logic is all about matching up request/result type. Removing the types here makes things simpler. Please review this at http://gwt-code-reviews.appspot.com/1427808/ Affected files: M dev/core/src/com/google/gwt/dev/jjs/ast/JGwtCreate.java M dev/core/src/com/google/gwt/dev/jjs/ast/JReboundEntryPoint.java M dev/core/src/com/google/gwt/dev/jjs/impl/RecordRebinds.java M dev/core/src/com/google/gwt/dev/jjs/impl/ResolveRebinds.java M dev/core/src/com/google/gwt/dev/jjs/impl/ToStringGenerationVisitor.java -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Doing TODOs: make precedence detection automatic which also fixes a bug in the original approach... (issue1422813)
+ gwt contrib http://gwt-code-reviews.appspot.com/1422813/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Add ability to include SafeHtml objects in dom based UI's if the laay widget option is being use... (issue1425811)
I'll patch this in and try to figure out the TextBox thing. http://gwt-code-reviews.appspot.com/1425811/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Add ability to include SafeHtml objects in dom based UI's if the laay widget option is being use... (issue1425811)
http://gwt-code-reviews.appspot.com/1425811/diff/5001/user/test/com/google/gwt/uibinder/test/client/LazyWidgetBuildersTest.java File user/test/com/google/gwt/uibinder/test/client/LazyWidgetBuildersTest.java (right): http://gwt-code-reviews.appspot.com/1425811/diff/5001/user/test/com/google/gwt/uibinder/test/client/LazyWidgetBuildersTest.java#newcode52 user/test/com/google/gwt/uibinder/test/client/LazyWidgetBuildersTest.java:52: assertEquals(Hello bBob/b, domUi.div.getInnerHTML()); You might want call .toLowerCase() on both strings to normalize. IE does funny things to tag names, you'll get dinged in the cross browser tests. http://gwt-code-reviews.appspot.com/1425811/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Doing TODOs: make precedence detection automatic which also fixes a bug in the original approach... (issue1422813)
http://gwt-code-reviews.appspot.com/1422813/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Doing TODOs: make precedence detection automatic which also fixes a bug in the original approach... (issue1422813)
http://gwt-code-reviews.appspot.com/1422813/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Add ability to include SafeHtml objects in dom based UI's if the laay widget option is being use... (issue1425811)
LGTM TextBox isn't in the fake widget set defined by com.google.gwt.uibinder.test.UiJavaResources, and Label is. Your new code reports such errors now, which is great. http://gwt-code-reviews.appspot.com/1425811/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Doing TODOs: make precedence detection automatic which also fixes a bug in the original approach... (issue1422813)
Thanks, Ray. Submitting via SQ http://gwt-code-reviews.appspot.com/1422813/diff/1/user/src/com/google/gwt/uibinder/elementparsers/WidgetInterpreter.java File user/src/com/google/gwt/uibinder/elementparsers/WidgetInterpreter.java (right): http://gwt-code-reviews.appspot.com/1422813/diff/1/user/src/com/google/gwt/uibinder/elementparsers/WidgetInterpreter.java#newcode88 user/src/com/google/gwt/uibinder/elementparsers/WidgetInterpreter.java:88: // We must guarantee that child fields are built before the container. On 2011/04/27 23:14:53, rjrjr wrote: This comment doesn't really mean anything now, does it? Done. http://gwt-code-reviews.appspot.com/1422813/diff/1/user/src/com/google/gwt/uibinder/rebind/FieldManager.java File user/src/com/google/gwt/uibinder/rebind/FieldManager.java (right): http://gwt-code-reviews.appspot.com/1422813/diff/1/user/src/com/google/gwt/uibinder/rebind/FieldManager.java#newcode328 user/src/com/google/gwt/uibinder/rebind/FieldManager.java:328: FieldWriter parent = parsedFieldStack.getFirst(); On 2011/04/27 23:14:53, rjrjr wrote: Would you mind wrapping parsedFieldStack.getFirst() in a private peek() method? Sure, no problem. http://gwt-code-reviews.appspot.com/1422813/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Making ui:style builders always called in the Widgets ctor. Also add a (issue1422814)
Reviewers: rjrjr, rdcastro,
Description:
Making ui:style builders always called in the Widgets ctor. Also add a
final clause in field builders.
Please review this at http://gwt-code-reviews.appspot.com/1422814/
Affected files:
M user/src/com/google/gwt/uibinder/rebind/AbstractFieldWriter.java
M
user/src/com/google/gwt/uibinder/rebind/FieldWriterOfGeneratedCssResource.java
M user/src/com/google/gwt/uibinder/rebind/UiBinderWriter.java
Index: user/src/com/google/gwt/uibinder/rebind/AbstractFieldWriter.java
===
--- user/src/com/google/gwt/uibinder/rebind/AbstractFieldWriter.java
(revision 10090)
+++ user/src/com/google/gwt/uibinder/rebind/AbstractFieldWriter.java
(working copy)
@@ -227,7 +227,7 @@
if (getterCount 1) {
w.write(%s = %s;, name, initializer);
} else {
- w.write(%s %s = %s;, getQualifiedSourceName(), name, initializer);
+ w.write(final %s %s = %s;, getQualifiedSourceName(), name,
initializer);
}
w.write(// Setup section.);
Index:
user/src/com/google/gwt/uibinder/rebind/FieldWriterOfGeneratedCssResource.java
===
---
user/src/com/google/gwt/uibinder/rebind/FieldWriterOfGeneratedCssResource.java
(revision 10090)
+++
user/src/com/google/gwt/uibinder/rebind/FieldWriterOfGeneratedCssResource.java
(working copy)
@@ -20,6 +20,7 @@
import com.google.gwt.core.ext.typeinfo.JType;
import com.google.gwt.uibinder.attributeparsers.CssNameConverter;
import com.google.gwt.uibinder.rebind.model.ImplicitCssResource;
+import com.google.gwt.uibinder.rebind.model.OwnerField;
import java.util.Set;
@@ -69,4 +70,11 @@
}
return super.getReturnType(path, logger);
}
+
+ @Override
+ public void writeFieldBuilder(IndentedWriter w,
+ int getterCount, OwnerField ownerField) throws
UnableToCompleteException {
+w.write(%s; // generated css resource must be always created.
Precedence: %s,
+FieldManager.getFieldBuilder(getName()), getBuildPrecedence());
+ }
}
Index: user/src/com/google/gwt/uibinder/rebind/UiBinderWriter.java
===
--- user/src/com/google/gwt/uibinder/rebind/UiBinderWriter.java (revision
10090)
+++ user/src/com/google/gwt/uibinder/rebind/UiBinderWriter.java (working
copy)
@@ -1083,7 +1083,9 @@
fieldManager.registerFieldOfGeneratedType(
oracle.findType(ClientBundle.class.getName()),
bundleClass.getPackageName(), bundleClass.getClassName(),
-bundleClass.getFieldName());
+bundleClass.getFieldName())
+ .setBuildPrecendence(Integer.MAX_VALUE); // must be the first thing
built.
+
// Allow GWT.create() to init the field, the default behavior
String rootField = new UiBinderParser(this, messages, fieldManager,
oracle,
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Doing TODOs: make precedence detection automatic which also fixes a bug in the original approach... (issue1422813)
http://gwt-code-reviews.appspot.com/1422813/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] [google-web-toolkit] r10091 committed - Fix a class of compiler bugs related to staticImpl....
Revision: 10091
Author: sco...@google.com
Date: Wed Apr 27 13:35:10 2011
Log: Fix a class of compiler bugs related to staticImpl.
Ran into this general class of issue... originally I set out to add
staticImpl handling logic to a couple more places, such as
ImplicitUpcastAnalyzer. But the more I thought about it, the more it
struck me as a real wart, and one that's not giving us much value. Perhaps
even negative value by causing code bloat. I think it's much simpler to
just never inline staticImpls into the calling virtual method.
http://gwt-code-reviews.appspot.com/1428804/
http://code.google.com/p/google-web-toolkit/source/detail?r=10091
Modified:
/trunk/dev/core/src/com/google/gwt/dev/jjs/impl/ControlFlowAnalyzer.java
/trunk/dev/core/src/com/google/gwt/dev/jjs/impl/MethodInliner.java
/trunk/dev/core/src/com/google/gwt/dev/jjs/impl/Pruner.java
/trunk/dev/core/src/com/google/gwt/dev/jjs/impl/SameParameterValueOptimizer.java
/trunk/dev/core/src/com/google/gwt/dev/jjs/impl/TypeTightener.java
===
---
/trunk/dev/core/src/com/google/gwt/dev/jjs/impl/ControlFlowAnalyzer.java
Tue Apr 19 10:10:18 2011
+++
/trunk/dev/core/src/com/google/gwt/dev/jjs/impl/ControlFlowAnalyzer.java
Wed Apr 27 13:35:10 2011
@@ -541,19 +541,6 @@
maybeRescueJavaScriptObjectPassingIntoJava(method.getType());
}
rescueOverridingMethods(method);
-
- /*
- * Special case: also rescue an associated staticImpl. Most of
the
- * time, this would happen naturally since the instance method
- * delegates to the static. However, in cases where the static
has
- * been inlined into the instance method, future optimization
could
- * tighten an instance call into a static call, reaching code
that was
- * pruned.
- */
- JMethod staticImpl = program.getStaticImpl(method);
- if (staticImpl != null) {
-rescue(staticImpl);
- }
return true;
}
}
===
--- /trunk/dev/core/src/com/google/gwt/dev/jjs/impl/MethodInliner.java Tue
Apr 19 10:10:18 2011
+++ /trunk/dev/core/src/com/google/gwt/dev/jjs/impl/MethodInliner.java Wed
Apr 27 13:35:10 2011
@@ -144,6 +144,20 @@
@Override
public boolean visit(JMethod x, Context ctx) {
currentMethod = x;
+ if (program.getStaticImpl(x) != null) {
+/*
+ * Never inline a static impl into the calling instance method. We
used
+ * to allow this, and it required all kinds of special logic in the
+ * optimizers to keep the AST sane. This was because it was
possible to
+ * tighten an instance call to its static impl after the static
impl had
+ * already been inlined, this meant any flow type optimizer
would have
+ * to fake artifical flow from the instance method to the static
impl.
+ *
+ * TODO: allow the inlining if we are the last remaining call
site, and
+ * prune the static impl? But it might tend to generate more code.
+ */
+return false;
+ }
return true;
}
===
--- /trunk/dev/core/src/com/google/gwt/dev/jjs/impl/Pruner.java Tue Apr 19
10:10:18 2011
+++ /trunk/dev/core/src/com/google/gwt/dev/jjs/impl/Pruner.java Wed Apr 27
13:35:10 2011
@@ -360,7 +360,8 @@
for (int i = 0; i type.getMethods().size(); ++i) {
JMethod method = type.getMethods().get(i);
-if (!methodIsReferenced(method) ||
pruneViaNoninstantiability(isInstantiated, method)) {
+if (!referencedNonTypes.contains(method)
+|| pruneViaNoninstantiability(isInstantiated, method)) {
// Never prune clinit directly out of the class.
if (i 0) {
type.removeMethod(i);
@@ -394,7 +395,7 @@
for (int i = 1; i type.getMethods().size(); ++i) {
JMethod method = type.getMethods().get(i);
// all other interface methods are instance and abstract
-if (!isInstantiated || !methodIsReferenced(method)) {
+if (!isInstantiated || !referencedNonTypes.contains(method)) {
type.removeMethod(i);
madeChanges();
--i;
@@ -426,6 +427,8 @@
* devirtualizations. If the instance method has been pruned, then
it's
* okay. Also, it's okay on the final pass since no more
* devirtualizations will occur.
+ *
+ * TODO: prune params; MakeCallsStatic smarter to account for it.
*/
JMethod staticImplFor = program.staticImplFor(x);
// Unless the instance method has already been pruned, of course.
@@ -484,31 +487,6 @@
}
return false;
}
-
-/**
- * Returns codetrue/code if a method is referenced.
- */
-private boolean methodIsReferenced(JMethod method) {
- // Is the
[gwt-contrib] Re: Adds a ui:safehtml tag to UiBinder (issue1422812)
Where is UiSafeHtmlInterpreter?
I imagine it's a copy / paste clone of UiTextInterpreter, and will have
the same bugs that one does. Instead, please make it a subclass of
UiTextInterpreter, overriding a protected
createComputedAttributeInstructor() method.
Please add the unit tests for both interpreters which would have
uncovered these bugs. It maybe simplest to do that by having the test
make its own ElementParser that uses one of them and driving it with
ElementParserTester
http://gwt-code-reviews.appspot.com/1422812/diff/1/user/src/com/google/gwt/uibinder/elementparsers/ComputedAttributeInterpreter.java
File
user/src/com/google/gwt/uibinder/elementparsers/ComputedAttributeInterpreter.java
(right):
http://gwt-code-reviews.appspot.com/1422812/diff/1/user/src/com/google/gwt/uibinder/elementparsers/ComputedAttributeInterpreter.java#newcode36
user/src/com/google/gwt/uibinder/elementparsers/ComputedAttributeInterpreter.java:36:
String getAttributeToken(UiBinderWriter writer, XMLAttribute attribute)
Just give the delegate the attribute. It can provide its own writer.
http://gwt-code-reviews.appspot.com/1422812/diff/1/user/src/com/google/gwt/uibinder/elementparsers/ComputedAttributeInterpreter.java#newcode89
user/src/com/google/gwt/uibinder/elementparsers/ComputedAttributeInterpreter.java:89:
throws UnableToCompleteException {
Remember what I said about too much copy paste? It is never okay to have
this much identical code.
• Make a new constructor, ComputedAttributeInterpreter(Delegate)
• Make ComputedAttributeInterpreter(UiBinderWriter) a convenience
wrapper for it that provides a default delegate implementation with the
behavior in this method, and have this method always hand out to the
delegate.
• Get rid of the other interpretElement call.
http://gwt-code-reviews.appspot.com/1422812/diff/1/user/src/com/google/gwt/uibinder/elementparsers/ComputedAttributeInterpreter.java#newcode107
user/src/com/google/gwt/uibinder/elementparsers/ComputedAttributeInterpreter.java:107:
String attToken =
writer.tokenForStringExpression(att.consumeStringValue());
Isn't this exactly what the delegate in UiTextInterpreter is doing? So
it doesn't need to do a delegate at all.
http://gwt-code-reviews.appspot.com/1422812/diff/1/user/src/com/google/gwt/uibinder/elementparsers/UiTextInterpreter.java
File
user/src/com/google/gwt/uibinder/elementparsers/UiTextInterpreter.java
(right):
http://gwt-code-reviews.appspot.com/1422812/diff/1/user/src/com/google/gwt/uibinder/elementparsers/UiTextInterpreter.java#newcode33
user/src/com/google/gwt/uibinder/elementparsers/UiTextInterpreter.java:33:
public final class UiTextDelegate implements
ComputedAttributeInterpreter.Delegate {
Why public? Why final?
http://gwt-code-reviews.appspot.com/1422812/diff/1/user/src/com/google/gwt/uibinder/elementparsers/UiTextInterpreter.java#newcode56
user/src/com/google/gwt/uibinder/elementparsers/UiTextInterpreter.java:56:
if (!elem.getAttribute(from).hasComputedValue()) {
NPE if there is no from attribute.
http://gwt-code-reviews.appspot.com/1422812/diff/1/user/src/com/google/gwt/uibinder/elementparsers/UiTextInterpreter.java#newcode64
user/src/com/google/gwt/uibinder/elementparsers/UiTextInterpreter.java:64:
if (fieldRef == null) {
Too late.
Should also check that from is the only attribute.
http://gwt-code-reviews.appspot.com/1422812/diff/1/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/1422812/diff/1/user/src/com/google/gwt/uibinder/rebind/UiBinderWriter.java#newcode809
user/src/com/google/gwt/uibinder/rebind/UiBinderWriter.java:809: public
String tokenForSafeHtmlMethod(String expression) {
Delete this method. Move the javadoc to Rafa's tokenForExpression,
above, and use that.
http://gwt-code-reviews.appspot.com/1422812/diff/1/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/1422812/diff/1/user/test/com/google/gwt/uibinder/test/client/UiBinderTest.java#newcode632
user/test/com/google/gwt/uibinder/test/client/UiBinderTest.java:632:
assertEquals(widgetUi.mySafeHtml.getHTML(), bThis text should be
bold!/b);
Also need tests that ui:text escapes markup. Should test that in both a
gwt:Label and a gwt:HTML
http://gwt-code-reviews.appspot.com/1422812/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Making ui:style builders always called in the Widgets ctor. Also add a (issue1422814)
LGTM http://gwt-code-reviews.appspot.com/1422814/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Model JGwtCreate/JReboundEntryPoint request/result types as strings (issue1427808)
LGTM http://gwt-code-reviews.appspot.com/1427808/diff/1/dev/core/src/com/google/gwt/dev/jjs/ast/JGwtCreate.java File dev/core/src/com/google/gwt/dev/jjs/ast/JGwtCreate.java (right): http://gwt-code-reviews.appspot.com/1427808/diff/1/dev/core/src/com/google/gwt/dev/jjs/ast/JGwtCreate.java#newcode68 dev/core/src/com/google/gwt/dev/jjs/ast/JGwtCreate.java:68: Might be nice to move this to a utility somewhere perhaps in the future, since the - ., .-$ and / - . .-/ replacements seem pretty common mangling/demangling ops. http://gwt-code-reviews.appspot.com/1427808/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Adding additional testing for GWT RPC. Some custom serialized objects (issue1424801)
Hmm - I just noticed this review - sorry! http://gwt-code-reviews.appspot.com/1424801/diff/1/user/test/com/google/gwt/user/client/rpc/CoreJavaTest.java File user/test/com/google/gwt/user/client/rpc/CoreJavaTest.java (right): http://gwt-code-reviews.appspot.com/1424801/diff/1/user/test/com/google/gwt/user/client/rpc/CoreJavaTest.java#newcode126 user/test/com/google/gwt/user/client/rpc/CoreJavaTest.java:126: * This test is disabled because it times out, apparently due to an infinite Where is the code to disable this? http://gwt-code-reviews.appspot.com/1424801/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Adding the SourceElement for use with Audio and Video, and adding convenience methods in those w... (issue1423810)
http://gwt-code-reviews.appspot.com/1423810/diff/1/user/src/com/google/gwt/dom/client/SourceElement.java File user/src/com/google/gwt/dom/client/SourceElement.java (right): http://gwt-code-reviews.appspot.com/1423810/diff/1/user/src/com/google/gwt/dom/client/SourceElement.java#newcode67 user/src/com/google/gwt/dom/client/SourceElement.java:67: * Sets the type of media represented by the src. Can you add a mention that codec info can be specified here too? Maybe provide an example in the doc like: type='audio/ogg; codecs=vorbis' http://gwt-code-reviews.appspot.com/1423810/diff/1/user/src/com/google/gwt/media/client/MediaBase.java File user/src/com/google/gwt/media/client/MediaBase.java (right): http://gwt-code-reviews.appspot.com/1423810/diff/1/user/src/com/google/gwt/media/client/MediaBase.java#newcode67 user/src/com/google/gwt/media/client/MediaBase.java:67: * files until it finds one it can play. choose download - choose to download http://gwt-code-reviews.appspot.com/1423810/diff/1/user/src/com/google/gwt/media/client/MediaBase.java#newcode536 user/src/com/google/gwt/media/client/MediaBase.java:536: Can you add methods such as getSource(index), getNumSources(), and removeSource(index)? http://gwt-code-reviews.appspot.com/1423810/diff/1/user/test/com/google/gwt/media/client/MediaTest.java File user/test/com/google/gwt/media/client/MediaTest.java (right): http://gwt-code-reviews.appspot.com/1423810/diff/1/user/test/com/google/gwt/media/client/MediaTest.java#newcode132 user/test/com/google/gwt/media/client/MediaTest.java:132: media.load(); AFAIK, load isn't synchronous. This seems like it should fail on most browsers. http://gwt-code-reviews.appspot.com/1423810/diff/1/user/test/com/google/gwt/media/client/MediaTest.java#newcode141 user/test/com/google/gwt/media/client/MediaTest.java:141: Can you add a test for Source's getType() and getSrc()? http://gwt-code-reviews.appspot.com/1423810/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Adding the SourceElement for use with Audio and Video, and adding convenience methods in those w... (issue1423810)
http://gwt-code-reviews.appspot.com/1423810/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Making ui:style builders always called in the Widgets ctor. Also add a (issue1422814)
LGTM http://gwt-code-reviews.appspot.com/1422814/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Model JGwtCreate/JReboundEntryPoint request/result types as strings (issue1427808)
http://gwt-code-reviews.appspot.com/1427808/diff/1/dev/core/src/com/google/gwt/dev/jjs/ast/JGwtCreate.java File dev/core/src/com/google/gwt/dev/jjs/ast/JGwtCreate.java (right): http://gwt-code-reviews.appspot.com/1427808/diff/1/dev/core/src/com/google/gwt/dev/jjs/ast/JGwtCreate.java#newcode68 dev/core/src/com/google/gwt/dev/jjs/ast/JGwtCreate.java:68: I did consider changing this to BinaryName.toSourceName, but I wanted to be sure my refactor was non-semantic. Will add a todo. http://gwt-code-reviews.appspot.com/1427808/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] [google-web-toolkit] r10093 committed - Model JGwtCreate/JReboundEntryPoint request/result types as strings....
Revision: 10093
Author: sco...@google.com
Date: Wed Apr 27 14:46:52 2011
Log: Model JGwtCreate/JReboundEntryPoint request/result types as
strings.
The actual types aren't important, the rebind logic is all about matching
up request/result type. Removing the types here makes things simpler.
http://gwt-code-reviews.appspot.com/1427808/
Review by: cromwell...@google.com
http://code.google.com/p/google-web-toolkit/source/detail?r=10093
Modified:
/trunk/dev/core/src/com/google/gwt/dev/jjs/ast/JGwtCreate.java
/trunk/dev/core/src/com/google/gwt/dev/jjs/ast/JReboundEntryPoint.java
/trunk/dev/core/src/com/google/gwt/dev/jjs/impl/RecordRebinds.java
/trunk/dev/core/src/com/google/gwt/dev/jjs/impl/ResolveRebinds.java
/trunk/dev/core/src/com/google/gwt/dev/jjs/impl/ToStringGenerationVisitor.java
===
--- /trunk/dev/core/src/com/google/gwt/dev/jjs/ast/JGwtCreate.java Tue Apr
19 10:10:18 2011
+++ /trunk/dev/core/src/com/google/gwt/dev/jjs/ast/JGwtCreate.java Wed Apr
27 14:46:52 2011
@@ -16,8 +16,10 @@
package com.google.gwt.dev.jjs.ast;
import com.google.gwt.dev.jjs.SourceInfo;
+import com.google.gwt.dev.util.collect.Lists;
import java.util.ArrayList;
+import java.util.Collection;
import java.util.List;
/**
@@ -48,9 +50,25 @@
// Call it, using a new expression as a qualifier
return new JNewInstance(info, noArgCtor, enclosingType);
}
+
+ static ListString nameOf(Collection? extends JType types) {
+ListString result = Lists.create();
+for (JType type : types) {
+ result = Lists.add(result, nameOf(type));
+}
+return Lists.normalizeUnmodifiable(result);
+ }
+
+ /**
+ * Rebinds are always on a source type name.
+ */
+ static String nameOf(JType type) {
+// TODO: replace with BinaryName.toSourceName(type.getName())?
+return type.getName().replace('$', '.');
+ }
private static ArrayListJExpression
createInstantiationExpressions(SourceInfo info,
- ListJClassType classTypes, JDeclaredType enclosingType) {
+ CollectionJClassType classTypes, JDeclaredType enclosingType) {
ArrayListJExpression exprs = new ArrayListJExpression();
for (JClassType classType : classTypes) {
JExpression expr = createInstantiationExpression(info, classType,
enclosingType);
@@ -61,44 +79,46 @@
}
private final ArrayListJExpression instantiationExpressions;
- private final ListJClassType resultTypes;
- private final JReferenceType sourceType;
+
+ private final ListString resultTypes;
+
+ private final String sourceType;
/*
* Initially object; will be updated by type tightening.
*/
private JType type;
+
+ /**
+ * Public constructor used during AST creation.
+ */
+ public JGwtCreate(SourceInfo info, JReferenceType sourceType,
CollectionJClassType resultTypes,
+ JType type, JDeclaredType enclosingType) {
+this(info, nameOf(sourceType), nameOf(resultTypes), type,
createInstantiationExpressions(info,
+resultTypes, enclosingType));
+ }
/**
* Constructor used for cloning an existing node.
*/
- public JGwtCreate(SourceInfo info, JReferenceType sourceType,
ListJClassType resultTypes,
- JType type, ArrayListJExpression instantiationExpressions) {
+ public JGwtCreate(SourceInfo info, String sourceType, ListString
resultTypes, JType type,
+ ArrayListJExpression instantiationExpressions) {
super(info);
this.sourceType = sourceType;
this.resultTypes = resultTypes;
this.type = type;
this.instantiationExpressions = instantiationExpressions;
}
-
- /**
- * Public constructor used during AST creation.
- */
- public JGwtCreate(SourceInfo info, JReferenceType sourceType,
ListJClassType resultTypes,
- JType type, JDeclaredType enclosingType) {
-this(info, sourceType, resultTypes, type,
createInstantiationExpressions(info, resultTypes,
-enclosingType));
- }
public ArrayListJExpression getInstantiationExpressions() {
return instantiationExpressions;
}
- public ListJClassType getResultTypes() {
+ public ListString getResultTypes() {
return resultTypes;
}
- public JReferenceType getSourceType() {
+ public String getSourceType() {
return sourceType;
}
===
--- /trunk/dev/core/src/com/google/gwt/dev/jjs/ast/JReboundEntryPoint.java
Wed Dec 9 09:10:40 2009
+++ /trunk/dev/core/src/com/google/gwt/dev/jjs/ast/JReboundEntryPoint.java
Wed Apr 27 14:46:52 2011
@@ -27,14 +27,14 @@
public class JReboundEntryPoint extends JStatement {
private final ListJExpression entryCalls;
- private final ListJClassType resultTypes;
- private final JDeclaredType sourceType;
-
- public JReboundEntryPoint(SourceInfo info, JDeclaredType sourceType,
+ private final ListString resultTypes;
+ private final String sourceType;
+
+ public JReboundEntryPoint(SourceInfo info, JReferenceType sourceType,
ListJClassType
Re: [gwt-contrib] GWT SDK 2.3.0.RC1
Since you asked so nicely, I can confirm that changing imports and the gwt.xml file was all I needed to do to fix 2 large gwt applications. On Tue, Apr 26, 2011 at 8:48 PM, Chris Ramsdale cramsd...@google.com wrote: Hey GWTC folks, We have a GWT SDK 2.3.0.RC1 build that we would love feedback on. A big change since M1 is the move of AutoBean and RequestFactory to a new package, com.google.web.bindery. The old locations of AutoBean and RequestFactory should still work, but are deprecated. Fixing the deprecation warnings for the most part should be as simple as changing some import lines. If early adopters could verify that assumption, we would be grateful. The RC1 download can be found here: http://code.google.com/p/google-web-toolkit/downloads/detail?name=gwt-2.3.0.rc1.zip -- Chris/Ray, on behalf of the GWT team -- http://groups.google.com/group/Google-Web-Toolkit-Contributors -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Model JGwtCreate/JReboundEntryPoint request/result types as strings (issue1427808)
LGTM http://gwt-code-reviews.appspot.com/1427808/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] [google-web-toolkit] r10094 committed - Ups a timeout from the requestfactory suite from 10 seconds to 30 seco...
Revision: 10094
Author: zun...@google.com
Date: Wed Apr 27 15:42:18 2011
Log: Ups a timeout from the requestfactory suite from 10 seconds to 30
seconds
in hopes of eliminating spurious timeouts during unit testing.
http://code.google.com/p/google-web-toolkit/source/detail?r=10094
Modified:
/trunk/user/test/com/google/web/bindery/requestfactory/gwt/client/RequestFactoryTest.java
===
---
/trunk/user/test/com/google/web/bindery/requestfactory/gwt/client/RequestFactoryTest.java
Thu Apr 21 12:44:49 2011
+++
/trunk/user/test/com/google/web/bindery/requestfactory/gwt/client/RequestFactoryTest.java
Wed Apr 27 15:42:18 2011
@@ -194,7 +194,7 @@
}
}
- private static final int DELAY_TEST_FINISH = 10 * 1000;
+ private static final int DELAY_TEST_FINISH = 30 * 1000;
public T extends EntityProxy void assertContains(CollectionT col, T
value) {
EntityProxyId? lookFor = value.stableId();
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] HasRequestContext and 2.3.0rc1
How does this actually work? If I implement HasRequestContextT anywhere, the containing request factory editor driver that is generated no longer compiles, saying that there is a missing constructor -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Re-architect how overrides are handled. (issue1422810)
http://gwt-code-reviews.appspot.com/1422810/diff/1003/dev/core/src/com/google/gwt/core/ext/soyc/HasOverrides.java File dev/core/src/com/google/gwt/core/ext/soyc/HasOverrides.java (left): http://gwt-code-reviews.appspot.com/1422810/diff/1003/dev/core/src/com/google/gwt/core/ext/soyc/HasOverrides.java#oldcode1 dev/core/src/com/google/gwt/core/ext/soyc/HasOverrides.java:1: /* 2011 http://gwt-code-reviews.appspot.com/1422810/diff/1003/dev/core/src/com/google/gwt/dev/jjs/ast/JMethod.java File dev/core/src/com/google/gwt/dev/jjs/ast/JMethod.java (right): http://gwt-code-reviews.appspot.com/1422810/diff/1003/dev/core/src/com/google/gwt/dev/jjs/ast/JMethod.java#newcode81 dev/core/src/com/google/gwt/dev/jjs/ast/JMethod.java:81: private JType originalReturnType; Consider changing this variable name to directOverride (see notes below for getOverrides method) http://gwt-code-reviews.appspot.com/1422810/diff/1003/dev/core/src/com/google/gwt/dev/jjs/ast/JMethod.java#newcode282 dev/core/src/com/google/gwt/dev/jjs/ast/JMethod.java:282: /** s/method overrides/method directly overrides/ http://gwt-code-reviews.appspot.com/1422810/diff/1003/dev/core/src/com/google/gwt/dev/jjs/ast/JMethod.java#newcode284 dev/core/src/com/google/gwt/dev/jjs/ast/JMethod.java:284: */ change name to something like setDirectOverride(JMethod directOverride) http://gwt-code-reviews.appspot.com/1422810/diff/1003/dev/core/src/com/google/gwt/dev/jjs/impl/ControlFlowAnalyzer.java File dev/core/src/com/google/gwt/dev/jjs/impl/ControlFlowAnalyzer.java (right): http://gwt-code-reviews.appspot.com/1422810/diff/1003/dev/core/src/com/google/gwt/dev/jjs/impl/ControlFlowAnalyzer.java#newcode794 dev/core/src/com/google/gwt/dev/jjs/impl/ControlFlowAnalyzer.java:794: * } did you mean for Bar to extend Foo here? http://gwt-code-reviews.appspot.com/1422810/diff/1003/dev/core/src/com/google/gwt/dev/jjs/impl/EnumOrdinalizer.java File dev/core/src/com/google/gwt/dev/jjs/impl/EnumOrdinalizer.java (right): http://gwt-code-reviews.appspot.com/1422810/diff/1003/dev/core/src/com/google/gwt/dev/jjs/impl/EnumOrdinalizer.java#newcode804 dev/core/src/com/google/gwt/dev/jjs/impl/EnumOrdinalizer.java:804: Update, in light of changes in other EnumOrdinalizer cleanup patch. This probably needs to end up in TypeRemapper. (but you already knew all this). http://gwt-code-reviews.appspot.com/1422810/diff/1003/dev/core/src/com/google/gwt/dev/jjs/impl/Finalizer.java File dev/core/src/com/google/gwt/dev/jjs/impl/Finalizer.java (right): http://gwt-code-reviews.appspot.com/1422810/diff/1003/dev/core/src/com/google/gwt/dev/jjs/impl/Finalizer.java#newcode127 dev/core/src/com/google/gwt/dev/jjs/impl/Finalizer.java:127: /** s/overriden/overridden/ http://gwt-code-reviews.appspot.com/1422810/diff/1003/dev/core/src/com/google/gwt/dev/jjs/impl/Finalizer.java#newcode208 dev/core/src/com/google/gwt/dev/jjs/impl/Finalizer.java:208: update spelling of this variable to: isOverridden http://gwt-code-reviews.appspot.com/1422810/diff/1003/dev/core/src/com/google/gwt/dev/jjs/impl/GenerateJavaAST.java File dev/core/src/com/google/gwt/dev/jjs/impl/GenerateJavaAST.java (right): http://gwt-code-reviews.appspot.com/1422810/diff/1003/dev/core/src/com/google/gwt/dev/jjs/impl/GenerateJavaAST.java#newcode2237 dev/core/src/com/google/gwt/dev/jjs/impl/GenerateJavaAST.java:2237: assert method.canBePolymorphic(); line wrap? http://gwt-code-reviews.appspot.com/1422810/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
Re: [gwt-contrib] GWT SDK 2.3.0.RC1
We just upgraded 3 apps too, with one gotcha: it turns out that you need to do a find/replace on com.google.gwt.requestfactory.client. - com.google.web.bindery.requestfactory.gwt.client. before you do com.google.gwt.requestfactory. - com.google.web.bindery.requestfactory.. After that, it seems to work well. On 28 April 2011 11:24, Patrick Julien pjul...@gmail.com wrote: Since you asked so nicely, I can confirm that changing imports and the gwt.xml file was all I needed to do to fix 2 large gwt applications. On Tue, Apr 26, 2011 at 8:48 PM, Chris Ramsdale cramsd...@google.com wrote: Hey GWTC folks, We have a GWT SDK 2.3.0.RC1 build that we would love feedback on. A big change since M1 is the move of AutoBean and RequestFactory to a new package, com.google.web.bindery. The old locations of AutoBean and RequestFactory should still work, but are deprecated. Fixing the deprecation warnings for the most part should be as simple as changing some import lines. If early adopters could verify that assumption, we would be grateful. The RC1 download can be found here: http://code.google.com/p/google-web-toolkit/downloads/detail?name=gwt-2.3.0.rc1.zip -- Chris/Ray, on behalf of the GWT team -- http://groups.google.com/group/Google-Web-Toolkit-Contributors -- http://groups.google.com/group/Google-Web-Toolkit-Contributors -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Correctly handle graph navigation determinism. (issue1420812)
Reviewers: rchandia,
Description:
Correctly handle graph navigation determinism.
[JSR 303 TCK Result] 122 of 257 (47.47%) Pass with 10 Failures and 7
Errors.
Please review this at http://gwt-code-reviews.appspot.com/1420812/
Affected files:
M user/src/com/google/gwt/validation/client/impl/GwtValidationContext.java
M
user/src/com/google/gwt/validation/rebind/GwtSpecificValidatorCreator.java
M user/test/org/hibernate/jsr303/tck/tests/validation/ValidateGwtTest.java
M
user/test/org/hibernate/jsr303/tck/tests/validation/graphnavigation/GraphNavigationGwtTest.java
M
user/test/org/hibernate/jsr303/tck/tests/validation/graphnavigation/TckTestValidatorFactory.java
Index:
user/src/com/google/gwt/validation/client/impl/GwtValidationContext.java
===
---
user/src/com/google/gwt/validation/client/impl/GwtValidationContext.java
(revision 10092)
+++
user/src/com/google/gwt/validation/client/impl/GwtValidationContext.java
(working copy)
@@ -62,7 +62,7 @@
this.beanDescriptor = beanDescriptor;
this.messageInterpolator = messageInterpolator;
this.validator = validator;
-this.validatedObjects = validatedObjects;
+this.validatedObjects = new HashSetObject(validatedObjects);
}
public final void addValidatedObject(Object o) {
Index:
user/src/com/google/gwt/validation/rebind/GwtSpecificValidatorCreator.java
===
---
user/src/com/google/gwt/validation/rebind/GwtSpecificValidatorCreator.java
(revision 10092)
+++
user/src/com/google/gwt/validation/rebind/GwtSpecificValidatorCreator.java
(working copy)
@@ -1483,7 +1483,7 @@
sw.println(\););
// TODO(nchalko) move this out of here to the Validate method
-if (p.isCascaded()) {
+if (p.isCascaded() hasValid(p, useField)) {
// if(value != null) {
sw.println(if(value != null) {);
Index:
user/test/org/hibernate/jsr303/tck/tests/validation/ValidateGwtTest.java
===
---
user/test/org/hibernate/jsr303/tck/tests/validation/ValidateGwtTest.java
(revision 10092)
+++
user/test/org/hibernate/jsr303/tck/tests/validation/ValidateGwtTest.java
(working copy)
@@ -14,8 +14,6 @@
* the License.
*/
package org.hibernate.jsr303.tck.tests.validation;
-
-import org.hibernate.jsr303.tck.util.client.Failing;
import javax.validation.ValidationException;
@@ -34,12 +32,10 @@
delegate.testConstraintViolation();
}
- @Failing(issue = 5930)
public void testGraphValidationWithArray() {
delegate.testGraphValidationWithArray();
}
- @Failing(issue = 5930)
public void testGraphValidationWithList() {
delegate.testGraphValidationWithList();
}
Index:
user/test/org/hibernate/jsr303/tck/tests/validation/graphnavigation/GraphNavigationGwtTest.java
===
---
user/test/org/hibernate/jsr303/tck/tests/validation/graphnavigation/GraphNavigationGwtTest.java
(revision 10092)
+++
user/test/org/hibernate/jsr303/tck/tests/validation/graphnavigation/GraphNavigationGwtTest.java
(working copy)
@@ -48,7 +48,6 @@
delegate.testFullGraphValidationBeforeNextGroupInSequence();
}
- @Failing(issue = 5946)
public void testGraphNavigationDeterminism() {
delegate.testGraphNavigationDeterminism();
}
Index:
user/test/org/hibernate/jsr303/tck/tests/validation/graphnavigation/TckTestValidatorFactory.java
===
---
user/test/org/hibernate/jsr303/tck/tests/validation/graphnavigation/TckTestValidatorFactory.java
(revision 10092)
+++
user/test/org/hibernate/jsr303/tck/tests/validation/graphnavigation/TckTestValidatorFactory.java
(working copy)
@@ -1,12 +1,12 @@
/*
* Copyright 2010 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
@@ -32,8 +32,8 @@
*/
@GwtValidation(value = {
AnimalCaretaker.class, Condor.class, Elephant.class,
GameReserve.class,
- MultiCage.class, MultiCage.class, Parent.class, SingleCage.class,
- User.class, Zebra.class, Zoo.class})
+ MultiCage.class, MultiCage.class, Order.class, Parent.class,
+ SingleCage.class, User.class, Zebra.class, Zoo.class})
public static interface GwtValidator extends Validator {
}
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
