[gwt-contrib] RR: Exercise RunAsyncCode class for prefetching fragments. (issue1453805)
Reviewers: jbrosenberg, tobyr, Description: RR: Exercise RunAsyncCode class for prefetching fragments. Does testPreload() look right? We knew there was a bug in RunAsyncCode.isLoaded(), but now that that bug is fixed, I can't seem to invoke the preloading functionality. Please review this at http://gwt-code-reviews.appspot.com/1453805/ Affected files: M user/test/com/google/gwt/core/CoreSuite.java A user/test/com/google/gwt/core/client/prefetch/RunAsyncCodeTest.java -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Add SafeUri type, similar to SafeHtml but for values in a URL attribute context. (issue1446815)
Reviewers: tbroyer, jlabanca, jat, skybrian, Description: Add SafeUri type, similar to SafeHtml but for values in a URL attribute context. Support SafeUri-typed arguments in SafeHtmlTemplates. Also added a few overloads in c.g.g.user.client. Note that this is a breaking change in the sense that DataResource and ImageResource have a new getSafeUri method, as well as overloaded constructors taking URIs in the form of SafeUri. Patch by: t.bro...@gmail.com Fixes issues: 6145 Please review this at http://gwt-code-reviews.appspot.com/1446815/ Affected files: M tools/api-checker/config/gwt23_24userApi.conf M user/src/com/google/gwt/cell/client/ImageResourceCell.java M user/src/com/google/gwt/resources/Resources.gwt.xml M user/src/com/google/gwt/resources/client/DataResource.java M user/src/com/google/gwt/resources/client/ImageResource.java A user/src/com/google/gwt/resources/client/impl/DataResourcePrototype.java M user/src/com/google/gwt/resources/client/impl/ExternalTextResourcePrototype.java M user/src/com/google/gwt/resources/client/impl/ImageResourcePrototype.java M user/src/com/google/gwt/resources/css/Spriter.java M user/src/com/google/gwt/resources/rg/DataResourceGenerator.java M user/src/com/google/gwt/resources/rg/ExternalTextResourceGenerator.java M user/src/com/google/gwt/resources/rg/ImageResourceGenerator.java M user/src/com/google/gwt/safehtml/rebind/SafeHtmlTemplatesImplMethodCreator.java M user/src/com/google/gwt/safehtml/shared/SafeHtmlUtils.java A user/src/com/google/gwt/safehtml/shared/SafeUri.java A user/src/com/google/gwt/safehtml/shared/SafeUriHostedModeUtils.java A user/src/com/google/gwt/safehtml/shared/SafeUriString.java M user/src/com/google/gwt/safehtml/shared/UriUtils.java M user/src/com/google/gwt/user/client/ui/AbstractImagePrototype.java M user/src/com/google/gwt/user/client/ui/FormPanel.java M user/src/com/google/gwt/user/client/ui/Frame.java M user/src/com/google/gwt/user/client/ui/Image.java M user/src/com/google/gwt/user/client/ui/ImageResourceRenderer.java M user/src/com/google/gwt/user/client/ui/impl/ClippedImageImpl.java M user/src/com/google/gwt/user/client/ui/impl/ClippedImageImplIE6.java M user/src/com/google/gwt/user/client/ui/impl/ClippedImagePrototype.java A user/super/com/google/gwt/safehtml/super/com/google/gwt/safehtml/shared/SafeUriHostedModeUtils.java M user/test/com/google/gwt/resources/client/ImageResourceTest.java M user/test/com/google/gwt/safehtml/client/SafeHtmlTemplatesTest.java A user/test/com/google/gwt/safehtml/shared/GwtUriUtilsTest.java A user/test/com/google/gwt/safehtml/shared/UriUtilsTest.java M user/test/com/google/gwt/user/client/ui/ImageTest.java M user/test/com/google/gwt/user/client/ui/impl/ClippedImagePrototypeTest.java -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Add SafeUri type, similar to SafeHtml but for values in a URL attribute context. (issue1447812)
http://gwt-code-reviews.appspot.com/1447812/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Add SafeUri type, similar to SafeHtml but for values in a URL attribute context. (issue1447812)
Thanks for the review! Please take another look...
--xtof
http://gwt-code-reviews.appspot.com/1447812/diff/1/tools/api-checker/config/gwt23_24userApi.conf
File tools/api-checker/config/gwt23_24userApi.conf (right):
http://gwt-code-reviews.appspot.com/1447812/diff/1/tools/api-checker/config/gwt23_24userApi.conf#newcode155
tools/api-checker/config/gwt23_24userApi.conf:155:
com.google.gwt.user.client.ui.impl.ClippedImageImpl::adjust(Lcom/google/gwt/dom/client/Element;Ljava/lang/String;)
MISSING
On 2011/06/02 13:47:05, jlabanca wrote:
Instead of all of these exceptions for ClippedImageImpl, you can
exclude
com.google.gwt.user.client.ui.impl from api checks. We don't make any
guarantees
about the API of impl classes.
Done.
http://gwt-code-reviews.appspot.com/1447812/diff/1/user/src/com/google/gwt/resources/client/DataResource.java
File user/src/com/google/gwt/resources/client/DataResource.java (right):
http://gwt-code-reviews.appspot.com/1447812/diff/1/user/src/com/google/gwt/resources/client/DataResource.java#newcode66
user/src/com/google/gwt/resources/client/DataResource.java:66: * will be
an absolute URL.
On 2011/06/02 13:47:05, jlabanca wrote:
Should this be deprecated like ImageResource?
Done.
http://gwt-code-reviews.appspot.com/1447812/diff/1/user/src/com/google/gwt/safehtml/rebind/SafeHtmlTemplatesImplMethodCreator.java
File
user/src/com/google/gwt/safehtml/rebind/SafeHtmlTemplatesImplMethodCreator.java
(right):
http://gwt-code-reviews.appspot.com/1447812/diff/1/user/src/com/google/gwt/safehtml/rebind/SafeHtmlTemplatesImplMethodCreator.java#newcode432
user/src/com/google/gwt/safehtml/rebind/SafeHtmlTemplatesImplMethodCreator.java:432:
if (isSafeUri(parameterType)) {
On 2011/06/02 13:47:05, jlabanca wrote:
Is it safe to use safeUri in a text context? Seems like a mistake at
the least.
It would be safe here, since it's going to be HTML escaped just like any
other string. I can't think of too many reasons anyone would
legitimately do this. Perhaps in a template used to linkify URLs, as in
a href='{0}'{0}/a
where {0} is a SafeUri.
Seems like a pretty unlikely scenario, and I think I'll remove this
special case here in the interest of simplicity. In any case, per your
comment above I've made the change so that this would throw an error.
http://gwt-code-reviews.appspot.com/1447812/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/1447812/diff/1/user/src/com/google/gwt/safehtml/shared/SafeUriHostedModeUtils.java#newcode32
user/src/com/google/gwt/safehtml/shared/SafeUriHostedModeUtils.java:32:
public class SafeUriHostedModeUtils {
On 2011/06/02 13:47:05, jlabanca wrote:
Should this class be package protected? It looks like an impl class,
but its
public in a non-impl package, which makes it look like anyone can use
it.
That doesn't seem to work; if I make it package private I get
java.lang.IllegalAccessError:
com/google/gwt/safehtml/shared/SafeUriHostedModeUtils
at
com.google.gwt.safehtml.shared.UriUtils.fromTrustedString(UriUtils.java:199
all over the unit tests. I'm guessing that super-sourced classes need
to be public.
http://gwt-code-reviews.appspot.com/1447812/diff/1/user/src/com/google/gwt/safehtml/shared/SafeUriHostedModeUtils.java#newcode39
user/src/com/google/gwt/safehtml/shared/SafeUriHostedModeUtils.java:39:
* RFC 3987bis Web Addresses/a
On 2011/06/02 13:47:05, jlabanca wrote:
Will this look correct in javadoc? You might need to exceed the 100
line limit
here.
Done.
http://gwt-code-reviews.appspot.com/1447812/diff/1/user/src/com/google/gwt/safehtml/shared/SafeUriHostedModeUtils.java#newcode54
user/src/com/google/gwt/safehtml/shared/SafeUriHostedModeUtils.java:54:
public static final String FORCE_CHECK_VALID_URI =
com.google.gwt.safehtml.ForceCheckValidUri;
On 2011/06/02 13:47:05, jlabanca wrote:
Can you add a comment explaining how this is set?
Done.
http://gwt-code-reviews.appspot.com/1447812/diff/1/user/src/com/google/gwt/safehtml/shared/UriUtils.java
File user/src/com/google/gwt/safehtml/shared/UriUtils.java (right):
http://gwt-code-reviews.appspot.com/1447812/diff/1/user/src/com/google/gwt/safehtml/shared/UriUtils.java#newcode209
user/src/com/google/gwt/safehtml/shared/UriUtils.java:209: *
safe!/strong
On 2011/06/02 17:51:54, jlabanca wrote:
unsafeCastFromUntrustedString() is better. Can we also deprecate the
method?
Done. I'm having second thoughts about deprecating its callers though
(a bunch of methods of Image). We haven't deprecated plain string
methods for SafeHtml versions elsewhere either, so this would be
somewhat inconsistent.
I think I'll leave the Image(String) methods alone for now?
Use code should always be able to use one of the other methods. Only
library
code (GWT and libraries based on GWT) have the legacy support problem.
On 2011/06/02 17:45:16, xtof wrote:
On
[gwt-contrib] Re: RR: Exercise RunAsyncCode class for prefetching fragments. (issue1453805)
http://gwt-code-reviews.appspot.com/1453805/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Exercise RunAsyncCode class for prefetching fragments. (issue1453805)
http://gwt-code-reviews.appspot.com/1453805/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Making media events bitless, freeing up a few event bits, since modern, (issue1447816)
Reviewers: jlabanca, Description: Making media events bitless, freeing up a few event bits, since modern, implementing browsers don't leak the way old ones did. The following constants, marked as 'experimental', have been removed: - com.google.gwt.user.client.Event.MEDIAEVENTS - com.google.gwt.user.client.Event.ONCANPLAYTHROUGH - com.google.gwt.user.client.Event.ONENDED - com.google.gwt.user.client.Event.ONPROGRESS Please review this at http://gwt-code-reviews.appspot.com/1447816/ Affected files: M tools/api-checker/config/gwt23_24userApi.conf M user/src/com/google/gwt/media/client/MediaBase.java M user/src/com/google/gwt/user/client/Event.java M user/src/com/google/gwt/user/client/impl/DOMImpl.java M user/src/com/google/gwt/user/client/impl/DOMImplStandard.java M user/test/com/google/gwt/user/client/MediaEventsSinkTest.java -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Add SafeUri type, similar to SafeHtml but for values in a URL attribute context. (issue1447812)
LGTM http://gwt-code-reviews.appspot.com/1447812/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Add SafeUri type, similar to SafeHtml but for values in a URL attribute context. (issue1447812)
LGTM
http://gwt-code-reviews.appspot.com/1447812/diff/1/user/src/com/google/gwt/safehtml/rebind/SafeHtmlTemplatesImplMethodCreator.java
File
user/src/com/google/gwt/safehtml/rebind/SafeHtmlTemplatesImplMethodCreator.java
(right):
http://gwt-code-reviews.appspot.com/1447812/diff/1/user/src/com/google/gwt/safehtml/rebind/SafeHtmlTemplatesImplMethodCreator.java#newcode432
user/src/com/google/gwt/safehtml/rebind/SafeHtmlTemplatesImplMethodCreator.java:432:
if (isSafeUri(parameterType)) {
On 2011/06/03 07:39:23, xtof wrote:
On 2011/06/02 13:47:05, jlabanca wrote:
Is it safe to use safeUri in a text context? Seems like a mistake
at the
least.
It would be safe here, since it's going to be HTML escaped just like
any other
string. I can't think of too many reasons anyone would legitimately do
this.
Perhaps in a template used to linkify URLs, as in
a href='{0}'{0}/a
where {0} is a SafeUri.
Seems like a pretty unlikely scenario, and I think I'll remove this
special case
here in the interest of simplicity. In any case, per your comment
above I've
made the change so that this would throw an error.
I don't think its even possible. The check in emitParameterExpression
ensures that SafeUri is only used in a URL_ATTRIBUTE_ENTIRE context.
http://gwt-code-reviews.appspot.com/1447812/diff/1003/user/test/com/google/gwt/safehtml/client/SafeHtmlTemplatesTest.java
File user/test/com/google/gwt/safehtml/client/SafeHtmlTemplatesTest.java
(right):
http://gwt-code-reviews.appspot.com/1447812/diff/1003/user/test/com/google/gwt/safehtml/client/SafeHtmlTemplatesTest.java#newcode58
user/test/com/google/gwt/safehtml/client/SafeHtmlTemplatesTest.java:58:
@Template(spanb{0}/bspan{1}/span)
Missing a closing span. If you aren't testing something specific to the
malformed HTML, I suggest you add the closing span back on.
http://gwt-code-reviews.appspot.com/1447812/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Making media events bitless, freeing up a few event bits, since modern, (issue1447816)
http://gwt-code-reviews.appspot.com/1447816/diff/1/user/src/com/google/gwt/user/client/impl/DOMImplStandard.java File user/src/com/google/gwt/user/client/impl/DOMImplStandard.java (right): http://gwt-code-reviews.appspot.com/1447816/diff/1/user/src/com/google/gwt/user/client/impl/DOMImplStandard.java#newcode223 user/src/com/google/gwt/user/client/impl/DOMImplStandard.java:223: if (eventTypeName == drag) Does javascript allow switch statements with strings? If so, we might want to use a switch statement instead of else if checks. http://gwt-code-reviews.appspot.com/1447816/diff/1/user/src/com/google/gwt/user/client/impl/DOMImplStandard.java#newcode240 user/src/com/google/gwt/user/client/impl/DOMImplStandard.java:240: elem.onprogress = @com.google.gwt.user.client.impl.DOMImplStandard::dispatchEvent; Are we missing canplaythrough? http://gwt-code-reviews.appspot.com/1447816/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Making media events bitless, freeing up a few event bits, since modern, (issue1447816)
Yep and yep. Thanks. http://gwt-code-reviews.appspot.com/1447816/diff/1/user/src/com/google/gwt/user/client/impl/DOMImplStandard.java File user/src/com/google/gwt/user/client/impl/DOMImplStandard.java (right): http://gwt-code-reviews.appspot.com/1447816/diff/1/user/src/com/google/gwt/user/client/impl/DOMImplStandard.java#newcode223 user/src/com/google/gwt/user/client/impl/DOMImplStandard.java:223: if (eventTypeName == drag) On 2011/06/03 17:10:39, jlabanca wrote: Does javascript allow switch statements with strings? If so, we might want to use a switch statement instead of else if checks. Done. http://gwt-code-reviews.appspot.com/1447816/diff/1/user/src/com/google/gwt/user/client/impl/DOMImplStandard.java#newcode240 user/src/com/google/gwt/user/client/impl/DOMImplStandard.java:240: elem.onprogress = @com.google.gwt.user.client.impl.DOMImplStandard::dispatchEvent; On 2011/06/03 17:10:39, jlabanca wrote: Are we missing canplaythrough? Done. http://gwt-code-reviews.appspot.com/1447816/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Making media events bitless, freeing up a few event bits, since modern, (issue1447816)
LGTM assuming you fix those two things. http://gwt-code-reviews.appspot.com/1447816/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: fixes a bug in TypeOracle that marked vararg methods with the transient modifier, which is illeg... (issue1447817)
LGTM http://gwt-code-reviews.appspot.com/1447817/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Add armor for exceptions thrown by Activity#onCancel (issue1446816)
Reviewers: jlabanca, rchandia, Description: Add armor for exceptions thrown by Activity#onCancel Please review this at http://gwt-code-reviews.appspot.com/1446816/ Affected files: M user/src/com/google/gwt/activity/shared/ActivityManager.java M user/test/com/google/gwt/activity/shared/ActivityManagerTest.java -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Add armor for exceptions thrown by Activity#onCancel (issue1446816)
LGTM http://gwt-code-reviews.appspot.com/1446816/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Support is/has methods in Editor framework. (issue1443812)
Reviewers: rjrjr, Description: Support is/has methods in Editor framework. Issue 6040. Patch by: bobv Review by: rjrjr Please review this at http://gwt-code-reviews.appspot.com/1443812/ Affected files: M user/src/com/google/gwt/editor/rebind/model/EditorModel.java M user/test/com/google/gwt/editor/rebind/model/EditorModelTest.java -- 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] [google-web-toolkit] r10272 committed - Add armor for exceptions thrown by Activity#onCancel...
Revision: 10272
Author: rj...@google.com
Date: Fri Jun 3 08:57:30 2011
Log: Add armor for exceptions thrown by Activity#onCancel
Review at http://gwt-code-reviews.appspot.com/1446816
http://code.google.com/p/google-web-toolkit/source/detail?r=10272
Modified:
/trunk/user/src/com/google/gwt/activity/shared/ActivityManager.java
/trunk/user/test/com/google/gwt/activity/shared/ActivityManagerTest.java
===
--- /trunk/user/src/com/google/gwt/activity/shared/ActivityManager.java Mon
Apr 18 13:47:45 2011
+++ /trunk/user/src/com/google/gwt/activity/shared/ActivityManager.java Fri
Jun 3 08:57:30 2011
@@ -95,13 +95,12 @@
* be minimized by decent caching. Perenially slow activities might
mitigate
* this by providing a widget immediately, with some kind of loading
* treatment.
- *
- * @see
com.google.gwt.place.shared.PlaceChangeEvent.Handler#onPlaceChange(PlaceChangeEvent)
*/
public void onPlaceChange(PlaceChangeEvent event) {
Activity nextActivity = getNextActivity(event);
Throwable caughtOnStop = null;
+Throwable caughtOnCancel = null;
Throwable caughtOnStart = null;
if (nextActivity == null) {
@@ -115,7 +114,7 @@
if (startingNext) {
// The place changed again before the new current activity showed its
// widget
- currentActivity.onCancel();
+ caughtOnCancel = tryStopOrCancel(false);
currentActivity = NULL_ACTIVITY;
startingNext = false;
} else if (!currentActivity.equals(NULL_ACTIVITY)) {
@@ -126,46 +125,26 @@
* them accidentally firing as a side effect of its tear down
*/
stopperedEventBus.removeHandlers();
-
- try {
-currentActivity.onStop();
- } catch (Throwable t) {
-caughtOnStop = t;
- } finally {
-/*
- * And kill them off again in case it was naughty and added new
ones
- * during onstop
- */
-stopperedEventBus.removeHandlers();
- }
+ caughtOnStop = tryStopOrCancel(true);
}
currentActivity = nextActivity;
if (currentActivity.equals(NULL_ACTIVITY)) {
showWidget(null);
- return;
-}
-
-startingNext = true;
-
-/*
- * Now start the thing. Wrap the actual display with a per-call
instance
- * that protects the display from canceled or stopped activities, and
which
- * maintains our startingNext state.
- */
-try {
- currentActivity.start(new ProtectedDisplay(currentActivity),
- stopperedEventBus);
-} catch (Throwable t) {
- caughtOnStart = t;
-}
-
-if (caughtOnStart != null || caughtOnStop != null) {
+} else {
+ startingNext = true;
+ caughtOnStart = tryStart();
+}
+
+if (caughtOnStart != null || caughtOnCancel != null || caughtOnStop !=
null) {
SetThrowable causes = new LinkedHashSetThrowable();
if (caughtOnStop != null) {
causes.add(caughtOnStop);
}
+ if (caughtOnCancel != null) {
+causes.add(caughtOnCancel);
+ }
if (caughtOnStart != null) {
causes.add(caughtOnStart);
}
@@ -180,9 +159,7 @@
* @see
com.google.gwt.place.shared.PlaceChangeRequestEvent.Handler#onPlaceChangeRequest(PlaceChangeRequestEvent)
*/
public void onPlaceChangeRequest(PlaceChangeRequestEvent event) {
-if (!currentActivity.equals(NULL_ACTIVITY)) {
- event.setWarning(currentActivity.mayStop());
-}
+event.setWarning(currentActivity.mayStop());
}
/**
@@ -203,6 +180,41 @@
updateHandlers(willBeActive);
}
}
+
+ public Throwable tryStart() {
+Throwable caughtOnStart = null;
+try {
+ /* Wrap the actual display with a per-call instance
+ * that protects the display from canceled or stopped activities,
and which
+ * maintains our startingNext state.
+ */
+ currentActivity.start(new ProtectedDisplay(currentActivity),
+ stopperedEventBus);
+} catch (Throwable t) {
+ caughtOnStart = t;
+}
+return caughtOnStart;
+ }
+
+ public Throwable tryStopOrCancel(boolean stop) {
+Throwable caughtOnStop = null;
+try {
+ if (stop) {
+currentActivity.onStop();
+ } else {
+currentActivity.onCancel();
+ }
+} catch (Throwable t) {
+ caughtOnStop = t;
+} finally {
+ /*
+ * Kill off the handlers again in case it was naughty and added new
ones
+ * during onstop or oncancel
+ */
+ stopperedEventBus.removeHandlers();
+}
+return caughtOnStop;
+ }
private Activity getNextActivity(PlaceChangeEvent event) {
if (display == null) {
===
---
/trunk/user/test/com/google/gwt/activity/shared/ActivityManagerTest.java
Mon Apr 18 16:25:25 2011
+++
/trunk/user/test/com/google/gwt/activity/shared/ActivityManagerTest.java
Fri Jun 3 08:57:30 2011
@@ -305,6 +305,53 @@
[gwt-contrib] Re: Support is/has methods in Editor framework. (issue1443812)
LGTM On 2011/06/03 18:49:28, bobv wrote: http://gwt-code-reviews.appspot.com/1443812/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Making media events bitless, freeing up a few event bits, since modern, (issue1447816)
http://gwt-code-reviews.appspot.com/1447816/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Making media events bitless, freeing up a few event bits, since modern, (issue1447816)
LGTM http://gwt-code-reviews.appspot.com/1447816/diff/2002/user/src/com/google/gwt/user/client/impl/DOMImplStandard.java File user/src/com/google/gwt/user/client/impl/DOMImplStandard.java (right): http://gwt-code-reviews.appspot.com/1447816/diff/2002/user/src/com/google/gwt/user/client/impl/DOMImplStandard.java#newcode248 user/src/com/google/gwt/user/client/impl/DOMImplStandard.java:248: // re-entrant safe Can you expand this comment to say: These events are re-entrant safe because they are only available on modern browsers that do not leak memory. http://gwt-code-reviews.appspot.com/1447816/diff/2002/user/src/com/google/gwt/user/client/impl/DOMImplStandard.java#newcode254 user/src/com/google/gwt/user/client/impl/DOMImplStandard.java:254: throw eventTypeName; change to: throw Trying to sink unknown event type + eventTypeName; http://gwt-code-reviews.appspot.com/1447816/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Make private two new methods accidentally exposed in ActivityManager (issue1449811)
Reviewers: rchandia,
Description:
Make private two new methods accidentally exposed in ActivityManager
Please review this at http://gwt-code-reviews.appspot.com/1449811/
Affected files:
M user/src/com/google/gwt/activity/shared/ActivityManager.java
Index: user/src/com/google/gwt/activity/shared/ActivityManager.java
===
--- user/src/com/google/gwt/activity/shared/ActivityManager.java (revision
10272)
+++ user/src/com/google/gwt/activity/shared/ActivityManager.java (working
copy)
@@ -181,7 +181,25 @@
}
}
- public Throwable tryStart() {
+ private Activity getNextActivity(PlaceChangeEvent event) {
+if (display == null) {
+ /*
+ * Display may have been nulled during PlaceChangeEvent dispatch.
Don't
+ * bother the mapper, just return a null to ensure we shut down the
+ * current activity
+ */
+ return null;
+}
+return mapper.getActivity(event.getNewPlace());
+ }
+
+ private void showWidget(IsWidget view) {
+if (display != null) {
+ display.setWidget(view);
+}
+ }
+
+ private Throwable tryStart() {
Throwable caughtOnStart = null;
try {
/* Wrap the actual display with a per-call instance
@@ -196,7 +214,7 @@
return caughtOnStart;
}
- public Throwable tryStopOrCancel(boolean stop) {
+ private Throwable tryStopOrCancel(boolean stop) {
Throwable caughtOnStop = null;
try {
if (stop) {
@@ -214,24 +232,6 @@
stopperedEventBus.removeHandlers();
}
return caughtOnStop;
- }
-
- private Activity getNextActivity(PlaceChangeEvent event) {
-if (display == null) {
- /*
- * Display may have been nulled during PlaceChangeEvent dispatch.
Don't
- * bother the mapper, just return a null to ensure we shut down the
- * current activity
- */
- return null;
-}
-return mapper.getActivity(event.getNewPlace());
- }
-
- private void showWidget(IsWidget view) {
-if (display != null) {
- display.setWidget(view);
-}
}
private void updateHandlers(boolean activate) {
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Make private two new methods accidentally exposed in ActivityManager (issue1449811)
LGTM http://gwt-code-reviews.appspot.com/1449811/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: CompileModule creates a serialized set of compilation units to represent (issue1448808)
http://gwt-code-reviews.appspot.com/1448808/diff/6001/dev/core/src/com/google/gwt/dev/CompileModule.java
File dev/core/src/com/google/gwt/dev/CompileModule.java (right):
http://gwt-code-reviews.appspot.com/1448808/diff/6001/dev/core/src/com/google/gwt/dev/CompileModule.java#newcode54
dev/core/src/com/google/gwt/dev/CompileModule.java:54: *
Can we automatically re-order a set of modules so they will be processed
in optimal order? Perhaps add a utility method to ModuleDefLoader?
Otherwise, users could fail to get optimal benefit from the caching, and
not be aware of it?
http://gwt-code-reviews.appspot.com/1448808/diff/6001/dev/core/src/com/google/gwt/dev/CompileModule.java#newcode173
dev/core/src/com/google/gwt/dev/CompileModule.java:173: SetString
archivedResourcePaths = new HashSetString();
Also add moduleToCompile attribute to this event?
http://gwt-code-reviews.appspot.com/1448808/diff/6001/dev/core/src/com/google/gwt/dev/CompileModule.java#newcode185
dev/core/src/com/google/gwt/dev/CompileModule.java:185:
SpeedTracerLogger.Event loadArchive =
maybe change module to dependentModule here; or subModule?
http://gwt-code-reviews.appspot.com/1448808/diff/6001/dev/core/src/com/google/gwt/dev/CompileModule.java#newcode191
dev/core/src/com/google/gwt/dev/CompileModule.java:191: // Pre-populate
CompilationStateBuilder with .gwt files
Add a comment for this if case?
http://gwt-code-reviews.appspot.com/1448808/diff/6001/dev/core/src/com/google/gwt/dev/CompileModule.java#newcode192
dev/core/src/com/google/gwt/dev/CompileModule.java:192: if
(archive.getTopModuleName().equals(moduleToCompile)) {
I still don't see how this doesn't essentially throw away any caching
for the
top level module. Can't it load units contained in it's archive, and
write them
back, along with any newly compiled ones, when you write back the
archive?
Otherwise, it seems no previously compiled, cached units can be used for
any top
level module, ever?
And since the first time a module is compiled, no archives would exist
for it,
or any of it's sub-modules, it seems then no caching would ever be
reused going
forward (since all units end up in the top level module, if there are no
compiled archives for sub-modules to start with)? Am I missing
something?
Recursively, this would apply for sub-modules too, no? They'd always
want to
recompile themselves entirely?
Isn't it simpler to only ever put units for a given module in it's
archive (and not include units for sub-modules)? Then things are not
dependent so much on the order modules are processed, and separate
top-level modules which might share dependencies on sub-modules won't
ever duplicate storage of cached units, etc.
http://gwt-code-reviews.appspot.com/1448808/diff/6001/dev/core/src/com/google/gwt/dev/cfg/ModuleDef.java
File dev/core/src/com/google/gwt/dev/cfg/ModuleDef.java (right):
http://gwt-code-reviews.appspot.com/1448808/diff/6001/dev/core/src/com/google/gwt/dev/cfg/ModuleDef.java#newcode207
dev/core/src/com/google/gwt/dev/cfg/ModuleDef.java:207: * Free up memory
no longer needed in later compile stages. After calling this
s/ResourceOraclewill/ResourceOracle will/
http://gwt-code-reviews.appspot.com/1448808/diff/6001/dev/core/src/com/google/gwt/dev/cfg/ModuleDef.java#newcode309
dev/core/src/com/google/gwt/dev/cfg/ModuleDef.java:309:
how about getAllCompilationUnitArchiveURLs()
Since it doesn't return a set of archives, but archiveURLs?
http://gwt-code-reviews.appspot.com/1448808/diff/6001/dev/core/src/com/google/gwt/dev/javac/CompilationProblemReporter.java
File
dev/core/src/com/google/gwt/dev/javac/CompilationProblemReporter.java
(right):
http://gwt-code-reviews.appspot.com/1448808/diff/6001/dev/core/src/com/google/gwt/dev/javac/CompilationProblemReporter.java#newcode53
dev/core/src/com/google/gwt/dev/javac/CompilationProblemReporter.java:53:
javadoc?
http://gwt-code-reviews.appspot.com/1448808/diff/6001/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/6001/dev/core/src/com/google/gwt/dev/javac/CompilationUnitArchive.java#newcode75
dev/core/src/com/google/gwt/dev/javac/CompilationUnitArchive.java:75:
javadoc? explanation for topModuleName param?
http://gwt-code-reviews.appspot.com/1448808/diff/6001/dev/core/src/com/google/gwt/dev/javac/CompilationUnitArchive.java#newcode103
dev/core/src/com/google/gwt/dev/javac/CompilationUnitArchive.java:103:
need to read/write topModuleName?
http://gwt-code-reviews.appspot.com/1448808/diff/6001/dev/core/test/com/google/gwt/dev/javac/CompilationUnitArchiveTest.java
File
dev/core/test/com/google/gwt/dev/javac/CompilationUnitArchiveTest.java
(right):
http://gwt-code-reviews.appspot.com/1448808/diff/6001/dev/core/test/com/google/gwt/dev/javac/CompilationUnitArchiveTest.java#newcode92
dev/core/test/com/google/gwt/dev/javac/CompilationUnitArchiveTest.java:92:
unit1 - unit, m2-archive
[gwt-contrib] Re: CompileModule creates a serialized set of compilation units to represent (issue1448808)
First set of comments
http://gwt-code-reviews.appspot.com/1448808/diff/6001/dev/core/src/com/google/gwt/dev/CompileModule.java
File dev/core/src/com/google/gwt/dev/CompileModule.java (right):
http://gwt-code-reviews.appspot.com/1448808/diff/6001/dev/core/src/com/google/gwt/dev/CompileModule.java#newcode122
dev/core/src/com/google/gwt/dev/CompileModule.java:122: public static
void main(String[] args) {
This code is almost identical to Compile.main as well as others. Can we
refactor it?
It's especially disturbing to see the 4-line comment about System.exit
duplicated in 11 different places in our codebase.
http://gwt-code-reviews.appspot.com/1448808/diff/6001/dev/core/src/com/google/gwt/dev/CompileModule.java#newcode192
dev/core/src/com/google/gwt/dev/CompileModule.java:192: if
(archive.getTopModuleName().equals(moduleToCompile)) {
Why are we not loading up CompilationUnits for the module we're
compiling? For example, if the module has 100 source files, but only 1
is stale, won't the caching we already have work correctly to keep the
99 non-stale units and throw away the single stale unit?
Also, since we're rolling up dependent modules, isn't this going to
cause a miss on all those dependent modules included in that archive?
http://gwt-code-reviews.appspot.com/1448808/diff/6001/dev/core/src/com/google/gwt/dev/cfg/ModuleDef.java
File dev/core/src/com/google/gwt/dev/cfg/ModuleDef.java (right):
http://gwt-code-reviews.appspot.com/1448808/diff/6001/dev/core/src/com/google/gwt/dev/cfg/ModuleDef.java#newcode94
dev/core/src/com/google/gwt/dev/cfg/ModuleDef.java:94: private final
SetURL archiveURLs = new LinkedHashSetURL();
I know this class is mostly comment free, but it would be great to have
a comment here explaining what archiveURLs are.
http://gwt-code-reviews.appspot.com/1448808/diff/6001/dev/core/src/com/google/gwt/dev/cfg/ModuleDef.java#newcode94
dev/core/src/com/google/gwt/dev/cfg/ModuleDef.java:94: private final
SetURL archiveURLs = new LinkedHashSetURL();
Turns out you don't want to use SetURL, because URL.equals() has
really bad performance behavior:
http://michaelscharf.blogspot.com/2006/11/javaneturlequals-and-hashcode-make.html
You can use a different datatype like URI or String, or change this to a
Collection that doesn't use equals by default.(Though that can be
dangerous, because someone can accidentally use a method like contains()
that will use equals).
http://gwt-code-reviews.appspot.com/1448808/diff/6001/dev/core/src/com/google/gwt/dev/cfg/ModuleDef.java#newcode141
dev/core/src/com/google/gwt/dev/cfg/ModuleDef.java:141: public void
addCompilationUnitArchiveURL(URL url) {
Should this be package access?
http://gwt-code-reviews.appspot.com/1448808/diff/6001/dev/core/src/com/google/gwt/dev/cfg/ModuleDef.java#newcode310
dev/core/src/com/google/gwt/dev/cfg/ModuleDef.java:310: public URL[]
getAllCompilationUnitArchives() {
Why not just use the Set? If you're being defensive, why not
Collections.unmodifiableSet()?
http://gwt-code-reviews.appspot.com/1448808/diff/6001/dev/core/src/com/google/gwt/dev/cfg/ModuleDefLoader.java
File dev/core/src/com/google/gwt/dev/cfg/ModuleDefLoader.java (left):
http://gwt-code-reviews.appspot.com/1448808/diff/6001/dev/core/src/com/google/gwt/dev/cfg/ModuleDefLoader.java#oldcode114
dev/core/src/com/google/gwt/dev/cfg/ModuleDefLoader.java:114: * {@link
#loadFromClassPath(logger, moduleName, false)}.
This should be {@link #loadFromClassPath(TreeLogger, String, boolean)
loadFromClassPath(logger, moduleName, false)}
http://gwt-code-reviews.appspot.com/1448808/diff/6001/dev/core/src/com/google/gwt/dev/cfg/ModuleDefLoader.java
File dev/core/src/com/google/gwt/dev/cfg/ModuleDefLoader.java (right):
http://gwt-code-reviews.appspot.com/1448808/diff/6001/dev/core/src/com/google/gwt/dev/cfg/ModuleDefLoader.java#newcode71
dev/core/src/com/google/gwt/dev/cfg/ModuleDefLoader.java:71: public
static final String COMPILATION_UNIT_ARCHIVE_SUFFIX = .gwt;
.gwt seems a bit generic. Some ideas: .gar (GWT archive), .gwt-mar (GWT
module archive), .gwt-module-cache (self explanatory)
http://gwt-code-reviews.appspot.com/1448808/diff/6001/dev/core/src/com/google/gwt/dev/cfg/ModuleDefLoader.java#newcode267
dev/core/src/com/google/gwt/dev/cfg/ModuleDefLoader.java:267: String
compilationUnitArchiveName = slashedModuleName +
ModuleDefLoader.COMPILATION_UNIT_ARCHIVE_SUFFIX;
It seems a bit strange to me that all dependent modules are being rolled
up. Doing that seems like you're going to end up with copies of
dependencies in different modules.
http://gwt-code-reviews.appspot.com/1448808/diff/6001/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/6001/dev/core/src/com/google/gwt/dev/javac/CompilationUnitArchive.java#newcode74
dev/core/src/com/google/gwt/dev/javac/CompilationUnitArchive.java:74:
private transient SortedMapString,
[gwt-contrib] [google-web-toolkit] r10273 committed - Make private two new methods accidentally exposed in ActivityManager...
Revision: 10273
Author: rj...@google.com
Date: Fri Jun 3 09:52:12 2011
Log: Make private two new methods accidentally exposed in
ActivityManager
Review at http://gwt-code-reviews.appspot.com/1449811
Review by: rchan...@google.com
http://code.google.com/p/google-web-toolkit/source/detail?r=10273
Modified:
/trunk/user/src/com/google/gwt/activity/shared/ActivityManager.java
===
--- /trunk/user/src/com/google/gwt/activity/shared/ActivityManager.java Fri
Jun 3 08:57:30 2011
+++ /trunk/user/src/com/google/gwt/activity/shared/ActivityManager.java Fri
Jun 3 09:52:12 2011
@@ -181,7 +181,25 @@
}
}
- public Throwable tryStart() {
+ private Activity getNextActivity(PlaceChangeEvent event) {
+if (display == null) {
+ /*
+ * Display may have been nulled during PlaceChangeEvent dispatch.
Don't
+ * bother the mapper, just return a null to ensure we shut down the
+ * current activity
+ */
+ return null;
+}
+return mapper.getActivity(event.getNewPlace());
+ }
+
+ private void showWidget(IsWidget view) {
+if (display != null) {
+ display.setWidget(view);
+}
+ }
+
+ private Throwable tryStart() {
Throwable caughtOnStart = null;
try {
/* Wrap the actual display with a per-call instance
@@ -196,7 +214,7 @@
return caughtOnStart;
}
- public Throwable tryStopOrCancel(boolean stop) {
+ private Throwable tryStopOrCancel(boolean stop) {
Throwable caughtOnStop = null;
try {
if (stop) {
@@ -215,24 +233,6 @@
}
return caughtOnStop;
}
-
- private Activity getNextActivity(PlaceChangeEvent event) {
-if (display == null) {
- /*
- * Display may have been nulled during PlaceChangeEvent dispatch.
Don't
- * bother the mapper, just return a null to ensure we shut down the
- * current activity
- */
- return null;
-}
-return mapper.getActivity(event.getNewPlace());
- }
-
- private void showWidget(IsWidget view) {
-if (display != null) {
- display.setWidget(view);
-}
- }
private void updateHandlers(boolean activate) {
if (activate) {
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Add SafeUri type, similar to SafeHtml but for values in a URL attribute context. (issue1447812)
http://gwt-code-reviews.appspot.com/1447812/diff/1/user/src/com/google/gwt/safehtml/rebind/SafeHtmlTemplatesImplMethodCreator.java
File
user/src/com/google/gwt/safehtml/rebind/SafeHtmlTemplatesImplMethodCreator.java
(right):
http://gwt-code-reviews.appspot.com/1447812/diff/1/user/src/com/google/gwt/safehtml/rebind/SafeHtmlTemplatesImplMethodCreator.java#newcode432
user/src/com/google/gwt/safehtml/rebind/SafeHtmlTemplatesImplMethodCreator.java:432:
if (isSafeUri(parameterType)) {
On 2011/06/03 17:00:04, jlabanca wrote:
On 2011/06/03 07:39:23, xtof wrote:
On 2011/06/02 13:47:05, jlabanca wrote:
Is it safe to use safeUri in a text context? Seems like a mistake
at the
least.
It would be safe here, since it's going to be HTML escaped just like
any other
string. I can't think of too many reasons anyone would legitimately
do this.
Perhaps in a template used to linkify URLs, as in
a href='{0}'{0}/a
where {0} is a SafeUri.
Seems like a pretty unlikely scenario, and I think I'll remove this
special
case
here in the interest of simplicity. In any case, per your comment
above I've
made the change so that this would throw an error.
I don't think its even possible. The check in emitParameterExpression
ensures
that SafeUri is only used in a URL_ATTRIBUTE_ENTIRE context.
Right. Sorry that's what I meant to say, but clearly didn't state it
very well :)
http://gwt-code-reviews.appspot.com/1447812/diff/1003/user/test/com/google/gwt/safehtml/client/SafeHtmlTemplatesTest.java
File user/test/com/google/gwt/safehtml/client/SafeHtmlTemplatesTest.java
(right):
http://gwt-code-reviews.appspot.com/1447812/diff/1003/user/test/com/google/gwt/safehtml/client/SafeHtmlTemplatesTest.java#newcode58
user/test/com/google/gwt/safehtml/client/SafeHtmlTemplatesTest.java:58:
@Template(spanb{0}/bspan{1}/span)
On 2011/06/03 17:00:04, jlabanca wrote:
Missing a closing span. If you aren't testing something specific to
the
malformed HTML, I suggest you add the closing span back on.
Done.
http://gwt-code-reviews.appspot.com/1447812/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: CompileModule creates a serialized set of compilation units to represent (issue1448808)
http://gwt-code-reviews.appspot.com/1448808/diff/6001/dev/core/src/com/google/gwt/dev/cfg/ModuleDef.java File dev/core/src/com/google/gwt/dev/cfg/ModuleDef.java (right): http://gwt-code-reviews.appspot.com/1448808/diff/6001/dev/core/src/com/google/gwt/dev/cfg/ModuleDef.java#newcode94 dev/core/src/com/google/gwt/dev/cfg/ModuleDef.java:94: private final SetURL archiveURLs = new LinkedHashSetURL(); I think though in this case, these would be local file URL's, and won't generally try to make blocking net connections, no? http://gwt-code-reviews.appspot.com/1448808/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] [google-web-toolkit] r10274 committed - Making media events bitless, freeing up a few event bits, since modern...
Revision: 10274
Author: fre...@google.com
Date: Fri Jun 3 10:56:12 2011
Log: Making media events bitless, freeing up a few event bits, since
modern,
implementing browsers don't leak the way old ones did.
The following constants, marked as 'experimental', have been removed:
- com.google.gwt.user.client.Event.MEDIAEVENTS
- com.google.gwt.user.client.Event.ONCANPLAYTHROUGH
- com.google.gwt.user.client.Event.ONENDED
- com.google.gwt.user.client.Event.ONPROGRESS
Review at http://gwt-code-reviews.appspot.com/1447816
Review by: jlaba...@google.com
http://code.google.com/p/google-web-toolkit/source/detail?r=10274
Modified:
/trunk/tools/api-checker/config/gwt23_24userApi.conf
/trunk/user/src/com/google/gwt/media/client/MediaBase.java
/trunk/user/src/com/google/gwt/user/client/Event.java
/trunk/user/src/com/google/gwt/user/client/impl/DOMImpl.java
/trunk/user/src/com/google/gwt/user/client/impl/DOMImplStandard.java
/trunk/user/test/com/google/gwt/user/client/MediaEventsSinkTest.java
===
--- /trunk/tools/api-checker/config/gwt23_24userApi.conf Thu Jun 2
11:49:52 2011
+++ /trunk/tools/api-checker/config/gwt23_24userApi.conf Fri Jun 3
10:56:12 2011
@@ -142,3 +142,8 @@
# Overloading AbstractHasData constructor to allow a widget or element as
the root.
com.google.gwt.user.cellview.client.AbstractHasData::AbstractHasData(Lcom/google/gwt/dom/client/Element;ILcom/google/gwt/view/client/ProvidesKey;)
OVERLOADED_METHOD_CALL
+# Bitless media events no longer these experimental API constants
+com.google.gwt.user.client.Event::MEDIAEVENTS MISSING
+com.google.gwt.user.client.Event::ONCANPLAYTHROUGH MISSING
+com.google.gwt.user.client.Event::ONENDED MISSING
+com.google.gwt.user.client.Event::ONPROGRESS MISSING
===
--- /trunk/user/src/com/google/gwt/media/client/MediaBase.java Mon May 2
06:45:06 2011
+++ /trunk/user/src/com/google/gwt/media/client/MediaBase.java Fri Jun 3
10:56:12 2011
@@ -49,17 +49,20 @@
setElement(element);
}
+ @Override
public HandlerRegistration addCanPlayThroughHandler(
CanPlayThroughHandler handler) {
-return addDomHandler(handler, CanPlayThroughEvent.getType());
+return addBitlessDomHandler(handler, CanPlayThroughEvent.getType());
}
+ @Override
public HandlerRegistration addEndedHandler(EndedHandler handler) {
-return addDomHandler(handler, EndedEvent.getType());
+return addBitlessDomHandler(handler, EndedEvent.getType());
}
+ @Override
public HandlerRegistration addProgressHandler(ProgressHandler handler) {
-return addDomHandler(handler, ProgressEvent.getType());
+return addBitlessDomHandler(handler, ProgressEvent.getType());
}
/**
===
--- /trunk/user/src/com/google/gwt/user/client/Event.java Wed Mar 23
13:32:42 2011
+++ /trunk/user/src/com/google/gwt/user/client/Event.java Fri Jun 3
10:56:12 2011
@@ -398,50 +398,6 @@
*/
public static final int GESTUREEVENTS = ONGESTURESTART | ONGESTURECHANGE
| ONGESTUREEND;
- /**
- * Fired when media is fully buffered and can be played through to the
end.
- *
- * p
- * span style=color:redExperimental API: This API is still under
development
- * and is subject to change.
- * /span
- * /p
- */
- public static final int ONCANPLAYTHROUGH = 0x2000;
-
- /**
- * Fired when media stops playing.
- *
- * p
- * span style=color:redExperimental API: This API is still under
development
- * and is subject to change.
- * /span
- * /p
- */
- public static final int ONENDED = 0x800;
-
- /**
- * Fired when progress is made downloading media.
- *
- * p
- * span style=color:redExperimental API: This API is still under
development
- * and is subject to change.
- * /span
- * /p
- */
- public static final int ONPROGRESS = 0x1000;
-
- /**
- * A bit-mask covering all media events.
- *
- * p
- * span style=color:redExperimental API: This API is still under
development
- * and is subject to change.
- * /span
- * /p
- */
- public static final int MEDIAEVENTS = ONCANPLAYTHROUGH | ONENDED |
ONPROGRESS;
-
/**
* Value returned by accessors when the actual integer value is
undefined. In
* Development Mode, most accessors assert that the requested attribute
is
===
--- /trunk/user/src/com/google/gwt/user/client/impl/DOMImpl.java Wed Mar 23
13:32:42 2011
+++ /trunk/user/src/com/google/gwt/user/client/impl/DOMImpl.java Fri Jun 3
10:56:12 2011
@@ -92,9 +92,6 @@
case gesturestart: return 0x100;
case gesturechange: return 0x200;
case gestureend: return 0x400;
-case ended: return 0x800;
-case progress: return 0x1000;
-case canplaythrough: return 0x2000;
default: return -1;
}
}-*/;
===
---
[gwt-contrib] Fix enum ordinalization black-listing for upcasts in new array initializers (issue1449812)
Reviewers: cromwellian, zundel,
Description:
Fix enum ordinalization black-listing for upcasts in new array
initializers
Please review this at http://gwt-code-reviews.appspot.com/1449812/
Affected files:
M dev/core/src/com/google/gwt/dev/jjs/impl/ImplicitUpcastAnalyzer.java
M dev/core/test/com/google/gwt/dev/jjs/impl/EnumOrdinalizerTest.java
Index: dev/core/src/com/google/gwt/dev/jjs/impl/ImplicitUpcastAnalyzer.java
===
--- dev/core/src/com/google/gwt/dev/jjs/impl/ImplicitUpcastAnalyzer.java
(revision 10269)
+++ dev/core/src/com/google/gwt/dev/jjs/impl/ImplicitUpcastAnalyzer.java
(working copy)
@@ -25,6 +25,7 @@
import com.google.gwt.dev.jjs.ast.JField;
import com.google.gwt.dev.jjs.ast.JMethod;
import com.google.gwt.dev.jjs.ast.JMethodCall;
+import com.google.gwt.dev.jjs.ast.JNewArray;
import com.google.gwt.dev.jjs.ast.JParameter;
import com.google.gwt.dev.jjs.ast.JPrimitiveType;
import com.google.gwt.dev.jjs.ast.JProgram;
@@ -147,6 +148,16 @@
}
@Override
+ public void endVisit(JNewArray x, Context ctx) {
+JType elementType = x.getArrayType().getElementType();
+if (x.initializers != null) {
+ for (JExpression init : x.initializers) {
+processIfTypesNotEqual(init.getType(), elementType,
x.getSourceInfo());
+ }
+}
+ }
+
+ @Override
public void endVisit(JReturnStatement x, Context ctx) {
if (x.getExpr() != null) {
// check against the current method return type
Index: dev/core/test/com/google/gwt/dev/jjs/impl/EnumOrdinalizerTest.java
===
--- dev/core/test/com/google/gwt/dev/jjs/impl/EnumOrdinalizerTest.java
(revision 10269)
+++ dev/core/test/com/google/gwt/dev/jjs/impl/EnumOrdinalizerTest.java
(working copy)
@@ -32,10 +32,6 @@
* makes sense to test the output in this way. Thus, we provide confidence
* that the AST is left in a coherent state, but it is not a complete test
that
* ordinalization has completed correctly in every respec.
- *
- * TODO(jbrosenberg): Provide a test to assert that ordinalization has
completed
- * correctly, by inspecting the AST in detail, specifically for ordinal
- * replacements, after the EnumOrdinalizer completes.
*/
public class EnumOrdinalizerTest extends OptimizerTestBase {
@Override
@@ -796,6 +792,68 @@
return retString;,
});
optimize(void, String stringApple = getEnumString(Fruit.APPLE););
+
+EnumOrdinalizer.Tracker tracker = EnumOrdinalizer.getTracker();
+assertTrue(tracker.isVisited(test.EntryPoint$Fruit));
+assertFalse(tracker.isOrdinalized(test.EntryPoint$Fruit));
+ }
+
+ public void testNotOrdinalizableImplicitUpcastMethodCallArgsNewArray()
+ throws UnableToCompleteException {
+EnumOrdinalizer.resetTracker();
+
+setupFruitEnum();
+addSnippetClassDecl(public static String getEnumString(Enum[]
myEnumArray) {,
+ String retString = \\;,
+ for (Enum myEnum : myEnumArray) {,
+retString += myEnum.name();,
+ },
+ return retString;,
+});
+optimize(void, String stringFruits = getEnumString(new Enum[]
{Fruit.APPLE, Fruit.ORANGE}););
+
+EnumOrdinalizer.Tracker tracker = EnumOrdinalizer.getTracker();
+assertTrue(tracker.isVisited(test.EntryPoint$Fruit));
+assertFalse(tracker.isOrdinalized(test.EntryPoint$Fruit));
+ }
+
+ public void testNotOrdinalizableImplicitUpcastMethodCallVarArgs()
+ throws UnableToCompleteException {
+EnumOrdinalizer.resetTracker();
+
+setupFruitEnum();
+addSnippetClassDecl(public static String
getEnumString(Enum...myEnumArray) {,
+ String retString = \\;,
+ for (Enum myEnum : myEnumArray) {,
+retString += myEnum.name();,
+ },
+ return retString;,
+});
+optimize(void, String stringFruits = getEnumString(Fruit.APPLE,
Fruit.ORANGE););
+
+EnumOrdinalizer.Tracker tracker = EnumOrdinalizer.getTracker();
+assertTrue(tracker.isVisited(test.EntryPoint$Fruit));
+assertFalse(tracker.isOrdinalized(test.EntryPoint$Fruit));
+ }
+
+ public void testNotOrdinalizableImplicitUpcastNewArrayElements()
+ throws UnableToCompleteException {
+EnumOrdinalizer.resetTracker();
+
+setupFruitEnum();
+optimize(void, Enum[] enums = new Enum[] {Fruit.APPLE,
Fruit.ORANGE};);
+
+EnumOrdinalizer.Tracker tracker = EnumOrdinalizer.getTracker();
+assertTrue(tracker.isVisited(test.EntryPoint$Fruit));
+assertFalse(tracker.isOrdinalized(test.EntryPoint$Fruit));
+ }
+
+ public void testNotOrdinalizableImplicitUpcastNewArrayArrayElements()
+ throws UnableToCompleteException {
+
[gwt-contrib] Re: Making media events bitless, freeing up a few event bits, since modern, (issue1447816)
Committed as r10274 http://gwt-code-reviews.appspot.com/1447816/diff/2002/user/src/com/google/gwt/user/client/impl/DOMImplStandard.java File user/src/com/google/gwt/user/client/impl/DOMImplStandard.java (right): http://gwt-code-reviews.appspot.com/1447816/diff/2002/user/src/com/google/gwt/user/client/impl/DOMImplStandard.java#newcode248 user/src/com/google/gwt/user/client/impl/DOMImplStandard.java:248: // re-entrant safe On 2011/06/03 19:39:41, jlabanca wrote: Can you expand this comment to say: These events are re-entrant safe because they are only available on modern browsers that do not leak memory. I think the intent of my comment wasn't clear, which was directed at my use of removeEventListener Updated it to read: // First call removeEventListener, so as not to add the same event listener more than once http://gwt-code-reviews.appspot.com/1447816/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Add concrete SourceInfo for varargs in method calls (issue1454801)
On 2011/06/03 23:06:36, jbrosenberg wrote: What's the motivation for this? http://gwt-code-reviews.appspot.com/1454801/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] [google-web-toolkit] r10275 committed - fixes a bug in TypeOracle that marked vararg methods with the transien...
Revision: 10275
Author: goderba...@google.com
Date: Fri Jun 3 12:47:44 2011
Log: fixes a bug in TypeOracle that marked vararg methods with the
transient modifier, which is illegal for methods
Review at http://gwt-code-reviews.appspot.com/1447817
Review by: jbrosenb...@google.com
http://code.google.com/p/google-web-toolkit/source/detail?r=10275
Modified:
/trunk/dev/core/src/com/google/gwt/dev/javac/Shared.java
/trunk/dev/core/src/com/google/gwt/dev/javac/typemodel/JConstructor.java
/trunk/dev/core/src/com/google/gwt/dev/javac/typemodel/JField.java
/trunk/dev/core/src/com/google/gwt/dev/javac/typemodel/JMethod.java
/trunk/dev/core/src/com/google/gwt/dev/javac/typemodel/TypeOracle.java
===
--- /trunk/dev/core/src/com/google/gwt/dev/javac/Shared.java Tue Nov 10
20:42:30 2009
+++ /trunk/dev/core/src/com/google/gwt/dev/javac/Shared.java Fri Jun 3
12:47:44 2011
@@ -33,8 +33,6 @@
import java.io.ByteArrayOutputStream;
import java.io.IOException;
import java.io.InputStream;
-import java.util.ArrayList;
-import java.util.List;
/**
* A grab bag of utility functions useful for dealing with java files.
@@ -164,48 +162,4 @@
path = path.substring(0, path.lastIndexOf('.'));
return path.replace('/', '.');
}
-
- static String[] modifierBitsToNames(int bits) {
-ListString strings = new ArrayListString();
-
-// The order is based on the order in which we want them to appear.
-//
-if (0 != (bits MOD_PUBLIC)) {
- strings.add(public);
-}
-
-if (0 != (bits MOD_PRIVATE)) {
- strings.add(private);
-}
-
-if (0 != (bits MOD_PROTECTED)) {
- strings.add(protected);
-}
-
-if (0 != (bits MOD_STATIC)) {
- strings.add(static);
-}
-
-if (0 != (bits MOD_ABSTRACT)) {
- strings.add(abstract);
-}
-
-if (0 != (bits MOD_FINAL)) {
- strings.add(final);
-}
-
-if (0 != (bits MOD_NATIVE)) {
- strings.add(native);
-}
-
-if (0 != (bits MOD_TRANSIENT)) {
- strings.add(transient);
-}
-
-if (0 != (bits MOD_VOLATILE)) {
- strings.add(volatile);
-}
-
-return strings.toArray(NO_STRINGS);
- }
-}
+}
===
---
/trunk/dev/core/src/com/google/gwt/dev/javac/typemodel/JConstructor.java
Thu Dec 9 10:54:59 2010
+++
/trunk/dev/core/src/com/google/gwt/dev/javac/typemodel/JConstructor.java
Fri Jun 3 12:47:44 2011
@@ -62,7 +62,7 @@
@Override
public String getReadableDeclaration() {
-String[] names = TypeOracle.modifierBitsToNames(getModifierBits());
+String[] names =
TypeOracle.modifierBitsToNamesForMethod(getModifierBits());
StringBuilder sb = new StringBuilder();
for (String name : names) {
sb.append(name);
===
--- /trunk/dev/core/src/com/google/gwt/dev/javac/typemodel/JField.java Wed
Feb 9 13:08:24 2011
+++ /trunk/dev/core/src/com/google/gwt/dev/javac/typemodel/JField.java Fri
Jun 3 12:47:44 2011
@@ -135,7 +135,7 @@
@Override
public String toString() {
-String[] names = TypeOracle.modifierBitsToNames(modifierBits);
+String[] names = TypeOracle.modifierBitsToNamesForField(modifierBits);
StringBuffer sb = new StringBuffer();
for (int i = 0; i names.length; i++) {
if (i 0) {
===
--- /trunk/dev/core/src/com/google/gwt/dev/javac/typemodel/JMethod.java Thu
Dec 9 10:54:59 2010
+++ /trunk/dev/core/src/com/google/gwt/dev/javac/typemodel/JMethod.java Fri
Jun 3 12:47:44 2011
@@ -129,7 +129,7 @@
}
String getReadableDeclaration(int modifierBits) {
-String[] names = TypeOracle.modifierBitsToNames(modifierBits);
+String[] names = TypeOracle.modifierBitsToNamesForMethod(modifierBits);
StringBuilder sb = new StringBuilder();
for (String name : names) {
sb.append(name);
===
--- /trunk/dev/core/src/com/google/gwt/dev/javac/typemodel/TypeOracle.java
Mon Mar 21 12:22:19 2011
+++ /trunk/dev/core/src/com/google/gwt/dev/javac/typemodel/TypeOracle.java
Fri Jun 3 12:47:44 2011
@@ -204,7 +204,48 @@
});
}
- static String[] modifierBitsToNames(int bits) {
+ static String[] modifierBitsToNamesForField(int bits) {
+ListString strings = modifierBitsToNamesForMethodsAndFields(bits);
+
+if (0 != (bits MOD_VOLATILE)) {
+ strings.add(volatile);
+}
+
+if (0 != (bits MOD_TRANSIENT)) {
+ strings.add(transient);
+}
+
+return strings.toArray(NO_STRINGS);
+ }
+
+ static String[] modifierBitsToNamesForMethod(int bits) {
+ListString strings = modifierBitsToNamesForMethodsAndFields(bits);
+
+if (0 != (bits MOD_ABSTRACT)) {
+ strings.add(abstract);
+}
+
+if (0 != (bits MOD_NATIVE)) {
+ strings.add(native);
+}
+
+return strings.toArray(NO_STRINGS);
+ }
+
+ private static JClassType[]
[gwt-contrib] Re: Fix enum ordinalization black-listing for upcasts in new array initializers (issue1449812)
LGTM, I wonder how many more of these implicit upcasts are hiding :) http://gwt-code-reviews.appspot.com/1449812/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Add concrete SourceInfo for varargs in method calls (issue1454801)
The EnumOrdinalizer has the ability to report for each non-ordinalizable enum, the source location that caused it to be black-listed. I noticed that for the case of JNewArray's constructed in a method call with varargs, the source info was showing up as Unknown: Line 0. This change allows the source location to be reported. On Fri, Jun 3, 2011 at 7:15 PM, cromwell...@google.com wrote: On 2011/06/03 23:06:36, jbrosenberg wrote: What's the motivation for this? http://gwt-code-reviews.appspot.com/1454801/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Add concrete SourceInfo for varargs in method calls (issue1454801)
Be sure to get the one in GwtAstBuilder too. But don't makeChild() there, just use the existing reference. -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] [google-web-toolkit] r10276 committed - Autoformat JavaToJavaScriptCompiler....
Revision: 10276
Author: sco...@google.com
Date: Fri Jun 3 15:15:01 2011
Log: Autoformat JavaToJavaScriptCompiler.
Review by: zun...@google.com
http://code.google.com/p/google-web-toolkit/source/detail?r=10276
Modified:
/trunk/dev/core/src/com/google/gwt/dev/jjs/JavaToJavaScriptCompiler.java
===
---
/trunk/dev/core/src/com/google/gwt/dev/jjs/JavaToJavaScriptCompiler.java
Thu Jun 2 11:49:52 2011
+++
/trunk/dev/core/src/com/google/gwt/dev/jjs/JavaToJavaScriptCompiler.java
Fri Jun 3 15:15:01 2011
@@ -450,8 +450,7 @@
}
toReturn.addArtifacts(makeSoycArtifacts(logger, permutationId,
jprogram, js, sizeBreakdowns,
sourceInfoMaps, dependencies, jjsmap, obfuscateMap,
unifiedAst.getModuleMetrics(),
- unifiedAst.getPrecompilationMetrics(), compilationMetrics,
- options.isSoycHtmlDisabled()));
+ unifiedAst.getPrecompilationMetrics(), compilationMetrics,
options.isSoycHtmlDisabled()));
logTrackingStats(logger);
if (logger.isLoggable(TreeLogger.TRACE)) {
@@ -1034,24 +1033,29 @@
for (int i = 0; i js.length; i++) {
DefaultTextOutput out = new
DefaultTextOutput(options.getOutput().shouldMinimize());
JsSourceGenerationVisitorWithSizeBreakdown v;
-
+
if (sourceInfoMaps != null) {
v = new JsReportGenerationVisitor(out, jjsMap);
} else {
v = new JsSourceGenerationVisitorWithSizeBreakdown(out, jjsMap);
}
v.accept(jsProgram.getFragmentBlock(i));
-
+
StatementRanges statementRanges = v.getStatementRanges();
String code = out.toString();
MapRange, SourceInfo infoMap = (sourceInfoMaps != null) ?
v.getSourceInfoMap() : null;
-
- JsAbstractTextTransformer transformer =
+
+ JsAbstractTextTransformer transformer =
new JsAbstractTextTransformer(code, statementRanges, infoMap) {
-@Override public void exec() { }
-@Override protected void updateSourceInfoMap() { }
- };
-
+@Override
+public void exec() {
+}
+
+@Override
+protected void updateSourceInfoMap() {
+}
+ };
+
/**
* Reorder function decls to improve compression ratios. Also
restructures
* the top level blocks into sub-blocks if they exceed 32767
statements.
@@ -1063,13 +1067,13 @@
transformer.exec();
}
functionClusterEvent.end();
-
+
// rewrite top-level blocks to limit the number of statements
if (splitBlocks) {
transformer = new JsIEBlockTextTransformer(transformer);
transformer.exec();
}
-
+
js[i] = transformer.getJs();
ranges[i] = transformer.getStatementRanges();
if (sizeBreakdowns != null) {
@@ -1080,7 +1084,7 @@
}
}
}
-
+
/**
* This method can be used to fetch the list of referenced classs.
*
@@ -1162,7 +1166,7 @@
JavaToJavaScriptMap jjsmap, MapJsName, String obfuscateMap,
ModuleMetricsArtifact moduleMetricsArtifact,
PrecompilationMetricsArtifact precompilationMetricsArtifact,
- CompilationMetricsArtifact compilationMetrics, boolean
htmlReportsDisabled)
+ CompilationMetricsArtifact compilationMetrics, boolean
htmlReportsDisabled)
throws IOException, UnableToCompleteException {
Memory.maybeDumpMemory(makeSoycArtifactsStart);
ListSyntheticArtifact soycArtifacts = new
ArrayListSyntheticArtifact();
@@ -1245,7 +1249,7 @@
soycArtifacts.addAll(outDir.getArtifacts());
generateCompileReport.end();
}
-
+
soycEvent.end();
return soycArtifacts;
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
