[gwt-contrib] Re: I don't believe in magic, but as Scott points out, there are consistent (issue1453804)
I would appreciate a once over by Ray or Scott before I commit.
http://gwt-code-reviews.appspot.com/1453804/diff/8001/dev/core/src/com/google/gwt/dev/jjs/impl/CastNormalizer.java
File dev/core/src/com/google/gwt/dev/jjs/impl/CastNormalizer.java
(right):
http://gwt-code-reviews.appspot.com/1453804/diff/8001/dev/core/src/com/google/gwt/dev/jjs/impl/CastNormalizer.java#newcode439
dev/core/src/com/google/gwt/dev/jjs/impl/CastNormalizer.java:439: *
On 2011/06/08 05:24:10, jbrosenberg wrote:
javadoc style {@link ...} syntax? Or @see ...?
I did this and made the comment javadoc, which seems weird to me because
it is in the middle of a block of code, but it makes the links work kind
of sort of in eclipse.
http://gwt-code-reviews.appspot.com/1453804/diff/8001/dev/core/src/com/google/gwt/dev/jjs/impl/CastNormalizer.java#newcode446
dev/core/src/com/google/gwt/dev/jjs/impl/CastNormalizer.java:446:
JMethod method =
program.getIndexedMethod(Cast.throwClassCastExceptionUnlessNull);
On 2011/06/08 05:24:10, jbrosenberg wrote:
HmmI think there's more to it than that. It just wouldn't be
correct to
implicitly upcast the expression to be of type Object (even if it's a
null
value). The magic is to make the method return a type JNullType,
which is
compatible with an expression of any type that is guaranteed to be
null valued
(since null can be cast to any type). We can't force the type to the
original
type of the cast, because that type info is lost at this point (since
it was
overwritten to JNullType).
The reason the source method throwClassCastExceptionUnlessNull is
defined with
a return type of Object in Cast.java, is because it needs to be valid
java (no
JNullType in java).
How about:
Note, we must update the type of this method call to return the null
type.
Done.
http://gwt-code-reviews.appspot.com/1453804/diff/8001/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/1453804/diff/8001/dev/core/src/com/google/gwt/dev/jjs/impl/ControlFlowAnalyzer.java#newcode377
dev/core/src/com/google/gwt/dev/jjs/impl/ControlFlowAnalyzer.java:377: *
On 2011/06/08 05:24:10, jbrosenberg wrote:
How about (just quibbling here):
Code flow analysis is run separately on methods which implement
RunAsyncCallback.onSuccess(), as top-level entry points.
Done.
http://gwt-code-reviews.appspot.com/1453804/diff/8001/dev/core/src/com/google/gwt/dev/jjs/impl/ReplaceRunAsyncs.java
File dev/core/src/com/google/gwt/dev/jjs/impl/ReplaceRunAsyncs.java
(right):
http://gwt-code-reviews.appspot.com/1453804/diff/8001/dev/core/src/com/google/gwt/dev/jjs/impl/ReplaceRunAsyncs.java#newcode65
dev/core/src/com/google/gwt/dev/jjs/impl/ReplaceRunAsyncs.java:65: /*
On 2011/06/08 05:24:10, jbrosenberg wrote:
to implementations of RunAsyncCallback.onSuccess()
Done.
http://gwt-code-reviews.appspot.com/1453804/diff/8001/dev/core/src/com/google/gwt/dev/jjs/impl/TypeTightener.java
File dev/core/src/com/google/gwt/dev/jjs/impl/TypeTightener.java
(right):
http://gwt-code-reviews.appspot.com/1453804/diff/8001/dev/core/src/com/google/gwt/dev/jjs/impl/TypeTightener.java#newcode346
dev/core/src/com/google/gwt/dev/jjs/impl/TypeTightener.java:346: *
Exception.
On 2011/06/08 05:24:10, jbrosenberg wrote:
See {@link CastNormalizer}.
Done.
http://gwt-code-reviews.appspot.com/1453804/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: I don't believe in magic, but as Scott points out, there are consistent (issue1453804)
http://gwt-code-reviews.appspot.com/1453804/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] HTML5 Geolocation support in GWT (issue1451811)
Reviewers: jlabanca, Description: HTML5 Geolocation support in GWT Review by: jlaba...@google.com Please review this at http://gwt-code-reviews.appspot.com/1451811/ Affected files: A user/src/com/google/gwt/geolocation/Geolocation.gwt.xml A user/src/com/google/gwt/geolocation/client/Geolocation.java A user/src/com/google/gwt/geolocation/client/GeolocationImpl.java A user/src/com/google/gwt/geolocation/client/Position.java A user/src/com/google/gwt/geolocation/client/PositionError.java A user/src/com/google/gwt/geolocation/client/PositionImpl.java A user/src/com/google/gwt/geolocation/client/package.html M user/src/com/google/gwt/user/User.gwt.xml A user/test/com/google/gwt/geolocation/GeolocationSuite.java A user/test/com/google/gwt/geolocation/client/GeolocationTest.java -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Disable regex checking of URLs in SafeUriHostedModeUtils; this regex appears to (issue1443813)
On 2011/06/07 16:34:24, xtof wrote: Thomas, here's a patch that adds a test case to repro the bug FYI, with openjdk-6-sdk on Ubuntu 11.04, the test passes, but the testFromTrustedString_withInvalidUrl fails to throw the expected IllegalArgumentException; which means the surrogate-pair detection regexp doesn't work as expected. Strange… I think I'm going to rewrite it the scan each char way. http://gwt-code-reviews.appspot.com/1443813/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
Re: [gwt-contrib] Earn $1000-$2500 per month
On Tue, Jun 7, 2011 at 12:32 PM, David Chandler drfibona...@google.comwrote: Nope, and I reject several spammers daily. On the GWT group, some have gotten smarter and are now posting legit-sounding messages in advance. Earn $1000-$2500 per month doesn't sound very legit on our forum. I mean, with GWT, you can easily earn 10 times that much a month! /dmc On Tue, Jun 7, 2011 at 4:49 AM, roseanje...@rediffmail.com roseanje...@rediffmail.com wrote: Earn $1000-$2500 per month If you Register your name You Get Sign-up bonus $5 AND Get $.20 cent for each referral. Further details http://www.earnbyforex.com/index.php?id=35678365 -- http://groups.google.com/group/Google-Web-Toolkit-Contributors -- David Chandler Developer Programs Engineer, Google Web Toolkit w: http://code.google.com/ b: http://googlewebtoolkit.blogspot.com/ t: @googledevtools -- http://groups.google.com/group/Google-Web-Toolkit-Contributors -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Disable regex checking of URLs in SafeUriHostedModeUtils; this regex appears to (issue1443813)
On 2011/06/08 14:48:12, tbroyer wrote: On 2011/06/07 16:34:24, xtof wrote: Thomas, here's a patch that adds a test case to repro the bug FYI, with openjdk-6-sdk on Ubuntu 11.04, the test passes, but the testFromTrustedString_withInvalidUrl fails to throw the expected IllegalArgumentException; which means the surrogate-pair detection regexp doesn't work as expected. Strange… I think I'm going to rewrite it the scan each char way. Oops! Silly me! Forgot to uncomment the check. Now the *_withInvalidUrl passes, but so does the testFromTrustedString w/ LONG_DATA_URL. (but then, isn't the testFromTrustedString_withInvalidUrl also failing for you with the check commented as in r10285? or is new URI throwing a URISyntaxException then?) http://gwt-code-reviews.appspot.com/1443813/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: HTML5 Geolocation support in GWT (issue1451811)
See also a previous attempt: http://gwt-code-reviews.appspot.com/1060801/ http://gwt-code-reviews.appspot.com/1451811/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: HTML5 Geolocation support in GWT (issue1451811)
http://gwt-code-reviews.appspot.com/1451811/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] [google-web-toolkit] r10294 committed - Merges RenderableComposite into Composite. Version that adds a setReso...
Revision: 10294
Author: rdcas...@google.com
Date: Wed Jun 8 04:46:32 2011
Log: Merges RenderableComposite into Composite. Version that adds a
setResolver method to PotentialElement.
Review at http://gwt-code-reviews.appspot.com/1449813
http://code.google.com/p/google-web-toolkit/source/detail?r=10294
Modified:
/trunk/user/src/com/google/gwt/user/client/ui/Composite.java
/trunk/user/src/com/google/gwt/user/client/ui/PotentialElement.java
/trunk/user/src/com/google/gwt/user/client/ui/RenderableComposite.java
===
--- /trunk/user/src/com/google/gwt/user/client/ui/Composite.java Tue Jun 7
11:13:59 2011
+++ /trunk/user/src/com/google/gwt/user/client/ui/Composite.java Wed Jun 8
04:46:32 2011
@@ -15,6 +15,11 @@
*/
package com.google.gwt.user.client.ui;
+import com.google.gwt.core.client.GWT;
+import com.google.gwt.dom.client.Element;
+import com.google.gwt.safehtml.client.SafeHtmlTemplates;
+import com.google.gwt.safehtml.shared.SafeHtml;
+import com.google.gwt.safehtml.shared.SafeHtmlBuilder;
import com.google.gwt.event.logical.shared.AttachEvent;
import com.google.gwt.user.client.DOM;
import com.google.gwt.user.client.Event;
@@ -23,21 +28,34 @@
* A type of widget that can wrap another widget, hiding the wrapped
widget's
* methods. When added to a panel, a composite behaves exactly as if the
widget
* it wraps had been added.
- *
+ *
* p
* The composite is useful for creating a single widget out of an
aggregate of
* multiple other widgets contained in a single panel.
* /p
- *
+ *
* p
* h3Example/h3
* {@example com.google.gwt.examples.CompositeExample}
* /p
+ *
+ * TODO(rdcastro): Remove the final qualifier from IsRenderable overrides.
*/
-public abstract class Composite extends Widget {
+public abstract class Composite extends Widget implements IsRenderable {
+
+ interface HTMLTemplates extends SafeHtmlTemplates {
+@Template(span id=\{0}\/span)
+ SafeHtml renderWithId(String id);
+ }
+ private static final HTMLTemplates TEMPLATE =
+ GWT.create(HTMLTemplates.class);
private Widget widget;
+ private IsRenderable renderable;
+
+ private Element elementToWrap;
+
@Override
public boolean isAttached() {
if (widget != null) {
@@ -54,6 +72,45 @@
// Delegate events to the widget.
widget.onBrowserEvent(event);
}
+
+ @Override
+ public final void performDetachedInitialization() {
+if (renderable != null) {
+ renderable.performDetachedInitialization();
+} else {
+ elementToWrap.getParentNode().replaceChild(widget.getElement(),
elementToWrap);
+}
+ }
+
+ @Override
+ public final SafeHtml render(String id) {
+if (renderable != null) {
+ return renderable.render(id);
+} else {
+ SafeHtmlBuilder builder = new SafeHtmlBuilder();
+ render(id, builder);
+ return builder.toSafeHtml();
+}
+ }
+
+ @Override
+ public final void render(String id, SafeHtmlBuilder builder) {
+if (renderable != null) {
+ renderable.render(id, builder);
+} else {
+ builder.append(TEMPLATE.renderWithId(id));
+}
+ }
+
+ @Override
+ public final void wrapElement(Element element) {
+if (renderable != null) {
+ renderable.wrapElement(element);
+ setElement(widget.getElement());
+} else {
+ this.elementToWrap = element;
+}
+ }
/**
* Provides subclasses access to the topmost widget that defines this
@@ -78,13 +135,23 @@
throw new IllegalStateException(Composite.initWidget() may only be
+ called once.);
}
+
+if (widget instanceof IsRenderable) {
+ // In case the Widget being wrapped is an IsRenderable, we save that
fact.
+ this.renderable = (IsRenderable) widget;
+}
// Detach the new child.
widget.removeFromParent();
// Use the contained widget's element as the composite's element,
// effectively merging them within the DOM.
-setElement(widget.getElement());
+Element elem = widget.getElement();
+setElement(elem);
+
+if (PotentialElement.isPotential(elem)) {
+ PotentialElement.as(elem).setResolver(this);
+}
// Logical attach.
this.widget = widget;
@@ -125,6 +192,12 @@
widget.onDetach();
}
}
+
+ @Override
+ protected Element resolvePotentialElement() {
+setElement(widget.resolvePotentialElement());
+return getElement();
+ }
/**
* This method was for initializing the Widget to be wrapped by this
===
--- /trunk/user/src/com/google/gwt/user/client/ui/PotentialElement.java Mon
Jun 6 04:09:34 2011
+++ /trunk/user/src/com/google/gwt/user/client/ui/PotentialElement.java Wed
Jun 8 04:46:32 2011
@@ -37,6 +37,11 @@
*/
public class PotentialElement extends Element {
+ public static PotentialElement as(Element e) {
+assert isPotential(e);
+return (PotentialElement) e;
+ }
+
/**
* Builds a new
[gwt-contrib] Rewrite SafeUriHostedModeUtils#isValid without regexp (issue1449814)
Reviewers: xtof, rjrjr, jlabanca, jat, pdr,
Message:
Rewrite SafeUriHostedModeUtils#isValid without regexp to workaround what
looks like a bug in the Sun/Oracle JVM (error not reproduced with
OpenJDK).
This is a follow-up to issue
http://gwt-code-reviews.appspot.com/1443813/ so I set the same
reviewers.
Description:
Rewrite SafeUriHostedModeUtils#isValid without regexp to workaround what
looks like a bug in the Sun/Oracle JVM (error not reproduced with
OpenJDK).
Please review this at http://gwt-code-reviews.appspot.com/1449814/
Affected files:
M user/src/com/google/gwt/safehtml/shared/SafeUriHostedModeUtils.java
M user/test/com/google/gwt/safehtml/shared/GwtUriUtilsTest.java
Index: user/src/com/google/gwt/safehtml/shared/SafeUriHostedModeUtils.java
diff --git
a/user/src/com/google/gwt/safehtml/shared/SafeUriHostedModeUtils.java
b/user/src/com/google/gwt/safehtml/shared/SafeUriHostedModeUtils.java
index
7b3dbe2602fd6cca029f889a95de8ada75ca8188..242fb02af3e0eb6814152f48ffae300c3b9de31f
100644
--- a/user/src/com/google/gwt/safehtml/shared/SafeUriHostedModeUtils.java
+++ b/user/src/com/google/gwt/safehtml/shared/SafeUriHostedModeUtils.java
@@ -36,20 +36,16 @@ import java.net.URISyntaxException;
public class SafeUriHostedModeUtils {
/**
- * All valid Web Addresses, i.e. the href-ucschar production from RFC
3987bis.
+ * All valid Web Addresses discrete characters, i.e. the href-ucschar
production from RFC
+ * 3987bis, with the exception of ranges.
*
* @see a href=http://tools.ietf.org/html/rfc3986#section-2;RFC
3986/a
* @see a
href=http://tools.ietf.org/html/draft-ietf-iri-3987bis-05#section-7.2;RFC
3987bis Web Addresses/a
*/
- static final String HREF_UCSCHAR = (
-+ [
-+ :/?#\\[\\]@!$'()*+,;= // reserved
-+ a-zA-Z0-9\\-._~ // iunreserved
-+ \{}|^`\u-\u001F\u001F-\uD7FF\uE000-\uFFFD //
href-ucschar
-+ ]
-+ |
-+ [\uD800-\uDBFF][\uDC00-\uDFFF] // surrogate pairs
-+ )*;
+ static final String HREF_DISCRETE_UCSCHAR =
+ :/?#[]@!$'()*+,;= // reserved
+ + -._~ // iunreserved
+ + \{}|\\^`;// href-ucschar
/**
* Name of system property that if set, enables checks in server-side
code
@@ -104,11 +100,38 @@ public class SafeUriHostedModeUtils {
}
private static boolean isValidUri(String uri) {
-// TODO(xtof): The regex appears to cause stack overflows in some
cases.
-// Investigate and re-enable.
-// if (!uri.matches(HREF_UCSCHAR)) {
-// return false;
-// }
+int len = uri.length();
+int i = 0;
+while (i len) {
+ int codePoint = uri.codePointAt(i);
+ i += Character.charCount(codePoint);
+ if (!Character.isValidCodePoint(codePoint)) {
+return false;
+ }
+ if (Character.isSupplementaryCodePoint(codePoint)) {
+continue;
+ }
+ // from now on, we know codePoint is in the Basic Multilingual Plane
+ // (i.e. it can be cast to 'char' without loss of information)
+ // Let's take advantage of this to detect unpaired surrogates
+ if (Character.isHighSurrogate((char) codePoint) ||
Character.isLowSurrogate((char) codePoint)) {
+return false;
+ }
+ if (HREF_DISCRETE_UCSCHAR.indexOf(codePoint) = 0) {
+continue;
+ }
+ // iunreserved ranges
+ if (('a' = codePoint codePoint = 'z') || ('A' = codePoint
codePoint = 'Z') || ('0' = codePoint codePoint= '9')) {
+continue;
+ }
+ // href-ucschar ranges
+ if ((0 = codePoint codePoint = 0x1F) || (0x7F = codePoint
codePoint = 0xD7FF) || (0xE000 = codePoint codePoint = 0xFFFD)) {
+continue;
+ }
+ // unknown char (neither whitelisted not explicitly blacklisted)
+ return false;
+}
+
/*
* pre-process to turn href-ucschars into ucschars, and encode to URI.
*
Index: user/test/com/google/gwt/safehtml/shared/GwtUriUtilsTest.java
diff --git a/user/test/com/google/gwt/safehtml/shared/GwtUriUtilsTest.java
b/user/test/com/google/gwt/safehtml/shared/GwtUriUtilsTest.java
index
f76c66f2df4a380d0122cdefe970e363d96d3504..5025ec0cceb2732f458126c9299868a22c511613
100644
--- a/user/test/com/google/gwt/safehtml/shared/GwtUriUtilsTest.java
+++ b/user/test/com/google/gwt/safehtml/shared/GwtUriUtilsTest.java
@@ -26,9 +26,25 @@ public class GwtUriUtilsTest extends GWTTestCase {
private static final String JAVASCRIPT_URL
= javascript:alert('BOOM!');;
private static final String MAILTO_URL = mailto:f...@example.com;;
private static final String CONSTANT_URL =
-
http://gwt.google.com/samples/Showcase/Showcase.html?locale=fr#!CwCheckBox;;
+
http://gwt.google.com/samples/Showcase/Showcase.html?locale=fr#!CwCheckBox;;
private static final String EMPTY_GIF_DATA_URL =
-
data:image/gif;base64,R0lGODlhAQABAPABAP///wAAACH5BAEKLAABAAEAAAICRAEAOw==;
+
[gwt-contrib] Re: Disable regex checking of URLs in SafeUriHostedModeUtils; this regex appears to (issue1443813)
On 2011/06/08 15:00:42, tbroyer wrote: On 2011/06/08 14:48:12, tbroyer wrote: On 2011/06/07 16:34:24, xtof wrote: Thomas, here's a patch that adds a test case to repro the bug FYI, with openjdk-6-sdk on Ubuntu 11.04, the test passes, but the testFromTrustedString_withInvalidUrl fails to throw the expected IllegalArgumentException; which means the surrogate-pair detection regexp doesn't work as expected. Strange… I think I'm going to rewrite it the scan each char way. Oops! Silly me! Forgot to uncomment the check. Now the *_withInvalidUrl passes, but so does the testFromTrustedString w/ LONG_DATA_URL. (but then, isn't the testFromTrustedString_withInvalidUrl also failing for you with the check commented as in r10285? or is new URI throwing a URISyntaxException then?) See http://gwt-code-reviews.appspot.com/1449814/ http://gwt-code-reviews.appspot.com/1443813/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Disable regex checking of URLs in SafeUriHostedModeUtils; this regex appears to (issue1443813)
On Wed, Jun 8, 2011 at 08:00, t.bro...@gmail.com wrote: On 2011/06/08 14:48:12, tbroyer wrote: On 2011/06/07 16:34:24, xtof wrote: Thomas, here's a patch that adds a test case to repro the bug FYI, with openjdk-6-sdk on Ubuntu 11.04, the test passes, but the testFromTrustedString_withInvalidUrl fails to throw the expected IllegalArgumentException; which means the surrogate-pair detection regexp doesn't work as expected. Strange… I think I'm going to rewrite it the scan each char way. Oops! Silly me! Forgot to uncomment the check. Now the *_withInvalidUrl passes, but so does the testFromTrustedString w/ LONG_DATA_URL. (but then, isn't the testFromTrustedString_withInvalidUrl also failing for you with the check commented as in r10285? or is new URI throwing a URISyntaxException then?) The tests pass for me at r10285. I was a little surprised that they pass with the checks disabled but didn't have time to think about it. It might mean that a test is passing vacuously? Hmm, you're probably right that new URI is failing. http://gwt-code-reviews.appspot.com/1443813/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Rewrite SafeUriHostedModeUtils#isValid without regexp (issue1449814)
http://gwt-code-reviews.appspot.com/1449814/diff/1/user/src/com/google/gwt/safehtml/shared/SafeUriHostedModeUtils.java
File user/src/com/google/gwt/safehtml/shared/SafeUriHostedModeUtils.java
(right):
http://gwt-code-reviews.appspot.com/1449814/diff/1/user/src/com/google/gwt/safehtml/shared/SafeUriHostedModeUtils.java#newcode117
user/src/com/google/gwt/safehtml/shared/SafeUriHostedModeUtils.java:117:
if (Character.isHighSurrogate((char) codePoint) ||
Character.isLowSurrogate((char) codePoint)) {
Long lines, here and below.
http://gwt-code-reviews.appspot.com/1449814/diff/1/user/test/com/google/gwt/safehtml/shared/GwtUriUtilsTest.java
File user/test/com/google/gwt/safehtml/shared/GwtUriUtilsTest.java
(right):
http://gwt-code-reviews.appspot.com/1449814/diff/1/user/test/com/google/gwt/safehtml/shared/GwtUriUtilsTest.java#newcode96
user/test/com/google/gwt/safehtml/shared/GwtUriUtilsTest.java:96:
assertEquals(CONSTANT_URL,
UriUtils.fromTrustedString(CONSTANT_URL).asString());
While you're here, could you change fromTrustedString to
fromSafeConstant where the argument is a constant? In keeping with the
desire that fromTrustedString should be used as little as possible...
http://gwt-code-reviews.appspot.com/1449814/diff/1/user/test/com/google/gwt/safehtml/shared/GwtUriUtilsTest.java#newcode109
user/test/com/google/gwt/safehtml/shared/GwtUriUtilsTest.java:109:
public void testFromTrustedString_withInvalidUrl() {
Given that this test passed even with the original regex check commented
out, would it make sense to to split the character set check of
isValidUri out and test it separately?
http://gwt-code-reviews.appspot.com/1449814/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: HTML5 Geolocation support in GWT (issue1451811)
http://gwt-code-reviews.appspot.com/1451811/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Adds setTagName to PotentialElement, so that PotentialElement instances can be passed to the as(... (issue1451810)
http://gwt-code-reviews.appspot.com/1451810/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: HTML5 Geolocation support in GWT (issue1451811)
Getting closer!
http://gwt-code-reviews.appspot.com/1451811/diff/2013/user/src/com/google/gwt/geolocation/client/Geolocation.java
File user/src/com/google/gwt/geolocation/client/Geolocation.java
(right):
http://gwt-code-reviews.appspot.com/1451811/diff/2013/user/src/com/google/gwt/geolocation/client/Geolocation.java#newcode28
user/src/com/google/gwt/geolocation/client/Geolocation.java:28: *
codeGWT.create(Geolocation.class)/code.
You can remove this comment since the user can now call
createIfSupported().
http://gwt-code-reviews.appspot.com/1451811/diff/2013/user/src/com/google/gwt/geolocation/client/Geolocation.java#newcode68
user/src/com/google/gwt/geolocation/client/Geolocation.java:68: private
static boolean supported = detectSupport();
This should be non-static to ensure we don't end up with clinits(). I'm
not 100% sure that it matters, but its better to be safe.
http://gwt-code-reviews.appspot.com/1451811/diff/2013/user/src/com/google/gwt/geolocation/client/Geolocation.java#newcode93
user/src/com/google/gwt/geolocation/client/Geolocation.java:93: public
class PositionOptions {
This could be a JSO. Make its constructor private, then add a static
method Geolocation.createPositionOptions(), which in turn calls
JavaScriptObject.createObject() and casts it to a PositionOptions.
Then, you can get rid of the fields and just update the object directly.
You also won't need to convert back and forth from a JSO in the impl
class.
http://gwt-code-reviews.appspot.com/1451811/diff/2013/user/src/com/google/gwt/geolocation/client/Geolocation.java#newcode112
user/src/com/google/gwt/geolocation/client/Geolocation.java:112: public
PositionOptions setEnableHighAccuracy(boolean enable) {
setEnableHighAccuracy/setHighAccuracyEnabled
http://gwt-code-reviews.appspot.com/1451811/diff/2013/user/src/com/google/gwt/geolocation/client/Geolocation.java#newcode121
user/src/com/google/gwt/geolocation/client/Geolocation.java:121: *
locate the user and cache and return the position.
Add a comment that if the maximum age is 0, then there is no max (if
that is true).
http://gwt-code-reviews.appspot.com/1451811/diff/2013/user/src/com/google/gwt/geolocation/client/Geolocation.java#newcode131
user/src/com/google/gwt/geolocation/client/Geolocation.java:131: * takes
more than this amount of time, an error will result.
Add a comment that setting the timeout to -1 results in no timeout.
http://gwt-code-reviews.appspot.com/1451811/diff/2013/user/src/com/google/gwt/geolocation/client/Geolocation.java#newcode138
user/src/com/google/gwt/geolocation/client/Geolocation.java:138:
Add a static method isSupported() that returns a boolean.
http://gwt-code-reviews.appspot.com/1451811/diff/2013/user/src/com/google/gwt/geolocation/client/Geolocation.java#newcode139
user/src/com/google/gwt/geolocation/client/Geolocation.java:139: public
static Geolocation createIfSupported() {
Since there is only on Geolocation object, you should rename this to
getGeolocationIfSupported() (see Storage.java).
It looks like you always return the same implementation anyway.
http://gwt-code-reviews.appspot.com/1451811/diff/2013/user/src/com/google/gwt/geolocation/client/Geolocation.java#newcode194
user/src/com/google/gwt/geolocation/client/Geolocation.java:194: public
boolean isSupported() {
Remove the isSupported() instance method and add a static one. If you
have a Geolocation instance, then its already supported.
http://gwt-code-reviews.appspot.com/1451811/diff/2013/user/src/com/google/gwt/geolocation/client/GeolocationImpl.java
File user/src/com/google/gwt/geolocation/client/GeolocationImpl.java
(right):
http://gwt-code-reviews.appspot.com/1451811/diff/2013/user/src/com/google/gwt/geolocation/client/GeolocationImpl.java#newcode26
user/src/com/google/gwt/geolocation/client/GeolocationImpl.java:26:
class GeolocationImpl extends Geolocation {
Can you roll GeolocationImpl into Geolocation now that you only create a
Geolocation if supported?
http://gwt-code-reviews.appspot.com/1451811/diff/2013/user/src/com/google/gwt/geolocation/client/GeolocationImpl.java#newcode28
user/src/com/google/gwt/geolocation/client/GeolocationImpl.java:28:
private static native boolean detectSupport() /*-{
Remove - this was moved to the Detector
http://gwt-code-reviews.appspot.com/1451811/diff/2013/user/src/com/google/gwt/geolocation/client/GeolocationImpl.java#newcode36
user/src/com/google/gwt/geolocation/client/GeolocationImpl.java:36:
private static void handleSuccess(CallbackPosition, PositionError
callback, JavaScriptObject obj) {
I think you can change the JavaScriptObject parameter to a PositionImpl,
and GWT will cast it for you
http://gwt-code-reviews.appspot.com/1451811/diff/2013/user/src/com/google/gwt/geolocation/client/GeolocationImpl.java#newcode41
user/src/com/google/gwt/geolocation/client/GeolocationImpl.java:41:
private static native JavaScriptObject toJso(PositionOptions options)
/*-{
Is PositionOptions is a JSO, you can eliminate this method.
[gwt-contrib] Re: Rewrite SafeUriHostedModeUtils#isValid without regexp (issue1449814)
http://gwt-code-reviews.appspot.com/1449814/diff/1/user/src/com/google/gwt/safehtml/shared/SafeUriHostedModeUtils.java
File user/src/com/google/gwt/safehtml/shared/SafeUriHostedModeUtils.java
(right):
http://gwt-code-reviews.appspot.com/1449814/diff/1/user/src/com/google/gwt/safehtml/shared/SafeUriHostedModeUtils.java#newcode117
user/src/com/google/gwt/safehtml/shared/SafeUriHostedModeUtils.java:117:
if (Character.isHighSurrogate((char) codePoint) ||
Character.isLowSurrogate((char) codePoint)) {
On 2011/06/08 16:41:02, xtof wrote:
Long lines, here and below.
Huh, right. I formatted the file and it was totally messed up as a
result (for instance, but not only, it also reformatted the license
header with 100-chars long lines, is it expected?) so I reverted it...
and forgot to format the changes locally.
http://gwt-code-reviews.appspot.com/1449814/diff/1/user/test/com/google/gwt/safehtml/shared/GwtUriUtilsTest.java
File user/test/com/google/gwt/safehtml/shared/GwtUriUtilsTest.java
(right):
http://gwt-code-reviews.appspot.com/1449814/diff/1/user/test/com/google/gwt/safehtml/shared/GwtUriUtilsTest.java#newcode96
user/test/com/google/gwt/safehtml/shared/GwtUriUtilsTest.java:96:
assertEquals(CONSTANT_URL,
UriUtils.fromTrustedString(CONSTANT_URL).asString());
On 2011/06/08 16:41:02, xtof wrote:
While you're here, could you change fromTrustedString to
fromSafeConstant where
the argument is a constant? In keeping with the desire that
fromTrustedString
should be used as little as possible...
As I said in the other review, these are unit-tests for
fromTrustedString. Maybe we should duplicate them for fromSafeConstant
then? (IIRC, initially, fromSafeConstant didn't validate the value; that
would explain the lack of unit test)
http://gwt-code-reviews.appspot.com/1449814/diff/1/user/test/com/google/gwt/safehtml/shared/GwtUriUtilsTest.java#newcode109
user/test/com/google/gwt/safehtml/shared/GwtUriUtilsTest.java:109:
public void testFromTrustedString_withInvalidUrl() {
On 2011/06/08 16:41:02, xtof wrote:
Given that this test passed even with the original regex check
commented out,
would it make sense to to split the character set check of isValidUri
out and
test it separately?
Yes, there should probably be a SafeUriHostedModeUtilsTest. I'll try to
do it tomorrow morning (UTC+2)
http://gwt-code-reviews.appspot.com/1449814/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Don't initialize SpeedTracerLogger thread or process time keepers if not enabled. (issue1456801)
Reviewers: Josh Humphries,
Description:
Don't initialize SpeedTracerLogger thread or process time keepers if not
enabled.
Please review this at http://gwt-code-reviews.appspot.com/1456801/
Affected files:
M
dev/core/src/com/google/gwt/dev/util/log/speedtracer/SpeedTracerLogger.java
Index:
dev/core/src/com/google/gwt/dev/util/log/speedtracer/SpeedTracerLogger.java
===
---
dev/core/src/com/google/gwt/dev/util/log/speedtracer/SpeedTracerLogger.java
(revision 10290)
+++
dev/core/src/com/google/gwt/dev/util/log/speedtracer/SpeedTracerLogger.java
(working copy)
@@ -113,7 +113,9 @@
Event() {
if (enabled) {
-threadCpuTimeKeeper.resetTimeBase();
+if (logThreadCpuTime) {
+ threadCpuTimeKeeper.resetTimeBase();
+}
recordStartTime();
this.data = Lists.create();
this.children = Lists.create();
@@ -702,10 +704,11 @@
private final ElapsedNormalizedTimeKeeper elapsedTimeKeeper = new
ElapsedNormalizedTimeKeeper();
- private final ProcessNormalizedTimeKeeper processCpuTimeKeeper =
- new ProcessNormalizedTimeKeeper();
-
- private final ThreadNormalizedTimeKeeper threadCpuTimeKeeper = new
ThreadNormalizedTimeKeeper();
+ private final ProcessNormalizedTimeKeeper processCpuTimeKeeper =
+ (logProcessCpuTime) ? new ProcessNormalizedTimeKeeper() :
null;
+
+ private final ThreadNormalizedTimeKeeper threadCpuTimeKeeper =
+ (logThreadCpuTime) ? new ThreadNormalizedTimeKeeper() : null;
/**
* Constructor intended for unit testing.
@@ -899,9 +902,11 @@
if (!threadPendingEvents.isEmpty()) {
parent = threadPendingEvents.peek();
} else {
- // reset the thread CPU time base for top-level events (so events
can be
- // properly sequenced chronologically)
- threadCpuTimeKeeper.resetTimeBase();
+ if (logThreadCpuTime) {
+// reset the thread CPU time base for top-level events (so events
can be
+// properly sequenced chronologically)
+threadCpuTimeKeeper.resetTimeBase();
+ }
}
Event newEvent = new Event(session, parent, type, data);
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Fixes a bug in StackTraceDeobfuscator where line numbers from the symbol map were being used in ... (issue1457801)
Reviewers: fredsa,
Description:
Fixes a bug in StackTraceDeobfuscator where line numbers from the symbol
map were being used in resymbolization only if the line number from
StackTraceElement is 0, where it should be used if the line number is
-1, per StackTraceElement's javadoc:
http://download.oracle.com/javase/6/docs/api/java/lang/StackTraceElement.html
Please review this at http://gwt-code-reviews.appspot.com/1457801/
Affected files:
M user/src/com/google/gwt/logging/server/StackTraceDeobfuscator.java
Index: user/src/com/google/gwt/logging/server/StackTraceDeobfuscator.java
===
--- user/src/com/google/gwt/logging/server/StackTraceDeobfuscator.java
(revision 10294)
+++ user/src/com/google/gwt/logging/server/StackTraceDeobfuscator.java
(working copy)
@@ -136,11 +136,11 @@
int lineNumber = ste.getLineNumber();
/*
- * When lineNumber is zero, either because compiler.stackMode is
not
+ * When lineNumber is -1, either because compiler.stackMode is not
* emulated or compiler.emulatedStack.recordLineNumbers is false,
use
* the method declaration line number from the symbol map.
*/
-if (lineNumber == 0) {
+if (lineNumber == -1) {
lineNumber = Integer.parseInt(parts[4]);
}
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Don't initialize SpeedTracerLogger thread or process time keepers if not enabled. (issue1456801)
LGTM http://gwt-code-reviews.appspot.com/1456801/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: HTML5 Geolocation support in GWT (issue1451811)
http://gwt-code-reviews.appspot.com/1451811/diff/2013/user/src/com/google/gwt/geolocation/client/Geolocation.java
File user/src/com/google/gwt/geolocation/client/Geolocation.java
(right):
http://gwt-code-reviews.appspot.com/1451811/diff/2013/user/src/com/google/gwt/geolocation/client/Geolocation.java#newcode28
user/src/com/google/gwt/geolocation/client/Geolocation.java:28: *
codeGWT.create(Geolocation.class)/code.
On 2011/06/08 18:42:47, jlabanca wrote:
You can remove this comment since the user can now call
createIfSupported().
Done.
http://gwt-code-reviews.appspot.com/1451811/diff/2013/user/src/com/google/gwt/geolocation/client/Geolocation.java#newcode68
user/src/com/google/gwt/geolocation/client/Geolocation.java:68: private
static boolean supported = detectSupport();
On 2011/06/08 18:42:47, jlabanca wrote:
This should be non-static to ensure we don't end up with clinits().
I'm not
100% sure that it matters, but its better to be safe.
Done.
http://gwt-code-reviews.appspot.com/1451811/diff/2013/user/src/com/google/gwt/geolocation/client/Geolocation.java#newcode93
user/src/com/google/gwt/geolocation/client/Geolocation.java:93: public
class PositionOptions {
On 2011/06/08 18:42:47, jlabanca wrote:
This could be a JSO. Make its constructor private, then add a static
method
Geolocation.createPositionOptions(), which in turn calls
JavaScriptObject.createObject() and casts it to a PositionOptions.
Then, you can get rid of the fields and just update the object
directly. You
also won't need to convert back and forth from a JSO in the impl
class.
If this is a JSO, JUnit tests can't use it, right? I agree the
POJO-to-JSO conversion is kind of gross, but it was the best I could
come up with that was JRE-compatible.
http://gwt-code-reviews.appspot.com/1451811/diff/2013/user/src/com/google/gwt/geolocation/client/Geolocation.java#newcode112
user/src/com/google/gwt/geolocation/client/Geolocation.java:112: public
PositionOptions setEnableHighAccuracy(boolean enable) {
On 2011/06/08 18:42:47, jlabanca wrote:
setEnableHighAccuracy/setHighAccuracyEnabled
I'm not sure I understand the comment, but I changed it to match what I
think you meant...
http://gwt-code-reviews.appspot.com/1451811/diff/2013/user/src/com/google/gwt/geolocation/client/Geolocation.java#newcode121
user/src/com/google/gwt/geolocation/client/Geolocation.java:121: *
locate the user and cache and return the position.
On 2011/06/08 18:42:47, jlabanca wrote:
Add a comment that if the maximum age is 0, then there is no max (if
that is
true).
Done.
http://gwt-code-reviews.appspot.com/1451811/diff/2013/user/src/com/google/gwt/geolocation/client/Geolocation.java#newcode131
user/src/com/google/gwt/geolocation/client/Geolocation.java:131: * takes
more than this amount of time, an error will result.
On 2011/06/08 18:42:47, jlabanca wrote:
Add a comment that setting the timeout to -1 results in no timeout.
Done.
http://gwt-code-reviews.appspot.com/1451811/diff/2013/user/src/com/google/gwt/geolocation/client/Geolocation.java#newcode138
user/src/com/google/gwt/geolocation/client/Geolocation.java:138:
On 2011/06/08 18:42:47, jlabanca wrote:
Add a static method isSupported() that returns a boolean.
Done.
http://gwt-code-reviews.appspot.com/1451811/diff/2013/user/src/com/google/gwt/geolocation/client/Geolocation.java#newcode139
user/src/com/google/gwt/geolocation/client/Geolocation.java:139: public
static Geolocation createIfSupported() {
On 2011/06/08 18:42:47, jlabanca wrote:
Since there is only on Geolocation object, you should rename this to
getGeolocationIfSupported() (see Storage.java).
It looks like you always return the same implementation anyway.
Done.
http://gwt-code-reviews.appspot.com/1451811/diff/2013/user/src/com/google/gwt/geolocation/client/Geolocation.java#newcode194
user/src/com/google/gwt/geolocation/client/Geolocation.java:194: public
boolean isSupported() {
On 2011/06/08 18:42:47, jlabanca wrote:
Remove the isSupported() instance method and add a static one. If you
have a
Geolocation instance, then its already supported.
Done.
http://gwt-code-reviews.appspot.com/1451811/diff/2013/user/src/com/google/gwt/geolocation/client/GeolocationImpl.java
File user/src/com/google/gwt/geolocation/client/GeolocationImpl.java
(right):
http://gwt-code-reviews.appspot.com/1451811/diff/2013/user/src/com/google/gwt/geolocation/client/GeolocationImpl.java#newcode28
user/src/com/google/gwt/geolocation/client/GeolocationImpl.java:28:
private static native boolean detectSupport() /*-{
On 2011/06/08 18:42:47, jlabanca wrote:
Remove - this was moved to the Detector
Done.
http://gwt-code-reviews.appspot.com/1451811/diff/2013/user/src/com/google/gwt/geolocation/client/GeolocationImpl.java#newcode36
user/src/com/google/gwt/geolocation/client/GeolocationImpl.java:36:
private static void handleSuccess(CallbackPosition, PositionError
callback, JavaScriptObject obj) {
On 2011/06/08 18:42:47, jlabanca wrote:
I think you
[gwt-contrib] Re: HTML5 Geolocation support in GWT (issue1451811)
http://gwt-code-reviews.appspot.com/1451811/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: HTML5 Geolocation support in GWT (issue1451811)
On 2011/06/08 19:21:22, jasonhall wrote: Oh, I forgot to fold GeolocationImpl into Geolocation. Doing that now. http://gwt-code-reviews.appspot.com/1451811/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: HTML5 Geolocation support in GWT (issue1451811)
http://gwt-code-reviews.appspot.com/1451811/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Fixes a bug in StackTraceDeobfuscator where line numbers from the symbol map were being used in ... (issue1457801)
LGTM
http://gwt-code-reviews.appspot.com/1457801/diff/4001/user/src/com/google/gwt/logging/server/StackTraceDeobfuscator.java
File user/src/com/google/gwt/logging/server/StackTraceDeobfuscator.java
(right):
http://gwt-code-reviews.appspot.com/1457801/diff/4001/user/src/com/google/gwt/logging/server/StackTraceDeobfuscator.java#newcode145
user/src/com/google/gwt/logging/server/StackTraceDeobfuscator.java:145:
if (lineNumber == -1) {
Sorry, one more thought: I think it would be good to extract -1 into a
static final, say LINE_NUMBER_UNKNOWN, similar what's in
StackTraceCreator.java
http://gwt-code-reviews.appspot.com/1457801/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Fixes a bug in StackTraceDeobfuscator where line numbers from the symbol map were being used in ... (issue1457801)
http://gwt-code-reviews.appspot.com/1457801/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] [google-web-toolkit] r10296 committed - Don't initialize SpeedTracerLogger thread or process time keepers if n...
Revision: 10296
Author: jbrosenb...@google.com
Date: Wed Jun 8 10:02:34 2011
Log: Don't initialize SpeedTracerLogger thread or process time keepers
if not enabled.
Review at http://gwt-code-reviews.appspot.com/1456801
Review by: jhumphr...@google.com
http://code.google.com/p/google-web-toolkit/source/detail?r=10296
Modified:
/trunk/dev/core/src/com/google/gwt/dev/util/log/speedtracer/SpeedTracerLogger.java
===
---
/trunk/dev/core/src/com/google/gwt/dev/util/log/speedtracer/SpeedTracerLogger.java
Wed May 11 07:35:17 2011
+++
/trunk/dev/core/src/com/google/gwt/dev/util/log/speedtracer/SpeedTracerLogger.java
Wed Jun 8 10:02:34 2011
@@ -113,7 +113,9 @@
Event() {
if (enabled) {
-threadCpuTimeKeeper.resetTimeBase();
+if (logThreadCpuTime) {
+ threadCpuTimeKeeper.resetTimeBase();
+}
recordStartTime();
this.data = Lists.create();
this.children = Lists.create();
@@ -702,10 +704,11 @@
private final ElapsedNormalizedTimeKeeper elapsedTimeKeeper = new
ElapsedNormalizedTimeKeeper();
- private final ProcessNormalizedTimeKeeper processCpuTimeKeeper =
- new ProcessNormalizedTimeKeeper();
-
- private final ThreadNormalizedTimeKeeper threadCpuTimeKeeper = new
ThreadNormalizedTimeKeeper();
+ private final ProcessNormalizedTimeKeeper processCpuTimeKeeper =
+ (logProcessCpuTime) ? new ProcessNormalizedTimeKeeper() :
null;
+
+ private final ThreadNormalizedTimeKeeper threadCpuTimeKeeper =
+ (logThreadCpuTime) ? new ThreadNormalizedTimeKeeper() : null;
/**
* Constructor intended for unit testing.
@@ -899,9 +902,11 @@
if (!threadPendingEvents.isEmpty()) {
parent = threadPendingEvents.peek();
} else {
- // reset the thread CPU time base for top-level events (so events
can be
- // properly sequenced chronologically)
- threadCpuTimeKeeper.resetTimeBase();
+ if (logThreadCpuTime) {
+// reset the thread CPU time base for top-level events (so events
can be
+// properly sequenced chronologically)
+threadCpuTimeKeeper.resetTimeBase();
+ }
}
Event newEvent = new Event(session, parent, type, data);
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] [google-web-toolkit] r10297 committed - Fixes a bug in StackTraceDeobfuscator where line numbers from the symb...
Revision: 10297
Author: ahume...@google.com
Date: Wed Jun 8 10:11:34 2011
Log: Fixes a bug in StackTraceDeobfuscator where line numbers from the
symbol map were being used in resymbolization only if the line number from
StackTraceElement is 0, where it should be used if the line number is -1,
per StackTraceElement's javadoc:
http://download.oracle.com/javase/6/docs/api/java/lang/StackTraceElement.html
Review at http://gwt-code-reviews.appspot.com/1457801
http://code.google.com/p/google-web-toolkit/source/detail?r=10297
Modified:
/trunk/user/src/com/google/gwt/logging/server/StackTraceDeobfuscator.java
===
---
/trunk/user/src/com/google/gwt/logging/server/StackTraceDeobfuscator.java
Wed Feb 23 11:44:20 2011
+++
/trunk/user/src/com/google/gwt/logging/server/StackTraceDeobfuscator.java
Wed Jun 8 10:11:34 2011
@@ -53,6 +53,10 @@
private static Pattern JsniRefPattern =
Pattern.compile(@?([^:]+)::([^(]+)(\\((.*)\\))?);
+ // The javadoc for StackTraceElement.getLineNumber() says it returns -1
when
+ // the line number is unavailable
+ private static final int LINE_NUMBER_UNKNOWN = -1;
+
private File symbolMapsDirectory;
private MapString, SymbolMap symbolMaps =
@@ -136,11 +140,12 @@
int lineNumber = ste.getLineNumber();
/*
- * When lineNumber is zero, either because compiler.stackMode is
not
- * emulated or compiler.emulatedStack.recordLineNumbers is false,
use
- * the method declaration line number from the symbol map.
+ * When lineNumber is LINE_NUMBER_UNKNOWN, either because
+ * compiler.stackMode is not emulated or
+ * compiler.emulatedStack.recordLineNumbers is false, use the
method
+ * declaration line number from the symbol map.
*/
-if (lineNumber == 0) {
+if (lineNumber == LINE_NUMBER_UNKNOWN) {
lineNumber = Integer.parseInt(parts[4]);
}
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Fixes a bug in StackTraceDeobfuscator where line numbers from the symbol map were being used in ... (issue1457801)
On 2011/06/08 20:01:41, ahumesky wrote: Committed in r10297 http://gwt-code-reviews.appspot.com/1457801/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Formatting update for com/google/gwt/user/rebind/rpc/* (issue1454805)
Reviewers: zundel, Description: Formatting update for com/google/gwt/user/rebind/rpc/* Please review this at http://gwt-code-reviews.appspot.com/1454805/ Affected files: M dev/core/src/com/google/gwt/dev/util/log/speedtracer/SpeedTracerLogger.java M user/src/com/google/gwt/user/rebind/rpc/BlacklistTypeFilter.java M user/src/com/google/gwt/user/rebind/rpc/CustomFieldSerializerValidator.java M user/src/com/google/gwt/user/rebind/rpc/FieldSerializerCreator.java M user/src/com/google/gwt/user/rebind/rpc/JModTypeVisitor.java M user/src/com/google/gwt/user/rebind/rpc/JTypeVisitor.java M user/src/com/google/gwt/user/rebind/rpc/ProblemReport.java M user/src/com/google/gwt/user/rebind/rpc/ProxyCreator.java M user/src/com/google/gwt/user/rebind/rpc/RemoteServiceAsyncValidator.java M user/src/com/google/gwt/user/rebind/rpc/SerializableTypeOracle.java M user/src/com/google/gwt/user/rebind/rpc/SerializableTypeOracleBuilder.java M user/src/com/google/gwt/user/rebind/rpc/SerializationUtils.java M user/src/com/google/gwt/user/rebind/rpc/ServiceInterfaceProxyGenerator.java M user/src/com/google/gwt/user/rebind/rpc/Shared.java M user/src/com/google/gwt/user/rebind/rpc/TypeConstrainer.java M user/src/com/google/gwt/user/rebind/rpc/TypeHierarchyUtils.java M user/src/com/google/gwt/user/rebind/rpc/TypeParameterExposureComputer.java M user/src/com/google/gwt/user/rebind/rpc/TypePaths.java M user/src/com/google/gwt/user/rebind/rpc/TypeSerializerCreator.java -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Formatting update for com/google/gwt/user/rebind/rpc/* (issue1454805)
http://gwt-code-reviews.appspot.com/1454805/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: HTML5 Geolocation support in GWT (issue1451811)
http://gwt-code-reviews.appspot.com/1451811/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Adds support for runtime evaluation of JavaScriptObject methods from a debugger. Primarily inten... (issue1453808)
http://gwt-code-reviews.appspot.com/1453808/diff/1/dev/core/src/com/google/gwt/core/ext/debug/JsoEval.java
File dev/core/src/com/google/gwt/core/ext/debug/JsoEval.java (right):
http://gwt-code-reviews.appspot.com/1453808/diff/1/dev/core/src/com/google/gwt/core/ext/debug/JsoEval.java#newcode47
dev/core/src/com/google/gwt/core/ext/debug/JsoEval.java:47: public class
JsoEval {
Add a TODO indicating that you're going to add a ref to the doc
explaining the JSO rewriting...it will make this class' implementation
easier to understand.
http://gwt-code-reviews.appspot.com/1453808/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Adds support for runtime evaluation of JavaScriptObject methods from a debugger. Primarily inten... (issue1453808)
LGTM. http://gwt-code-reviews.appspot.com/1453808/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Formatting update for com/google/gwt/user/rebind/rpc/* (issue1454805)
LGTM http://gwt-code-reviews.appspot.com/1454805/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: HTML5 Geolocation support in GWT (issue1451811)
http://gwt-code-reviews.appspot.com/1451811/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] [google-web-toolkit] r10299 committed - Minor changes to dashboard notifier interface to enable better session...
Revision: 10299
Author: jhumphr...@google.com
Date: Wed Jun 8 11:36:57 2011
Log: Minor changes to dashboard notifier interface to enable better
session-tracking
in notifier implementation. Also added some stuff to
...dev.shell.BrowserChannelServerTest to verify that the integration with
the
dashboard is behaving correctly.
Review at http://gwt-code-reviews.appspot.com/1450812
Review by: to...@google.com
http://code.google.com/p/google-web-toolkit/source/detail?r=10299
Modified:
/trunk/dev/core/src/com/google/gwt/dev/shell/BrowserChannelServer.java
/trunk/dev/core/src/com/google/gwt/dev/util/log/dashboard/DashboardNotifier.java
/trunk/dev/core/src/com/google/gwt/dev/util/log/dashboard/NoOpDashboardNotifier.java
/trunk/dev/core/test/com/google/gwt/dev/shell/BrowserChannelServerTest.java
/trunk/dev/core/test/com/google/gwt/dev/util/log/dashboard/DashboardNotifierFactoryTest.java
/trunk/dev/core/test/com/google/gwt/dev/util/log/dashboard/SpeedTracerLoggerTestMockNotifier.java
===
--- /trunk/dev/core/src/com/google/gwt/dev/shell/BrowserChannelServer.java
Fri May 6 10:22:03 2011
+++ /trunk/dev/core/src/com/google/gwt/dev/shell/BrowserChannelServer.java
Wed Jun 8 11:36:57 2011
@@ -19,6 +19,7 @@
import com.google.gwt.core.ext.TreeLogger.HelpInfo;
import
com.google.gwt.dev.shell.BrowserChannel.SessionHandler.ExceptionOrReturnValue;
import com.google.gwt.dev.shell.JsValue.DispatchObject;
+import com.google.gwt.dev.util.log.dashboard.DashboardNotifier;
import com.google.gwt.dev.util.log.dashboard.DashboardNotifierFactory;
import java.io.IOException;
@@ -150,7 +151,7 @@
this.ignoreRemoteDeath = ignoreRemoteDeath;
init(initialLogger);
}
-
+
/**
* Indicate that Java no longer has references to the supplied JS
objects.
*
@@ -380,6 +381,7 @@
* @throws IOException
*/
public void shutdown() throws IOException {
+getDashboardNotifier().devModeSessionEnded(devModeSession);
QuitMessage.send(this);
}
@@ -644,7 +646,16 @@
break;
}
}
-
+
+ /**
+ * Returns the {@code DashboardNotifier} used to send notices to a
dashboard
+ * service.
+ */
+ // @VisibleForTesting
+ DashboardNotifier getDashboardNotifier() {
+return DashboardNotifierFactory.getNotifier();
+ }
+
/**
* Creates the {@code DevModeSession} that represents the current browser
* connection, sets it as the default session for the current thread,
and
@@ -653,7 +664,7 @@
private void createDevModeSession() {
devModeSession = new DevModeSession(moduleName, userAgent);
DevModeSession.setSessionForCurrentThread(devModeSession);
-DashboardNotifierFactory.getNotifier().devModeSession(devModeSession);
+getDashboardNotifier().devModeSession(devModeSession);
}
/**
===
---
/trunk/dev/core/src/com/google/gwt/dev/util/log/dashboard/DashboardNotifier.java
Wed May 11 07:35:17 2011
+++
/trunk/dev/core/src/com/google/gwt/dev/util/log/dashboard/DashboardNotifier.java
Wed Jun 8 11:36:57 2011
@@ -28,19 +28,24 @@
// TODO(jhumphries) Add interface methods for collecting data from the
// compiler
- // Second: Dev-Mode related entry points
+ // Second: Devmode related entry points
/**
- * Records a top-level event to the dashboard.
+ * Notifies the dashboard of a top-level event.
*/
void devModeEvent(DevModeSession session, String eventType, long
startTimeNanos,
long durationNanos);
/**
- * Records a new module/session.
+ * Notifies the dashboard of a new session starting.
*/
void devModeSession(DevModeSession session);
+ /**
+ * Notifies the dashboard of a session ending.
+ */
+ void devModeSessionEnded(DevModeSession session);
+
// Third: Test related entry points
// TODO(jhumphries) Add interface methods for collecting data from
automated
===
---
/trunk/dev/core/src/com/google/gwt/dev/util/log/dashboard/NoOpDashboardNotifier.java
Wed May 11 07:35:17 2011
+++
/trunk/dev/core/src/com/google/gwt/dev/util/log/dashboard/NoOpDashboardNotifier.java
Wed Jun 8 11:36:57 2011
@@ -33,5 +33,10 @@
public void devModeSession(DevModeSession session) {
// do nothing
}
+
+ @Override
+ public void devModeSessionEnded(DevModeSession session) {
+// do nothing
+ }
}
===
---
/trunk/dev/core/test/com/google/gwt/dev/shell/BrowserChannelServerTest.java
Fri May 6 10:22:03 2011
+++
/trunk/dev/core/test/com/google/gwt/dev/shell/BrowserChannelServerTest.java
Wed Jun 8 11:36:57 2011
@@ -27,6 +27,7 @@
import com.google.gwt.dev.shell.BrowserChannel.UserAgentIconMessage;
import com.google.gwt.dev.shell.BrowserChannel.Value;
import com.google.gwt.dev.shell.BrowserChannelServer.SessionHandlerServer;
+import com.google.gwt.dev.util.log.dashboard.DashboardNotifier;
import
[gwt-contrib] [google-web-toolkit] r10300 committed - Adds support for runtime evaluation of JavaScriptObject methods from a...
Revision: 10300
Author: to...@google.com
Date: Wed Jun 8 11:39:45 2011
Log: Adds support for runtime evaluation of JavaScriptObject methods
from a debugger. Primarily intended as support API for debuggers, but
developers can also use it directly in a debugger (for example, in watch
windows or breakpoint expressions).
Review at http://gwt-code-reviews.appspot.com/1453808
Review by: rda...@google.com
http://code.google.com/p/google-web-toolkit/source/detail?r=10300
Added:
/trunk/dev/core/src/com/google/gwt/core/ext/debug
/trunk/dev/core/src/com/google/gwt/core/ext/debug/JsoEval.java
Modified:
/trunk/dev/core/src/com/google/gwt/dev/shell/CompilingClassLoader.java
===
--- /dev/null
+++ /trunk/dev/core/src/com/google/gwt/core/ext/debug/JsoEval.java Wed Jun
8 11:39:45 2011
@@ -0,0 +1,471 @@
+/*
+ * Copyright 2011 Google Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the License); you may
not
+ * use this file except in compliance with the License. You may obtain a
copy of
+ * the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an AS IS BASIS,
WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+ * License for the specific language governing permissions and limitations
under
+ * the License.
+ */
+package com.google.gwt.core.ext.debug;
+
+import java.io.PrintWriter;
+import java.io.StringWriter;
+import java.lang.reflect.InvocationTargetException;
+import java.lang.reflect.Method;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.HashMap;
+import java.util.Iterator;
+import java.util.Map;
+
+/**
+ * Provides facilities for debuggers to call methods on
+ * {@link com.google.gwt.core.client.JavaScriptObject JavaScriptObjects}.
+ * p/
+ * Because devmode does extensive rewriting of JSO bytecode, debuggers
can't
+ * figure out how to evaluate JSO method calls. This class can be used
directly
+ * by users to evaluate JSO methods in their debuggers. Additionally,
debuggers
+ * with GWT support use this class to transparently evaluate JSO
expressions in
+ * breakpoints, watch windows, etc.
+ * p
+ * Example uses:
+ * codepre
+ * JsoEval.call(Element.class, myElement, getAbsoluteTop);
+ * JsoEval.call(Node.class, myNode, cloneNode, Boolean.TRUE);
+ * JsoEval.call(Element.class,
element.getFirstChildElement(), setPropertyString, phase,
+ * gamma);
+ * /pre/code
+ * @noinspection UnusedDeclaration
+ */
+public class JsoEval {
+
+ /* TODO: Error messages generated from JsoEval are reported with mangled
+ * method names and signatures instead of original source code values.
+ * We could de-mangle the names for the errors, but it really only
matters
+ * for users who don't have IDE support.
+ */
+
+ // TODO: Update the wiki doc to include a better description of JSO
transformations and reference
+ // it from here.
+
+ private static MapClass,Class boxedTypeForPrimitiveType = new
HashMapClass,Class(8);
+ private static MapClass,Class primitiveTypeForBoxedType = new
HashMapClass,Class(8);
+
+ private static final String JSO_IMPL_CLASS
= com.google.gwt.core.client.JavaScriptObject$;
+
+ static {
+boxedTypeForPrimitiveType.put(boolean.class, Boolean.class);
+boxedTypeForPrimitiveType.put(byte.class, Byte.class);
+boxedTypeForPrimitiveType.put(short.class, Short.class);
+boxedTypeForPrimitiveType.put(char.class, Character.class);
+boxedTypeForPrimitiveType.put(int.class, Integer.class);
+boxedTypeForPrimitiveType.put(float.class, Float.class);
+boxedTypeForPrimitiveType.put(long.class, Long.class);
+boxedTypeForPrimitiveType.put(double.class, Double.class);
+
+for (Map.EntryClass,Class entry :
boxedTypeForPrimitiveType.entrySet()) {
+ primitiveTypeForBoxedType.put(entry.getValue(), entry.getKey());
+}
+ }
+
+ /**
+ * Reflectively invokes a method on a JavaScriptObject.
+ *
+ * @param klass Either a class of type JavaScriptObject or an interface
+ * implemented by a JavaScriptObject. The class must contain the method
to
+ * be invoked.
+ * @param obj The JavaScriptObject to invoke the method on. Must be null
if
+ * the method is static. Must be not-null if the method is not static
+ * @param methodName The name of the method
+ * @param types The types of the arguments
+ * @param args The values of the arguments
+ *
+ * @return The result of the method invocation or the failure as a String
+ */
+ public static Object call(Class klass, Object obj, String methodName,
Class[] types,
+ Object... args) {
+try {
+ return callEx(klass, obj, methodName, types, args);
+} catch (Exception e) {
+ return toString(e);
+}
+ }
+
+ /**
+ * A convenience form of
+ * {@link #call(Class, Object,
[gwt-contrib] Introduces the HasVisibility characteristic interface, due to popular (issue1451813)
Reviewers: jlabanca,
Description:
Introduces the HasVisibility characteristic interface, due to popular
demand.
Please review this at http://gwt-code-reviews.appspot.com/1451813/
Affected files:
A user/src/com/google/gwt/user/client/ui/HasVisibility.java
M user/src/com/google/gwt/user/client/ui/UIObject.java
Index: user/src/com/google/gwt/user/client/ui/HasVisibility.java
===
--- user/src/com/google/gwt/user/client/ui/HasVisibility.java (revision 0)
+++ user/src/com/google/gwt/user/client/ui/HasVisibility.java (revision 0)
@@ -0,0 +1,43 @@
+/*
+ * Copyright 2011 Google Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the License); you may
not
+ * use this file except in compliance with the License. You may obtain a
copy of
+ * the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an AS IS BASIS,
WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+ * License for the specific language governing permissions and limitations
under
+ * the License.
+ */
+package com.google.gwt.user.client.ui;
+
+/**
+ * Implemented by objects that have the visibility trait.
+ */
+public interface HasVisibility {
+
+ /**
+ * Determines whether or not this object is visible. Note that this does
not
+ * necessarily take into account whether or not the receiver's parent is
+ * visible, or even if it is attached to the
+ * {@link com.google.gwt.dom.client.Document Document}. The default
+ * implementation of this trait in {@link UIObject} is based on the
value of a
+ * dom element's style object's display attribute.
+ *
+ * @return codetrue/code if the object is visible
+ */
+ boolean isVisible();
+
+ /**
+ * Sets whether this object is visible.
+ *
+ * @param visible codetrue/code to show the object,
codefalse/code to
+ * hide it
+ */
+ void setVisible(boolean visible);
+
+}
\ No newline at end of file
Index: user/src/com/google/gwt/user/client/ui/UIObject.java
===
--- user/src/com/google/gwt/user/client/ui/UIObject.java(revision 10299)
+++ user/src/com/google/gwt/user/client/ui/UIObject.java(working copy)
@@ -123,7 +123,7 @@
*
* Style names can be space or comma separated.
*/
-public abstract class UIObject {
+public abstract class UIObject implements HasVisibility {
/**
* Stores a regular expression object to extract float values from the
@@ -583,10 +583,16 @@
}
/**
- * Determines whether or not this object is visible.
+ * Determines whether or not this object is visible. Note that this does
not
+ * necessarily take into account whether or not the receiver's parent is
+ * visible, or even if it is attached to the
+ * {@link com.google.gwt.dom.client.Document Document}. This default
+ * implementation is based on the value of the underlying element's style
+ * object's display attribute.
*
* @return codetrue/code if the object is visible
*/
+ @Override
public boolean isVisible() {
return isVisible(getElement());
}
@@ -735,6 +741,7 @@
* @param visible codetrue/code to show the object,
codefalse/code to
* hide it
*/
+ @Override
public void setVisible(boolean visible) {
setVisible(getElement(), visible);
}
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: HTML5 Geolocation support in GWT (issue1451811)
http://gwt-code-reviews.appspot.com/1451811/diff/2026/user/src/com/google/gwt/geolocation/client/Geolocation.java
File user/src/com/google/gwt/geolocation/client/Geolocation.java
(right):
http://gwt-code-reviews.appspot.com/1451811/diff/2026/user/src/com/google/gwt/geolocation/client/Geolocation.java#newcode160
user/src/com/google/gwt/geolocation/client/Geolocation.java:160: if
(isSupported()) {
This is inverted. getIfSupported() returns null if isSupported() is
true.
http://gwt-code-reviews.appspot.com/1451811/diff/3004/user/src/com/google/gwt/geolocation/client/Geolocation.java#newcode191
user/src/com/google/gwt/geolocation/client/Geolocation.java:191:
opt.enableHighAccuracy =
optio...@com.google.gwt.geolocation.client.Geolocation.PositionOptions::enableHighAccuracy;
inverted conditional? You want to execute this block if options is not
null.
http://gwt-code-reviews.appspot.com/1451811/diff/3004/user/src/com/google/gwt/geolocation/client/Geolocation.java#newcode212
user/src/com/google/gwt/geolocation/client/Geolocation.java:212: * /p
This comment isn't needed. You can't call this method is geolocation
isn't supported. Same for the other methods.
http://gwt-code-reviews.appspot.com/1451811/diff/3004/user/test/com/google/gwt/geolocation/client/GeolocationTest.java
File user/test/com/google/gwt/geolocation/client/GeolocationTest.java
(right):
http://gwt-code-reviews.appspot.com/1451811/diff/3004/user/test/com/google/gwt/geolocation/client/GeolocationTest.java#newcode45
user/test/com/google/gwt/geolocation/client/GeolocationTest.java:45:
assertNull(geolocation);
This is inverted. It won't be null if supported. But at least it
matches the class its testing :p
http://gwt-code-reviews.appspot.com/1451811/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Introduces the HasVisibility characteristic interface, due to popular (issue1451813)
LGTM http://gwt-code-reviews.appspot.com/1451813/diff/1/user/src/com/google/gwt/user/client/ui/UIObject.java File user/src/com/google/gwt/user/client/ui/UIObject.java (right): http://gwt-code-reviews.appspot.com/1451813/diff/1/user/src/com/google/gwt/user/client/ui/UIObject.java#newcode591 user/src/com/google/gwt/user/client/ui/UIObject.java:591: * object's display attribute. FWIW - The JavaDoc should be inherited from the interface. http://gwt-code-reviews.appspot.com/1451813/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: HTML5 Geolocation support in GWT (issue1451811)
http://gwt-code-reviews.appspot.com/1451811/diff/2026/user/src/com/google/gwt/geolocation/client/Geolocation.java
File user/src/com/google/gwt/geolocation/client/Geolocation.java
(right):
http://gwt-code-reviews.appspot.com/1451811/diff/2026/user/src/com/google/gwt/geolocation/client/Geolocation.java#newcode160
user/src/com/google/gwt/geolocation/client/Geolocation.java:160: if
(isSupported()) {
On 2011/06/08 21:58:44, jlabanca wrote:
This is inverted. getIfSupported() returns null if isSupported() is
true.
Whoops! Done.
http://gwt-code-reviews.appspot.com/1451811/diff/3004/user/src/com/google/gwt/geolocation/client/Geolocation.java#newcode191
user/src/com/google/gwt/geolocation/client/Geolocation.java:191:
opt.enableHighAccuracy =
optio...@com.google.gwt.geolocation.client.Geolocation.PositionOptions::enableHighAccuracy;
On 2011/06/08 21:58:44, jlabanca wrote:
inverted conditional? You want to execute this block if options is
not null.
Done.
http://gwt-code-reviews.appspot.com/1451811/diff/3004/user/src/com/google/gwt/geolocation/client/Geolocation.java#newcode212
user/src/com/google/gwt/geolocation/client/Geolocation.java:212: * /p
On 2011/06/08 21:58:44, jlabanca wrote:
This comment isn't needed. You can't call this method is geolocation
isn't
supported. Same for the other methods.
Done.
http://gwt-code-reviews.appspot.com/1451811/diff/3004/user/test/com/google/gwt/geolocation/client/GeolocationTest.java
File user/test/com/google/gwt/geolocation/client/GeolocationTest.java
(right):
http://gwt-code-reviews.appspot.com/1451811/diff/3004/user/test/com/google/gwt/geolocation/client/GeolocationTest.java#newcode45
user/test/com/google/gwt/geolocation/client/GeolocationTest.java:45:
assertNull(geolocation);
On 2011/06/08 21:58:44, jlabanca wrote:
This is inverted. It won't be null if supported. But at least it
matches the
class its testing :p
Done.
http://gwt-code-reviews.appspot.com/1451811/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: HTML5 Geolocation support in GWT (issue1451811)
http://gwt-code-reviews.appspot.com/1451811/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: HTML5 Geolocation support in GWT (issue1451811)
LGTM Since you've made so many changes, can you just sanity check that the code still works? http://gwt-code-reviews.appspot.com/1451811/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: HTML5 Geolocation support in GWT (issue1451811)
SGTM On 2011/06/08 22:28:52, jasonhall wrote: On 2011/06/08 22:21:30, jlabanca wrote: LGTM Since you've made so many changes, can you just sanity check that the code still works? I'm planning on writing a sample app tomorrow, which will catch any errors if they exist. Everything at least compiles and the simple test I have passes. Can I submit now, with the proviso that if I dig something up tomorrow in writing the sample app, I'll fix it here in a followup CL? http://gwt-code-reviews.appspot.com/1451811/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: CompileModule creates a serialized set of compilation units to represent (issue1448808)
LGTM with a few comments. http://gwt-code-reviews.appspot.com/1448808/diff/8042/dev/core/src/com/google/gwt/dev/javac/CompilationStateBuilder.java File dev/core/src/com/google/gwt/dev/javac/CompilationStateBuilder.java (right): http://gwt-code-reviews.appspot.com/1448808/diff/8042/dev/core/src/com/google/gwt/dev/javac/CompilationStateBuilder.java#newcode351 dev/core/src/com/google/gwt/dev/javac/CompilationStateBuilder.java:351: // them out. I'm confused about this. I see what looks like code for supporting this in UnitCache/PersistentUnitCache, but I'm not sure it's wired up. Should this be using unitCache.addArchivedUnit() ? http://gwt-code-reviews.appspot.com/1448808/diff/8042/dev/core/src/com/google/gwt/dev/javac/CompilationUnitArchive.java File dev/core/src/com/google/gwt/dev/javac/CompilationUnitArchive.java (right): http://gwt-code-reviews.appspot.com/1448808/diff/8042/dev/core/src/com/google/gwt/dev/javac/CompilationUnitArchive.java#newcode69 dev/core/src/com/google/gwt/dev/javac/CompilationUnitArchive.java:69: * the same order every time so that the output is deterministic. Stale comment. http://gwt-code-reviews.appspot.com/1448808/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] This patch substantially reduces the overhead of Java types in the output by minimizing vtable s... (issue1447821)
Reviewers: scottb, jbrosenberg,
Description:
This patch substantially reduces the overhead of Java types in the
output by minimizing vtable setup and virtual class literal fetches.
Improvements are anywhere from 5% to 10% uncompressed obfuscated JS.
The patch includes the following changes:
1) A new Immortal CodeGenType. Immortal CodeGenTypes are CodeGenTypes
that cannot even be pruned by the final pruner pass. They exist in order
to conveniently define code in Java (as opposed to encoding JS in Java
strings) which must be injected into the Javascript AST during
construction. They must consist of only static methods and static
fields, and static field initializers are only permitted to be literals
and JSO.createObject()/createArray(). In addition, they are guaranteed
to be hoisted prior to the first non-Immortal type statement.
2) A new SeedUtil Immortal type which provides utility functions for
setting up vtables. It eliminates empty constructor seed functions by
using 2 helper functions to setup anonymous closure versions. Something
like this before the patch:
function fooSeed() {}
_ = fooSeed.prototype = FooConstructor.prototype = new superType();
_.castableTypeMap = { ... }
_.getClass = function() {
return classLiteral;
}
is replaced with
defineSeed(seedId, superSeedid, { castableTypeMap }, FooConstructor1,
FooConstructor2, ...)
This has two effects. First, it reduces both compressed and uncompressed
codesize by a non-trivial amount by eliminating empty globally named
seed functions which are only used as prototype placeholders. Secondly,
it frees up extra obfuscated identifiers (by using numeric ids to
identifer function seeds) in the global scope. Note: the seedId is not
necessarily the queryId. Third, prototypes can be
looked up by seedId.
3) Eliminate of All getClass() overrides. Two designs were tried, one
which removes the overrides during AST generation and the other which
leaves them in, but removes them later in the compilation. The latter
turned out to be simpler and produce some side benefits. This works as
follows:
First, java.lang.Object is given a new Class? clazz field and
Object.getClass() returns it.
Second, the ClassLiteralHolder fields, which are evaluated last, install
the appropriate Class instance into this field on the appropriate
prototype. That is, the Class.createForClass() method for example,
creates a new ClassLiteral, looks up the appropriate prototype, and
installs it into the Object.clazz field for that type.
Third, a final pass, just prior to generating the Javascript AST, prunes
all getClass() overrides. The overrides are left in during the
GenerateJavaAST phase, because it is advantageous to allow some of them
to be inlined when
method call tightening allows it, and because it minimizes the amount of
changes to ControlFlowAnalyzer. CFA did had
to be changed to rescue ClassLiterals for any instantiated type, if the
getClass() method is live.
deRPC had to be modified to deal with the new scheme of looking up the
prototype/seed function by number instead of name, as well as the new
naming scheme when -XdisableClassMetadata is active.
Please review this at http://gwt-code-reviews.appspot.com/1447821/
Affected files:
M dev/core/src/com/google/gwt/core/ext/linker/SymbolData.java
M dev/core/src/com/google/gwt/core/ext/linker/impl/StandardSymbolData.java
M dev/core/src/com/google/gwt/dev/javac/testing/impl/JavaResourceBase.java
M dev/core/src/com/google/gwt/dev/jjs/JavaToJavaScriptCompiler.java
M dev/core/src/com/google/gwt/dev/jjs/ast/JProgram.java
A dev/core/src/com/google/gwt/dev/jjs/ast/JSeedIdOf.java
M dev/core/src/com/google/gwt/dev/jjs/ast/JTypeOracle.java
M dev/core/src/com/google/gwt/dev/jjs/ast/JVisitor.java
M dev/core/src/com/google/gwt/dev/jjs/impl/CodeSplitter.java
M dev/core/src/com/google/gwt/dev/jjs/impl/ControlFlowAnalyzer.java
M dev/core/src/com/google/gwt/dev/jjs/impl/FragmentExtractor.java
M dev/core/src/com/google/gwt/dev/jjs/impl/GenerateJavaAST.java
M dev/core/src/com/google/gwt/dev/jjs/impl/GenerateJavaScriptAST.java
M dev/core/src/com/google/gwt/dev/jjs/impl/GwtAstBuilder.java
M
dev/core/src/com/google/gwt/dev/jjs/impl/ImplementClassLiteralsAsFields.java
M dev/core/src/com/google/gwt/dev/jjs/impl/JavaToJavaScriptMap.java
M dev/core/src/com/google/gwt/dev/jjs/impl/Pruner.java
A dev/core/src/com/google/gwt/dev/jjs/impl/ReplaceGetClassOverrides.java
M dev/core/src/com/google/gwt/dev/jjs/impl/ToStringGenerationVisitor.java
M
dev/core/src/com/google/gwt/dev/js/JsSourceGenerationVisitorWithSizeBreakdown.java
M dev/core/src/com/google/gwt/dev/js/JsStackEmulator.java
M dev/core/src/com/google/gwt/dev/js/JsToStringGenerationVisitor.java
A dev/core/src/com/google/gwt/dev/js/ast/JsSeedIdOf.java
M dev/core/src/com/google/gwt/dev/js/ast/JsVisitor.java
M
dev/core/super/com/google/gwt/dev/jjs/intrinsic/com/google/gwt/lang/Array.java
A
[gwt-contrib] Re: This patch substantially reduces the overhead of Java types in the output by minimizing vtable s... (issue1447821)
BTW Scott, I know you might have liked this as three separate CLs,
unfortunately, I did the refactoring work by iterating continuously on
the three separate items (seed funcs, class literals, and injection of
code), and there were interdependencies (when I changed seed func
code, I had to update the class literal optimization and vice versa)
Also, in the past we have discussed using prototype copying instead of
chaining. It's something to look at in the future once we have a
benchmarking test for it, but I think it's elegant to use chaining or
now because it makes the deferred class literal installation easier.
-Ray
On Wed, Jun 8, 2011 at 4:05 PM, cromwell...@google.com wrote:
Reviewers: scottb, jbrosenberg,
Description:
This patch substantially reduces the overhead of Java types in the
output by minimizing vtable setup and virtual class literal fetches.
Improvements are anywhere from 5% to 10% uncompressed obfuscated JS.
The patch includes the following changes:
1) A new Immortal CodeGenType. Immortal CodeGenTypes are CodeGenTypes
that cannot even be pruned by the final pruner pass. They exist in order
to conveniently define code in Java (as opposed to encoding JS in Java
strings) which must be injected into the Javascript AST during
construction. They must consist of only static methods and static
fields, and static field initializers are only permitted to be literals
and JSO.createObject()/createArray(). In addition, they are guaranteed
to be hoisted prior to the first non-Immortal type statement.
2) A new SeedUtil Immortal type which provides utility functions for
setting up vtables. It eliminates empty constructor seed functions by
using 2 helper functions to setup anonymous closure versions. Something
like this before the patch:
function fooSeed() {}
_ = fooSeed.prototype = FooConstructor.prototype = new superType();
_.castableTypeMap = { ... }
_.getClass = function() {
return classLiteral;
}
is replaced with
defineSeed(seedId, superSeedid, { castableTypeMap }, FooConstructor1,
FooConstructor2, ...)
This has two effects. First, it reduces both compressed and uncompressed
codesize by a non-trivial amount by eliminating empty globally named
seed functions which are only used as prototype placeholders. Secondly,
it frees up extra obfuscated identifiers (by using numeric ids to
identifer function seeds) in the global scope. Note: the seedId is not
necessarily the queryId. Third, prototypes can be
looked up by seedId.
3) Eliminate of All getClass() overrides. Two designs were tried, one
which removes the overrides during AST generation and the other which
leaves them in, but removes them later in the compilation. The latter
turned out to be simpler and produce some side benefits. This works as
follows:
First, java.lang.Object is given a new Class? clazz field and
Object.getClass() returns it.
Second, the ClassLiteralHolder fields, which are evaluated last, install
the appropriate Class instance into this field on the appropriate
prototype. That is, the Class.createForClass() method for example,
creates a new ClassLiteral, looks up the appropriate prototype, and
installs it into the Object.clazz field for that type.
Third, a final pass, just prior to generating the Javascript AST, prunes
all getClass() overrides. The overrides are left in during the
GenerateJavaAST phase, because it is advantageous to allow some of them
to be inlined when
method call tightening allows it, and because it minimizes the amount of
changes to ControlFlowAnalyzer. CFA did had
to be changed to rescue ClassLiterals for any instantiated type, if the
getClass() method is live.
deRPC had to be modified to deal with the new scheme of looking up the
prototype/seed function by number instead of name, as well as the new
naming scheme when -XdisableClassMetadata is active.
Please review this at http://gwt-code-reviews.appspot.com/1447821/
Affected files:
M dev/core/src/com/google/gwt/core/ext/linker/SymbolData.java
M dev/core/src/com/google/gwt/core/ext/linker/impl/StandardSymbolData.java
M dev/core/src/com/google/gwt/dev/javac/testing/impl/JavaResourceBase.java
M dev/core/src/com/google/gwt/dev/jjs/JavaToJavaScriptCompiler.java
M dev/core/src/com/google/gwt/dev/jjs/ast/JProgram.java
A dev/core/src/com/google/gwt/dev/jjs/ast/JSeedIdOf.java
M dev/core/src/com/google/gwt/dev/jjs/ast/JTypeOracle.java
M dev/core/src/com/google/gwt/dev/jjs/ast/JVisitor.java
M dev/core/src/com/google/gwt/dev/jjs/impl/CodeSplitter.java
M dev/core/src/com/google/gwt/dev/jjs/impl/ControlFlowAnalyzer.java
M dev/core/src/com/google/gwt/dev/jjs/impl/FragmentExtractor.java
M dev/core/src/com/google/gwt/dev/jjs/impl/GenerateJavaAST.java
M dev/core/src/com/google/gwt/dev/jjs/impl/GenerateJavaScriptAST.java
M dev/core/src/com/google/gwt/dev/jjs/impl/GwtAstBuilder.java
M
dev/core/src/com/google/gwt/dev/jjs/impl/ImplementClassLiteralsAsFields.java
M
[gwt-contrib] [google-web-toolkit] r10301 committed - Introduces the HasVisibility characteristic interface, due to popular...
Revision: 10301
Author: rj...@google.com
Date: Wed Jun 8 13:02:49 2011
Log: Introduces the HasVisibility characteristic interface, due to
popular
demand.
Review at http://gwt-code-reviews.appspot.com/1451813
Review by: jlaba...@google.com
http://code.google.com/p/google-web-toolkit/source/detail?r=10301
Added:
/trunk/user/src/com/google/gwt/user/client/ui/HasVisibility.java
Modified:
/trunk/user/src/com/google/gwt/user/client/ui/UIObject.java
===
--- /dev/null
+++ /trunk/user/src/com/google/gwt/user/client/ui/HasVisibility.java Wed
Jun 8 13:02:49 2011
@@ -0,0 +1,43 @@
+/*
+ * Copyright 2011 Google Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the License); you may
not
+ * use this file except in compliance with the License. You may obtain a
copy of
+ * the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an AS IS BASIS,
WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+ * License for the specific language governing permissions and limitations
under
+ * the License.
+ */
+package com.google.gwt.user.client.ui;
+
+/**
+ * Implemented by objects that have the visibility trait.
+ */
+public interface HasVisibility {
+
+ /**
+ * Determines whether or not this object is visible. Note that this does
not
+ * necessarily take into account whether or not the receiver's parent is
+ * visible, or even if it is attached to the
+ * {@link com.google.gwt.dom.client.Document Document}. The default
+ * implementation of this trait in {@link UIObject} is based on the
value of a
+ * dom element's style object's display attribute.
+ *
+ * @return codetrue/code if the object is visible
+ */
+ boolean isVisible();
+
+ /**
+ * Sets whether this object is visible.
+ *
+ * @param visible codetrue/code to show the object,
codefalse/code to
+ * hide it
+ */
+ void setVisible(boolean visible);
+
+}
===
--- /trunk/user/src/com/google/gwt/user/client/ui/UIObject.java Mon Jun 6
04:09:34 2011
+++ /trunk/user/src/com/google/gwt/user/client/ui/UIObject.java Wed Jun 8
13:02:49 2011
@@ -123,7 +123,7 @@
*
* Style names can be space or comma separated.
*/
-public abstract class UIObject {
+public abstract class UIObject implements HasVisibility {
/**
* Stores a regular expression object to extract float values from the
@@ -582,11 +582,7 @@
return DOM.getElementProperty(getElement(), title);
}
- /**
- * Determines whether or not this object is visible.
- *
- * @return codetrue/code if the object is visible
- */
+ @Override
public boolean isVisible() {
return isVisible(getElement());
}
@@ -729,12 +725,7 @@
}
}
- /**
- * Sets whether this object is visible.
- *
- * @param visible codetrue/code to show the object,
codefalse/code to
- * hide it
- */
+ @Override
public void setVisible(boolean visible) {
setVisible(getElement(), visible);
}
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Autoformat of Widget in preparation for some new additions. (issue1447822)
Reviewers: pdr, Description: Autoformat of Widget in preparation for some new additions. Please review this at http://gwt-code-reviews.appspot.com/1447822/ Affected files: M user/src/com/google/gwt/user/client/ui/Widget.java -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Autoformat of Widget in preparation for some new additions. (issue1447822)
On 2011/06/08 23:14:41, rjrjr wrote: LGTM. http://gwt-code-reviews.appspot.com/1447822/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Enhancement to JsDuplicateFunctionRemover to remove duplicate virtual methods as well. That is, ... (issue1454806)
Reviewers: scottb, jbrosenberg,
Description:
Enhancement to JsDuplicateFunctionRemover to remove duplicate virtual
methods as well. That is, given
_.method1 = function(a) { body }
_.method2 = function(b) { body }
Hoist the duplicate virtual methods and replace with a named global
function.
function c(a) { body }
_.method1 = c;
_.method2 = c;
Produces a 1%-3% improvement in code size when stack stripping is
enabled.
Please review this at http://gwt-code-reviews.appspot.com/1454806/
Affected files:
M dev/core/src/com/google/gwt/dev/jjs/JavaToJavaScriptCompiler.java
M dev/core/src/com/google/gwt/dev/js/JsDuplicateFunctionRemover.java
Index: dev/core/src/com/google/gwt/dev/jjs/JavaToJavaScriptCompiler.java
===
--- dev/core/src/com/google/gwt/dev/jjs/JavaToJavaScriptCompiler.java
(revision 10277)
+++ dev/core/src/com/google/gwt/dev/jjs/JavaToJavaScriptCompiler.java
(working copy)
@@ -391,6 +391,8 @@
}
if (changed) {
JsUnusedFunctionRemover.exec(jsProgram);
+// run again
+JsObfuscateNamer.exec(jsProgram);
}
}
}
Index: dev/core/src/com/google/gwt/dev/js/JsDuplicateFunctionRemover.java
===
--- dev/core/src/com/google/gwt/dev/js/JsDuplicateFunctionRemover.java
(revision 10277)
+++ dev/core/src/com/google/gwt/dev/js/JsDuplicateFunctionRemover.java
(working copy)
@@ -46,9 +46,16 @@
private final MapJsName, JsName duplicateOriginalMap = new
IdentityHashMapJsName, JsName();
+private final MapJsFunction, JsFunction duplicateMethodOriginalMap =
new IdentityHashMapJsFunction, JsFunction();
+
+
private final StackJsNameRef invocationQualifiers = new
StackJsNameRef();
+// static / global methods
private final MapString, JsName uniqueBodies = new HashMapString,
JsName();
+
+// vtable methods
+private final MapString, JsFunction uniqueMethodBodies = new
HashMapString, JsFunction();
public DuplicateFunctionBodyRecorder() {
// Add sentinel to stop Stack.peek() from throwing exception.
@@ -84,20 +91,31 @@
return duplicateOriginalMap;
}
+public MapJsFunction, JsFunction getDuplicateMethodMap() {
+ return duplicateMethodOriginalMap;
+}
+
@Override
public boolean visit(JsFunction x, JsContext ctx) {
+ String fnSource = x.toSource();
+ String body = fnSource.substring(fnSource.indexOf(());
/*
- * Don't process anonymous functions.
+ * Static function processed separate from virtual functions
*/
if (x.getName() != null) {
-String fnSource = x.toSource();
-String body = fnSource.substring(fnSource.indexOf(());
JsName original = uniqueBodies.get(body);
if (original != null) {
duplicateOriginalMap.put(x.getName(), original);
} else {
uniqueBodies.put(body, x.getName());
}
+ } else if (x.isFromJava()) {
+ JsFunction original = uniqueMethodBodies.get(body);
+ if (original != null) {
+ duplicateMethodOriginalMap.put(x, original);
+ } else {
+ uniqueMethodBodies.put(body, x);
+ }
}
return true;
}
@@ -114,13 +132,27 @@
private class ReplaceDuplicateInvocationNameRefs extends JsModVisitor {
private final SetJsName blacklist;
+private MapJsFunction, JsFunction dupMethodMap;
+private MapJsFunction, JsName hoistMap;
private final MapJsName, JsName duplicateMap;
public ReplaceDuplicateInvocationNameRefs(MapJsName, JsName
duplicateMap,
-SetJsName blacklist) {
+SetJsName blacklist, MapJsFunction, JsFunction dupMethodMap,
+MapJsFunction, JsName hoistMap) {
this.duplicateMap = duplicateMap;
this.blacklist = blacklist;
+ this.dupMethodMap = dupMethodMap;
+ this.hoistMap = hoistMap;
+}
+
+@Override
+public void endVisit(JsFunction x, JsContext ctx) {
+ if (dupMethodMap.containsKey(x)) {
+
ctx.replaceMe(hoistMap.get(dupMethodMap.get(x)).makeRef(x.getSourceInfo()));
+ } else if (hoistMap.containsKey(x)) {
+ctx.replaceMe(hoistMap.get(x).makeRef(x.getSourceInfo()));
+ }
}
@Override
@@ -152,8 +184,25 @@
private boolean execImpl(JsBlock fragment) {
DuplicateFunctionBodyRecorder dfbr = new
DuplicateFunctionBodyRecorder();
dfbr.accept(fragment);
+int count = 0;
+MapJsFunction, JsName hoistMap = new HashMapJsFunction, JsName();
+// Hoist all anonymous versions
+MapJsFunction, JsFunction dupMethodMap =
dfbr.getDuplicateMethodMap();
+for (JsFunction x : dupMethodMap.values()) {
+ if (!hoistMap.containsKey(x)) {
+JsName newName = program.getScope().declareName(_DUP + count++);
+JsFunction newFunc = new
[gwt-contrib] JavaAstConstructor uses UnifyAst. (issue1453810)
Reviewers: zundel, jbrosenberg, Message: Convert JavaAstConstructor / JJSTestBase to use UnifyAst. Please review this at http://gwt-code-reviews.appspot.com/1453810/ Affected files: A dev/core/src/com/google/gwt/dev/jjs/AstConstructor.java M dev/core/src/com/google/gwt/dev/jjs/impl/GwtAstBuilder.java M dev/core/src/com/google/gwt/dev/jjs/impl/ReplaceRunAsyncs.java M dev/core/src/com/google/gwt/dev/jjs/impl/UnifyAst.java M dev/core/test/com/google/gwt/dev/jjs/JavaAstConstructor.java M dev/core/test/com/google/gwt/dev/jjs/impl/JJSTestBase.java -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: This patch substantially reduces the overhead of Java types in the output by minimizing vtable s... (issue1447821)
http://gwt-code-reviews.appspot.com/1447821/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: JavaAstConstructor uses UnifyAst. (issue1453810)
General question: I'm curious why this couldn't be simpler, that is, why does unification depend on control flow? Couldn't you unify everything without respect to control flow, and then run a pruning pass afterwards reusing the existing CFA code, or is doing it in an integrated pass an attempt to scope down the amount of work done? http://gwt-code-reviews.appspot.com/1453810/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] [google-web-toolkit] r10302 committed - Autoformat of Widget in preparation for some new additions....
Revision: 10302
Author: rj...@google.com
Date: Wed Jun 8 14:30:16 2011
Log: Autoformat of Widget in preparation for some new additions.
Review at http://gwt-code-reviews.appspot.com/1447822
Review by: p...@google.com
http://code.google.com/p/google-web-toolkit/source/detail?r=10302
Modified:
/trunk/user/src/com/google/gwt/user/client/ui/Widget.java
===
--- /trunk/user/src/com/google/gwt/user/client/ui/Widget.java Wed May 4
11:04:47 2011
+++ /trunk/user/src/com/google/gwt/user/client/ui/Widget.java Wed Jun 8
14:30:16 2011
@@ -1,12 +1,12 @@
/*
* Copyright 2008 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
@@ -29,26 +29,25 @@
import com.google.gwt.user.client.EventListener;
/**
- * The base class for the majority of user-interface objects. Widget adds
- * support for receiving events from the browser and being added directly
to
- * {@link com.google.gwt.user.client.ui.Panel panels}.
+ * The base class for the majority of user-interface objects. Widget adds
support for receiving
+ * events from the browser and being added directly to {@link
com.google.gwt.user.client.ui.Panel
+ * panels}.
*/
-public class Widget extends UIObject implements EventListener,
HasAttachHandlers,
-IsWidget {
+public class Widget extends UIObject implements EventListener,
HasAttachHandlers, IsWidget {
/**
- * This convenience method makes a null-safe call to
- * {@link IsWidget#asWidget()}.
- *
+ * This convenience method makes a null-safe call to {@link
IsWidget#asWidget()}.
+ *
* @return the widget aspect, or codenull/code if w is null
*/
public static Widget asWidgetOrNull(IsWidget w) {
return w == null ? null : w.asWidget();
}
+
/**
- * A bit-map of the events that should be sunk when the widget is
attached to
- * the DOM. (We delay the sinking of events to improve startup
performance.)
- * When the widget is attached, this is set to -1
+ * A bit-map of the events that should be sunk when the widget is
attached to the DOM. (We delay
+ * the sinking of events to improve startup performance.) When the
widget is attached, this is set
+ * to -1
* p
* Package protected to allow Composite to see it.
*/
@@ -63,21 +62,19 @@
}
/**
- * For a href=
-
* http://code.google.com/p/google-web-toolkit/wiki/UnderstandingMemoryLeaks;
- * browsers which do not leak/a, adds a native event handler to the
widget.
- * Note that, unlike the
- * {@link #addDomHandler(EventHandler,
com.google.gwt.event.dom.client.DomEvent.Type)}
- * implementation, there is no need to attach the widget to the DOM in
order
- * to cause the event handlers to be attached.
- *
+ * For a
href= http://code.google.com/p/google-web-toolkit/wiki/UnderstandingMemoryLeaks;
+ * browsers which do not leak/a, adds a native event handler to the
widget. Note that, unlike
+ * the {@link #addDomHandler(EventHandler,
com.google.gwt.event.dom.client.DomEvent.Type)}
+ * implementation, there is no need to attach the widget to the DOM in
order to cause the event
+ * handlers to be attached.
+ *
* @param H the type of handler to add
* @param type the event key
* @param handler the handler
* @return {@link HandlerRegistration} used to remove the handler
*/
- public final H extends EventHandler HandlerRegistration
addBitlessDomHandler(
- final H handler, DomEvent.TypeH type) {
+ public final H extends EventHandler HandlerRegistration
addBitlessDomHandler(final H handler,
+ DomEvent.TypeH type) {
assert handler != null : handler must not be null;
assert type != null : type must not be null;
sinkBitlessEvent(type.getName());
@@ -85,17 +82,16 @@
}
/**
- * Adds a native event handler to the widget and sinks the corresponding
- * native event. If you do not want to sink the native event, use the
generic
- * addHandler method instead.
- *
+ * Adds a native event handler to the widget and sinks the corresponding
native event. If you do
+ * not want to sink the native event, use the generic addHandler method
instead.
+ *
* @param H the type of handler to add
* @param type the event key
* @param handler the handler
* @return {@link HandlerRegistration} used to remove the handler
*/
- public final H extends EventHandler HandlerRegistration addDomHandler(
- final H handler, DomEvent.TypeH type) {
+ public final H extends EventHandler
[gwt-contrib] Re: JavaAstConstructor uses UnifyAst. (issue1453810)
On Wed, Jun 8, 2011 at 8:40 PM, cromwell...@google.com wrote: General question: I'm curious why this couldn't be simpler, that is, why does unification depend on control flow? Couldn't you unify everything without respect to control flow, and then run a pruning pass afterwards reusing the existing CFA code, or is doing it in an integrated pass an attempt to scope down the amount of work done? I think that could be one good reason; In some cases, there can be thousands of unreferenced types in a project. stitching those units would be a waste of time and memory. Also, since we often encounter projects that include untranslatable code on the source path, the JDT compile step and build Type Oracle step doesn't show the errors (unless you turn on strict mode). We rely on the AST building stage to report the important errors. -- Eric Z. Ayers Google Web Toolkit, Atlanta, GA USA -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] [google-web-toolkit] r10303 committed - I don't believe in magic, but as Scott points out, there are consisten...
Revision: 10303
Author: gwt.mirror...@gmail.com
Date: Wed Jun 8 16:12:42 2011
Log: I don't believe in magic, but as Scott points out, there are
consistent
uses of the term to mean compiler substitution of types, specifically
in java.lang.String and com.google.gwt.util.*. I took a stab at replacing
the word magic in some other contexts where I think different terminology
makes the comments or names clearer.
A grey area is the com.google.gwt.core.GWT class. Some of its methods are
re-written by the compiler, but in a way that is pretty clearly documented
and not as behind the scenes as the other uses.
Review at http://gwt-code-reviews.appspot.com/1453804
http://code.google.com/p/google-web-toolkit/source/detail?r=10303
Modified:
/trunk/dev/core/src/com/google/gwt/dev/jdt/AbstractCompiler.java
/trunk/dev/core/src/com/google/gwt/dev/jjs/impl/CastNormalizer.java
/trunk/dev/core/src/com/google/gwt/dev/jjs/impl/ControlFlowAnalyzer.java
/trunk/dev/core/src/com/google/gwt/dev/jjs/impl/ReplaceRunAsyncs.java
/trunk/dev/core/src/com/google/gwt/dev/jjs/impl/TypeTightener.java
/trunk/dev/core/src/com/google/gwt/dev/shell/DispatchClassInfo.java
/trunk/dev/core/src/com/google/gwt/dev/util/HttpHeaders.java
/trunk/dev/core/test/com/google/gwt/core/ext/linker/impl/SelectionScriptJavaScriptTest.java
/trunk/dev/core/test/com/google/gwt/dev/jjs/JavaAstConstructor.java
/trunk/dev/core/test/com/google/gwt/lang/LongLibTestBase.java
/trunk/user/src/com/google/gwt/core/client/impl/AsyncFragmentLoader.java
/trunk/user/src/com/google/gwt/i18n/client/LocalizableResource.java
/trunk/user/src/com/google/gwt/resources/rg/CssResourceGenerator.java
/trunk/user/src/com/google/gwt/user/rebind/ui/ImageBundleGenerator.java
/trunk/user/super/com/google/gwt/emul/java/lang/Object.java
===
--- /trunk/dev/core/src/com/google/gwt/dev/jdt/AbstractCompiler.java Thu
May 19 06:11:59 2011
+++ /trunk/dev/core/src/com/google/gwt/dev/jdt/AbstractCompiler.java Wed
Jun 8 16:12:42 2011
@@ -242,8 +242,9 @@
TreeLogger branch =
logger.branch(TreeLogger.SPAM, Scanning for additional
dependencies: + loc, null);
-// Examine the cud for magic types.
-//
+// Examine the cud for types outside of the flow of the original
Java
+// source.
+//
String[] typeNames = outer.doFindAdditionalTypesUsingJsni(branch,
unit);
addAdditionalTypes(branch, typeNames);
@@ -266,9 +267,9 @@
}
/**
- * Helper method for process() that receives the types found by
magic.
- * This causes the compiler to find the additional type, possibly
winding
- * its back to ask for the compilation unit from the source oracle.
+ * Helper method for process() that receives the types found outside
the
+ * flow of the original Java source. This causes the compiler to
find the
+ * additional type, possibly causing the type to be compiled from
source.
*/
private void addAdditionalTypes(TreeLogger logger, String[]
typeNames) {
for (String typeName : typeNames) {
===
--- /trunk/dev/core/src/com/google/gwt/dev/jjs/impl/CastNormalizer.java Tue
Jun 7 11:03:06 2011
+++ /trunk/dev/core/src/com/google/gwt/dev/jjs/impl/CastNormalizer.java Wed
Jun 8 16:12:42 2011
@@ -431,17 +431,20 @@
}
SourceInfo info = x.getSourceInfo();
if (toType instanceof JNullType) {
-/*
- * Magic: a null type cast means the user tried a cast that
couldn't
- * possibly work. Typically this means either the statically
resolvable
- * arg type is incompatible with the target type, or the target
type was
- * globally uninstantiable. We handle this cast by throwing a
- * ClassCastException, unless the argument is null.
+/**
+ * A null type cast is used as a placeholder value to indicate
that the
+ * user tried a cast that couldn't possibly work. Typically this
means
+ * either the statically resolvable arg type is incompatible with
the
+ * target type, or the target type was globally uninstantiable.
+ *
+ * See {@link
com.google.gwt.dev.jjs.impl.TypeTightener.TightenTypesVisitor#endVisit(JCastOperation,
+ * Context)}
+ *
+ * We handle this cast by throwing a ClassCastException, unless the
+ * argument is null.
*/
JMethod method =
program.getIndexedMethod(Cast.throwClassCastExceptionUnlessNull);
-/*
- * Override the type of the magic method with the null type.
- */
+// Note, we must update the method call to return the null type.
JMethodCall call = new JMethodCall(info, null, method, toType);
call.addArg(expr);
replaceExpr = call;
@@ -455,6 +458,7 @@
// just remove the cast
[gwt-contrib] Re: JavaAstConstructor uses UnifyAst. (issue1453810)
What Eric said. I haven't nailed down all the error reporting yet (although it's closer now), but the idea is that we really can avoid reporting errors on unreachable types. And not reaching code means fewer deserialized types, which is better for memory and takes less time. But there's an even bigger one... When code flow reaches GWT.create() calls, we have to run rebinds, and therefore generators, and reach even more code. So there's a spiraling effect. JDT used to limit the total reach by only pulling in the types necessarily to finish a compilation. If we didn't do SOME kind of prune off, we might end up being SLOWER than before. Since we have to do some prune off then, it makes sense to do a good job with it. -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: This patch substantially reduces the overhead of Java types in the output by minimizing vtable s... (issue1447821)
Don't worry about it, since I'm punting the review over to Jason and Eric. :P -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
