Lex, Mostly LGTM except for what looks to me like a missing fall through case in HandleCrossIslandReferences. I'm still somewhat iffy on the stringify-and-eval stuff because I suspect some bloat, but maybe we can bench alternatives later.
http://gwt-code-reviews.appspot.com/213801/diff/1/7 File dev/core/src/com/google/gwt/dev/jjs/impl/FragmentLoaderCreator.java (right): http://gwt-code-reviews.appspot.com/213801/diff/1/7#newcode164 dev/core/src/com/google/gwt/dev/jjs/impl/FragmentLoaderCreator.java:164: srcWriter.println(" new AsyncFragmentLoader.LoadFinishedHandler() {"); Nit. I wouldn't let this comment stop a commit, but this is still somewhat confusing to me, because it takes an exception, and invokes failure code, it really does seem to be a function that deals with 'errors' and not 'successes'. 'Finished' seems to denote success. http://gwt-code-reviews.appspot.com/213801/diff/1/8 File dev/core/src/com/google/gwt/dev/jjs/impl/HandleCrossIslandReferences.java (right): http://gwt-code-reviews.appspot.com/213801/diff/1/8#newcode50 dev/core/src/com/google/gwt/dev/jjs/impl/HandleCrossIslandReferences.java:50: */ Is there a difference between Islands and Fragments, or do they both refer to the same thing? http://gwt-code-reviews.appspot.com/213801/diff/1/8#newcode79 dev/core/src/com/google/gwt/dev/jjs/impl/HandleCrossIslandReferences.java:79: public void endVisit(JsVars x, JsContext<JsStatement> ctx) { Would it help to check if the var's scope is global/topscope? It seems there's an implicit check below in the sense that non-program scope variables will have zero cross-island references, and therefore won't be considered, but still end up being in the defs-set. http://gwt-code-reviews.appspot.com/213801/diff/1/8#newcode174 dev/core/src/com/google/gwt/dev/jjs/impl/HandleCrossIslandReferences.java:174: } If the var is in the namesToPredefine set, but doesn't have an initializer, it is simply deleted, is this correct? It seems like you could have a cross-island reference to a global var with no initializer. Should there be an else currentVar.add(var)? http://gwt-code-reviews.appspot.com/213801/diff/1/16 File user/src/com/google/gwt/core/client/impl/CrossSiteLoadingStrategy.java (right): http://gwt-code-reviews.appspot.com/213801/diff/1/16#newcode62 user/src/com/google/gwt/core/client/impl/CrossSiteLoadingStrategy.java:62: private static native JavaScriptObject createScriptTag(String url) /*-{ IIRC, this will fail on some browsers if a <head> element is missing, while some other browsers will synthesize a <head> if it doesn't exist. http://gwt-code-reviews.appspot.com/213801/diff/1/16#newcode114 user/src/com/google/gwt/core/client/impl/CrossSiteLoadingStrategy.java:114: __gwtModuleFunction['runAsyncCallback'+fragment] = callback; For cleanliness, maybe we should "delete __gwtModuleFunction['runAsyncCallback'+fragment] when done with it? http://gwt-code-reviews.appspot.com/213801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors To unsubscribe from this group, send email to google-web-toolkit-contributors+unsubscribegooglegroups.com or reply to this email with the words "REMOVE ME" as the subject.
