[gwt-contrib] Re: Gets the code splitter to be effective even in draft compile mode. (issue875801)
Thanks, Scott!
http://gwt-code-reviews.appspot.com/875801/diff/1/4
File dev/core/src/com/google/gwt/dev/jjs/impl/ReplaceRunAsyncs.java
(right):
http://gwt-code-reviews.appspot.com/875801/diff/1/4#newcode172
dev/core/src/com/google/gwt/dev/jjs/impl/ReplaceRunAsyncs.java:172: */
I'll add documentation.
It would be cleaner and more robust to give it the right type initially.
However, it would require some non-trivial refactoring. Currently,
FragmentLoaderCreator doesn't know which runAsync call it is generating
a loader for. It's just called enough times that enough loaders are
created. The association between loaders and runAsync calls is only made
once ReplaceRunAsyncs runs.
Whenever there is time for a refactor like that, it looks better to
simply eliminate FragmentLoaderCreator and have a single reusable
loader.
http://gwt-code-reviews.appspot.com/875801/diff/1/4#newcode319
dev/core/src/com/google/gwt/dev/jjs/impl/ReplaceRunAsyncs.java:319: if
(method.getName().equals(runAsync)) {
On 2010/09/18 22:39:26, scottb wrote:
If you move the ICE from getMethod() into getOnLoadMethod(), you could
call
getMethod() here.
Done.
http://gwt-code-reviews.appspot.com/875801/diff/1/5
File
dev/core/test/com/google/gwt/dev/jjs/impl/ReplaceRunAsyncsErrorMessagesTest.java
(right):
http://gwt-code-reviews.appspot.com/875801/diff/1/5#newcode92
dev/core/test/com/google/gwt/dev/jjs/impl/ReplaceRunAsyncsErrorMessagesTest.java:92:
com.google.gwt.lang.asyncloaders.AsyncLoader + sp + __Callback) {
On 2010/09/18 22:39:26, scottb wrote:
Should __Callback be replaced with
FragmentLoaderCreator.CALLBACK_LIST_SUFFIX?
(and below)
Done.
http://gwt-code-reviews.appspot.com/875801/show
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Removes @Override annotations on methods implementing interfaces. (issue884801)
Reviewers: rjrjr,
Description:
Removes @Override annotations on methods implementing interfaces.
They don't compile with a Java 1.5 compiler.
Review by: rj...@google.com
Please review this at http://gwt-code-reviews.appspot.com/884801/show
Affected files:
M dev/core/src/com/google/gwt/dev/Precompile.java
M dev/core/src/com/google/gwt/dev/jjs/JJSOptionsImpl.java
Index: dev/core/src/com/google/gwt/dev/Precompile.java
===
--- dev/core/src/com/google/gwt/dev/Precompile.java (revision 8783)
+++ dev/core/src/com/google/gwt/dev/Precompile.java (working copy)
@@ -211,7 +211,6 @@
return jjsOptions.isSoycExtra();
}
-@Override
public boolean isStrict() {
return jjsOptions.isStrict();
}
@@ -280,7 +279,6 @@
jjsOptions.setSoycExtra(soycExtra);
}
-@Override
public void setStrict(boolean strict) {
jjsOptions.setStrict(strict);
}
Index: dev/core/src/com/google/gwt/dev/jjs/JJSOptionsImpl.java
===
--- dev/core/src/com/google/gwt/dev/jjs/JJSOptionsImpl.java (revision 8783)
+++ dev/core/src/com/google/gwt/dev/jjs/JJSOptionsImpl.java (working copy)
@@ -94,7 +94,6 @@
return soycExtra;
}
- @Override
public boolean isStrict() {
return strict;
}
@@ -139,7 +138,6 @@
soycExtra = enabled;
}
- @Override
public void setStrict(boolean strict) {
this.strict = strict;
}
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Removes @Override annotations on methods implementing interfaces. (issue884801)
Thanks for flagging this problem, Ray. Can you review this tiny patch to fix it? http://gwt-code-reviews.appspot.com/884801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] The cross-site iframe linker now loads deferred JS code via script tags holding (issue868801)
Reviewers: unnurg,
Message:
This is a small change to the xsiframe linker. These hoops are a legacy
from the xs linker that is no longer needed. Since xsiframe has an
iframe, the code can be directly installed instead of being passed
around in a string.
Description:
The cross-site iframe linker now loads deferred JS code via script tags
holding
the code directly. It no longer has the code in a string literal that
gets passed
around through several layers of code before being evaluated.
Review by: unn...@google.com
Please review this at http://gwt-code-reviews.appspot.com/868801/show
Affected files:
M dev/core/src/com/google/gwt/core/linker/CrossSiteIframeLinker.java
M dev/core/src/com/google/gwt/core/linker/CrossSiteIframeTemplate.js
M
user/src/com/google/gwt/core/client/impl/CrossSiteIframeLoadingStrategy.java
Index: dev/core/src/com/google/gwt/core/linker/CrossSiteIframeLinker.java
===
--- dev/core/src/com/google/gwt/core/linker/CrossSiteIframeLinker.java
(revision 8762)
+++ dev/core/src/com/google/gwt/core/linker/CrossSiteIframeLinker.java
(working copy)
@@ -41,20 +41,6 @@
@Override
public String getDescription() {
return Cross-Site-Iframe;
- }
-
- @Override
- protected String generateDeferredFragment(TreeLogger logger,
- LinkerContext context, int fragment, String js) {
-StringBuilder sb = new StringBuilder();
-sb.append($wnd.);
-sb.append(context.getModuleFunctionName());
-sb.append(.runAsyncCallback);
-sb.append(fragment);
-sb.append(();
-sb.append(JsToStringGenerationVisitor.javaScriptString(js));
-sb.append();\n);
-return sb.toString();
}
@Override
Index: dev/core/src/com/google/gwt/core/linker/CrossSiteIframeTemplate.js
===
--- dev/core/src/com/google/gwt/core/linker/CrossSiteIframeTemplate.js
(revision 8762)
+++ dev/core/src/com/google/gwt/core/linker/CrossSiteIframeTemplate.js
(working copy)
@@ -236,10 +236,6 @@
type: 'end',
});
}
-
- // Install code pulled in via runAsync
- //
- __MODULE_FUNC__.installCode = installCode;
// --- STRAIGHT-LINE CODE ---
Index:
user/src/com/google/gwt/core/client/impl/CrossSiteIframeLoadingStrategy.java
===
---
user/src/com/google/gwt/core/client/impl/CrossSiteIframeLoadingStrategy.java
(revision 8762)
+++
user/src/com/google/gwt/core/client/impl/CrossSiteIframeLoadingStrategy.java
(working copy)
@@ -113,18 +113,7 @@
}
}-*/;
- private static native JavaScriptObject removeTagAndEvalCode(int fragment,
- JavaScriptObject tag) /*-{
- return function(code) {
- var head = document.getElementsByTagName('head').item(0);
-
@com.google.gwt.core.client.impl.CrossSiteIframeLoadingStrategy::clearOnSuccess(*)(fragment);
-
@com.google.gwt.core.client.impl.CrossSiteIframeLoadingStrategy::clearCallbacks(*)(tag);
- head.removeChild(tag);
- __gwtModuleFunction.installCode(code, false);
- }
- }-*/;
-
- private static native void setOnFailure(JavaScriptObject script,
+ private static native void setOnTerminated(JavaScriptObject script,
JavaScriptObject callback) /*-{
var exception =
@com.google.gwt.core.client.impl.CrossSiteIframeLoadingStrategy::LoadTerminated;
script.onerror = function() {
@@ -141,21 +130,12 @@
}
}-*/;
- /**
- * Set the success callback for fragment codefragment/code
- * to the supplied JavaScript function.
- */
- private static native void setOnSuccess(int fragment, JavaScriptObject
callback) /*-{
-__gwtModuleFunction['runAsyncCallback'+fragment] = callback;
- }-*/;
-
private final IntToIntMap serialNumbers = IntToIntMap.create();
public void startLoadingFragment(int fragment,
LoadTerminatedHandler loadFinishedHandler) {
JavaScriptObject tag = createScriptTag(getUrl(fragment));
-setOnSuccess(fragment, removeTagAndEvalCode(fragment, tag));
-setOnFailure(tag, removeTagAndCallErrorHandler(fragment, tag,
+setOnTerminated(tag, removeTagAndCallErrorHandler(fragment, tag,
loadFinishedHandler));
installScriptTag(tag);
}
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Cleanup and refactoring of GWT Bootstrap code. This cleanup is to try and make it more clear wh... (issue839802)
LGTM http://gwt-code-reviews.appspot.com/839802/diff/3001/4010 File user/src/com/google/gwt/core/CrossSiteIframeLinker.gwt.xml (right): http://gwt-code-reviews.appspot.com/839802/diff/3001/4010#newcode18 user/src/com/google/gwt/core/CrossSiteIframeLinker.gwt.xml:18: This file could be dropped from the patch. http://gwt-code-reviews.appspot.com/839802/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Gets the code splitter to be effective even in draft compile mode. (issue875801)
Reviewers: cromwellian, Description: Gets the code splitter to be effective even in draft compile mode. Review by: cromwell...@google.com Please review this at http://gwt-code-reviews.appspot.com/875801/show Affected files: M dev/core/src/com/google/gwt/dev/jjs/impl/FragmentLoaderCreator.java M dev/core/src/com/google/gwt/dev/jjs/impl/MethodCallTightener.java M dev/core/src/com/google/gwt/dev/jjs/impl/ReplaceRunAsyncs.java M dev/core/test/com/google/gwt/dev/jjs/impl/ReplaceRunAsyncsErrorMessagesTest.java -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Gets the code splitter to be effective even in draft compile mode. (issue875801)
This is ready for review. http://gwt-code-reviews.appspot.com/875801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Adds a -strict option to the GWT compiler. If this option is specified, (issue853801)
http://gwt-code-reviews.appspot.com/853801/diff/4001/5008
File user/test/com/google/gwt/dev/StrictModeTest.java (right):
http://gwt-code-reviews.appspot.com/853801/diff/4001/5008#newcode45
user/test/com/google/gwt/dev/StrictModeTest.java:45: private File
outDir;
The comment needs updating.
I started to drop support for the temp dir, but it's still needed for
the successful compiles. The output will go somewhere.
http://gwt-code-reviews.appspot.com/853801/diff/4001/5008#newcode54
user/test/com/google/gwt/dev/StrictModeTest.java:54: fail(Should have
compiled successfully);
Yes, I'll change it that way.
http://gwt-code-reviews.appspot.com/853801/diff/4001/5008#newcode66
user/test/com/google/gwt/dev/StrictModeTest.java:66: } catch
(UnableToCompleteException e) {
Sounds good.
http://gwt-code-reviews.appspot.com/853801/diff/4001/5008#newcode167
user/test/com/google/gwt/dev/StrictModeTest.java:167: private void
precompile(boolean good) throws UnableToCompleteException {
Nice trick! Booleans are hard to read because it's impossible to
remember what true and false refer to.
http://gwt-code-reviews.appspot.com/853801/show
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Adds a -strict option to the GWT compiler. If this option is specified, (issue853801)
http://gwt-code-reviews.appspot.com/853801/diff/4001/5008 File user/test/com/google/gwt/dev/StrictModeTest.java (right): http://gwt-code-reviews.appspot.com/853801/diff/4001/5008#newcode45 user/test/com/google/gwt/dev/StrictModeTest.java:45: private File outDir; You're right. I'll delete it. I had forgotten I changed it just to do a precompile. http://gwt-code-reviews.appspot.com/853801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Adds a -strict option to the GWT compiler. If this option is specified, (issue853801)
http://gwt-code-reviews.appspot.com/853801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Adds a -strict option to the GWT compiler. If this option is specified, (issue853801)
Those two changes are made now. It's ready for another round of review. http://gwt-code-reviews.appspot.com/853801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Adds source infos in the test AST nodes in ConstantsAssumptionTest. (issue831804)
Reviewers: aizatsky,
Description:
Adds source infos in the test AST nodes in ConstantsAssumptionTest.
Review by: aizat...@google.com
Please review this at http://gwt-code-reviews.appspot.com/831804/show
Affected files:
M
dev/core/test/com/google/gwt/dev/jjs/impl/gflow/constants/ConstantsAssumptionTest.java
Index:
dev/core/test/com/google/gwt/dev/jjs/impl/gflow/constants/ConstantsAssumptionTest.java
===
---
dev/core/test/com/google/gwt/dev/jjs/impl/gflow/constants/ConstantsAssumptionTest.java
(revision 8728)
+++
dev/core/test/com/google/gwt/dev/jjs/impl/gflow/constants/ConstantsAssumptionTest.java
(working copy)
@@ -15,6 +15,7 @@
*/
package com.google.gwt.dev.jjs.impl.gflow.constants;
+import com.google.gwt.dev.jjs.SourceOrigin;
import com.google.gwt.dev.jjs.ast.JIntLiteral;
import com.google.gwt.dev.jjs.ast.JLocal;
import com.google.gwt.dev.jjs.ast.JMethodBody;
@@ -104,12 +105,13 @@
assertTrue(a1.equals(a2));
}
-
+
private JIntLiteral newIntLiteral(int value) {
-return new JIntLiteral(null, value);
+return new JIntLiteral(SourceOrigin.UNKNOWN, value);
}
private JLocal newLocal(String name, JPrimitiveType type) {
-return JProgram.createLocal(null, name, type, false, new
JMethodBody(null));
+return JProgram.createLocal(SourceOrigin.UNKNOWN, name, type, false,
+new JMethodBody(SourceOrigin.UNKNOWN));
}
}
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Adds source infos in the test AST nodes in ConstantsAssumptionTest. (issue831804)
Can you review this tiny patch, Mike? Without this patch, there are assertion failures. http://gwt-code-reviews.appspot.com/831804/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Adds source infos in the test AST nodes in ConstantsAssumptionTest. (issue831804)
Thanks! http://gwt-code-reviews.appspot.com/831804/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Adds a -strict option to the GWT compiler. If this option is specified, (issue853801)
Reviewers: scottb, Description: Adds a -strict option to the GWT compiler. If this option is specified, then the compile will fail if any of the input files are bad. Review by: sco...@google.com Please review this at http://gwt-code-reviews.appspot.com/853801/show Affected files: M dev/core/src/com/google/gwt/dev/Precompile.java M dev/core/src/com/google/gwt/dev/javac/CompilationState.java M dev/core/src/com/google/gwt/dev/javac/CompilationStateBuilder.java M dev/core/src/com/google/gwt/dev/jjs/JJSOptions.java M dev/core/src/com/google/gwt/dev/jjs/JJSOptionsImpl.java A dev/core/src/com/google/gwt/dev/util/arg/ArgHandlerStrict.java A dev/core/src/com/google/gwt/dev/util/arg/OptionStrict.java M user/test/com/google/gwt/core/CoreSuite.java A user/test/com/google/gwt/dev/StrictModeTest.java -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
Re: [gwt-contrib] Re: Allow RPC stats system extensions (issue751801)
I don't know anything about the *current* stats support in RPC. Judging from the revision logs, it looks like Bob might have added that system. Added to the CC. -Lex On Tue, Aug 24, 2010 at 5:29 PM, Pascal Muetschard pmuetschard...@google.com pmuetschard%2...@google.com wrote: Could I please get some feedback on this and get it submitted? Thanks. On Aug 10, 2:13 pm, pmuetschard...@google.com wrote: Reviewers: Dan Rice, scottb, Lex, Description: This patch allows for the extension of the RPC stats system by moving the stats methods into an object, making them non-static. This would allow application developers to extend the ProxyCreator to use a different implementation of the stats methods. Please review this athttp://gwt-code-reviews.appspot.com/751801/show Affected files: user/src/com/google/gwt/rpc/client/impl/RpcCallbackAdapter.java user/src/com/google/gwt/rpc/client/impl/RpcServiceProxy.java user/src/com/google/gwt/user/client/rpc/impl/RemoteServiceProxy.java user/src/com/google/gwt/user/client/rpc/impl/RequestCallbackAdapter.java user/src/com/google/gwt/user/client/rpc/impl/RpcStatsContext.java user/src/com/google/gwt/user/rebind/rpc/ProxyCreator.java -- http://groups.google.com/group/Google-Web-Toolkit-Contributors -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Make xsiframe linker use a .js file for hosted mode so that cross site hosted mode will work (issue784801)
LGTM http://gwt-code-reviews.appspot.com/784801/diff/3002/7001 File dev/core/src/com/google/gwt/core/ext/linker/impl/hosted.js (right): http://gwt-code-reviews.appspot.com/784801/diff/3002/7001#newcode4 dev/core/src/com/google/gwt/core/ext/linker/impl/hosted.js:4: var $moduleName = window.name; Slick. http://gwt-code-reviews.appspot.com/784801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Add DevMode support for the xsiframe linker (issue779801)
LGTM http://gwt-code-reviews.appspot.com/779801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Faster edit-distance computation in JsFunctionClusterer (issue669801)
Ray, does the patch look good to you? -Lex -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Adds a new CrossSiteIframeLinker. This linker works cross-site, (issue726802)
Thanks, you two! I'll do a little more testing after the updates and
then submit it.
http://gwt-code-reviews.appspot.com/726802/diff/1/3
File dev/core/src/com/google/gwt/core/linker/CrossSiteIframeTemplate.js
(right):
http://gwt-code-reviews.appspot.com/726802/diff/1/3#newcode88
dev/core/src/com/google/gwt/core/linker/CrossSiteIframeTemplate.js:88:
(query.indexOf('gwt.hybrid') == -1);
Thanks. I'll leave it for now. Let's think about how to get rid of it as
a separate question.
http://gwt-code-reviews.appspot.com/726802/diff/1/3#newcode110
dev/core/src/com/google/gwt/core/linker/CrossSiteIframeTemplate.js:110:
scriptFrame.style.cssText = 'position:absolute; width:0; height:0;
border:none';
Will do. I'll change it to the following unless anyone speaks up:
scriptFrame.style.cssText = 'position:absolute; width:0; height:0;
border:none; left: -1000px; top: -1000px; !important';
http://gwt-code-reviews.appspot.com/726802/diff/1/3#newcode265
dev/core/src/com/google/gwt/core/linker/CrossSiteIframeTemplate.js:265:
softPermutationId = Number(strongName.substring(idx + 1));
Will do.
http://gwt-code-reviews.appspot.com/726802/show
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Ensure that we give createTempFile at least 3 characters for the prefix (issue699804)
LGTM http://gwt-code-reviews.appspot.com/699804/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Adds a new CrossSiteIframeLinker. This linker works cross-site, (issue726802)
Reviewers: jgw, Description: Adds a new CrossSiteIframeLinker. This linker works cross-site, because it uses a script tag to download code instead of XHR. However, like the iframe linker, it still uses an iframe to hold all the installed code. Review by: j...@google.com Please review this at http://gwt-code-reviews.appspot.com/726802/show Affected files: A dev/core/src/com/google/gwt/core/linker/CrossSiteIframeLinker.java A dev/core/src/com/google/gwt/core/linker/CrossSiteIframeTemplate.js M user/src/com/google/gwt/core/Core.gwt.xml A user/src/com/google/gwt/core/CrossSiteIframeLinker.gwt.xml A user/src/com/google/gwt/core/client/impl/CrossSiteIframeLoadingStrategy.java A user/test/com/google/gwt/core/ext/CrossSiteIframeLinkerTest.gwt.xml M user/test/com/google/gwt/core/ext/LinkerSuite.java A user/test/com/google/gwt/core/ext/test/CrossSiteIframeLinkerTest.java A user/test/com/google/gwt/dev/jjs/CompilerSuiteCrossSiteIframe.gwt.xml A user/test/com/google/gwt/dev/jjs/CrossSiteIframeRunAsyncFailure.gwt.xml A user/test/com/google/gwt/dev/jjs/CrossSiteIframeRunAsyncMetrics.gwt.xml A user/test/com/google/gwt/dev/jjs/CrossSiteIframeRunAsyncSuite.java A user/test/com/google/gwt/dev/jjs/test/CrossSiteIframeRunAsyncFailureTest.java A user/test/com/google/gwt/dev/jjs/test/CrossSiteIframeRunAsyncMetricsTest.java A user/test/com/google/gwt/dev/jjs/test/CrossSiteIframeRunAsyncTest.java -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Adds a new CrossSiteIframeLinker. This linker works cross-site, (issue726802)
This is ready for review. It's almost the same as this change: http://gwt-code-reviews.appspot.com/726802 The only difference is that this patch makes a new linker rather than updating the XS linker in place. The contents of the new linker are the same as what was in the previous patch. http://gwt-code-reviews.appspot.com/726802/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Adds a new CrossSiteIframeLinker. This linker works cross-site, (issue726802)
Err, make that the following issue: http://gwt-code-reviews.appspot.com/674802 http://gwt-code-reviews.appspot.com/726802/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
Re: [gwt-contrib] Phasing in a new, unified linker
I don't have a strong opinion about it. They can be non-final, with simply no particular effort to truly make them extensible. I think it might be possible to move the template JS files to GWT-translated code with extension points managed through rebinding and overriding. Until then, making changes that involve JS modifications effectively require you to cut and paste the whole file. There is now some templating, by the way. You asked about the __COMPUTE_SCRIPT_BASE__ reference. SelectionScriptLinker -- the base class for all the linkers in the subject line -- substitutes that string for the contents of computeScriptBase.js, a file included within gwt-user.jar. Thus, linkers that want the standard implementation of computeScriptBase() can simply include that string rather than copying the whole chunk of JS. Such templating is pretty limited even in principle, however, and in practice it's so far only done for that file and for one other one. Incidentally, you mention moving code into Java. That strategy actually came to pass for runAsync code fetching. Originally, linkers would emit a function that the code loader calls to download code. Now, there is a deferred binding, and the choice of linker causes the deferred binding choice to differ. Thus, code loaders are now simply Java classes that implement a simple Java interface. It's much easier to maintain. -Lex -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Faster edit-distance computation in JsFunctionClusterer (issue669801)
http://gwt-code-reviews.appspot.com/669801/diff/20001/21006
File dev/core/src/com/google/gwt/dev/jjs/impl/JsFunctionClusterer.java
(right):
http://gwt-code-reviews.appspot.com/669801/diff/20001/21006#newcode62
dev/core/src/com/google/gwt/dev/jjs/impl/JsFunctionClusterer.java:62: if
(code.startsWith(function )) {
I am having trouble finding your earlier comment. I would think, though,
that if you saw duplicate statements being output, then there's a
problem somewhere other than right here. This part of the code only
decides which statements can be reordered and which must be left in
place. If that decision is wrong, it still shouldn't cause anything to
be emitted twice.
Anyway, to see what the cross-site linker does, add the following to
Showcase.gwt.xml, compile Showcase, and look at
deferredjs/*/44.cache.js.
add-linker name='xs'/
You'll see things like the following, which should be clusterable:
jslink.UYc=function(b){/*code goes here*/}
http://gwt-code-reviews.appspot.com/669801/show
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Faster edit-distance computation in JsFunctionClusterer (issue669801)
On Wed, Jul 28, 2010 at 6:15 PM, avassalo...@google.com wrote: Oh, sorry. I made this comment somewhere else. The problem is the endStatements() method doesn't use the regex to recognize the other declaration style. Ah, yes! Well at the least this code should be moved to a subroutine. I believe the version with the regex is the desired version. In addition, I believe the current regex don't match the declaration emitted by the cross-linker. The dot in the name prevent a match. I thought so at first, but it's using find(). So it should still match. Perhaps it matches too many -Lex -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
Re: [gwt-contrib] Phasing in a new, unified linker
On Mon, Jul 26, 2010 at 6:56 PM, John Tamplin j...@google.com wrote: Well, we do know there will be other linkers, and if there aren't extension points defined they will be done via cut-and-paste, which is what led to the current state we are in. No question that extension points are useful. Let's add them, but only when we have an idea of what we are supporting with them. Note that IFrameLinker and XSLinker have several extension points, and yet nonetheless there is a lot of cut and paste going on. We didn't add (all of) the ones that people really ended up needing. -Lex -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
Re: [gwt-contrib] Phasing in a new, unified linker
On Mon, Jul 26, 2010 at 12:37 PM, Scott Blum sco...@google.com wrote: SGTM as far as process. Is the new linker designed to curtail extension, or to sanely encourage it? The existing primary linkers ended up getting extended in brittle ways. That's a good point. Let's make it a final class to start with, and open up extension points later as issues come up. There are no known needs for extensions at this point. -Lex -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Modifies the cross-site linker to put its code in an iframe (issue674802)
Reviewers: jgw, Description: Modifies the cross-site linker to put its code in an iframe rather than a wrapper function. Review by: j...@google.com Please review this at http://gwt-code-reviews.appspot.com/674802/show Affected files: M dev/core/src/com/google/gwt/core/linker/XSLinker.java M dev/core/src/com/google/gwt/core/linker/XSTemplate.js M dev/core/src/com/google/gwt/dev/jjs/JavaToJavaScriptCompiler.java D dev/core/src/com/google/gwt/dev/jjs/impl/HandleCrossFragmentReferences.java M user/src/com/google/gwt/core/CompilerParameters.gwt.xml M user/src/com/google/gwt/core/XSLinker.gwt.xml -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Wrap GWT.runAsync() entry points with $entry . (issue706801)
Reviewers: robertvawter,
Description:
Wrap GWT.runAsync() entry points with $entry .
Please review this at http://gwt-code-reviews.appspot.com/706801/show
Affected files:
M dev/core/src/com/google/gwt/dev/jjs/impl/FragmentExtractor.java
M user/src/com/google/gwt/core/client/impl/CrossSiteLoadingStrategy.java
M user/src/com/google/gwt/core/client/impl/XhrLoadingStrategy.java
Index: dev/core/src/com/google/gwt/dev/jjs/impl/FragmentExtractor.java
===
--- dev/core/src/com/google/gwt/dev/jjs/impl/FragmentExtractor.java
(revision 8393)
+++ dev/core/src/com/google/gwt/dev/jjs/impl/FragmentExtractor.java
(working copy)
@@ -226,7 +226,7 @@
SourceInfo sourceInfo = jsprogram.getSourceInfo().makeChild(
FragmentExtractor.class, call to entry function + splitPoint);
JsInvocation call = new JsInvocation(sourceInfo);
- call.setQualifier(name.makeRef(sourceInfo));
+ call.setQualifier(wrapWithEntry(name.makeRef(sourceInfo)));
callStats.add(call.makeStmt());
}
return callStats;
@@ -244,7 +244,7 @@
FragmentExtractor.class,
call to browserLoaderLeftoversFragmentHasLoaded );
JsInvocation call = new JsInvocation(sourceInfo);
-call.setQualifier(loadedMethodName.makeRef(sourceInfo));
+call.setQualifier(wrapWithEntry(loadedMethodName.makeRef(sourceInfo)));
ListJsStatement newStats = Collections.JsStatement
singletonList(call.makeStmt());
return newStats;
}
@@ -630,4 +630,15 @@
return null;
}
+ /**
+ * Wrap an expression with a call to $entry.
+ */
+ private JsInvocation wrapWithEntry(JsExpression exp) {
+SourceInfo sourceInfo = exp.getSourceInfo().makeChild(
+FragmentExtractor.class, wrapping with a call to $entry);
+JsInvocation call = new JsInvocation(sourceInfo);
+call.setQualifier(new JsNameRef(sourceInfo, $entry));
+call.getArguments().add(exp);
+return call;
+ }
}
Index:
user/src/com/google/gwt/core/client/impl/CrossSiteLoadingStrategy.java
===
--- user/src/com/google/gwt/core/client/impl/CrossSiteLoadingStrategy.java
(revision 8393)
+++ user/src/com/google/gwt/core/client/impl/CrossSiteLoadingStrategy.java
(working copy)
@@ -104,8 +104,10 @@
@com.google.gwt.core.client.impl.CrossSiteLoadingStrategy::clearOnSuccess(*)(fragment);
@com.google.gwt.core.client.impl.CrossSiteLoadingStrategy::clearCallbacks(*)(tag);
head.removeChild(tag);
-
loadfinishedhandl...@com.google.gwt.core.client.impl.asyncfragmentloader.loadterminatedhandler::loadTerminated(*)(
- exception);
+ function callLoadTerminated() {
+
loadfinishedhandl...@com.google.gwt.core.client.impl.asyncfragmentloader.loadterminatedhandler::loadTerminated(*)(exception);
+ }
+ $entry(callLoadTerminated)();
}
}-*/;
Index: user/src/com/google/gwt/core/client/impl/XhrLoadingStrategy.java
===
--- user/src/com/google/gwt/core/client/impl/XhrLoadingStrategy.java
(revision 8393)
+++ user/src/com/google/gwt/core/client/impl/XhrLoadingStrategy.java
(working copy)
@@ -199,7 +199,7 @@
function loadFailed(e) {
loaderrorhandl...@com.google.gwt.core.client.impl.asyncfragmentloader$loadterminatedhandler::loadTerminated(*)(e);
}
-return __gwtStartLoadingFragment(fragment, loadFailed);
+return __gwtStartLoadingFragment(fragment, $entry(loadFailed));
}-*/;
/**
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Wrap GWT.runAsync() entry points with $entry . (issue706801)
This is ready for review. http://gwt-code-reviews.appspot.com/706801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Wrap GWT.runAsync() entry points with $entry . (issue706801)
Thanks! http://gwt-code-reviews.appspot.com/706801/diff/1/2 File dev/core/src/com/google/gwt/dev/jjs/impl/FragmentExtractor.java (right): http://gwt-code-reviews.appspot.com/706801/diff/1/2#newcode640 dev/core/src/com/google/gwt/dev/jjs/impl/FragmentExtractor.java:640: call.setQualifier(new JsNameRef(sourceInfo, $entry)); On 2010/07/20 17:55:59, bobv wrote: $entry is defined in JsRootScope. I think you should get the JsName from there just in case it should ever become an obfuscated symbol. Done. http://gwt-code-reviews.appspot.com/706801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Improve ExpressionAnalyzer with newer compiler info (issue707801)
LGTM http://gwt-code-reviews.appspot.com/707801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Add first-class support for [array].length in the compiler. (issue702801)
http://gwt-code-reviews.appspot.com/702801/diff/7001/8006 File dev/core/src/com/google/gwt/dev/jjs/impl/ExpressionAnalyzer.java (right): http://gwt-code-reviews.appspot.com/702801/diff/7001/8006#newcode163 dev/core/src/com/google/gwt/dev/jjs/impl/ExpressionAnalyzer.java:163: canThrowException = true; Sounds good to me. http://gwt-code-reviews.appspot.com/702801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: use ParameterizedType at GWT#create method (issue429801)
This is a pretty amazing first contribution! Also, the feature is certainly desirable. However, more work needs to be done to support that feature while also handling all of the odd edge cases. For example, here are a few issues to consider: 1. The GWT compiler uses MapString,String to represent rebind decisions from query type to result type. I'm not sure it's safe to simply run toString on a parametric type to use as the key to one of these maps. 2. Not all parametric types are going to be supportable. For example, ArrayString is okay but ArrayT doesn't make sense. Such a patch should spec out an exact set of parameterized types that are supported, and it should generate compile-time errors 3. In at least one place, this patch does a toString and then drops everything before the first occurrence of . I believe the code should be handling the generic types directly rather than converting to a string and reasoning about the string. http://gwt-code-reviews.appspot.com/429801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Allows Linkers to mark themselves as shardable by including a (issue678802)
http://gwt-code-reviews.appspot.com/678802/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Allows Linkers to mark themselves as shardable by including a (issue678802)
http://gwt-code-reviews.appspot.com/678802/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: SafeASTVisitor unilaterally avoids visiting error/unreachable local types (issue682801)
LGTM http://gwt-code-reviews.appspot.com/682801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Fixes a bug where temp local declarations can sometimes end up in a for statement's increments list. (issue677801)
Good find. Just one nit. http://gwt-code-reviews.appspot.com/677801/diff/1/2 File dev/core/src/com/google/gwt/dev/jjs/ast/Context.java (right): http://gwt-code-reviews.appspot.com/677801/diff/1/2#newcode33 dev/core/src/com/google/gwt/dev/jjs/ast/Context.java:33: boolean isLvalue(); Cool, that should simplify many visitors. http://gwt-code-reviews.appspot.com/677801/diff/1/9 File dev/core/test/com/google/gwt/dev/jjs/impl/TempLocalVisitorTest.java (right): http://gwt-code-reviews.appspot.com/677801/diff/1/9#newcode54 dev/core/test/com/google/gwt/dev/jjs/impl/TempLocalVisitorTest.java:54: local.getDeclarationStatement().initializer = x; It's a legacy problem in this test case, but the above replacement is actually not sound. Instead of overwriting the declaration statement's initializer, it would be better to replace x by (t=x,t). Or even just (t=x). See below for an example. http://gwt-code-reviews.appspot.com/677801/diff/1/9#newcode98 dev/core/test/com/google/gwt/dev/jjs/impl/TempLocalVisitorTest.java:98: expected.append(for (int $t0 = 0, i = $t0; $t1; i += $t2);); Imagine that instead of i += 1 it was i += foo(). In that case, it's vital to reevaluate the foo() each time through the loop. http://gwt-code-reviews.appspot.com/677801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Fixes a bug where temp local declarations can sometimes end up in a for statement's increments list. (issue677801)
Sure, LGTM. http://gwt-code-reviews.appspot.com/677801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: TempLocalVisitorTest now uses non-destructive code transforms (improves clarity) (issue679801)
LGTM. It makes the test case a better example of how to use the facility. http://gwt-code-reviews.appspot.com/679801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Optimize JsFunctionClusterer with faster edit distance algorithms (issue669801)
LGTM http://gwt-code-reviews.appspot.com/669801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Optimize JsFunctionClusterer with faster edit distance algorithms (issue669801)
Thanks, Alex! Just nits: - Let's attribute the original author(s) in the change description - Let's put the edit-distance classes in a subpackage of com.google.gwt.dev.util. Perhaps simply com.google.gwt.dev.util.editdistance ? http://gwt-code-reviews.appspot.com/669801/diff/1/7 File dev/core/src/com/google/gwt/dev/jjs/impl/JsFunctionClusterer.java (right): http://gwt-code-reviews.appspot.com/669801/diff/1/7#newcode41 dev/core/src/com/google/gwt/dev/jjs/impl/JsFunctionClusterer.java:41: private static final int GOOD_ENOUGH_DISTANCE = 5; Please add comments for these new variables. Adding one for MAX_DISTANCE_LIMIT wouldn't be bad, either, if you get on a roll. http://gwt-code-reviews.appspot.com/669801/diff/1/7#newcode98 dev/core/src/com/google/gwt/dev/jjs/impl/JsFunctionClusterer.java:98: while (it.hasNext() bestDistance GOOD_ENOUGH_DISTANCE I presume you tested that the GOOD_ENOUGH_DISTANCE heuristic is worth it? That is, the output is surely a little worse, but you must have verified that we get a lot of speedup in return? http://gwt-code-reviews.appspot.com/669801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
Re: [gwt-contrib] proposing a hypothetically breaking change to gwt-servlet.jar: comments?
On Mon, Jun 28, 2010 at 3:57 PM, Freeland Abbott fabb...@google.com wrote: So, how breaking are we willing to be to correct that? My knee-jerk reaction is that we don't want to do a lot of breaking just to tidy up the definition of gwt-servlet. The only benefit is to reduce the size of the white list, right? That's a small benefit. Nonetheless, we still would benefit to make gwt-servlet be based on a whitelist rather than a blacklist. It would immediately shrink the jar size, and it would prevent us from accidentally sprawling it even larger. -Lex -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
Re: [gwt-contrib] proposing a hypothetically breaking change to gwt-servlet.jar: comments?
I think everyone is saying the same thing. Use Miroslav's suggested ASM tool to get a first cut, and then bake the resulting whitelist into the ant files. Then, little by little, refactor things so that the whitelist can shrink, until all that's left is **/shared/** and **/server/** . I'd only add that the ASM tool does get into 4+ hours of work. So if the list Freeland has already looks pretty good, we might instead start there. -Lex -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Adds a simple JJS test to ensure enums work (issue640803)
LGTM http://gwt-code-reviews.appspot.com/640803/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Add useful hooks into GWT to allow other tools to parse and analyze Java code. (issue631801)
LGTM http://gwt-code-reviews.appspot.com/631801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Fix issue 4423 - RPC ProxyCreator doesn't support methods with Type Parameters (issue575801)
Thank you for the minified example. I'll follow up on the issue. http://code.google.com/p/google-web-toolkit/issues/detail?id=4423 http://gwt-code-reviews.appspot.com/575801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
Re: [gwt-contrib] Command pattern and GWT.runAsync
On Fri, Jun 4, 2010 at 5:13 AM, David david.no...@gmail.com wrote:
Less maintenance on the async, declarative transaction management,
undo, batching, less web.xml tweeking, ... there are many reasons why
we also use a command pattern.
With the system as it is, I believe your best bet is to make abstract class
plus *templates* of key methods. The templates are then copied down to each
subclass. An example is in GWT's showcase sample, where each content pane
implements the following method:
@Override
protected void asyncOnInitialize(final AsyncCallbackWidget callback) {
GWT.runAsync(CwAbsolutePanel.class, new RunAsyncCallback() {
public void onFailure(Throwable caught) {
callback.onFailure(caught);
}
public void onSuccess() {
callback.onSuccess(onInitialize());
}
});
}
This method calls onInitialize() to do the work specific to each example
pane. You could imagine it also calling some other protected methods in key
places.
In general, it would be nicer if runAsync included some extra callbacks in
key places so that this kind of thing isn't necessary. Then whenever you
change the template, you could change it in one place instead of having to
modify all the copies in all the subclasses. Initially, the issue was that
it wasn't obvious what hook points people would want. At this point, the
issue is more a matter of priorities. To do it well would require surveying
what everyone is doing with runAsync and making sure the right hooks are
there for the majority of them.
Lex
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Fix issue 4423 - RPC ProxyCreator doesn't support methods with Type Parameters (issue575801)
I believe type parameters work fine. Can you give an example that doesn't work? I just tried modifying EnumTest to verify that at least in simple cases, it works. The way this works relies on Java generics having been defined to have compatibility with pre-generic Java. You can always erase a type and use that instead of the original, complex type--though of course losing the benefits of type checking. By using erased types, GWT's current implementation is a bit simpler. That's the theory. Do you have an example of something you'd like to work that does not? If so, we can patch it as you describe. However, one more thing that needs to be done is to handle bounds on the type parameters. For this facility to be useful in practice, the type parameters will need to be bounded anyway, because otherwise the RPC interface will effectively specify that objects must be passed back and forth. http://gwt-code-reviews.appspot.com/575801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
Re: [gwt-contrib] Tabless
On Thu, Jun 3, 2010 at 12:56 PM, Ray Ryan rj...@google.com wrote: No argument. And since we've never, ever managed to actually delete a deprecated class so far as I know, the issue may not come up for a while… There are some counterexamples. For example: http://gwt-code-reviews.appspot.com/139804/show To get deprecated things removed, it's key that users have something to switch to. Lex -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Fix issue 4423 - RPC ProxyCreator doesn't support methods with Type Parameters (issue575801)
Thanks for the extra detail. I can verify that the given interface does not translate correctly. The surprising thing is that this problem was supposed to be fixed in JDT 3.4.2, which is exactly the version GWT is using: https://bugs.eclipse.org/bugs/show_bug.cgi?id=243820 Maybe we aren't really using 3.4.2, or maybe the fix really isn't in there, or maybe this is a new instance of that JDT bug. Further investigation is needed to see which. Alternatively, we could generate type parameters. However, it requires more development of the patch. One thing is that the bounds on the type parameters need to be added. Also, what about type parameters of the enclosing class? Those would need to be handled as well. Using erased types means that all these questions go away, so it would be great if we can simply upgrade our version of the JDT to solve the problem. http://gwt-code-reviews.appspot.com/575801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Additional testing api emulation (issue586801)
LGTM http://gwt-code-reviews.appspot.com/586801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Model enclosing types in the GWT AST (issue588801)
LGTM, though extending the comment as indicated would be nice. http://gwt-code-reviews.appspot.com/588801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Model enclosing types in the GWT AST (issue588801)
http://gwt-code-reviews.appspot.com/588801/diff/1/2 File dev/core/src/com/google/gwt/dev/jjs/ast/JDeclaredType.java (right): http://gwt-code-reviews.appspot.com/588801/diff/1/2#newcode64 dev/core/src/com/google/gwt/dev/jjs/ast/JDeclaredType.java:64: private JDeclaredType enclosingType; This surprised me at first. It's worth noting that this information is only for tracking the information through the compiler. All nested classes are still converted to top-level classes in GenerateJavaAST. http://gwt-code-reviews.appspot.com/588801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Enums can be abstract (issue587801)
LGTM http://gwt-code-reviews.appspot.com/587801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
Re: [gwt-contrib] Re: CompilePerms and classpath loading problem.
On Tue, Jun 1, 2010 at 11:56 AM, Ray Ryan rj...@google.com wrote: Yup. Is the fix to make it use the resource oracle? To play it safe: First check resource oracle. Next check the context class loader, as in Marko's email. Then check wherever it looks now (the system class loader?). Lex -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
Re: [gwt-contrib] Re: CompilePerms and classpath loading problem.
On Tue, Jun 1, 2010 at 2:35 PM, Marko Vuksanovic markovuksano...@gmail.comwrote: Class Loaders are checked in parent to child direction - so if you try to fetch a resource from a context class loader, system class loader is the first that will be checked and only after resource is not found there, next child will be checked... and so on... So if something is found in context class loader, all parent class loaders have been checked. Somebody correct me if I'm wrong. I don't believe it's necessarily true for the system loader to be a parent of the context loader. It's common, but not necessary. The only loader you can't get away from is the boot class loader. That said, if it's not on the context loader, you might want to ignore it if you can get away with it. For that matter, the same goes for things not in the resource oracle. -Lex -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Added benchmark task to ant in user/ (issue547801)
LGTM http://gwt-code-reviews.appspot.com/547801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Fixes a bug in the compile report dashboard where entries with same sizes were (issue566801)
LGTM with nits. No need to rereview if you like the suggested changes.
These changes will make SOYC a lot easier to understand.
http://gwt-code-reviews.appspot.com/566801/diff/1/3
File dev/core/src/com/google/gwt/soyc/MakeTopLevelHtmlForPerm.java
(right):
http://gwt-code-reviews.appspot.com/566801/diff/1/3#newcode179
dev/core/src/com/google/gwt/soyc/MakeTopLevelHtmlForPerm.java:179:
outFile.println(if (element.name == \packageBreakdown\) {);
Putting ifs here doesn't scale well if we add more popups -- and we
should!
There are multiple ways to avoid an ever increasing chain of ifs here. A
simple way would be to remove the element parameter and add one with
the ID of the help popup.
http://gwt-code-reviews.appspot.com/566801/diff/1/3#newcode284
dev/core/src/com/google/gwt/soyc/MakeTopLevelHtmlForPerm.java:284:
onMouseOut=\hide(this);\Package breakdown/a/h2/div);
If you go with the changed parameter, then show(this) would be changed
to show(\packageBreakdownPopup\);, and likewise for hide().
http://gwt-code-reviews.appspot.com/566801/diff/1/3#newcode359
dev/core/src/com/google/gwt/soyc/MakeTopLevelHtmlForPerm.java:359:
outFile.println(thSize span
class=\soyc-th-units\(Bytes)/span/th);
Nice. It pains me to think how many times I looked at that table header
and didn't notice the problem.
http://gwt-code-reviews.appspot.com/566801/diff/1/3#newcode728
dev/core/src/com/google/gwt/soyc/MakeTopLevelHtmlForPerm.java:728:
TreeMapFloat, SetString sortedCodeTypes = new TreeMapFloat,
SetString (
Very nice, and likewise for the other ones. Could you also change the
keys from Float to Integer? Comparison on floats is begging for weird
corner cases.
http://gwt-code-reviews.appspot.com/566801/show
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: When SoycReportLinker looks for report files, it now tolerates (issue545801)
Kathrin, it sounds like you have more time than Ray to look at SOYC problems. Can you review this patch? http://gwt-code-reviews.appspot.com/545801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: When SoycReportLinker looks for report files, it now tolerates (issue545801)
Thanks! http://gwt-code-reviews.appspot.com/545801/diff/1/3 File dev/core/test/com/google/gwt/core/linker/SoycReportLinkerTest.java (right): http://gwt-code-reviews.appspot.com/545801/diff/1/3#newcode80 dev/core/test/com/google/gwt/core/linker/SoycReportLinkerTest.java:80: } From face to face discussion, better tests of the HTML sound good, but as a followon patch. http://gwt-code-reviews.appspot.com/545801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Have IE's DOMImpl invoke its event dispatching function via a (issue543801)
Thanks for walking me through how all this works! http://gwt-code-reviews.appspot.com/543801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: [GFlow] Supporting break statement outside of loops. (issue553801)
LGTM. I am used to GWT turning up oddities about JavaScript. This patch does something less common: showing a surprise about Java! http://gwt-code-reviews.appspot.com/553801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Added benchmark task to ant in user/ (issue547801)
Let's see if the build file can be simplified some. The rest looks good. http://gwt-code-reviews.appspot.com/547801/diff/1/4 File /user/build.xml (right): http://gwt-code-reviews.appspot.com/547801/diff/1/4#newcode598 /user/build.xml:598: property name=compile.tests.complete value=true/ This looks too tricky. Is it here so that the benchmark targets can depend on various targets, and yet prevent those targets from actually building? If so, why not simply depend on more specifically what the targets require to be built? If the existing ant targets are too coarse, then they can certainly be factored into more specific ones. http://gwt-code-reviews.appspot.com/547801/diff/1/4#newcode608 /user/build.xml:608: -- I don't see anything running in parallel here. Drop the parallel and sequential tags and make it simply three antcalls ? http://gwt-code-reviews.appspot.com/547801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: JsParserException better error reporting (issue546801)
Very nice. LGTM. http://gwt-code-reviews.appspot.com/546801/diff/2001/3001 File dev/core/src/com/google/gwt/core/ext/linker/impl/StandardLinkerContext.java (right): http://gwt-code-reviews.appspot.com/546801/diff/2001/3001#newcode422 dev/core/src/com/google/gwt/core/ext/linker/impl/StandardLinkerContext.java:422: throw new RuntimeException(Unexpected reading in-memory stream, e); Insert error http://gwt-code-reviews.appspot.com/546801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
Re: The new 2.1M1 release appears to have a few challenges
The new data-flow optimizer is choking on the code. Some sort of uncommon syntax in the code is breaking one of its assertions. Let me poke around a little to see what's up. Filed as Issue 4957: http://code.google.com/p/google-web-toolkit/issues/detail?id=4957 Lex -- You received this message because you are subscribed to the Google Groups Google Web Toolkit group. To post to this group, send email to google-web-tool...@googlegroups.com. To unsubscribe from this group, send email to google-web-toolkit+unsubscr...@googlegroups.com. For more options, visit this group at http://groups.google.com/group/google-web-toolkit?hl=en.
[gwt-contrib] When SoycReportLinker looks for report files, it now tolerates (issue545801)
Reviewers: cromwellian, Description: When SoycReportLinker looks for report files, it now tolerates missing soycReport from the beginning of the emitted artifact paths. Please review this at http://gwt-code-reviews.appspot.com/545801/show Affected files: M dev/core/src/com/google/gwt/core/linker/SoycReportLinker.java A dev/core/test/com/google/gwt/core/linker/SoycReportLinkerTest.java -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: When SoycReportLinker looks for report files, it now tolerates (issue545801)
Ray, Can you review this, whenever your I/O involvement gives you time to do so? http://gwt-code-reviews.appspot.com/545801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Fixes a memory leak on IE with the cross-site fragment loading strategy. (issue544801)
Thanks! http://gwt-code-reviews.appspot.com/544801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Have IE's DOMImpl invoke its event dispatching function via a (issue543801)
Reviewers: jgw, Description: Have IE's DOMImpl invoke its event dispatching function via a round-trip through the window object. That way, the only reference from DOM objects into GWT's code is on the window object, a place IE is careful to clean up when the browser navigates to a new page. This fixes a problem where the cross-site linker on IE could lead to memory leaking across page refreshes. Please review this at http://gwt-code-reviews.appspot.com/543801/show Affected files: M user/src/com/google/gwt/user/client/impl/DOMImplTrident.java -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Fixes a memory leak on IE with the cross-site fragment loading strategy. (issue544801)
Reviewers: jgw, jlabanca,
Description:
Fixes a memory leak on IE with the cross-site fragment loading strategy.
Now the callbacks on script tags are removed when they are invoked.
Please review this at http://gwt-code-reviews.appspot.com/544801/show
Affected files:
M user/src/com/google/gwt/core/client/impl/CrossSiteLoadingStrategy.java
Index:
user/src/com/google/gwt/core/client/impl/CrossSiteLoadingStrategy.java
===
--- user/src/com/google/gwt/core/client/impl/CrossSiteLoadingStrategy.java
(revision 8109)
+++ user/src/com/google/gwt/core/client/impl/CrossSiteLoadingStrategy.java
(working copy)
@@ -60,6 +60,18 @@
Code download terminated);
/**
+ * Clear callbacks on script objects. This is important on IE 6 and 7 to
+ * prevent a memory leak. If the callbacks aren't cleared, there is a
cyclical
+ * chain of references between the script tag and the function callback,
and
+ * IE 6/7 can't garbage collect them.
+ */
+ @SuppressWarnings(unused)
+ private static native void clearCallbacks(JavaScriptObject script) /*-{
+var nop = new Function('');
+script.onerror = script.onload = script.onreadystatechange = nop;
+ }-*/;
+
+ /**
* Clear the success callback for fragment codefragment/code.
*/
@SuppressWarnings(unused)
@@ -88,8 +100,9 @@
return;
}
var head = document.getElementsByTagName('head').item(0);
+
@com.google.gwt.core.client.impl.CrossSiteLoadingStrategy::clearOnSuccess(*)(fragment);
+
@com.google.gwt.core.client.impl.CrossSiteLoadingStrategy::clearCallbacks(*)(tag);
head.removeChild(tag);
-
@com.google.gwt.core.client.impl.CrossSiteLoadingStrategy::clearOnSuccess(*)(fragment);
loadfinishedhandl...@com.google.gwt.core.client.impl.asyncfragmentloader.loadterminatedhandler::loadTerminated(*)(
exception);
}
@@ -99,8 +112,9 @@
JavaScriptObject tag) /*-{
return function(code) {
var head = document.getElementsByTagName('head').item(0);
+
@com.google.gwt.core.client.impl.CrossSiteLoadingStrategy::clearOnSuccess(*)(fragment);
+
@com.google.gwt.core.client.impl.CrossSiteLoadingStrategy::clearCallbacks(*)(tag);
head.removeChild(tag);
-
@com.google.gwt.core.client.impl.CrossSiteLoadingStrategy::clearOnSuccess(*)(fragment);
__gwtModuleFunction.installCode(code);
}
}-*/;
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Have IE's DOMImpl invoke its event dispatching function via a (issue543801)
This is ready for review. http://gwt-code-reviews.appspot.com/543801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Fixes a memory leak on IE with the cross-site fragment loading strategy. (issue544801)
This is ready for review. It's a tiny patch, and either of you would seem appropriate to me. Joel, this is a simpler memory leak than the one in the other patch I sent you. John, you recently reviewed a similar change to this class having to do with handling on-load and on-error events. http://gwt-code-reviews.appspot.com/544801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Introducing PrunerTest. It generates 85% of branch coverage for Pruner. (issue474803)
LVGTM.
Scott, you wrote OptimizerTestBase. What do you think about the divide
between JJSTestBase and OptimizerTestBase? JJS is for arbitrary tests
of JJS passes, while Optimizer is for ones that optimize one method into
another. Optimizer now has a stock Result class that can be reused for
many different tests.
Mike, you are a hero if you can speed up optimization. Kudos for writing
tests first.
http://gwt-code-reviews.appspot.com/474803/diff/3001/4009
File dev/core/test/com/google/gwt/dev/jjs/impl/OptimizerTestBase.java
(right):
http://gwt-code-reviews.appspot.com/474803/diff/3001/4009#newcode2
dev/core/test/com/google/gwt/dev/jjs/impl/OptimizerTestBase.java:2: *
Copyright 2010 Google Inc.
Just leave the original copyright year; we have stopped updating them.
http://gwt-code-reviews.appspot.com/474803/diff/3001/4009#newcode28
dev/core/test/com/google/gwt/dev/jjs/impl/OptimizerTestBase.java:28:
public abstract class OptimizerTestBase extends JJSTestBase {
A comment would be good. Adds a Result class or the like.
http://gwt-code-reviews.appspot.com/474803/diff/3001/4010
File dev/core/test/com/google/gwt/dev/jjs/impl/PrunerTest.java (right):
http://gwt-code-reviews.appspot.com/474803/diff/3001/4010#newcode109
dev/core/test/com/google/gwt/dev/jjs/impl/PrunerTest.java:109:
assertNull(result.findClass(EntryPoint$UnusedInterface));
Very nice.
http://gwt-code-reviews.appspot.com/474803/show
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Lazy initial enum maps, preinitialize values array. (issue280801)
Sorry, lost track of this patch. LGTM.
http://gwt-code-reviews.appspot.com/280801/diff/1/5
File user/super/com/google/gwt/emul/java/lang/Enum.java (right):
http://gwt-code-reviews.appspot.com/280801/diff/1/5#newcode32
user/super/com/google/gwt/emul/java/lang/Enum.java:32: if
(enumValueOfFunc == null) {
Can this happen, barring a compiler bug? If not, we may as well drop it
and save a few bytes of output per enum.
http://gwt-code-reviews.appspot.com/280801/show
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
Re: [gwt-contrib] Re: Fix GWT logging in Devmode (issue437801)
On Wed, May 5, 2010 at 2:15 PM, Ray Ryan rj...@google.com wrote: You should be able to throw subclasses of RuntimeException, no? For example, UnsupportedOperationException. -Lex -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
Re: [gwt-contrib] Re: Question about CompilePerms and adding gwt module source at runtime.
On Mon, May 3, 2010 at 7:36 AM, Marko Vuksanovic markovuksano...@gmail.comwrote: I solved the problem... This had nothing to do with the GWT. The problem was with adding a folder to java class path dynamically. Although at first I thought I had done it correctly, it turned out that that's a little tricky... For anyone else with the same problem - here's a gist on how to solve it... http://gist.github.com/387972 As you can see, a call to protected method is required in order to add a folder to class path. It would also be possible to create a new class loader and set that as the context class loader. Then GWT should use the new one. Alternatively, when the JVM is launched on the remote machine, pass the extra directories in using the -classpath option. -Lex -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Undo the collapse-all-permutations in JUnit.gwt.xml for now. (issue457801)
Reviewers: mmendez, Description: Undo the collapse-all-permutations in JUnit.gwt.xml for now. Review by: mmen...@google.com Please review this at http://gwt-code-reviews.appspot.com/457801/show Affected files: M user/src/com/google/gwt/junit/JUnit.gwt.xml Index: user/src/com/google/gwt/junit/JUnit.gwt.xml === --- user/src/com/google/gwt/junit/JUnit.gwt.xml (revision 8015) +++ user/src/com/google/gwt/junit/JUnit.gwt.xml (working copy) @@ -41,7 +41,4 @@ servlet path='/junithost/*' class='com.google.gwt.junit.server.JUnitHostImpl'/ inherits name=com.google.gwt.benchmarks.Benchmarks/ - - !-- Speed up test compilations by producing one permutation -- - collapse-all-properties / /module -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
Re: [gwt-contrib] Re: Question about CompilePerms and adding gwt module source at runtime.
On Tue, May 4, 2010 at 12:25 PM, Marko Vuksanovic markovuksano...@gmail.com wrote: Hi Lex, The first solution seems interesting... could you please provide a code snippet just to get me started... Did you mean something like Thread.currentThread().setContextClassLoader(urlClassloader); Exactly. Where urlClassloader is a freshly made URLClassLoader that you create. -Lex -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Introducing TOP value for constants analysis and using it (issue436801)
LGTM http://gwt-code-reviews.appspot.com/436801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Fixing division by zero while evaluating constant expression. (issue435801)
LGTM http://gwt-code-reviews.appspot.com/435801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: CFG: fixing case fallthrough. (issue434801)
LGTM I tricked myself into thinking fallthrough work, because fallthrough does happen from a statement to the following case: node. However, the case: node turns into a conditional, so it's important to skip that conditional if there's a fallthrough. I'm surprised this hasn't bitten more people than it has! http://gwt-code-reviews.appspot.com/434801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Introducing TOP value for constants analysis and using it (issue436801)
This is a great change! It simplifies the setup code for constant
analysis, and it means that the common case of a top assumption doesn't
require an explicit entry in the map.
Some small changes are requested in the implementation.
http://gwt-code-reviews.appspot.com/436801/diff/1/2
File
dev/core/src/com/google/gwt/dev/jjs/impl/gflow/constants/ConstantsAnalysis.java
(right):
http://gwt-code-reviews.appspot.com/436801/diff/1/2#newcode49
dev/core/src/com/google/gwt/dev/jjs/impl/gflow/constants/ConstantsAnalysis.java:49:
ConstantsAssumption.TOP, assumptionMap);
Nice to see this part go back to being so simple!
http://gwt-code-reviews.appspot.com/436801/diff/1/3
File
dev/core/src/com/google/gwt/dev/jjs/impl/gflow/constants/ConstantsAssumption.java
(right):
http://gwt-code-reviews.appspot.com/436801/diff/1/3#newcode157
dev/core/src/com/google/gwt/dev/jjs/impl/gflow/constants/ConstantsAssumption.java:157:
ConstantsAssumption other = (ConstantsAssumption) obj;
It won't come up much in practice, but it seems like there should be a
type test here before doing the cast. If the object is the wrong type,
return false.
http://gwt-code-reviews.appspot.com/436801/diff/1/3#newcode158
dev/core/src/com/google/gwt/dev/jjs/impl/gflow/constants/ConstantsAssumption.java:158:
return values.equals(other.values);
Shouldn't this apply equal(JValueLiteral,JValueLiteral) to each element?
Otherwise it will return false unnecessarily.
http://gwt-code-reviews.appspot.com/436801/diff/1/3#newcode188
dev/core/src/com/google/gwt/dev/jjs/impl/gflow/constants/ConstantsAssumption.java:188:
return null;
Null is the bottom assumption. Shouldn't this case return the receiver?
http://gwt-code-reviews.appspot.com/436801/diff/1/3#newcode211
dev/core/src/com/google/gwt/dev/jjs/impl/gflow/constants/ConstantsAssumption.java:211:
if (this == TOP) {
This should ideally check isEmpty(), too. Or else, make the class
constructor and the set() method private, so that isEmpty() isn't
needed.
http://gwt-code-reviews.appspot.com/436801/diff/1/6
File
dev/core/test/com/google/gwt/dev/jjs/impl/gflow/constants/ConstantsAnalysisTest.java
(right):
http://gwt-code-reviews.appspot.com/436801/diff/1/6#newcode1
dev/core/test/com/google/gwt/dev/jjs/impl/gflow/constants/ConstantsAnalysisTest.java:1:
package com.google.gwt.dev.jjs.impl.gflow.constants;
This files is also missing a copyright header.
http://gwt-code-reviews.appspot.com/436801/diff/1/7
File
dev/core/test/com/google/gwt/dev/jjs/impl/gflow/constants/ConstantsAssumptionTest.java
(right):
http://gwt-code-reviews.appspot.com/436801/diff/1/7#newcode1
dev/core/test/com/google/gwt/dev/jjs/impl/gflow/constants/ConstantsAssumptionTest.java:1:
package com.google.gwt.dev.jjs.impl.gflow.constants;
Needs copyright header.
http://gwt-code-reviews.appspot.com/436801/show
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Correcting the output names for SoycTest.java (issue442801)
LGTM Let's not quibble about the exact numbers right now. There was a lot of discussion about them, but I can't remember what the decision was. The important thing now is that the test passes. If someone can argue the number should be different, let's file that as a separate bug. http://gwt-code-reviews.appspot.com/442801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Don't devirtualize calls from AsyncLoader123.runAsync to (issue399803)
Reviewers: scottb,
Description:
Don't devirtualize calls from AsyncLoader123.runAsync to
AsyncLoader123.runCallbacks. Doing so spoils code
splitting for runAsync calls that are only reachable
via other runAsync calls.
Review by: sco...@google.com
Please review this at http://gwt-code-reviews.appspot.com/399803/show
Affected files:
M dev/core/src/com/google/gwt/dev/jjs/impl/FragmentLoaderCreator.java
M dev/core/src/com/google/gwt/dev/jjs/impl/MakeCallsStatic.java
Index: dev/core/src/com/google/gwt/dev/jjs/impl/FragmentLoaderCreator.java
===
--- dev/core/src/com/google/gwt/dev/jjs/impl/FragmentLoaderCreator.java
(revision 7979)
+++ dev/core/src/com/google/gwt/dev/jjs/impl/FragmentLoaderCreator.java
(working copy)
@@ -42,6 +42,7 @@
public static final String BROWSER_LOADER
= AsyncFragmentLoader.BROWSER_LOADER;
public static final String LOADER_METHOD_RUN_ASYNC = runAsync;
public static final String RUN_ASYNC_CALLBACK
= com.google.gwt.core.client.RunAsyncCallback;
+ public static final String RUN_CALLBACKS = runCallbacks;
private static final String GWT_CLASS =
FindDeferredBindingSitesVisitor.MAGIC_CLASS;
private static final String PROP_RUN_ASYNC_NEVER_RUNS
= gwt.jjs.runAsyncNeverRuns;
private static final String UNCAUGHT_EXCEPTION_HANDLER_CLASS
= GWT.UncaughtExceptionHandler;
@@ -110,7 +111,7 @@
srcWriter.println(BROWSER_LOADER + .logEventProgress(\runCallbacks
+ entryNumber + \, \begin\););
-srcWriter.println(instance.runCallbacks(););
+srcWriter.println(instance. + RUN_CALLBACKS + (););
srcWriter.println(BROWSER_LOADER + .logEventProgress(\runCallbacks
+ entryNumber + \, \end\););
@@ -138,7 +139,7 @@
srcWriter.println(});
srcWriter.println(if (instance != null) {);
-srcWriter.println( instance.runCallbacks(););
+srcWriter.println( instance. + RUN_CALLBACKS + (););
srcWriter.println( return;);
srcWriter.println(});
srcWriter.println(if (! + BROWSER_LOADER + .isLoading( +
entryNumber
Index: dev/core/src/com/google/gwt/dev/jjs/impl/MakeCallsStatic.java
===
--- dev/core/src/com/google/gwt/dev/jjs/impl/MakeCallsStatic.java (revision
7979)
+++ dev/core/src/com/google/gwt/dev/jjs/impl/MakeCallsStatic.java (working
copy)
@@ -313,6 +313,14 @@
return;
}
+ if (isRunCallbacksMethod(x.getTarget())) {
+/*
+ * Don't devirtualize these calls created by FragmentLoaderCreator,
+ * because it spoils code splitting.
+ */
+return;
+ }
+
ctx.replaceMe(makeStaticCall(x, newMethod));
}
@@ -367,6 +375,18 @@
}
}
+ private static boolean isRunCallbacksMethod(JMethod method) {
+if (method.getEnclosingType() != null
+ method.getEnclosingType().getName().startsWith(
+FragmentLoaderCreator.ASYNC_LOADER_PACKAGE + .
++ FragmentLoaderCreator.ASYNC_LOADER_CLASS_PREFIX)
+ method.getName().equals(FragmentLoaderCreator.RUN_CALLBACKS)) {
+ return true;
+}
+
+return false;
+ }
+
protected SetJMethod toBeMadeStatic = new HashSetJMethod();
private final JProgram program;
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Don't devirtualize calls from AsyncLoader123.runAsync to (issue399803)
This kind of patch is awfully tricky. Adding a JRunAsync node to the AST would make it a lot more straightforward. I found it very hard to make a test case with the current factoring. What we could do is divide CodeSplitter into two parts, one that does the code mapping and one that divides up the JS. I started to work on that, but it's a multi-hour project. http://gwt-code-reviews.appspot.com/399803/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Don't devirtualize calls from AsyncLoader123.runAsync to (issue399803)
Thanks!
http://gwt-code-reviews.appspot.com/399803/diff/1/2
File dev/core/src/com/google/gwt/dev/jjs/impl/FragmentLoaderCreator.java
(right):
http://gwt-code-reviews.appspot.com/399803/diff/1/2#newcode112
dev/core/src/com/google/gwt/dev/jjs/impl/FragmentLoaderCreator.java:112:
srcWriter.println(BROWSER_LOADER + .logEventProgress(\runCallbacks
On 2010/04/29 17:27:28, scottb wrote:
Use constant?
Done.
http://gwt-code-reviews.appspot.com/399803/diff/1/2#newcode168
dev/core/src/com/google/gwt/dev/jjs/impl/FragmentLoaderCreator.java:168:
srcWriter.println(public void runCallbacks() {);
On 2010/04/29 17:27:28, scottb wrote:
Need the constant here, for sure.
Done.
http://gwt-code-reviews.appspot.com/399803/diff/1/3
File dev/core/src/com/google/gwt/dev/jjs/impl/MakeCallsStatic.java
(right):
http://gwt-code-reviews.appspot.com/399803/diff/1/3#newcode319
dev/core/src/com/google/gwt/dev/jjs/impl/MakeCallsStatic.java:319: *
because it spoils code splitting.
On 2010/04/29 17:27:28, scottb wrote:
Maybe add a TODO here.
Done.
http://gwt-code-reviews.appspot.com/399803/show
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: For compatibility with linkers that don't yet understand soft (issue353801)
Committed at r7925. http://gwt-code-reviews.appspot.com/353801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Unify temp local tracking (issue354801)
LGTM.
http://gwt-code-reviews.appspot.com/354801/diff/1/2
File
dev/core/src/com/google/gwt/dev/jjs/impl/CompoundAssignmentNormalizer.java
(left):
http://gwt-code-reviews.appspot.com/354801/diff/1/2#oldcode426
dev/core/src/com/google/gwt/dev/jjs/impl/CompoundAssignmentNormalizer.java:426:
private JLocal getTempLocal(JType type) {
Rest in peace, getTempLocal. The approach in this patch is a great
improvement.
http://gwt-code-reviews.appspot.com/354801/diff/1/5
File dev/core/src/com/google/gwt/dev/jjs/impl/TempLocalVisitor.java
(right):
http://gwt-code-reviews.appspot.com/354801/diff/1/5#newcode2
dev/core/src/com/google/gwt/dev/jjs/impl/TempLocalVisitor.java:2: *
Copyright 2008 Google Inc.
Copyright year.
http://gwt-code-reviews.appspot.com/354801/diff/1/6
File dev/core/test/com/google/gwt/dev/jjs/impl/TempLocalVisitorTest.java
(right):
http://gwt-code-reviews.appspot.com/354801/diff/1/6#newcode20
dev/core/test/com/google/gwt/dev/jjs/impl/TempLocalVisitorTest.java:20:
* Test for SameParameterValueOptimizer.
Update comment.
http://gwt-code-reviews.appspot.com/354801/show
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Inlining local variables when the hold field value obtained from final context. (issue308801)
Sorry for the delay. Managing the latest GWT snapshot branch has taken
all my time.
This is a sweet optimization overall.
I believe two little things should be modified. The one about equality
can wait, if you'd prefer, but it seems like an easy patch that will
make it much more effective.
On a broad note, have you thought about how we will avoid duelling
optimizations if we also add the refactoring where a field reference is
replaced by a local? Locals are faster than field refs and can also be
better for code size. It's not something to deal with right now, but it
will be an issue as optimization improves.
http://gwt-code-reviews.appspot.com/308801/diff/1/4
File
dev/core/src/com/google/gwt/dev/jjs/impl/gflow/copy/CopyAssumption.java
(right):
http://gwt-code-reviews.appspot.com/308801/diff/1/4#newcode145
dev/core/src/com/google/gwt/dev/jjs/impl/gflow/copy/CopyAssumption.java:145:
return other.varValues.equals(varValues);
See below for comments on equality of variable values.
http://gwt-code-reviews.appspot.com/308801/diff/1/4#newcode174
dev/core/src/com/google/gwt/dev/jjs/impl/gflow/copy/CopyAssumption.java:174:
if (value == other.varValues.get(v)) {
This is too conservative, because it insists on object identity for the
referenced expressions. They might still be the same thing, e.g. two
distinct references to the variable t3, and yet compare false with ==.
Why not add a static method equals(JVariableRef,JVariableRef) and use
it?
http://gwt-code-reviews.appspot.com/308801/diff/1/4#newcode223
dev/core/src/com/google/gwt/dev/jjs/impl/gflow/copy/CopyAssumption.java:223:
if (instance instanceof JVariableRef) {
This should be a cast, not an if. To explain why the cast is safe,
perhaps add a comment pointing to isSupportedRhs in CopyFlowFunction.
http://gwt-code-reviews.appspot.com/308801/show
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: the initial assumption for parameters is top (issue405801)
http://gwt-code-reviews.appspot.com/405801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Unify temp local tracking (issue354801)
http://gwt-code-reviews.appspot.com/354801/diff/1/6 File dev/core/test/com/google/gwt/dev/jjs/impl/TempLocalVisitorTest.java (right): http://gwt-code-reviews.appspot.com/354801/diff/1/6#newcode1 dev/core/test/com/google/gwt/dev/jjs/impl/TempLocalVisitorTest.java:1: // Copyright 2009 Google Inc. All Rights Reserved. Sure! 2009 is the right year. http://gwt-code-reviews.appspot.com/354801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: the initial assumption for parameters is top (issue405801)
Thanks, Mike! Description updated. http://gwt-code-reviews.appspot.com/405801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] The gflow constant analyzer assumes that parameters (issue405801)
Reviewers: scottb, aizatsky, Description: The gflow constant analyzer assumes that parameters could initially hold anything, rather than assuming they haven't been initialized. Please review this at http://gwt-code-reviews.appspot.com/405801/show Affected files: M dev/core/src/com/google/gwt/dev/jjs/impl/gflow/constants/ConstantsAnalysis.java M dev/core/test/com/google/gwt/dev/jjs/impl/OptimizerTestBase.java M dev/core/test/com/google/gwt/dev/jjs/impl/gflow/CfgAnalysisTestBase.java M dev/core/test/com/google/gwt/dev/jjs/impl/gflow/constants/ConstantsAnalysisTest.java -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: the initial assumption for parameters is top (issue405801)
@Mike: does the gflow change look reasonable? @Scott: the new test case fundamentally needs that some method parameters be in scope of the test snippet. Does the approach in this patch look reasonable? http://gwt-code-reviews.appspot.com/405801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Fix floating-point string compare in CoverageTest (issue312804)
LGTM. An additional way to make such a test more reliable is to choose floating point numbers that are exactly representable, such as 1.5 and 1.25, and thus don't depend on single- versus double-precision. http://gwt-code-reviews.appspot.com/312804/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: CFG: Always jumping to first case statement (issue318802)
LGTM http://gwt-code-reviews.appspot.com/318802/diff/1/2 File dev/core/src/com/google/gwt/dev/jjs/impl/gflow/cfg/CfgBuilder.java (right): http://gwt-code-reviews.appspot.com/318802/diff/1/2#newcode619 dev/core/src/com/google/gwt/dev/jjs/impl/gflow/cfg/CfgBuilder.java:619: CfgSwitchGotoNode gotoNode = pushNode(new CfgSwitchGotoNode(parent, x)); Comment it? // goto the first non-default case http://gwt-code-reviews.appspot.com/318802/diff/1/2#newcode632 dev/core/src/com/google/gwt/dev/jjs/impl/gflow/cfg/CfgBuilder.java:632: addExit(gotoExit); Add a comment? // first non-default case http://gwt-code-reviews.appspot.com/318802/diff/1/3 File dev/core/src/com/google/gwt/dev/jjs/impl/gflow/cfg/CfgSwitchGotoNode.java (right): http://gwt-code-reviews.appspot.com/318802/diff/1/3#newcode2 dev/core/src/com/google/gwt/dev/jjs/impl/gflow/cfg/CfgSwitchGotoNode.java:2: * Copyright 2009 Google Inc. Year. http://gwt-code-reviews.appspot.com/318802/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: PopupPanel.show() can enter an invalid state if attached (issue298804)
LGTM http://gwt-code-reviews.appspot.com/298804/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors To unsubscribe, reply using remove me as the subject.
