[gwt-contrib] Re: Switch to internal implementation of StringInterner to avoid class loader (issue1578804)
LGTM
Still not really fond of the manual sharding, but it's hard to argue
against fixed without regression.
http://gwt-code-reviews.appspot.com/1578804/diff/4004/dev/core/test/com/google/gwt/dev/util/StringInternerTest.java
File dev/core/test/com/google/gwt/dev/util/StringInternerTest.java
(right):
http://gwt-code-reviews.appspot.com/1578804/diff/4004/dev/core/test/com/google/gwt/dev/util/StringInternerTest.java#newcode87
dev/core/test/com/google/gwt/dev/util/StringInternerTest.java:87:
private static String makeString(int[] array) {
Dead code?
http://gwt-code-reviews.appspot.com/1578804/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Switch to internal implementation of StringInterner to avoid class loader (issue1578804)
http://gwt-code-reviews.appspot.com/1578804/diff/1/dev/core/src/com/google/gwt/dev/util/StringInterner.java File dev/core/src/com/google/gwt/dev/util/StringInterner.java (right): http://gwt-code-reviews.appspot.com/1578804/diff/1/dev/core/src/com/google/gwt/dev/util/StringInterner.java#newcode33 dev/core/src/com/google/gwt/dev/util/StringInterner.java:33: private static int SHARD_BITS = 8; Can all this sharding code just be replaced with a ThreadLocal? http://gwt-code-reviews.appspot.com/1578804/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Switch to internal implementation of StringInterner to avoid class loader (issue1578804)
http://gwt-code-reviews.appspot.com/1578804/diff/1/dev/core/src/com/google/gwt/dev/util/StringInterner.java File dev/core/src/com/google/gwt/dev/util/StringInterner.java (right): http://gwt-code-reviews.appspot.com/1578804/diff/1/dev/core/src/com/google/gwt/dev/util/StringInterner.java#newcode33 dev/core/src/com/google/gwt/dev/util/StringInterner.java:33: private static int SHARD_BITS = 8; On 2011/10/21 00:50:36, skybrian wrote: On 2011/10/20 22:10:55, tobyr wrote: Can all this sharding code just be replaced with a ThreadLocal? Maybe. It would use more memory when different threads intern the same string. (But maybe we don't care that much?) I don't think it'd be large enough to warrant the extra complexity, but you can test that emperically on some programs. http://gwt-code-reviews.appspot.com/1578804/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Improves Dev Mode reporting of JavaScriptException to include method (issue1570803)
On 2011/10/14 17:47:41, rjrjr wrote: Some day I'll remember to run tests *before* posting for review. But since that day is not today, PTAL LGTM. http://gwt-code-reviews.appspot.com/1570803/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Improves Dev Mode reporting of JavaScriptException to include method (issue1570803)
http://gwt-code-reviews.appspot.com/1570803/diff/1/dev/core/src/com/google/gwt/dev/shell/BrowserChannelServer.java
File dev/core/src/com/google/gwt/dev/shell/BrowserChannelServer.java
(right):
http://gwt-code-reviews.appspot.com/1570803/diff/1/dev/core/src/com/google/gwt/dev/shell/BrowserChannelServer.java#newcode245
dev/core/src/com/google/gwt/dev/shell/BrowserChannelServer.java:245:
exceptionValue, methodName + ( + args + ), return value: +
returnJsValue);
Do you want Arrays.toString(args) instead of args?
http://gwt-code-reviews.appspot.com/1570803/diff/1/dev/core/src/com/google/gwt/dev/shell/ModuleSpace.java
File dev/core/src/com/google/gwt/dev/shell/ModuleSpace.java (right):
http://gwt-code-reviews.appspot.com/1570803/diff/1/dev/core/src/com/google/gwt/dev/shell/ModuleSpace.java#newcode60
dev/core/src/com/google/gwt/dev/shell/ModuleSpace.java:60: * session's
thread.
Rather than duping the doc, something like:
Equivalent to {@link
#createJavaScriptException(ClassLoader,Object,String)
createJavaScriptException(cl, exception, )}
http://gwt-code-reviews.appspot.com/1570803/diff/1/user/src/com/google/gwt/core/client/JavaScriptException.java
File user/src/com/google/gwt/core/client/JavaScriptException.java
(right):
http://gwt-code-reviews.appspot.com/1570803/diff/1/user/src/com/google/gwt/core/client/JavaScriptException.java#newcode191
user/src/com/google/gwt/core/client/JavaScriptException.java:191:
description = description + : + getDescription(e);
I'm a little confused by what's going on here, since you seem to be
overloading the meaning of description.
http://gwt-code-reviews.appspot.com/1570803/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Improves Dev Mode reporting of JavaScriptException to include method (issue1570803)
LGTM http://gwt-code-reviews.appspot.com/1570803/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Changed the interface used to send metrics to a dashboard application. The (issue1499811)
On 2011/09/14 18:58:04, jhumphries wrote: LGTM again http://gwt-code-reviews.appspot.com/1499811/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Changed the interface used to send metrics to a dashboard application. The (issue1499811)
On 2011/09/14 18:58:04, jhumphries wrote: LGTM again http://gwt-code-reviews.appspot.com/1499811/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Tweak to allow parent ClassLoader at design time. (issue1529803)
LGTM http://gwt-code-reviews.appspot.com/1529803/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Changed the interface used to send metrics to a dashboard application. The (issue1499811)
LGTM http://gwt-code-reviews.appspot.com/1499811/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Fix: http://code.google.com/p/google-web-toolkit/issues/detail?id=6367 (issue1508802)
On 2011/08/03 00:22:49, meder wrote: LGTM http://gwt-code-reviews.appspot.com/1508802/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Update to the staleness check when loading the PersistentUnitCache (issue1483803)
LGTM http://gwt-code-reviews.appspot.com/1483803/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Handle arrays of generic types, in rpc result cacheability checking (issue1463809)
http://gwt-code-reviews.appspot.com/1463809/diff/1/user/src/com/google/gwt/user/rebind/rpc/CachedRpcTypeInformation.java File user/src/com/google/gwt/user/rebind/rpc/CachedRpcTypeInformation.java (right): http://gwt-code-reviews.appspot.com/1463809/diff/1/user/src/com/google/gwt/user/rebind/rpc/CachedRpcTypeInformation.java#newcode166 user/src/com/google/gwt/user/rebind/rpc/CachedRpcTypeInformation.java:166: // we have a type that is an array with a primitive leafType On 2011/06/24 22:56:41, jbrosenberg wrote: Well, it's a recursive call on the leafType, which in turn can arrive here. If the leafType is a JPrimitiveType, then it will behave as previously. The change is to handle if the leaf type is a JRawType, which in turn will be handled by the second recursive call, and then will end up in the JRealClassType case above (JGenericType is a sub-class of JRealType). I understand what you're saying, but in the previous code the path to get to this check was direct, so the comment makes sense there. Specifically, you can see typeToCheck get assigned to the leafType, when it's not JRawType or JRealClassType. In this case, you're flat out testing for JPrimitiveType, and it may just happen to come through from a recursive call involving a leafType. For example, in the previous code if a JPrimitiveType was passed directly, then an assert on instanceof JRealClassType would fail. In this code, if you passed a JPrimitiveType directly, it would return Long.MAX_VALUE. http://gwt-code-reviews.appspot.com/1463809/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Enable generator result caching, by default. (issue1467808)
LGTM http://gwt-code-reviews.appspot.com/1467808/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Fix for rpc generator result caching (fix potential NPE) (issue1462809)
http://gwt-code-reviews.appspot.com/1462809/diff/1/user/src/com/google/gwt/user/rebind/rpc/CachedRpcTypeInformation.java
File
user/src/com/google/gwt/user/rebind/rpc/CachedRpcTypeInformation.java
(right):
http://gwt-code-reviews.appspot.com/1462809/diff/1/user/src/com/google/gwt/user/rebind/rpc/CachedRpcTypeInformation.java#newcode51
user/src/com/google/gwt/user/rebind/rpc/CachedRpcTypeInformation.java:51:
if (customSerializersUsed != null) {
Can you just make this an empty list instead of somewhere else?
http://gwt-code-reviews.appspot.com/1462809/diff/1/user/src/com/google/gwt/user/rebind/rpc/CachedRpcTypeInformation.java#newcode197
user/src/com/google/gwt/user/rebind/rpc/CachedRpcTypeInformation.java:197:
if (sto == null) {
When is this null?
http://gwt-code-reviews.appspot.com/1462809/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Fix for rpc generator result caching (fix potential NPE) (issue1462809)
LGTM
http://gwt-code-reviews.appspot.com/1462809/diff/2003/user/src/com/google/gwt/user/rebind/rpc/CachedRpcTypeInformation.java
File
user/src/com/google/gwt/user/rebind/rpc/CachedRpcTypeInformation.java
(right):
http://gwt-code-reviews.appspot.com/1462809/diff/2003/user/src/com/google/gwt/user/rebind/rpc/CachedRpcTypeInformation.java#newcode128
user/src/com/google/gwt/user/rebind/rpc/CachedRpcTypeInformation.java:128:
private void addCustomSerializerType(JType type) {
This is why I hate the sorting part of our code style. So much diffage
for so little change.
http://gwt-code-reviews.appspot.com/1462809/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Handle arrays of generic types, in rpc result cacheability checking (issue1463809)
LGTM http://gwt-code-reviews.appspot.com/1463809/diff/1/user/src/com/google/gwt/user/rebind/rpc/CachedRpcTypeInformation.java File user/src/com/google/gwt/user/rebind/rpc/CachedRpcTypeInformation.java (right): http://gwt-code-reviews.appspot.com/1463809/diff/1/user/src/com/google/gwt/user/rebind/rpc/CachedRpcTypeInformation.java#newcode166 user/src/com/google/gwt/user/rebind/rpc/CachedRpcTypeInformation.java:166: // we have a type that is an array with a primitive leafType I think this comment is stale now. http://gwt-code-reviews.appspot.com/1463809/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Last change prevented re-loading modules, but caused it to write too many (issue1464803)
http://gwt-code-reviews.appspot.com/1464803/diff/5/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/1464803/diff/5/dev/core/src/com/google/gwt/dev/CompileModule.java#newcode211
dev/core/src/com/google/gwt/dev/CompileModule.java:211: // written out.
Should NOT be written out?
http://gwt-code-reviews.appspot.com/1464803/diff/5/dev/core/src/com/google/gwt/dev/CompileModule.java#newcode251
dev/core/src/com/google/gwt/dev/CompileModule.java:251: for
(CompilationUnit unit : archive.getUnits().values()) {
I think you want to drop this for loop, by changing the above
declaration to:
SetString archivedUnits = new
HashSetString(archive.getUnits().values());
http://gwt-code-reviews.appspot.com/1464803/diff/5/dev/core/src/com/google/gwt/dev/CompileModule.java#newcode282
dev/core/src/com/google/gwt/dev/CompileModule.java:282:
CompilationUnitArchive outputModule = new
CompilationUnitArchive(moduleToCompile);
Not from this patch, but I think this would be more clear if it were
named moduleArchive.
http://gwt-code-reviews.appspot.com/1464803/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Last change prevented re-loading modules, but caused it to write too many (issue1464803)
LGTM
http://gwt-code-reviews.appspot.com/1464803/diff/5/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/1464803/diff/5/dev/core/src/com/google/gwt/dev/CompileModule.java#newcode251
dev/core/src/com/google/gwt/dev/CompileModule.java:251: for
(CompilationUnit unit : archive.getUnits().values()) {
On 2011/06/23 17:06:21, zundel wrote:
On 2011/06/23 16:50:31, tobyr wrote:
I think you want to drop this for loop, by changing the above
declaration to:
SetString archivedUnits = new
HashSetString(archive.getUnits().values());
Unfortunately, I'm not adding the units themselves, I'm adding the
getResourcePath() value from each one.
Oops. Too bad this isn't Scala where it'd be a nice one-liner.
http://gwt-code-reviews.appspot.com/1464803/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Fix for HTMLPanel.addAndReplaceElement() logical detach bug. (issue1462808)
On 2011/06/23 19:18:27, gfotos wrote: Testing to see if a message gets sent to gwt-contrib. http://gwt-code-reviews.appspot.com/1462808/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Update client bundle result caching to track absent resources (issue1466804)
LGTM http://gwt-code-reviews.appspot.com/1466804/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Turns on reading from archives by default. (issue1461801)
LGTM http://gwt-code-reviews.appspot.com/1461801/diff/4007/dev/core/src/com/google/gwt/dev/ArchivePreloader.java File dev/core/src/com/google/gwt/dev/ArchivePreloader.java (right): http://gwt-code-reviews.appspot.com/1461801/diff/4007/dev/core/src/com/google/gwt/dev/ArchivePreloader.java#newcode59 dev/core/src/com/google/gwt/dev/ArchivePreloader.java:59: if (!alreadyLoaded.containsKey(toLoad) Would prefer to see the key queried once outside the if stmt, rather than twice - once in contains and once in get. Would also make the if stmt more readable. http://gwt-code-reviews.appspot.com/1461801/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Disabling test for null on getBrowserById for DOM elements without DTD when running on Chrome 11... (issue1462805)
LGTM http://gwt-code-reviews.appspot.com/1462805/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Turns on reading from archives by default. (issue1461801)
LGTM http://gwt-code-reviews.appspot.com/1461801/diff/4002/dev/core/src/com/google/gwt/dev/ArchivePreloader.java File dev/core/src/com/google/gwt/dev/ArchivePreloader.java (right): http://gwt-code-reviews.appspot.com/1461801/diff/4002/dev/core/src/com/google/gwt/dev/ArchivePreloader.java#newcode44 dev/core/src/com/google/gwt/dev/ArchivePreloader.java:44: logger.log(TreeLogger.TRACE, Looking for precompiled archives. To disable, use -Dgwt.usearchives=false); 100 chars http://gwt-code-reviews.appspot.com/1461801/diff/4002/dev/core/src/com/google/gwt/dev/cfg/ModuleDefSchema.java File dev/core/src/com/google/gwt/dev/cfg/ModuleDefSchema.java (right): http://gwt-code-reviews.appspot.com/1461801/diff/4002/dev/core/src/com/google/gwt/dev/cfg/ModuleDefSchema.java#newcode493 dev/core/src/com/google/gwt/dev/cfg/ModuleDefSchema.java:493: TreeLogger branch = logger.branch(TreeLogger.DEBUG, Why is this log even needed? We log from ModuleDefLoader.nestedLoad. http://gwt-code-reviews.appspot.com/1461801/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Updated rpc generator result caching for field serializers to use type signature instead of last... (issue1446818)
LGTM http://gwt-code-reviews.appspot.com/1446818/diff/1/user/src/com/google/gwt/user/rebind/rpc/TypeSerializerCreator.java File user/src/com/google/gwt/user/rebind/rpc/TypeSerializerCreator.java (right): http://gwt-code-reviews.appspot.com/1446818/diff/1/user/src/com/google/gwt/user/rebind/rpc/TypeSerializerCreator.java#newcode69 user/src/com/google/gwt/user/rebind/rpc/TypeSerializerCreator.java:69: public static final String CACHED_TYPE_INFO_KEY = ctik; Unless there are some real constraints, would prefer the actual value of this constant to be a little less brief. For example, would be less opaque in debugging contexts. http://gwt-code-reviews.appspot.com/1446818/diff/1010/dev/core/src/com/google/gwt/dev/javac/rebind/CachedPropertyInformation.java File dev/core/src/com/google/gwt/dev/javac/rebind/CachedPropertyInformation.java (right): http://gwt-code-reviews.appspot.com/1446818/diff/1010/dev/core/src/com/google/gwt/dev/javac/rebind/CachedPropertyInformation.java#newcode111 dev/core/src/com/google/gwt/dev/javac/rebind/CachedPropertyInformation.java:111: Found problem checking configuration property: + e.getMessage()); Not good to squash the stack trace. http://gwt-code-reviews.appspot.com/1446818/diff/1010/user/src/com/google/gwt/user/rebind/rpc/TypeSerializerCreator.java File user/src/com/google/gwt/user/rebind/rpc/TypeSerializerCreator.java (right): http://gwt-code-reviews.appspot.com/1446818/diff/1010/user/src/com/google/gwt/user/rebind/rpc/TypeSerializerCreator.java#newcode254 user/src/com/google/gwt/user/rebind/rpc/TypeSerializerCreator.java:254: boolean foundMatch = This seems like a superfluous variable declaration http://gwt-code-reviews.appspot.com/1446818/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Exercise RunAsyncCode class for prefetching fragments. (issue1453805)
LGTM http://gwt-code-reviews.appspot.com/1453805/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Add JSO inspection support. (issue1454807)
LGTM http://gwt-code-reviews.appspot.com/1454807/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: CompileModule creates a serialized set of compilation units to represent (issue1448808)
LGTM with a few comments. http://gwt-code-reviews.appspot.com/1448808/diff/8042/dev/core/src/com/google/gwt/dev/javac/CompilationStateBuilder.java File dev/core/src/com/google/gwt/dev/javac/CompilationStateBuilder.java (right): http://gwt-code-reviews.appspot.com/1448808/diff/8042/dev/core/src/com/google/gwt/dev/javac/CompilationStateBuilder.java#newcode351 dev/core/src/com/google/gwt/dev/javac/CompilationStateBuilder.java:351: // them out. I'm confused about this. I see what looks like code for supporting this in UnitCache/PersistentUnitCache, but I'm not sure it's wired up. Should this be using unitCache.addArchivedUnit() ? http://gwt-code-reviews.appspot.com/1448808/diff/8042/dev/core/src/com/google/gwt/dev/javac/CompilationUnitArchive.java File dev/core/src/com/google/gwt/dev/javac/CompilationUnitArchive.java (right): http://gwt-code-reviews.appspot.com/1448808/diff/8042/dev/core/src/com/google/gwt/dev/javac/CompilationUnitArchive.java#newcode69 dev/core/src/com/google/gwt/dev/javac/CompilationUnitArchive.java:69: * the same order every time so that the output is deterministic. Stale comment. http://gwt-code-reviews.appspot.com/1448808/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Adds support for runtime evaluation of JavaScriptObject methods from a debugger. Primarily inten... (issue1453808)
Reviewers: rdayal, Description: Adds support for runtime evaluation of JavaScriptObject methods from a debugger. Primarily intended as support API for debuggers, but developers can also use it directly in a debugger (for example, in watch windows or breakpoint expressions). Review by: rda...@google.com Please review this at http://gwt-code-reviews.appspot.com/1453808/ Affected files: A dev/core/src/com/google/gwt/core/ext/debug/JsoEval.java M dev/core/src/com/google/gwt/dev/shell/CompilingClassLoader.java -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Minor changes to dashboard notifier interface to enable better session-tracking (issue1450812)
LGTM http://gwt-code-reviews.appspot.com/1450812/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[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] Re: Enables on the persistent unit cache by default. (issue1448801)
LGTM http://gwt-code-reviews.appspot.com/1448801/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Adding additional testing for GWT RPC. Some custom serialized objects (issue1441804)
http://gwt-code-reviews.appspot.com/1441804/diff/1/user/test/com/google/gwt/user/client/rpc/LoggingRPCTest.java File user/test/com/google/gwt/user/client/rpc/LoggingRPCTest.java (right): http://gwt-code-reviews.appspot.com/1441804/diff/1/user/test/com/google/gwt/user/client/rpc/LoggingRPCTest.java#newcode84 user/test/com/google/gwt/user/client/rpc/LoggingRPCTest.java:84: */ Stale comment? http://gwt-code-reviews.appspot.com/1441804/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Minor refactoring of dashboard interfaces in GWT. I'd made these changes this week in another ... (issue1439801)
Nice. LGTM http://gwt-code-reviews.appspot.com/1439801/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Auto format pass of some files before review (issue1444802)
On 2011/05/11 20:54:26, zundel wrote: On 2011/05/11 20:20:07, zundel wrote: I ran the eclipse autoformatter and edited one comment that got trashed. FYI, I went ahead and committed this change. LGTM http://gwt-code-reviews.appspot.com/1444802/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Disabling test for null on getBrowserById for DOM elements without DTD when running on Chrome 12 (issue1443801)
LGTM http://gwt-code-reviews.appspot.com/1443801/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Phase 1 of GWT Dashboard. This includes an interface in gwt-dev for posting to a dashboard (defa... (issue1427807)
Formatting changes look good. Just one more nit. http://gwt-code-reviews.appspot.com/1427807/diff/15015/dev/core/src/com/google/gwt/dev/util/log/speedtracer/SpeedTracerLogger.java File dev/core/src/com/google/gwt/dev/util/log/speedtracer/SpeedTracerLogger.java (right): http://gwt-code-reviews.appspot.com/1427807/diff/15015/dev/core/src/com/google/gwt/dev/util/log/speedtracer/SpeedTracerLogger.java#newcode71 dev/core/src/com/google/gwt/dev/util/log/speedtracer/SpeedTracerLogger.java:71: private static final boolean logThreadCpuTime = !logProcessCpuTime I'd rather see a test that throws an exception if both are set. That way it's not a silent failure. http://gwt-code-reviews.appspot.com/1427807/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Phase 1 of GWT Dashboard. This includes an interface in gwt-dev for posting to a dashboard (defa... (issue1427807)
On 2011/05/06 19:58:54, Josh Humphries wrote: http://gwt-code-reviews.appspot.com/1427807/diff/15015/dev/core/src/com/google/gwt/dev/util/log/speedtracer/SpeedTracerLogger.java File dev/core/src/com/google/gwt/dev/util/log/speedtracer/SpeedTracerLogger.java (right): http://gwt-code-reviews.appspot.com/1427807/diff/15015/dev/core/src/com/google/gwt/dev/util/log/speedtracer/SpeedTracerLogger.java#newcode71 dev/core/src/com/google/gwt/dev/util/log/speedtracer/SpeedTracerLogger.java:71: private static final boolean logThreadCpuTime = !logProcessCpuTime Like so? (see patchset 7) On 2011/05/06 19:03:11, tobyr wrote: I'd rather see a test that throws an exception if both are set. That way it's not a silent failure. LGTM http://gwt-code-reviews.appspot.com/1427807/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Phase 1 of GWT Dashboard. This includes an interface in gwt-dev for posting to a dashboard (defa... (issue1427807)
LGTM http://gwt-code-reviews.appspot.com/1427807/diff/10002/dev/core/src/com/google/gwt/dev/util/log/speedtracer/SpeedTracerLogger.java File dev/core/src/com/google/gwt/dev/util/log/speedtracer/SpeedTracerLogger.java (right): http://gwt-code-reviews.appspot.com/1427807/diff/10002/dev/core/src/com/google/gwt/dev/util/log/speedtracer/SpeedTracerLogger.java#newcode331 dev/core/src/com/google/gwt/dev/util/log/speedtracer/SpeedTracerLogger.java:331: * @param refEventthe event during which the garbage collections took place On 2011/05/05 18:40:55, Josh Humphries wrote: GWT formatting is explained at http://code.google.com/webtoolkit/makinggwtbetter.html. There are instructions for setting up Eclipse formatting settings in svn (under the eclipse folder). You should be using autoformat to get the correct formatting. On 2011/05/05 16:45:54, tobyr wrote: Is your Eclipse formatting aligning these this way? I don't think we do this normally. http://gwt-code-reviews.appspot.com/1427807/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Adds HTML5 App Cache support to MobileWebApp sample. (issue1428811)
http://gwt-code-reviews.appspot.com/1428811/diff/1/dev/core/src/com/google/gwt/core/linker/SimpleAppCacheLinker.java File dev/core/src/com/google/gwt/core/linker/SimpleAppCacheLinker.java (right): http://gwt-code-reviews.appspot.com/1428811/diff/1/dev/core/src/com/google/gwt/core/linker/SimpleAppCacheLinker.java#newcode39 dev/core/src/com/google/gwt/core/linker/SimpleAppCacheLinker.java:39: * liAdd a mime-mapping to your web.xml file (this is for AppEngine, adjust The sample XML below looks standard to me. Why are we mentioning App Engine? http://gwt-code-reviews.appspot.com/1428811/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Discards the jar file name in the resource location. It isn't necessary, and (issue1428805)
LGTM
Nice change. I like this better.
http://gwt-code-reviews.appspot.com/1428805/diff/1/dev/core/src/com/google/gwt/dev/javac/MemoryUnitCache.java
File dev/core/src/com/google/gwt/dev/javac/MemoryUnitCache.java (right):
http://gwt-code-reviews.appspot.com/1428805/diff/1/dev/core/src/com/google/gwt/dev/javac/MemoryUnitCache.java#newcode147
dev/core/src/com/google/gwt/dev/javac/MemoryUnitCache.java:147: try {
On 2011/04/30 16:08:28, jbrosenberg wrote:
Actually, this won't open the file, it will only parse the URL string,
and
return the name part, which is the substring after the '!'. It's
probably not
necessary to construct a JarEntry intermediate value. You can just do
connection.getEntryName().
You're right. It's not really obvious that's what's happening though
(and I've implemented my own URLConnection and URLStreamHandler).
http://gwt-code-reviews.appspot.com/1428805/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Discards the jar file name in the resource location. It isn't necessary, and (issue1428805)
LGTM
Nice change. I like this better.
http://gwt-code-reviews.appspot.com/1428805/diff/1/dev/core/src/com/google/gwt/dev/javac/MemoryUnitCache.java
File dev/core/src/com/google/gwt/dev/javac/MemoryUnitCache.java (right):
http://gwt-code-reviews.appspot.com/1428805/diff/1/dev/core/src/com/google/gwt/dev/javac/MemoryUnitCache.java#newcode147
dev/core/src/com/google/gwt/dev/javac/MemoryUnitCache.java:147: try {
On 2011/04/30 16:08:28, jbrosenberg wrote:
Actually, this won't open the file, it will only parse the URL string,
and
return the name part, which is the substring after the '!'. It's
probably not
necessary to construct a JarEntry intermediate value. You can just do
connection.getEntryName().
You're right. It's not really obvious that's what's happening though
(and I've implemented my own URLConnection and URLStreamHandler).
http://gwt-code-reviews.appspot.com/1428805/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Discards the jar file name in the resource location. It isn't necessary, and (issue1428805)
LGTM
Nice change. I like this better.
http://gwt-code-reviews.appspot.com/1428805/diff/1/dev/core/src/com/google/gwt/dev/javac/MemoryUnitCache.java
File dev/core/src/com/google/gwt/dev/javac/MemoryUnitCache.java (right):
http://gwt-code-reviews.appspot.com/1428805/diff/1/dev/core/src/com/google/gwt/dev/javac/MemoryUnitCache.java#newcode147
dev/core/src/com/google/gwt/dev/javac/MemoryUnitCache.java:147: try {
On 2011/04/30 16:08:28, jbrosenberg wrote:
Actually, this won't open the file, it will only parse the URL string,
and
return the name part, which is the substring after the '!'. It's
probably not
necessary to construct a JarEntry intermediate value. You can just do
connection.getEntryName().
You're right. It's not really obvious that's what's happening though
(and I've implemented my own URLConnection and URLStreamHandler).
http://gwt-code-reviews.appspot.com/1428805/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Discards the jar file name in the resource location. It isn't necessary, and (issue1428805)
LGTM Nice change. I like this better. (This failed last time. This might be a duplicate). http://gwt-code-reviews.appspot.com/1428805/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Discards the jar file name in the resource location. It isn't necessary, and (issue1428805)
http://gwt-code-reviews.appspot.com/1428805/diff/1/dev/core/src/com/google/gwt/dev/javac/MemoryUnitCache.java
File dev/core/src/com/google/gwt/dev/javac/MemoryUnitCache.java (right):
http://gwt-code-reviews.appspot.com/1428805/diff/1/dev/core/src/com/google/gwt/dev/javac/MemoryUnitCache.java#newcode80
dev/core/src/com/google/gwt/dev/javac/MemoryUnitCache.java:80: * The key
is resource location.
Update the doc for the key.
http://gwt-code-reviews.appspot.com/1428805/diff/1/dev/core/src/com/google/gwt/dev/javac/MemoryUnitCache.java#newcode147
dev/core/src/com/google/gwt/dev/javac/MemoryUnitCache.java:147: try {
Is there any way to do this cheaper? This seems like a lot of work
(opening up a Jar file and getting an entry), for every single cached
unit.
http://gwt-code-reviews.appspot.com/1428805/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Phase 1 of GWT Dashboard. This includes an interface in gwt-dev for posting to a dashboard (defa... (issue1427807)
http://gwt-code-reviews.appspot.com/1427807/diff/1/dev/core/src/com/google/gwt/dev/util/log/dashboard/DashboardNotifier.java
File
dev/core/src/com/google/gwt/dev/util/log/dashboard/DashboardNotifier.java
(right):
http://gwt-code-reviews.appspot.com/1427807/diff/1/dev/core/src/com/google/gwt/dev/util/log/dashboard/DashboardNotifier.java#newcode23
dev/core/src/com/google/gwt/dev/util/log/dashboard/DashboardNotifier.java:23:
public interface DashboardNotifier {
Meta-points.
1) I think all the thread wonkiness here is disconcerting, and I don't
understand the need for begin/end method pairs. I think I'd rather see
us provide a DevModeSession object to the dev-mode related APIs. For
example,
class DevModeSession {
int id; // maybe
String moduleName;
String userAgent;
}
void devModeStartSession(DevModeSession session);
void devModeRefresh(DevModeSession session, int duration);
void devModeEvent(DevModeSession session, Event event);
http://gwt-code-reviews.appspot.com/1427807/diff/1/dev/core/src/com/google/gwt/dev/util/log/dashboard/DashboardNotifierFactory.java
File
dev/core/src/com/google/gwt/dev/util/log/dashboard/DashboardNotifierFactory.java
(right):
http://gwt-code-reviews.appspot.com/1427807/diff/1/dev/core/src/com/google/gwt/dev/util/log/dashboard/DashboardNotifierFactory.java#newcode24
dev/core/src/com/google/gwt/dev/util/log/dashboard/DashboardNotifierFactory.java:24:
private static boolean notificationsEnabled =
I think this should just be String dashboardClass =
System.getProperty(gwt.dashboard).
http://gwt-code-reviews.appspot.com/1427807/diff/1/dev/core/src/com/google/gwt/dev/util/log/dashboard/DashboardNotifierFactory.java#newcode33
dev/core/src/com/google/gwt/dev/util/log/dashboard/DashboardNotifierFactory.java:33:
public static boolean areNotificationsEnabled() {
I don't think we need this.
http://gwt-code-reviews.appspot.com/1427807/diff/1/dev/core/src/com/google/gwt/dev/util/log/dashboard/DashboardNotifierFactory.java#newcode64
dev/core/src/com/google/gwt/dev/util/log/dashboard/DashboardNotifierFactory.java:64:
if (notificationsEnabled) {
Use dashboardClass != null
http://gwt-code-reviews.appspot.com/1427807/diff/1/dev/core/src/com/google/gwt/dev/util/log/dashboard/DashboardNotifierFactory.java#newcode65
dev/core/src/com/google/gwt/dev/util/log/dashboard/DashboardNotifierFactory.java:65:
try {
Use dashboardClass.
http://gwt-code-reviews.appspot.com/1427807/diff/1/dev/core/src/com/google/gwt/dev/util/log/dashboard/DashboardNotifierFactory.java#newcode68
dev/core/src/com/google/gwt/dev/util/log/dashboard/DashboardNotifierFactory.java:68:
} catch (ClassCastException e) {
While it's considered good style to explicitly catch different exception
types, I don't think your behavior is going to be any different for
expected exceptions vs unexpected exceptions. I think you'd be better
off catching Exception here.
http://gwt-code-reviews.appspot.com/1427807/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Another case identified where spam reduction reduced too much and didn't print a relavent (issue1420805)
On 2011/04/20 18:00:37, zundel wrote: LGTM http://gwt-code-reviews.appspot.com/1420805/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Treats jar file entries with special timestamps differently for purposes (issue1422802)
Does MockCompilationUnit need to be updated too?
http://gwt-code-reviews.appspot.com/1422802/diff/1/dev/core/src/com/google/gwt/dev/javac/CompilationStateBuilder.java
File dev/core/src/com/google/gwt/dev/javac/CompilationStateBuilder.java
(right):
http://gwt-code-reviews.appspot.com/1422802/diff/1/dev/core/src/com/google/gwt/dev/javac/CompilationStateBuilder.java#newcode365
dev/core/src/com/google/gwt/dev/javac/CompilationStateBuilder.java:365:
JarURLConnection jarConn = (JarURLConnection) url.openConnection();
It's not instantly obvious that url must be a jar URL. Mind adding an
@param?
http://gwt-code-reviews.appspot.com/1422802/diff/1/dev/core/src/com/google/gwt/dev/javac/CompilationStateBuilder.java#newcode367
dev/core/src/com/google/gwt/dev/javac/CompilationStateBuilder.java:367:
if (url.getProtocol().equals(file)) {
Is this code similar to jbrosenberg's treatment in ResourceBundle?
Should we refactor it into a utility method both can share?
http://gwt-code-reviews.appspot.com/1422802/diff/1/dev/core/src/com/google/gwt/dev/javac/CompilationStateBuilder.java#newcode504
dev/core/src/com/google/gwt/dev/javac/CompilationStateBuilder.java:504:
if (resourceUrl != null) {
I know there's a school of thought that single exit point is great, but
the level of indentation in here is starting to get pretty bad. Exiting
early usually reads nicely as a set of cases. In addition, you're
reusing resourceLastModified to mean different things, so it makes it
even harder to follow.
What do you think?
http://gwt-code-reviews.appspot.com/1422802/diff/1/dev/core/src/com/google/gwt/dev/javac/CompilationUnitBuilder.java
File dev/core/src/com/google/gwt/dev/javac/CompilationUnitBuilder.java
(right):
http://gwt-code-reviews.appspot.com/1422802/diff/1/dev/core/src/com/google/gwt/dev/javac/CompilationUnitBuilder.java#newcode65
dev/core/src/com/google/gwt/dev/javac/CompilationUnitBuilder.java:65:
Collection? extends JsniMethod jsniMethods, MethodArgNamesLookup
methodArgs,
All the formatting changes in here make it really difficult to tell what
actually changed.
http://gwt-code-reviews.appspot.com/1422802/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Allows the gwt.persistentunitcache property to accept a boolean (issue1415802)
LGTM http://gwt-code-reviews.appspot.com/1415802/diff/1/dev/core/src/com/google/gwt/dev/javac/UnitCacheFactory.java File dev/core/src/com/google/gwt/dev/javac/UnitCacheFactory.java (right): http://gwt-code-reviews.appspot.com/1415802/diff/1/dev/core/src/com/google/gwt/dev/javac/UnitCacheFactory.java#newcode31 dev/core/src/com/google/gwt/dev/javac/UnitCacheFactory.java:31: private static final String CONFIG_PROPERTY_VALUE = System.getProperty(gwt.persistentunitcache, camel-case http://gwt-code-reviews.appspot.com/1415802/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Users found that the error spam reduction hid too many errors. This change (issue1416801)
http://gwt-code-reviews.appspot.com/1416801/diff/1/dev/core/src/com/google/gwt/dev/javac/CompilationProblemReporter.java
File
dev/core/src/com/google/gwt/dev/javac/CompilationProblemReporter.java
(left):
http://gwt-code-reviews.appspot.com/1416801/diff/1/dev/core/src/com/google/gwt/dev/javac/CompilationProblemReporter.java#oldcode74
dev/core/src/com/google/gwt/dev/javac/CompilationProblemReporter.java:74:
if (Util.findSourceInClassPath(cl, missingType) == null) {
On 2011/04/13 19:20:43, zundel wrote:
this didn't make any sense to me - didn't the code above just check to
to see if
sourceURL was null?
Looks redundant to me.
http://gwt-code-reviews.appspot.com/1416801/diff/1/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/1416801/diff/1/dev/core/src/com/google/gwt/dev/javac/CompilationProblemReporter.java#newcode206
dev/core/src/com/google/gwt/dev/javac/CompilationProblemReporter.java:206:
String topLevelType = typeName.replaceAll(\\$.*$, );
The use of $'s for nested types is just naming convention. For
example, I can name a top-level class Foo$, without any problems.
You don't want to have to linear-scan over the unitMap to find the
matching CompilationUnit, so you should probably build an index from all
types (including nested) to CompilationUnits.
http://gwt-code-reviews.appspot.com/1416801/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Addresses ClassNotFoundException problems when the data structures serialized in (issue1412801)
http://gwt-code-reviews.appspot.com/1412801/diff/3003/dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java File dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java (right): http://gwt-code-reviews.appspot.com/1412801/diff/3003/dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java#newcode491 dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java:491: logger.log(TreeLogger.TRACE, Igorning and deleting cache log Typo Igorning http://gwt-code-reviews.appspot.com/1412801/diff/3003/dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java#newcode495 dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java:495: logger.log(TreeLogger.TRACE, Igorning and deleting cache log And here too. Would be nice if we had JDK 7's multi-catch blocks. http://gwt-code-reviews.appspot.com/1412801/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Addresses ClassNotFoundException problems when the data structures serialized in (issue1412801)
http://gwt-code-reviews.appspot.com/1412801/diff/1/dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java File dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java (right): http://gwt-code-reviews.appspot.com/1412801/diff/1/dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java#newcode493 dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java:493: cacheFile.delete(); Don't you need this same behavior for IOException? The original deserilization exception you were seeing was an InvalidClassException, which is a subclass of IOException. http://gwt-code-reviews.appspot.com/1412801/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Adds a cache (PersistenUnitCache) to store CompilationUnits (issue1375802)
http://gwt-code-reviews.appspot.com/1375802/diff/5002/dev/core/src/com/google/gwt/dev/DevMode.java
File dev/core/src/com/google/gwt/dev/DevMode.java (right):
http://gwt-code-reviews.appspot.com/1375802/diff/5002/dev/core/src/com/google/gwt/dev/DevMode.java#newcode417
dev/core/src/com/google/gwt/dev/DevMode.java:417: File
persistentCacheDir = new File(options.getWarDir(),
/WEB-INF/gwt-unitCache);
Make gwt-unitCache a constant somewhere (and replace all uses)?
http://gwt-code-reviews.appspot.com/1375802/diff/5002/dev/core/src/com/google/gwt/dev/DevModeBase.java
File dev/core/src/com/google/gwt/dev/DevModeBase.java (right):
http://gwt-code-reviews.appspot.com/1375802/diff/5002/dev/core/src/com/google/gwt/dev/DevModeBase.java#newcode889
dev/core/src/com/google/gwt/dev/DevModeBase.java:889:
PersistentUnitCache.init(getTopLogger().branch(TreeLogger.INFO,
Why not just branch the TreeLogger inside of init?
http://gwt-code-reviews.appspot.com/1375802/diff/5002/dev/core/src/com/google/gwt/dev/GWTCompiler.java
File dev/core/src/com/google/gwt/dev/GWTCompiler.java (right):
http://gwt-code-reviews.appspot.com/1375802/diff/5002/dev/core/src/com/google/gwt/dev/GWTCompiler.java#newcode180
dev/core/src/com/google/gwt/dev/GWTCompiler.java:180: } else {
This logic was a bit confusing to follow. Mind setting
persistentCacheDir to null in the if block instead of initializing it to
null? A comment might help too.
http://gwt-code-reviews.appspot.com/1375802/diff/5002/dev/core/src/com/google/gwt/dev/javac/CompilationStateBuilder.java
File dev/core/src/com/google/gwt/dev/javac/CompilationStateBuilder.java
(left):
http://gwt-code-reviews.appspot.com/1375802/diff/5002/dev/core/src/com/google/gwt/dev/javac/CompilationStateBuilder.java#oldcode422
dev/core/src/com/google/gwt/dev/javac/CompilationStateBuilder.java:422:
CompilationUnit existingUnit = unitCache.get(contentId);
Put the check of the unitCache and PUC together (for example, in a
method) so you don't duplicate the the cachedUnits.put/compileMoreLater
logic. Or... replace unitCache with PUC altogether.
http://gwt-code-reviews.appspot.com/1375802/diff/5002/dev/core/src/com/google/gwt/dev/javac/CompilationStateBuilder.java
File dev/core/src/com/google/gwt/dev/javac/CompilationStateBuilder.java
(right):
http://gwt-code-reviews.appspot.com/1375802/diff/5002/dev/core/src/com/google/gwt/dev/javac/CompilationStateBuilder.java#newcode231
dev/core/src/com/google/gwt/dev/javac/CompilationStateBuilder.java:231:
if (invalidatedUnits.size() 0) {
Have we figured out why this would ever be true on a no-op refresh?
http://gwt-code-reviews.appspot.com/1375802/diff/5002/dev/core/src/com/google/gwt/dev/javac/CompilationStateBuilder.java#newcode252
dev/core/src/com/google/gwt/dev/javac/CompilationStateBuilder.java:252:
PersistentUnitCache.cleanup(logger);
Is there some reason this can't just happen on a thread on a periodic
timer in PUC?
http://gwt-code-reviews.appspot.com/1375802/diff/5002/dev/core/src/com/google/gwt/dev/javac/CompilationStateBuilder.java#newcode393
dev/core/src/com/google/gwt/dev/javac/CompilationStateBuilder.java:393:
// run in this process.
Is it possible to just replace unitCache with PUC at this point? Under
what circumstances would we want something in the unitCache, but not in
the PersistentUnitCache?
http://gwt-code-reviews.appspot.com/1375802/diff/5002/dev/core/src/com/google/gwt/dev/javac/Dependencies.java
File dev/core/src/com/google/gwt/dev/javac/Dependencies.java (right):
http://gwt-code-reviews.appspot.com/1375802/diff/5002/dev/core/src/com/google/gwt/dev/javac/Dependencies.java#newcode142
dev/core/src/com/google/gwt/dev/javac/Dependencies.java:142: * classes.
Must be called before {@link #validate(String, Map, Map)}.
Update this link tag.
http://gwt-code-reviews.appspot.com/1375802/diff/5002/dev/core/src/com/google/gwt/dev/javac/Dependencies.java#newcode215
dev/core/src/com/google/gwt/dev/javac/Dependencies.java:215:
logger.log(TreeLogger.DEBUG, Invalid ref: + entry.getKey() + mine:
Wrap in isLoggable?
http://gwt-code-reviews.appspot.com/1375802/diff/5002/dev/core/src/com/google/gwt/dev/javac/Dependencies.java#newcode228
dev/core/src/com/google/gwt/dev/javac/Dependencies.java:228: logger
Wrap in isLoggable?
http://gwt-code-reviews.appspot.com/1375802/diff/5002/dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java
File dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java
(right):
http://gwt-code-reviews.appspot.com/1375802/diff/5002/dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java#newcode49
dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java:49: *
Each run of the compiler should call {@link cleanup()} when finished
adding
s/cleanup/#cleanup.
http://gwt-code-reviews.appspot.com/1375802/diff/5002/dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java#newcode55
dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java:55: *
single file.
This reads really strange. I think more explanation
[gwt-contrib] Re: Reformatting pass with updated gwt-format.xml Eclipse formatter before persistent unit cache patch (issue1368802)
LGTM http://gwt-code-reviews.appspot.com/1368802/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Reformatting pass with updated gwt-format.xml Eclipse formatter before persistent unit cache patch (issue1368802)
On 2011/02/28 14:33:36, zundel wrote: There should be nothing but formatting changes in these files, due to the recent update of gwt-format.xml for Eclipse's code formatter. Things look really poorly formatted. Discussing face to face. http://gwt-code-reviews.appspot.com/1368802/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Fixes a bug in serializing Dependencies.DirectRef and (issue1365801)
LGTM
http://gwt-code-reviews.appspot.com/1365801/diff/1/dev/core/src/com/google/gwt/dev/javac/Dependencies.java
File dev/core/src/com/google/gwt/dev/javac/Dependencies.java (right):
http://gwt-code-reviews.appspot.com/1365801/diff/1/dev/core/src/com/google/gwt/dev/javac/Dependencies.java#newcode63
dev/core/src/com/google/gwt/dev/javac/Dependencies.java:63: protected
Object writeReplace() {
Does this get rid of the random cache misses we were seeing?
http://gwt-code-reviews.appspot.com/1365801/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Records names of JSNI calls and callbacks in SpeedTracer events. (issue1356806)
Reviewers: zundel, jbrosenberg,
Description:
Records names of JSNI calls and callbacks in SpeedTracer events.
Please review this at http://gwt-code-reviews.appspot.com/1356806/show
Affected files:
M dev/core/src/com/google/gwt/dev/shell/ModuleSpaceOOPHM.java
M dev/core/src/com/google/gwt/dev/shell/OophmSessionHandler.java
M
dev/core/src/com/google/gwt/dev/util/log/speedtracer/SpeedTracerLogger.java
Index: dev/core/src/com/google/gwt/dev/shell/ModuleSpaceOOPHM.java
===
--- dev/core/src/com/google/gwt/dev/shell/ModuleSpaceOOPHM.java (revision
9706)
+++ dev/core/src/com/google/gwt/dev/shell/ModuleSpaceOOPHM.java (working
copy)
@@ -110,6 +110,9 @@
Invoke native method + name, null);
Event javaToJsCallEvent =
SpeedTracerLogger.start(DevModeEventType.JAVA_TO_JS_CALL);
+if (SpeedTracerLogger.jsniCallLoggingEnabled()) {
+ javaToJsCallEvent.addData(name, name);
+}
CompilingClassLoader isolatedClassLoader = getIsolatedClassLoader();
JsValueOOPHM jsthis = new JsValueOOPHM();
Index: dev/core/src/com/google/gwt/dev/shell/OophmSessionHandler.java
===
--- dev/core/src/com/google/gwt/dev/shell/OophmSessionHandler.java
(revision 9706)
+++ dev/core/src/com/google/gwt/dev/shell/OophmSessionHandler.java (working
copy)
@@ -115,6 +115,16 @@
JsValueOOPHM jsThis = new JsValueOOPHM();
channel.convertToJsValue(cl, localObjects, thisVal, jsThis);
+if (SpeedTracerLogger.jsniCallLoggingEnabled()) {
+ DispatchClassInfo clsInfo =
cl.getClassInfoByDispId(methodDispatchId);
+ if (clsInfo != null) {
+Member member = clsInfo.getMember(methodDispatchId);
+if (member != null) {
+ jsToJavaCallEvent.addData(name, member.toString());
+}
+ }
+}
+
TreeLogger branch = TreeLogger.NULL;
if (logger.isLoggable(TreeLogger.SPAM)) {
StringBuffer logMsg = new StringBuffer();
Index:
dev/core/src/com/google/gwt/dev/util/log/speedtracer/SpeedTracerLogger.java
===
---
dev/core/src/com/google/gwt/dev/util/log/speedtracer/SpeedTracerLogger.java
(revision 9706)
+++
dev/core/src/com/google/gwt/dev/util/log/speedtracer/SpeedTracerLogger.java
(working copy)
@@ -74,6 +74,11 @@
// Turn on logging estimating overhead used for speedtracer logging.
private static final boolean logOverheadTime =
getBooleanProperty(gwt.speedtracer.logOverheadTime);
+
+ // Disable logging of JSNI calls and callbacks to reduce memory usage
where
+ // the heap is already tight.
+ private static final boolean jsniCallLoggingEnabled =
+ !getBooleanProperty(gwt.speedtracer.disableJsniLogging);
/**
* Represents a node in a tree of SpeedTracer events.
@@ -430,6 +435,13 @@
}
/**
+ * Returns true if JSNI calls and callbacks are being logged.
+ */
+ public static boolean jsniCallLoggingEnabled() {
+return jsniCallLoggingEnabled;
+ }
+
+ /**
* Adds a LOG_MESSAGE SpeedTracer event to the log. This represents a
single
* point in time and has a special representation in the SpeedTracer UI.
*/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: TypeOracle to allow access to the list of annotations on an element. (issue1352805)
LGTM http://gwt-code-reviews.appspot.com/1352805/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Cache Document.get for DevMode. Kills thousands of JSNI calls. (issue1338806)
http://gwt-code-reviews.appspot.com/1338806/diff/1/2
File user/src/com/google/gwt/dom/client/Document.java (right):
http://gwt-code-reviews.appspot.com/1338806/diff/1/2#newcode51
user/src/com/google/gwt/dom/client/Document.java:51: private static
native Document nativeGet() /*-{
On 2011/02/08 21:23:14, knorton wrote:
I this method is out of sort order.
I think it's actually in order: get() is public static while nativeGet
is private static. I use a plugin to sort, but my checkstyle plugin
would complain if the ordering was bad.
http://gwt-code-reviews.appspot.com/1338806/show
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Modifies CompiledClass to be serializable (issue1329802)
http://gwt-code-reviews.appspot.com/1329802/diff/1004/5 File dev/core/src/com/google/gwt/dev/javac/CompiledClass.java (right): http://gwt-code-reviews.appspot.com/1329802/diff/1004/5#newcode47 dev/core/src/com/google/gwt/dev/javac/CompiledClass.java:47: private transient long cacheToken; I'm just going to echo our face to face discussion which is that disk caching this stuff seems strange. It's not particularly large and we know we access it several times. Throwing in intervening disk accesses can't help. Can we see if just keeping the bytes in memory improves things? http://gwt-code-reviews.appspot.com/1329802/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Make FieldSerializers use reflection directly instead of round-tripping through JSNI. Speeds up ... (issue1310807)
Strange issue with Rietveld and TypeSerializer. Maybe this second upload will be better. http://gwt-code-reviews.appspot.com/1310807/diff/1/2 File user/src/com/google/gwt/user/client/rpc/impl/ReflectionHelper.java (right): http://gwt-code-reviews.appspot.com/1310807/diff/1/2#newcode39 user/src/com/google/gwt/user/client/rpc/impl/ReflectionHelper.java:39: private static final int INITIAL_CAPACITY = 16; On 2011/02/04 22:28:41, jbrosenberg wrote: Should be 'LOAD_FACTOR' Weird. I remember fixing this exact problem. It may have been the victim of an untimely IDE crash. http://gwt-code-reviews.appspot.com/1310807/diff/1/2#newcode58 user/src/com/google/gwt/user/client/rpc/impl/ReflectionHelper.java:58: On 2011/02/04 22:28:41, jbrosenberg wrote: There are 2 getField() methods, one returns the Field itself, the other returns the Object value of the field. Can we differentiate the names of these 2? Perhaps have: getField getFieldValue (and also change setField to setFieldValue). Oh wow. Ditto here. I had changed this to findField, and so I do again. http://gwt-code-reviews.appspot.com/1310807/diff/1/3 File user/src/com/google/gwt/user/rebind/rpc/FieldSerializerCreator.java (right): http://gwt-code-reviews.appspot.com/1310807/diff/1/3#newcode44 user/src/com/google/gwt/user/rebind/rpc/FieldSerializerCreator.java:44: /** On 2011/02/04 22:28:41, jbrosenberg wrote: Can you add some comments explaining the reason for, and the differences between, the prod and dev mode paths? Done. http://gwt-code-reviews.appspot.com/1310807/diff/1/5 File user/super/com/google/gwt/user/translatable/com/google/gwt/user/client/rpc/impl/ReflectionHelper.java (right): http://gwt-code-reviews.appspot.com/1310807/diff/1/5#newcode26 user/super/com/google/gwt/user/translatable/com/google/gwt/user/client/rpc/impl/ReflectionHelper.java:26: On 2011/02/04 22:28:41, jbrosenberg wrote: Rename this (if it's also to be renamed in the non super source version (e.g. getFieldValue()) Not renamed. http://gwt-code-reviews.appspot.com/1310807/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Make FieldSerializers use reflection directly instead of round-tripping through JSNI. Speeds up ... (issue1310807)
http://gwt-code-reviews.appspot.com/1310807/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Make FieldSerializers use reflection directly instead of round-tripping through JSNI. Speeds up ... (issue1310807)
http://en.wikipedia.org/wiki/Nota_bene On 2011/02/04 23:06:26, jbrosenberg wrote: LGTM (but what's NB:?) http://gwt-code-reviews.appspot.com/1310807/diff/2003/4002 File user/src/com/google/gwt/user/rebind/rpc/FieldSerializerCreator.java (right): http://gwt-code-reviews.appspot.com/1310807/diff/2003/4002#newcode55 user/src/com/google/gwt/user/rebind/rpc/FieldSerializerCreator.java:55: NB? http://gwt-code-reviews.appspot.com/1310807/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Reduce logging work in STOB. Wrap logging calls with isLoggable and only create .rpc.log during ... (issue1310806)
http://gwt-code-reviews.appspot.com/1310806/diff/1/3
File
user/src/com/google/gwt/user/rebind/rpc/SerializableTypeOracleBuilder.java
(right):
http://gwt-code-reviews.appspot.com/1310806/diff/1/3#newcode841
user/src/com/google/gwt/user/rebind/rpc/SerializableTypeOracleBuilder.java:841:
// instantiation during code generation would flag them for us.
On 2011/02/04 23:01:37, jbrosenberg wrote:
This seems like it could result in looping through a long list of
problems,
and it's only done in DEBUG mode, perhaps we don't want to descend
into
problems.report in less isLoggable(DEBUG)
I think most of the work is probably done in constructing the problems
which unfortunately isn't easy to change based on the logging level. I
went ahead and added an isLoggable test to ProblemReport.doReport anyway
since it couldn't hurt.
http://gwt-code-reviews.appspot.com/1310806/diff/1/3#newcode1134
user/src/com/google/gwt/user/rebind/rpc/SerializableTypeOracleBuilder.java:1134:
On 2011/02/04 23:01:37, jbrosenberg wrote:
This too seems like it could do unnecessary work in calling
getArrayType (which
is a looping/recursive animal).
Good idea.
http://gwt-code-reviews.appspot.com/1310806/diff/1/3#newcode1537
user/src/com/google/gwt/user/rebind/rpc/SerializableTypeOracleBuilder.java:1537:
private void logReachableTypes(TreeLogger logger) {
On 2011/02/04 23:01:37, jbrosenberg wrote:
So, in dev mode, we don't ever want to do this, even if log level is
DEBUG?
I think you read this wrong. It's skip if dev_mode and can't log debug
http://gwt-code-reviews.appspot.com/1310806/show
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Reduce logging work in STOB. Wrap logging calls with isLoggable and only create .rpc.log during ... (issue1310806)
http://gwt-code-reviews.appspot.com/1310806/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Reduce logging work in STOB. Wrap logging calls with isLoggable and only create .rpc.log during ... (issue1310806)
http://gwt-code-reviews.appspot.com/1310806/diff/1/3
File
user/src/com/google/gwt/user/rebind/rpc/SerializableTypeOracleBuilder.java
(right):
http://gwt-code-reviews.appspot.com/1310806/diff/1/3#newcode1537
user/src/com/google/gwt/user/rebind/rpc/SerializableTypeOracleBuilder.java:1537:
private void logReachableTypes(TreeLogger logger) {
On 2011/02/05 00:01:58, jbrosenberg wrote:
Oops...Yeah, so...inProdMode, we always don't skip, even if logLevel
is not
DEBUG? I guess I'm not sure why we don't just have the check be only
for DEBUG,
and not care about isProdMode?
I think it's nice to be able to get the .rpc.log without having to
perform a webmode compile. Basing it on logging at DEBUG level seemed
reasonable.
http://gwt-code-reviews.appspot.com/1310806/show
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Refactors DefaultFilters and DefaultFiltersTest to accept a filter that will return .class files. (issue1336801)
The diffs are really large, but I think it's mostly due to the changes in ordering. Unfortunately, I think this is an inherent drawback to the forced-ordering in our coding style, given the current state-of-the-art of diff tools. (I.E. simple changes in names or modifiers produce huge diffs). LGTM where I could pick out the diffs. http://gwt-code-reviews.appspot.com/1336801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Speed up DevMode by getting rid of excessive JS to Java calls. (issue1337801)
Reviewers: jat,
Description:
Speed up DevMode by getting rid of excessive JS to Java calls.
Please review this at http://gwt-code-reviews.appspot.com/1337801/show
Affected files:
M user/src/com/google/gwt/i18n/client/Dictionary.java
Index: user/src/com/google/gwt/i18n/client/Dictionary.java
===
--- user/src/com/google/gwt/i18n/client/Dictionary.java (revision 9642)
+++ user/src/com/google/gwt/i18n/client/Dictionary.java (working copy)
@@ -175,11 +175,7 @@
}
void resourceError(String key) {
-CollectionString s = this.keySet();
String error = Cannot find ' + key + ' in + this;
-if (s.size() MAX_KEYS_TO_SHOW) {
- error += \n keys found: + s;
-}
throw new MissingResourceException(error, this.toString(), key);
}
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Add GeneratorContextExt.isProdMode. Allows generators to specialize output for dev or prod mode.... (issue1338801)
On 2011/01/31 23:11:33, jbrosenberg wrote: http://gwt-code-reviews.appspot.com/1338801/diff/1/3 File dev/core/src/com/google/gwt/core/ext/GeneratorContextExtWrapper.java (right): http://gwt-code-reviews.appspot.com/1338801/diff/1/3#newcode89 dev/core/src/com/google/gwt/core/ext/GeneratorContextExtWrapper.java:89: } HmmmI'm not sure it's always safe to assume that we'll be in prodMode here. One needed fix for most cases is to make GeneratorExt.generate() not call GeneratorContextExtWrapper.newInstance() if it knows it has an instance of GeneratorContextExt. That generate method (which results in an instance of this class being created) is only called now directly by other generators calling each other. And then it maybe should be an error to try calling isProdMode() here (throw an Exception). Or if defaulting to isProdMode() = true even when not really in prod mode is ok, then perhaps this is an ok default for now. I updated GeneratorExt such that it only wraps GeneratorContexts and not GeneratorContextExts. In this way, if users are just passing on our implementation, things are fine. The expectation is that users aren't manufacturing their own GeneratorContexts out of thin-air and handing them on to our Generators. http://gwt-code-reviews.appspot.com/1338801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Refactors DefaultFilters and DefaultFiltersTest to accept a filter that will return .class files. (issue1336801)
Overall LGTM.
http://gwt-code-reviews.appspot.com/1336801/diff/1/2
File dev/core/src/com/google/gwt/dev/resource/impl/DefaultFilters.java
(right):
http://gwt-code-reviews.appspot.com/1336801/diff/1/2#newcode84
dev/core/src/com/google/gwt/dev/resource/impl/DefaultFilters.java:84:
private static boolean fileTypeMatch(FilterFileType filterFileType,
I like the move to enums. Can you move all of the methods which are
essentially switch statements on the enums into instance methods on the
enums? For example, for this method:
enum FilterFileType {
RESOURCE_FILES,
JAVA_FILES {
public booleam matches(String path) {
return path.endsWith(.java);
}
},
...
public boolean matches(String path) {
return true;
}
}
http://gwt-code-reviews.appspot.com/1336801/diff/1/2#newcode278
dev/core/src/com/google/gwt/dev/resource/impl/DefaultFilters.java:278:
switch (filterFileType) {
Ditto above.
http://gwt-code-reviews.appspot.com/1336801/diff/1/2#newcode464
dev/core/src/com/google/gwt/dev/resource/impl/DefaultFilters.java:464:
switch (filterFileType) {
Ditto above.
http://gwt-code-reviews.appspot.com/1336801/diff/1/2#newcode476
dev/core/src/com/google/gwt/dev/resource/impl/DefaultFilters.java:476:
switch (filterFileType) {
Ditto above.
http://gwt-code-reviews.appspot.com/1336801/diff/1/3
File
dev/core/test/com/google/gwt/dev/resource/impl/DefaultFiltersTest.java
(right):
http://gwt-code-reviews.appspot.com/1336801/diff/1/3#newcode613
dev/core/test/com/google/gwt/dev/resource/impl/DefaultFiltersTest.java:613:
private static boolean fileTypeMatch(FilterFileType filterFileType,
Can't use the non-test versions of these?
http://gwt-code-reviews.appspot.com/1336801/show
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Add JREIndex optimization to JdtCompiler (issue1318802)
SG, but why not extract it to a shared class instead of copying? The index is immutable, and you'll end up building it twice (non-trivial cost) with this change as it's currently written - once for AbstractCompiler and once for JdtCompiler (right?). http://gwt-code-reviews.appspot.com/1318802/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Generator Result Caching implementation for ClientBundle (issue1236801)
LGTM now. I'll let you and Bob sort out the rest. http://gwt-code-reviews.appspot.com/1236801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Add JREIndex optimization to JdtCompiler (issue1318802)
On 2011/01/26 23:15:56, scottb wrote: I plan to kill AbstractCompiler soon :D Ok, it's up to you then. I feel squirrely about committing something in that state unless the next change is literally just hours out. http://gwt-code-reviews.appspot.com/1318802/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Add SSL support to DevMode. (issue1324801)
LGTM given changes to ServletContainerLaunch and removal of SecureServletContainerLauncher. http://gwt-code-reviews.appspot.com/1324801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Generator Result Caching implementation for ClientBundle (issue1236801)
Almost there
http://gwt-code-reviews.appspot.com/1236801/diff/39001/40001
File dev/core/src/com/google/gwt/core/ext/typeinfo/JRealClassType.java
(right):
http://gwt-code-reviews.appspot.com/1236801/diff/39001/40001#newcode28
dev/core/src/com/google/gwt/core/ext/typeinfo/JRealClassType.java:28:
void addByteCode(byte[] byteCode);
Why is this method in the public interface? Can we just remove it and
leave the implementation in typemodel.JRealClassType?
http://gwt-code-reviews.appspot.com/1236801/diff/39001/40003
File
dev/core/src/com/google/gwt/dev/javac/rebind/CachedClientDataMap.java
(right):
http://gwt-code-reviews.appspot.com/1236801/diff/39001/40003#newcode39
dev/core/src/com/google/gwt/dev/javac/rebind/CachedClientDataMap.java:39:
// this will throw a ClassCastException if value is not Serializable
Comment is pointless as is. Document with javadoc instead?
http://gwt-code-reviews.appspot.com/1236801/diff/39001/40008
File dev/core/src/com/google/gwt/dev/javac/typemodel/JRealClassType.java
(right):
http://gwt-code-reviews.appspot.com/1236801/diff/39001/40008#newcode290
dev/core/src/com/google/gwt/dev/javac/typemodel/JRealClassType.java:290:
lazyTypeStrongHash = Util.computeStrongName(byteCode);
This isn't exactly what we want either, but it's better than assuming
always stale or taking a long time to compute a hash. Can you add a
comment here that explains that ideally we want to identify staleness
based on a type's structure, not including its code, but we don't have a
quick way to compute a hash for type structure yet?
http://gwt-code-reviews.appspot.com/1236801/diff/39001/40013
File
user/src/com/google/gwt/resources/ext/SupportsGeneratorResultCaching.java
(right):
http://gwt-code-reviews.appspot.com/1236801/diff/39001/40013#newcode41
user/src/com/google/gwt/resources/ext/SupportsGeneratorResultCaching.java:41:
public interface HasFindableResourceDependencies extends
It's cute, but is there actually a benefit to nesting this?
http://gwt-code-reviews.appspot.com/1236801/diff/39001/40014
File
user/src/com/google/gwt/resources/rebind/context/AbstractClientBundleGenerator.java
(right):
http://gwt-code-reviews.appspot.com/1236801/diff/39001/40014#newcode232
user/src/com/google/gwt/resources/rebind/context/AbstractClientBundleGenerator.java:232:
for (JClassType superType : superTypes) {
Why is this for loop necessary? Doesn't getFlattenedSupertypeHierarchy
already do this?
http://gwt-code-reviews.appspot.com/1236801/diff/39001/40014#newcode249
user/src/com/google/gwt/resources/rebind/context/AbstractClientBundleGenerator.java:249:
if (!(type instanceof JRealClassType)) {
Under what situations does this occur?
http://gwt-code-reviews.appspot.com/1236801/diff/39001/40014#newcode510
user/src/com/google/gwt/resources/rebind/context/AbstractClientBundleGenerator.java:510:
* Loop through each of our ResouceGenerator classes, and check those
that
s/that that/that
http://gwt-code-reviews.appspot.com/1236801/diff/39001/40014#newcode532
user/src/com/google/gwt/resources/rebind/context/AbstractClientBundleGenerator.java:532:
// use the jar file itself for the modified date test
Document why
http://gwt-code-reviews.appspot.com/1236801/show
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Generator Result Caching implementation for ClientBundle (issue1236801)
http://gwt-code-reviews.appspot.com/1236801/diff/39001/40001 File dev/core/src/com/google/gwt/core/ext/typeinfo/JRealClassType.java (right): http://gwt-code-reviews.appspot.com/1236801/diff/39001/40001#newcode28 dev/core/src/com/google/gwt/core/ext/typeinfo/JRealClassType.java:28: void addByteCode(byte[] byteCode); Sorted confusion out offline. In a nutshell, we should remove addByteCode (not getTypeStrongHash). On 2011/01/25 00:48:19, jbrosenberg wrote: On 2011/01/24 19:14:07, tobyr wrote: Why is this method in the public interface? Can we just remove it and leave the implementation in typemodel.JRealClassType? Well, I thought about doing that, but then we'd lose the benefits of the interface layer. The generator calling getTypeStrongHash shouldn't need to know the specific implementation for JRealClassType (and of type oracle). The generators refer to the interface versions of all the type oracle and class type apis, and so this was in keeping with that. Once we get the changes into TypeOracleMediator to be able to more specifically check type structure/version, we can add a capability interface (as Bob suggests), e.g. HasTypeStructureSignature. http://gwt-code-reviews.appspot.com/1236801/diff/39001/40014 File user/src/com/google/gwt/resources/rebind/context/AbstractClientBundleGenerator.java (right): http://gwt-code-reviews.appspot.com/1236801/diff/39001/40014#newcode532 user/src/com/google/gwt/resources/rebind/context/AbstractClientBundleGenerator.java:532: // use the jar file itself for the modified date test On 2011/01/25 00:48:19, jbrosenberg wrote: On 2011/01/24 19:14:07, tobyr wrote: Document why Done. Sorry, I meant document why we'd want to use the jar file timestamp instead of the entry timestamp. http://gwt-code-reviews.appspot.com/1236801/diff/39001/40014#newcode538 user/src/com/google/gwt/resources/rebind/context/AbstractClientBundleGenerator.java:538: File file = new File(url.getPath()); Can we get rid of this special-casing and just use url.getLastModified? http://gwt-code-reviews.appspot.com/1236801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: DevMode generates a default web.xml when servlet tags are present and no web.xml exists. (issue1266801)
SGTM and code LGTM On 2011/01/07 19:04:37, scottb wrote: That seems good to me as a mid-term goal. Wouldn't this still be a useful stop-gap moving in that direction but with the current servlet spec? On Fri, Jan 7, 2011 at 1:41 PM, Toby Reyelts mailto:to...@google.com wrote: It sounds to me as if we should be integrating with the facilities in Servlet 3.0, since it has been final now for over a year. Specifically see: Section 4.4: Configuration methods - An API to add and configure servlets, filters, and listeners at runtime. Chapter 8: Annotations and Pluggability - Capabilities for configuring web.xml behavior piecemeal through annotations and web.xml fragments separate from web.xml On Fri, Jan 7, 2011 at 1:21 PM, John Tamplin mailto:j...@google.com wrote: On Fri, Jan 7, 2011 at 1:11 PM, mailto:zun...@google.com wrote: I don't see any problem with the code. Is this really something we want to happen by default? I'm concerned it will be baffling to a user why a web.xml gets created only some of the time. It seems to me that an external tool that could parse xml could create such a file if it was needed without too much work. There's nothing in here that couldn't be done in a python script with access to the module files. I think we lost useful functionality when we lost the ability to put servlet tags in a module (think about a self-contained module that contains client code and the servlet needed for the server-side piece - it would be nice to just inherit it and it works, such as for server-side script selection). Should we reconsider the decision to have web.xml authoritive and instead have a template which is used to generate the web.xml file? If there is already a web.xml file, we keep the behavior of today, and if not then we are always generating a web.xml file, whether from web-template.xml (or whatever) or creating a default one if the user didn't supply one. -- John A. Tamplin Software Engineer (GWT), Google http://gwt-code-reviews.appspot.com/1266801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: ModuleDef's resource oracles really should be lazy (issue1264801)
http://gwt-code-reviews.appspot.com/1264801/diff/1/2
File dev/core/src/com/google/gwt/dev/cfg/ModuleDef.java (left):
http://gwt-code-reviews.appspot.com/1264801/diff/1/2#oldcode418
dev/core/src/com/google/gwt/dev/cfg/ModuleDef.java:418: public
synchronized void refresh(TreeLogger logger) {
Remove TreeLogger as an arg? It causes a static analysis warning as is.
http://gwt-code-reviews.appspot.com/1264801/diff/1/2
File dev/core/src/com/google/gwt/dev/cfg/ModuleDef.java (right):
http://gwt-code-reviews.appspot.com/1264801/diff/1/2#newcode324
dev/core/src/com/google/gwt/dev/cfg/ModuleDef.java:324:
doRefresh(TreeLogger.NULL);
Looks like we are always passing TreeLogger.NULL. If so, why not just
remove the logging that no longer takes place?
http://gwt-code-reviews.appspot.com/1264801/diff/1/2#newcode492
dev/core/src/com/google/gwt/dev/cfg/ModuleDef.java:492: needsRefresh =
true;
Consider calling refresh() instead.
http://gwt-code-reviews.appspot.com/1264801/show
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: ModuleDef's resource oracles really should be lazy (issue1264801)
http://gwt-code-reviews.appspot.com/1264801/diff/1/2
File dev/core/src/com/google/gwt/dev/cfg/ModuleDef.java (left):
http://gwt-code-reviews.appspot.com/1264801/diff/1/2#oldcode418
dev/core/src/com/google/gwt/dev/cfg/ModuleDef.java:418: public
synchronized void refresh(TreeLogger logger) {
On 2011/01/07 20:38:28, scottb wrote:
I wasn't sure if there were non-GWT callers on this public method.
Worth
fixing?
My understanding is that only com.google.gwt.core.ext.* is public API
and anybody using anything else is depending upon implementation
details. Unless we know it would cause egregious breakage, I think we
should keep the codebase clean.
http://gwt-code-reviews.appspot.com/1264801/diff/1/2
File dev/core/src/com/google/gwt/dev/cfg/ModuleDef.java (right):
http://gwt-code-reviews.appspot.com/1264801/diff/1/2#newcode324
dev/core/src/com/google/gwt/dev/cfg/ModuleDef.java:324:
doRefresh(TreeLogger.NULL);
On 2011/01/07 20:38:28, scottb wrote:
Oops, already committed. Think it's worth another CL to clean this up?
Please.
http://gwt-code-reviews.appspot.com/1264801/show
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Adds some documentation on TypeOracle and JType (issue1268801)
LGTM
http://gwt-code-reviews.appspot.com/1268801/diff/1/2
File dev/core/src/com/google/gwt/core/ext/typeinfo/JType.java (right):
http://gwt-code-reviews.appspot.com/1268801/diff/1/2#newcode29
dev/core/src/com/google/gwt/core/ext/typeinfo/JType.java:29: * Returns
the equivalent of {...@link Class#getName()} if the type were loaded.
This isn't true as far as I know. I believe this always returns the
field descriptor for a type. See JVMS 4.3. I think we should javadoc
it as such. Adding a couple of examples may not hurt: String.class =
Ljava/lang/String;. boolean.class = Z
http://gwt-code-reviews.appspot.com/1268801/show
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Adds a unit test for the TypeOracleMediator that feeds byte code instead of source. (issue1254801)
Overall LGTM with a few changes.
http://gwt-code-reviews.appspot.com/1254801/diff/14001/15001
File dev/core/src/com/google/gwt/dev/javac/TypeOracleMediator.java
(right):
http://gwt-code-reviews.appspot.com/1254801/diff/14001/15001#newcode792
dev/core/src/com/google/gwt/dev/javac/TypeOracleMediator.java:792: *
Note: OpenJDK byte code doesn't create a type signature for non-static
inner
s/OpenJDK byte code/The OpenJDK compiler
http://gwt-code-reviews.appspot.com/1254801/diff/14001/15001#newcode795
dev/core/src/com/google/gwt/dev/javac/TypeOracleMediator.java:795: *
represented in the type oracle.
Mention that these differences also show up in java.lang.reflect.
http://gwt-code-reviews.appspot.com/1254801/diff/14001/15002
File
dev/core/test/com/google/gwt/dev/javac/TypeOracleMediatorFromByteCodeTest.java
(right):
http://gwt-code-reviews.appspot.com/1254801/diff/14001/15002#newcode27
dev/core/test/com/google/gwt/dev/javac/TypeOracleMediatorFromByteCodeTest.java:27:
* This test uses the byte code on the class path created when compiling
this
Add a one-line summary of the class above this note.
http://gwt-code-reviews.appspot.com/1254801/diff/14001/15004
File
dev/core/test/com/google/gwt/dev/javac/TypeOracleMediatorTestBase.java
(right):
http://gwt-code-reviews.appspot.com/1254801/diff/14001/15004#newcode127
dev/core/test/com/google/gwt/dev/javac/TypeOracleMediatorTestBase.java:127:
DataInputStream instream = new DataInputStream(istream);
Why use DataInputStream?
http://gwt-code-reviews.appspot.com/1254801/diff/14001/15004#newcode130
dev/core/test/com/google/gwt/dev/javac/TypeOracleMediatorTestBase.java:130:
while (true) {
Use com.google.gwt.util.Util here instead?
http://gwt-code-reviews.appspot.com/1254801/diff/14001/15004#newcode143
dev/core/test/com/google/gwt/dev/javac/TypeOracleMediatorTestBase.java:143:
/* For building the type oracle from bytecode */
Single-line comment style
http://gwt-code-reviews.appspot.com/1254801/diff/14001/15004#newcode182
dev/core/test/com/google/gwt/dev/javac/TypeOracleMediatorTestBase.java:182:
DataInputStream instream = new DataInputStream(istream);
Same above about DataInputStream and Util.
http://gwt-code-reviews.appspot.com/1254801/diff/14001/15004#newcode196
dev/core/test/com/google/gwt/dev/javac/TypeOracleMediatorTestBase.java:196:
return out.toString();
Note that using out.toString uses the default platform charset. (Bad).
You can use out.toString(String encoding) instead, but if you just use
Util.readStreamAsString, it uses UTF8. (Good).
Typically when trying to read chars from a stream (instead of bytes),
you'd use some kind of Reader to do the character conversion, like a
BufferedReader.
http://gwt-code-reviews.appspot.com/1254801/diff/14001/15004#newcode196
dev/core/test/com/google/gwt/dev/javac/TypeOracleMediatorTestBase.java:196:
return out.toString();
Most of this code is identical to getByteCode. Refactor?
http://gwt-code-reviews.appspot.com/1254801/diff/14001/15004#newcode214
dev/core/test/com/google/gwt/dev/javac/TypeOracleMediatorTestBase.java:214:
String source = ;
Why are these two separate lines?
http://gwt-code-reviews.appspot.com/1254801/diff/14001/15004#newcode225
dev/core/test/com/google/gwt/dev/javac/TypeOracleMediatorTestBase.java:225:
assertNotNull(source);
Move the comment text into a message you provide in the assert?
http://gwt-code-reviews.appspot.com/1254801/diff/14001/15004#newcode243
dev/core/test/com/google/gwt/dev/javac/TypeOracleMediatorTestBase.java:243:
Package p = aClass.getPackage();
FYI, getPackage can return null. A package only exists for a class if
the ClassLoader that loaded it assigned one in the process. This is true
for URLClassLoader but not others. Because you can always reliably
determine a package name from a class name, it's better to use that. See
com.google.gwt.dev.javac.Shared.getPackageNameFromBinary.
http://gwt-code-reviews.appspot.com/1254801/diff/14001/15004#newcode649
dev/core/test/com/google/gwt/dev/javac/TypeOracleMediatorTestBase.java:649:
// assertNull(type.isDefaultInstantiable());
Did you fix this?
http://gwt-code-reviews.appspot.com/1254801/diff/14001/15004#newcode711
dev/core/test/com/google/gwt/dev/javac/TypeOracleMediatorTestBase.java:711:
protected static final CheckedJavaResource
CU_ReferencesGenericListConstant = new CheckedJavaResource(
Wrapping? Also for following declarations.
http://gwt-code-reviews.appspot.com/1254801/diff/14001/15004#newcode804
dev/core/test/com/google/gwt/dev/javac/TypeOracleMediatorTestBase.java:804:
//
assertFalse(publicTypeNameToTestCupMap.containsKey(qualifiedTypeName));
Why's this commented out?
http://gwt-code-reviews.appspot.com/1254801/diff/14001/15008
File
dev/core/test/com/google/gwt/dev/javac/mediatortest/BindToTypeScope.java
(right):
http://gwt-code-reviews.appspot.com/1254801/diff/14001/15008#newcode2
dev/core/test/com/google/gwt/dev/javac/mediatortest/BindToTypeScope.java:2:
* This code must be kept in sync with {...@link
[gwt-contrib] Re: Generator Result Caching implementation for ClientBundle (issue1236801)
http://gwt-code-reviews.appspot.com/1236801/diff/22001/6004
File
dev/core/src/com/google/gwt/dev/javac/rebind/CachedClientDataMap.java
(right):
http://gwt-code-reviews.appspot.com/1236801/diff/22001/6004#newcode30
dev/core/src/com/google/gwt/dev/javac/rebind/CachedClientDataMap.java:30:
public CachedClientDataMap() {
Is this supposed to be for persistent caching across runs? Is there code
that serializes/deserializes it somewhere? If it fails to deserialize,
then we probably need to handle it by running the generator again.
http://gwt-code-reviews.appspot.com/1236801/diff/22001/6009
File dev/core/src/com/google/gwt/dev/javac/typemodel/JClassType.java
(right):
http://gwt-code-reviews.appspot.com/1236801/diff/22001/6009#newcode574
dev/core/src/com/google/gwt/dev/javac/typemodel/JClassType.java:574: *
structure of a type. Use a type's publicly accessible api to build a
I think you need to reword this, since you're building the signature
based on the type's structure, not the public API.
http://gwt-code-reviews.appspot.com/1236801/diff/22001/6009#newcode683
dev/core/src/com/google/gwt/dev/javac/typemodel/JClassType.java:683:
lazyTypeStructureSignature =
Util.computeStrongName(sb.toString().getBytes());
This function looks expensive:
1) We're iterating over a lot of stuff. Probably can't be helped.
2) We're using lots of functions that create lists of strings from bits.
Seems like we could just use the bits directly.
3) The hash function seems potentially expensive.
Can we measure it? How many times will it be called? (For example, 100us
* 1000 = 100ms, which is 2% of our total budget). We don't want to add
new work into the cost of refreshes, so we need to have a plan for
dealing with this if it turns out to be non-trivial.
http://gwt-code-reviews.appspot.com/1236801/diff/22001/6011
File user/src/com/google/gwt/resources/ext/ClientBundleRequirements.java
(right):
http://gwt-code-reviews.appspot.com/1236801/diff/22001/6011#newcode66
user/src/com/google/gwt/resources/ext/ClientBundleRequirements.java:66:
void addTypeAndSuperTypeHierarchy(JClassType type);
How about just addTypeHierarchy?
http://gwt-code-reviews.appspot.com/1236801/show
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Generator Result Caching implementation for ClientBundle (issue1236801)
http://gwt-code-reviews.appspot.com/1236801/diff/22001/6009 File dev/core/src/com/google/gwt/dev/javac/typemodel/JClassType.java (right): http://gwt-code-reviews.appspot.com/1236801/diff/22001/6009#newcode574 dev/core/src/com/google/gwt/dev/javac/typemodel/JClassType.java:574: * structure of a type. Use a type's publicly accessible api to build a On 2011/01/04 22:25:03, jbrosenberg wrote: HmmmI guess I thought the 2 were interchangeable here, since from the standpoint of a generator, what matters is what might have changed based on what can be returned by a type's public api.I think I'm only using public api here to build the structure (except maybe some of the annotation getters, which by rights should be public api) I must be missing something. I thought that the TypeOracle provided private members. A quick poke in the source code and talk with jat seems to confirm that. For example, a generator could use an annotation on a private field to control behavior, right? Where do you think you're excluding non-public members? On 2011/01/04 21:39:06, tobyr wrote: I think you need to reword this, since you're building the signature based on the type's structure, not the public API. http://gwt-code-reviews.appspot.com/1236801/diff/22001/6009#newcode683 dev/core/src/com/google/gwt/dev/javac/typemodel/JClassType.java:683: lazyTypeStructureSignature = Util.computeStrongName(sb.toString().getBytes()); On 2011/01/04 22:25:03, jbrosenberg wrote: I thought about using references to each object (instead of Strings), but I think that won't work in the case of persistent caching. To individually go through each structural component (with it's own sub-structure implementation) felt problematic, since the implementations can change for Methods, Fields, etc.But yeah, I think we can optimize it to generate a byte[] or byte[][] array, without the intervening Strings I don't understand how implementation is related to this. Specific examples are things like access specifiers and other modifiers. No need to generate the string private static transient volatile when you can append a single 16-bit word that's already been read from the bytecode. JVMS signatures also tend to be more compact and are also directly available from the bytecode (no computation required). I think the MD5 algorithm used by Util.computeStrongHash is pretty efficient, but I'm sure there are simpler, faster things we can use (or maybe don't hash it at all, just keep the full raw signaturetrade-off being more memory to store and slower comparison times between signatures)... The number of type signatures that get created will increase if this is utilized also for RPC, of course, so agreed, need to characterize the impact. The alternative being what I had before, which was a simple last modified date stamp on the source file (which is problematic for freshly generated source files, or those living in jars, etc.). Do you feel like the api here is perhaps good enough for now (we can continue to optimize later with a subsequent patch)? I would feel best if you can get a measurement, prove it's a net win, then file an issue to make sure we take care of it. On 2011/01/04 21:39:06, tobyr wrote: This function looks expensive: 1) We're iterating over a lot of stuff. Probably can't be helped. 2) We're using lots of functions that create lists of strings from bits. Seems like we could just use the bits directly. 3) The hash function seems potentially expensive. Can we measure it? How many times will it be called? (For example, 100us * 1000 = 100ms, which is 2% of our total budget). We don't want to add new work into the cost of refreshes, so we need to have a plan for dealing with this if it turns out to be non-trivial. http://gwt-code-reviews.appspot.com/1236801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Reduces class and JSNI loading for RPC in devmode. (issue1215801)
http://gwt-code-reviews.appspot.com/1215801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Reduces class and JSNI loading for RPC in devmode. (issue1215801)
http://gwt-code-reviews.appspot.com/1215801/diff/1/4
File dev/core/src/com/google/gwt/dev/javac/StandardGeneratorContext.java
(right):
http://gwt-code-reviews.appspot.com/1215801/diff/1/4#newcode231
dev/core/src/com/google/gwt/dev/javac/StandardGeneratorContext.java:231:
eventsByGeneratorType.put(com.google.gwt.resources.rebind.context.InlineClientBundleGenerator,
CompilerEventType.GENERATOR_CLIENT_BUNDLE);
On 2010/12/14 14:33:58, zundel wrote:
line length 100
Done.
http://gwt-code-reviews.appspot.com/1215801/diff/1/4#newcode231
dev/core/src/com/google/gwt/dev/javac/StandardGeneratorContext.java:231:
eventsByGeneratorType.put(com.google.gwt.resources.rebind.context.InlineClientBundleGenerator,
CompilerEventType.GENERATOR_CLIENT_BUNDLE);
On 2010/12/14 14:33:58, zundel wrote:
Wouldn't it be more robust to refer to these types by class constants?
I know it
would introduce dependencies through import statements, but in effect,
the code
does depend on these classes by using their package names like this,
and will
silently break if there is some refactoring.
It would be a reverse dependency (bad), and also wouldn't even compile.
If something changes causing it to fail, the result would be that the
time would get lumped in as GENERATOR_OTHER and the data would include
the generator class. I think that's fine.
Or maybe it would be better to include an extendable method for
Generator to
return a SpeedTracer event type.
I don't think we should head in that direction. I'd rather see us
improve on tools to slice and dice events by data, so that I could
decide after the fact that I want to see a breakdown of GENERATOR events
by typename.
http://gwt-code-reviews.appspot.com/1215801/diff/1/6
File dev/core/src/com/google/gwt/dev/shell/ModuleSpaceOOPHM.java
(right):
http://gwt-code-reviews.appspot.com/1215801/diff/1/6#newcode49
dev/core/src/com/google/gwt/dev/shell/ModuleSpaceOOPHM.java:49: if
(jsniMethod.isScriptOnly()) {
On 2010/12/14 14:33:58, zundel wrote:
I would appreciate a comment here that this is for a performance
optimization.
I added a reference from JsniMethod.isScriptOnly to GwtScriptOnly which
documents this as a performance optimization.
http://gwt-code-reviews.appspot.com/1215801/diff/1/13
File user/src/com/google/gwt/user/client/rpc/impl/ReflectionHelper.java
(right):
http://gwt-code-reviews.appspot.com/1215801/diff/1/13#newcode27
user/src/com/google/gwt/user/client/rpc/impl/ReflectionHelper.java:27:
// as the syntax is correct, e.g. [[Ljava.lang.String;
On 2010/12/14 14:33:58, zundel wrote:
I'm not sure its so surprising the names match, since the
SerializationUtils.getRpcTypeName() is explicitly documented as such.
Maybe
reference that method?
There's intentionally no direct relationship to
SerializationUtils.getRpcTypeName and ReflectionHelper. I expect that
we'll eventually move this class out of rpc.
I added method javadoc and dropped the comment since it's clear from
Class.forName javadoc that it accepts Class.getName syntax.
http://gwt-code-reviews.appspot.com/1215801/show
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Reduces class and JSNI loading for RPC in devmode. (issue1215801)
http://gwt-code-reviews.appspot.com/1215801/diff/1/4
File dev/core/src/com/google/gwt/dev/javac/StandardGeneratorContext.java
(right):
http://gwt-code-reviews.appspot.com/1215801/diff/1/4#newcode231
dev/core/src/com/google/gwt/dev/javac/StandardGeneratorContext.java:231:
eventsByGeneratorType.put(com.google.gwt.resources.rebind.context.InlineClientBundleGenerator,
CompilerEventType.GENERATOR_CLIENT_BUNDLE);
On 2010/12/14 22:11:56, scottb wrote:
I thought it was possible to create new event types on the fly. Maybe
we should
just do that for every generator? I get that this is very practical,
feels
slightly hacky tho. :)
I don't think that's practical. For example, I'm doing things like
manually categorizing groups of generators together. (For example, deRPC
+ RPC, and all i18n generators). You'd also have to synthesize
reasonable colors for these things.
Again, I think we can improve our tooling instead and make this go away.
http://gwt-code-reviews.appspot.com/1215801/diff/7001/8004
File dev/core/src/com/google/gwt/dev/shell/CompilingClassLoader.java
(right):
http://gwt-code-reviews.appspot.com/1215801/diff/7001/8004#newcode1255
dev/core/src/com/google/gwt/dev/shell/CompilingClassLoader.java:1255:
event.end();
On 2010/12/14 22:11:56, scottb wrote:
Try/finally? I think the call can fail, if say the comm channel
drops.
Done.
http://gwt-code-reviews.appspot.com/1215801/diff/7001/8006
File
dev/core/src/com/google/gwt/dev/util/log/speedtracer/CompilerEventType.java
(right):
http://gwt-code-reviews.appspot.com/1215801/diff/7001/8006#newcode38
dev/core/src/com/google/gwt/dev/util/log/speedtracer/CompilerEventType.java:38:
GENERATOR_OTHER(Generator (Other), Red), //
On 2010/12/14 22:11:56, scottb wrote:
Feels kind of ugh to include these here. I'm starting to think just
creating
event types on the fly for generator types is the way to go. Could
all be done
inside StandardGeneratorContext.
FWIW, I did actually try this and it turned out to be actively worse.
http://gwt-code-reviews.appspot.com/1215801/diff/7001/8009
File tools/api-checker/config/gwt21_22userApi.conf (right):
http://gwt-code-reviews.appspot.com/1215801/diff/7001/8009#newcode123
tools/api-checker/config/gwt21_22userApi.conf:123:
com.google.gwt.user.client.rpc.core.java.util.Collections.SingletonList_CustomFieldSerializer::concreteType()
RETURN_TYPE_ERROR
On 2010/12/14 22:11:56, scottb wrote:
Should we just exclude that whole package? This is a non-user-facing
implementation detail.
Done.
http://gwt-code-reviews.appspot.com/1215801/diff/7001/8017
File user/src/com/google/gwt/user/rebind/rpc/TypeSerializerCreator.java
(right):
http://gwt-code-reviews.appspot.com/1215801/diff/7001/8017#newcode327
user/src/com/google/gwt/user/rebind/rpc/TypeSerializerCreator.java:327:
* Writes a method to produce a map of type string - {...@link TypeHandler}
On 2010/12/14 22:11:56, scottb wrote:
map of type string - type handler class name?
Done.
http://gwt-code-reviews.appspot.com/1215801/diff/7001/8017#newcode444
user/src/com/google/gwt/user/rebind/rpc/TypeSerializerCreator.java:444:
* Writes a method to produce a map of class to type string for Java.
On 2010/12/14 22:11:56, scottb wrote:
also needs update
Done.
http://gwt-code-reviews.appspot.com/1215801/show
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Reduces class and JSNI loading for RPC in devmode. (issue1215801)
Reviewers: zundel, scottb, Description: Reduces class and JSNI loading for RPC in devmode. - Makes GwtScriptOnly work with JSNI methods. - Modifies the RPC generator to tag native methods with GwtScriptOnly. - Modifies the RPC generator to defer class loads of FieldSerializers until needed. Please review this at http://gwt-code-reviews.appspot.com/1215801/show Affected files: M dev/core/src/com/google/gwt/dev/javac/JsniCollector.java M dev/core/src/com/google/gwt/dev/javac/JsniMethod.java M dev/core/src/com/google/gwt/dev/javac/StandardGeneratorContext.java M dev/core/src/com/google/gwt/dev/shell/CompilingClassLoader.java M dev/core/src/com/google/gwt/dev/shell/ModuleSpaceOOPHM.java M dev/core/src/com/google/gwt/dev/util/log/speedtracer/CompilerEventType.java M dev/core/src/com/google/gwt/dev/util/log/speedtracer/DevModeEventType.java M dev/core/super/com/google/gwt/core/client/GwtScriptOnly.java M tools/api-checker/config/gwt21_22userApi.conf M user/src/com/google/gwt/user/client/rpc/core/java/util/Arrays.java M user/src/com/google/gwt/user/client/rpc/core/java/util/Collections.java A user/src/com/google/gwt/user/client/rpc/impl/ReflectionHelper.java M user/src/com/google/gwt/user/client/rpc/impl/SerializerBase.java M user/src/com/google/gwt/user/rebind/rpc/CustomFieldSerializerValidator.java M user/src/com/google/gwt/user/rebind/rpc/FieldSerializerCreator.java M user/src/com/google/gwt/user/rebind/rpc/ProxyCreator.java M user/src/com/google/gwt/user/rebind/rpc/TypeSerializerCreator.java A user/super/com/google/gwt/user/translatable/com/google/gwt/user/client/rpc/impl/ReflectionHelper.java -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Support adding code-gen/runtime related classes directly to the secondary JDT compilation for th... (issue830801)
Reviewers: scottb,
Description:
Support adding code-gen/runtime related classes directly to the
secondary JDT compilation for the GWT AST.
Please review this at http://gwt-code-reviews.appspot.com/830801/show
Affected files:
M dev/core/src/com/google/gwt/dev/jdt/BasicWebModeCompiler.java
M dev/core/src/com/google/gwt/dev/jdt/WebModeCompilerFrontEnd.java
M dev/core/src/com/google/gwt/dev/jjs/UnifiedAst.java
Index: dev/core/src/com/google/gwt/dev/jdt/BasicWebModeCompiler.java
===
--- dev/core/src/com/google/gwt/dev/jdt/BasicWebModeCompiler.java (revision
8689)
+++ dev/core/src/com/google/gwt/dev/jdt/BasicWebModeCompiler.java (working
copy)
@@ -30,6 +30,7 @@
import org.eclipse.jdt.internal.compiler.env.ICompilationUnit;
import java.util.ArrayList;
+import java.util.Collections;
import java.util.HashSet;
import java.util.List;
import java.util.Map;
@@ -64,8 +65,8 @@
* Build the initial set of compilation units.
*/
public CompilationResults getCompilationUnitDeclarations(
- TreeLogger logger, String[] seedTypeNames)
- throws UnableToCompleteException {
+ TreeLogger logger, String[] seedTypeNames,
+ ICompilationUnit... additionalUnits) throws
UnableToCompleteException {
TypeOracle oracle = compilationState.getTypeOracle();
SetJClassType intfTypes = oracle.getSingleJsoImplInterfaces();
@@ -80,7 +81,9 @@
SetCompilationUnit alreadyAdded = new HashSetCompilationUnit();
ListICompilationUnit icus = new ArrayListICompilationUnit(
-seedTypeNames.length + intfTypes.size());
+seedTypeNames.length + intfTypes.size() + additionalUnits.length);
+
+Collections.addAll(icus, additionalUnits);
for (String seedTypeName : seedTypeNames) {
CompilationUnit unit = getUnitForType(logger, classMapBySource,
Index: dev/core/src/com/google/gwt/dev/jdt/WebModeCompilerFrontEnd.java
===
--- dev/core/src/com/google/gwt/dev/jdt/WebModeCompilerFrontEnd.java
(revision 8689)
+++ dev/core/src/com/google/gwt/dev/jdt/WebModeCompilerFrontEnd.java
(working copy)
@@ -27,6 +27,7 @@
import com.google.gwt.dev.util.log.speedtracer.SpeedTracerLogger.Event;
import org.eclipse.jdt.internal.compiler.ast.CompilationUnitDeclaration;
+import org.eclipse.jdt.internal.compiler.env.ICompilationUnit;
import org.eclipse.jdt.internal.compiler.lookup.MethodBinding;
import org.eclipse.jdt.internal.compiler.lookup.ReferenceBinding;
import org.eclipse.jdt.internal.compiler.lookup.TypeBinding;
@@ -46,12 +47,13 @@
public static CompilationResults getCompilationUnitDeclarations(
TreeLogger logger, String[] seedTypeNames,
- RebindPermutationOracle rebindPermOracle, TypeLinker linker)
- throws UnableToCompleteException {
+ RebindPermutationOracle rebindPermOracle, TypeLinker linker,
+ ICompilationUnit... additionalUnits) throws
UnableToCompleteException {
Event getCompilationUnitsEvent =
SpeedTracerLogger.start(CompilerEventType.GET_COMPILATION_UNITS);
CompilationResults results = new
WebModeCompilerFrontEnd(rebindPermOracle,
-linker).getCompilationUnitDeclarations(logger, seedTypeNames);
+linker).getCompilationUnitDeclarations(logger, seedTypeNames,
+additionalUnits);
getCompilationUnitsEvent.end();
return results;
}
Index: dev/core/src/com/google/gwt/dev/jjs/UnifiedAst.java
===
--- dev/core/src/com/google/gwt/dev/jjs/UnifiedAst.java (revision 8689)
+++ dev/core/src/com/google/gwt/dev/jjs/UnifiedAst.java (working copy)
@@ -54,7 +54,7 @@
return jProgram;
}
-JsProgram getJsProgram() {
+public JsProgram getJsProgram() {
return jsProgram;
}
}
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: For synthetic this refs, use params rather than fields while in constructors (issue752801)
LGTM http://gwt-code-reviews.appspot.com/752801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: For synthetic this refs, use params rather than fields while in constructors (issue752801)
LGTM http://gwt-code-reviews.appspot.com/752801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Put in a new fix for DevMode logging that uses bytecode rewriting rather than swapping out (issue725801)
LGTM http://gwt-code-reviews.appspot.com/725801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Put in a new fix for DevMode logging that uses bytecode rewriting rather than swapping out (issue725801)
http://gwt-code-reviews.appspot.com/725801/diff/12001/13004
File dev/core/src/com/google/gwt/dev/shell/GWTBridgeImpl.java (right):
http://gwt-code-reviews.appspot.com/725801/diff/12001/13004#newcode26
dev/core/src/com/google/gwt/dev/shell/GWTBridgeImpl.java:26: protected
static ThreadLocalString uniqueID =
I don't think these changes to GWT* are necessary. See comment in
DevModeLoggingFixes.
http://gwt-code-reviews.appspot.com/725801/diff/12001/13006
File
dev/core/src/com/google/gwt/dev/shell/rewrite/UseMirroredClasses.java
(right):
http://gwt-code-reviews.appspot.com/725801/diff/12001/13006#newcode39
dev/core/src/com/google/gwt/dev/shell/rewrite/UseMirroredClasses.java:39:
private static class MyMethodVisitor extends MethodAdapter {
Better name for this? How about MethodInterceptor?
http://gwt-code-reviews.appspot.com/725801/diff/12001/13006#newcode45
dev/core/src/com/google/gwt/dev/shell/rewrite/UseMirroredClasses.java:45:
mirroredMethods = new HashMapString, String();
This is fairly high maintenance, and I can think of a lot of ways to
improve it (mostly using annotations). The improvements are also pretty
incremental, so they don't have to be big bang.
It's not necessary that you do that now, but maybe you should add a TODO
to that effect so we don't allow this code to grow unwieldy?
http://gwt-code-reviews.appspot.com/725801/diff/12001/13006#newcode67
dev/core/src/com/google/gwt/dev/shell/rewrite/UseMirroredClasses.java:67:
String mirrorClassMethod = mirroredMethods.get(owner + . + name);
If you're not going to change this code to handle the above, then you
might consider optimizing it into a two-level lookup to avoid generating
garbage. It will run for every single instruction for every single class
in the client program, right? That could be millions of times.
http://gwt-code-reviews.appspot.com/725801/diff/12001/13006#newcode67
dev/core/src/com/google/gwt/dev/shell/rewrite/UseMirroredClasses.java:67:
String mirrorClassMethod = mirroredMethods.get(owner + . + name);
This won't catch dispatches via subtypes on instance or static methods.
Here's some example code:
public class TestIntercept {
static class A {
public void foo() {
}
public static void bar() {
}
}
static class B extends A {
}
public static void main(String[] args) {
new A().foo();
new B().foo();
A.bar();
B.bar();
}
}
public static void main(java.lang.String[]);
Code:
0: new #2; //class TestIntercept$A
3: dup
4: invokespecial #3; //Method TestIntercept$A.init:()V
7: invokevirtual #4; //Method TestIntercept$A.foo:()V
10: new #5; //class TestIntercept$B
13: dup
14: invokespecial #6; //Method TestIntercept$B.init:()V
17: invokevirtual #7; //Method TestIntercept$B.foo:()V
20: invokestatic#8; //Method TestIntercept$A.bar:()V
23: invokestatic#9; //Method TestIntercept$B.bar:()V
26: return
}
If you had an intercept rule for A.foo, or A.bar, then your intercept
rule would not have caught the calls to them via the subclass B. Writing
that code is tricky, because you need to look at the class hierarchy of
the owner of the invoke you're examining.
I don't know how important this is to you for your specific case, (How
many people subclass Logger or LogManager?), but you should at least
document that this code doesn't work under those general circumstances.
http://gwt-code-reviews.appspot.com/725801/diff/12001/13006#newcode68
dev/core/src/com/google/gwt/dev/shell/rewrite/UseMirroredClasses.java:68:
if (mirrorClassMethod != null) {
As a code style, I personally prefer early returns. It prevents having
lots of levels of indentation, like you have here. It's also
easier(personally) to see all the code paths.
http://gwt-code-reviews.appspot.com/725801/diff/12001/13006#newcode73
dev/core/src/com/google/gwt/dev/shell/rewrite/UseMirroredClasses.java:73:
if (temp.length 1) {
Shouldn't this be an assert?
http://gwt-code-reviews.appspot.com/725801/diff/12001/13006#newcode83
dev/core/src/com/google/gwt/dev/shell/rewrite/UseMirroredClasses.java:83:
// using the method descriptor string
You should only change the argument types if you're changing from a
virtual dispatch to a static dispatch. You'll still need to change the
owner though.
http://gwt-code-reviews.appspot.com/725801/diff/12001/13013
File user/src/com/google/gwt/logging/impl/DevModeLoggingFixes.java
(right):
http://gwt-code-reviews.appspot.com/725801/diff/12001/13013#newcode69
user/src/com/google/gwt/logging/impl/DevModeLoggingFixes.java:69: return
GWT.getUniqueThreadId() + .;
For simplicity's sake, why not just use the actual thread id?
http://gwt-code-reviews.appspot.com/725801/diff/12001/13013#newcode72
user/src/com/google/gwt/logging/impl/DevModeLoggingFixes.java:72:
private static String removeLoggerPrefix(String name) {
Dead code?
http://gwt-code-reviews.appspot.com/725801/diff/12001/13014
File user/src/com/google/gwt/logging/impl/LoggerImplRegular.java
[gwt-contrib] Re: Remove bogus @SuppressWarnings that eclipse wanted (issue729801)
LGTM http://gwt-code-reviews.appspot.com/729801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Ensure bridgeMethods is initialized, even when constructed from other subtypes, such as MissingT... (issue708802)
Reviewers: scottb, Description: Ensure bridgeMethods is initialized, even when constructed from other subtypes, such as MissingTypeBinding. Please review this at http://gwt-code-reviews.appspot.com/708802/show Affected files: M dev/core/src/org/eclipse/jdt/internal/compiler/lookup/BinaryTypeBinding.java Index: dev/core/src/org/eclipse/jdt/internal/compiler/lookup/BinaryTypeBinding.java === --- dev/core/src/org/eclipse/jdt/internal/compiler/lookup/BinaryTypeBinding.java (revision 8426) +++ dev/core/src/org/eclipse/jdt/internal/compiler/lookup/BinaryTypeBinding.java (working copy) @@ -38,7 +38,7 @@ protected ReferenceBinding[] superInterfaces; protected FieldBinding[] fields; protected MethodBinding[] methods; - protected MethodBinding[] bridgeMethods; + protected MethodBinding[] bridgeMethods = Binding.NO_METHODS; protected ReferenceBinding[] memberTypes; protected TypeVariableBinding[] typeVariables; -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] More steps toward separate compilation. (issue702802)
Reviewers: scottb, Description: More steps toward separate compilation. Please review this at http://gwt-code-reviews.appspot.com/702802/show Affected files: M dev/core/src/com/google/gwt/dev/jdt/AbstractCompiler.java M dev/core/src/com/google/gwt/dev/jdt/BasicWebModeCompiler.java M dev/core/src/com/google/gwt/dev/jdt/WebModeCompilerFrontEnd.java M dev/core/src/com/google/gwt/dev/jjs/JavaToJavaScriptCompiler.java M dev/core/src/com/google/gwt/dev/jjs/ast/JProgram.java M dev/core/src/com/google/gwt/dev/jjs/impl/BuildTypeMap.java M dev/core/src/com/google/gwt/dev/jjs/impl/GenerateJavaAST.java A dev/core/src/com/google/gwt/dev/jjs/impl/TypeLinker.java M dev/core/src/com/google/gwt/dev/jjs/impl/TypeMap.java A dev/core/src/org/eclipse/jdt/internal/compiler/lookup/BinaryTypeBinding.java M dev/core/test/com/google/gwt/dev/jjs/JavaAstConstructor.java -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Add some missing ASM classes (issue699802)
Reviewers: scottb, Description: Add some missing ASM classes Review by: sco...@google.com Please review this at http://gwt-code-reviews.appspot.com/699802/show Affected files: A dev/core/src/com/google/gwt/dev/asm/commons/JSRInlinerAdapter.java A dev/core/src/com/google/gwt/dev/asm/tree/AbstractInsnNode.java A dev/core/src/com/google/gwt/dev/asm/tree/AnnotationNode.java A dev/core/src/com/google/gwt/dev/asm/tree/ClassNode.java A dev/core/src/com/google/gwt/dev/asm/tree/FieldInsnNode.java A dev/core/src/com/google/gwt/dev/asm/tree/FieldNode.java A dev/core/src/com/google/gwt/dev/asm/tree/FrameNode.java A dev/core/src/com/google/gwt/dev/asm/tree/IincInsnNode.java A dev/core/src/com/google/gwt/dev/asm/tree/InnerClassNode.java A dev/core/src/com/google/gwt/dev/asm/tree/InsnList.java A dev/core/src/com/google/gwt/dev/asm/tree/InsnNode.java A dev/core/src/com/google/gwt/dev/asm/tree/IntInsnNode.java A dev/core/src/com/google/gwt/dev/asm/tree/JumpInsnNode.java A dev/core/src/com/google/gwt/dev/asm/tree/LabelNode.java A dev/core/src/com/google/gwt/dev/asm/tree/LdcInsnNode.java A dev/core/src/com/google/gwt/dev/asm/tree/LineNumberNode.java A dev/core/src/com/google/gwt/dev/asm/tree/LocalVariableNode.java A dev/core/src/com/google/gwt/dev/asm/tree/LookupSwitchInsnNode.java A dev/core/src/com/google/gwt/dev/asm/tree/MemberNode.java A dev/core/src/com/google/gwt/dev/asm/tree/MethodInsnNode.java A dev/core/src/com/google/gwt/dev/asm/tree/MethodNode.java A dev/core/src/com/google/gwt/dev/asm/tree/MultiANewArrayInsnNode.java A dev/core/src/com/google/gwt/dev/asm/tree/TableSwitchInsnNode.java A dev/core/src/com/google/gwt/dev/asm/tree/TryCatchBlockNode.java A dev/core/src/com/google/gwt/dev/asm/tree/TypeInsnNode.java A dev/core/src/com/google/gwt/dev/asm/tree/VarInsnNode.java A dev/core/src/com/google/gwt/dev/asm/tree/package.html -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Fixes a bug in the finally handling of doExecuteCommands which unilaterally squashes exceptions.... (issue699803)
Reviewers: robertvawter, mmendez,
Description:
Fixes a bug in the finally handling of doExecuteCommands which
unilaterally squashes exceptions. (Early returns from a finally block
via return statements or exceptions override pending exceptions).
Please review this at http://gwt-code-reviews.appspot.com/699803/show
Affected files:
M user/src/com/google/gwt/user/client/CommandExecutor.java
Index: user/src/com/google/gwt/user/client/CommandExecutor.java
===
--- user/src/com/google/gwt/user/client/CommandExecutor.java(revision 8400)
+++ user/src/com/google/gwt/user/client/CommandExecutor.java(working copy)
@@ -315,16 +315,10 @@
} finally {
wasCanceled = iterator.wasRemoved();
- if (wasCanceled) {
-/*
- * The iterator may have already had its remove method called,
if it
- * has, then we need to exit without updating any state
- */
-return;
- }
-
- if (removeCommand) {
-iterator.remove();
+ if (!wasCanceled) {
+if (removeCommand) {
+ iterator.remove();
+}
}
}
--
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/8001
File dev/core/src/com/google/gwt/dev/jjs/ast/JArrayLength.java (right):
http://gwt-code-reviews.appspot.com/702801/diff/7001/8001#newcode26
dev/core/src/com/google/gwt/dev/jjs/ast/JArrayLength.java:26: * This can
only be null if the referenced field is static.
On 2010/07/19 23:40:01, scottb wrote:
Remove stale copied comment.
Done.
http://gwt-code-reviews.appspot.com/702801/diff/7001/8001#newcode40
dev/core/src/com/google/gwt/dev/jjs/ast/JArrayLength.java:40: @Override
On 2010/07/19 23:40:01, scottb wrote:
@Override illegal in 1.5 for interface method.
Tricky. I used JFieldRef as a starting point, and it uses @override, but
it inherits an implementation from its super class. So in that case,
it's technically overriding it's super class method.
It'd be nice to upgrade to 1.6 and use -target 1.5, but I can see how
that'd make things more tricky in terms of keeping runtime
compatibility.
http://gwt-code-reviews.appspot.com/702801/diff/7001/8001#newcode52
dev/core/src/com/google/gwt/dev/jjs/ast/JArrayLength.java:52: if
(instance != null) {
On 2010/07/19 23:40:01, scottb wrote:
Should never be null.
Done.
http://gwt-code-reviews.appspot.com/702801/diff/7001/8004
File
dev/core/src/com/google/gwt/dev/jjs/impl/CompoundAssignmentNormalizer.java
(right):
http://gwt-code-reviews.appspot.com/702801/diff/7001/8004#newcode88
dev/core/src/com/google/gwt/dev/jjs/impl/CompoundAssignmentNormalizer.java:88:
public boolean visit(JArrayLength x, Context ctx) {
On 2010/07/19 23:40:01, scottb wrote:
This isn't necessary... this visitor is only ever run on LVALUE
expressions,
and array.length can never be an LVALUE.
Done.
http://gwt-code-reviews.appspot.com/702801/diff/7001/8005
File dev/core/src/com/google/gwt/dev/jjs/impl/ControlFlowAnalyzer.java
(right):
http://gwt-code-reviews.appspot.com/702801/diff/7001/8005#newcode457
dev/core/src/com/google/gwt/dev/jjs/impl/ControlFlowAnalyzer.java:457: }
else if (x instanceof JArrayLength) {
On 2010/07/19 23:40:01, scottb wrote:
I don't understand this change. This is another context where array
length
should never be encountered because we're in an LVALUE context here.
I was being conservative in guaranteeing that if JArrayLengths flowed
through the same code that JFieldRefs did, it would be treated as
before. I was leery of quirks in the GWT AST that show up during the
optimization phases.
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#newcode99
dev/core/src/com/google/gwt/dev/jjs/impl/ExpressionAnalyzer.java:99:
accessesFieldNonFinal = true;
On 2010/07/19 23:40:01, scottb wrote:
Definitely remove this line, array.length is immutable.
Ok. Thrown off by the non-final/volatile definition of length in the
intrinsic Array.
We also need canThrowException = true in case the instance happens to
be null.
Done.
http://gwt-code-reviews.appspot.com/702801/diff/7001/8008
File dev/core/src/com/google/gwt/dev/jjs/impl/GenerateJavaScriptAST.java
(right):
http://gwt-code-reviews.appspot.com/702801/diff/7001/8008#newcode257
dev/core/src/com/google/gwt/dev/jjs/impl/GenerateJavaScriptAST.java:257:
arrayLengthField.setObfuscatable(false);
On 2010/07/19 23:40:01, scottb wrote:
On review, I think I gave you bad advice to make it a sibling of
nullMethodName.
Making it a sibling of GenerateJavaScriptVisitor.prototype looks
better. Also
suggest just shorten it to arrayLength since field is not really a
term used
in JS.
Done.
http://gwt-code-reviews.appspot.com/702801/diff/7001/8008#newcode540
dev/core/src/com/google/gwt/dev/jjs/impl/GenerateJavaScriptAST.java:540:
JsNameRef ref = new JsNameRef(x.getSourceInfo(), arrayLengthField);
On 2010/07/19 23:40:01, scottb wrote:
JsNameRef ref = arrayLengthField.makeRef(x.getSourceInfo());
Done.
http://gwt-code-reviews.appspot.com/702801/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/17001/18004 File dev/core/src/com/google/gwt/dev/jjs/impl/ExpressionAnalyzer.java (right): http://gwt-code-reviews.appspot.com/702801/diff/17001/18004#newcode98 dev/core/src/com/google/gwt/dev/jjs/impl/ExpressionAnalyzer.java:98: accessesField = true; On 2010/07/20 21:01:34, scottb wrote: BTW: for the accessesField = true, can you add a TODO: do we actually need to set this? Done. http://gwt-code-reviews.appspot.com/702801/diff/17001/18004#newcode100 dev/core/src/com/google/gwt/dev/jjs/impl/ExpressionAnalyzer.java:100: canThrowException = true; On 2010/07/20 21:04:25, scottb wrote: By the way... I just submitted a change to this file: http://gwt-code-reviews.appspot.com/707801/diff/1/2 When you go and resolve your change against HEAD, please copy my JFieldRef change to this line as well, so that you only throw an exception if the instance may possibly be null. Done. http://gwt-code-reviews.appspot.com/702801/diff/17001/18004#newcode158 dev/core/src/com/google/gwt/dev/jjs/impl/ExpressionAnalyzer.java:158: * On 2010/07/20 21:01:34, scottb wrote: Eclipse formatter actually adds spaces here, if you wouldn't mind reverting the 3 whitespace delta lines. Done. http://gwt-code-reviews.appspot.com/702801/diff/17001/18006 File dev/core/src/com/google/gwt/dev/jjs/impl/GenerateJavaScriptAST.java (right): http://gwt-code-reviews.appspot.com/702801/diff/17001/18006#newcode519 dev/core/src/com/google/gwt/dev/jjs/impl/GenerateJavaScriptAST.java:519: private JsName arrayLength = objectScope.declareName(length); On 2010/07/20 21:01:34, scottb wrote: make 'final' and sort it up between alreadyRan and clinitMap Done. http://gwt-code-reviews.appspot.com/702801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Add first-class support for [array].length in the compiler. (issue702801)
Reviewers: scottb, Description: Add first-class support for [array].length in the compiler. Review by: sco...@google.com Please review this at http://gwt-code-reviews.appspot.com/702801/show Affected files: M dev/core/src/com/google/gwt/dev/jjs/ast/JVisitor.java M dev/core/src/com/google/gwt/dev/jjs/impl/CloneExpressionVisitor.java M dev/core/src/com/google/gwt/dev/jjs/impl/CompoundAssignmentNormalizer.java M dev/core/src/com/google/gwt/dev/jjs/impl/ControlFlowAnalyzer.java M dev/core/src/com/google/gwt/dev/jjs/impl/ExpressionAnalyzer.java M dev/core/src/com/google/gwt/dev/jjs/impl/GenerateJavaAST.java M dev/core/src/com/google/gwt/dev/jjs/impl/GenerateJavaScriptAST.java M dev/core/src/com/google/gwt/dev/jjs/impl/JavaPrecedenceVisitor.java -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
