Thanks, Bob and Scott! I left a couple of style things as they are; if you really think they should happen I will change them in a followup patch.
Committed at r5749. http://gwt-code-reviews.appspot.com/47806/diff/1/3 File dev/core/src/com/google/gwt/dev/jjs/impl/CodeSplitter.java (right): http://gwt-code-reviews.appspot.com/47806/diff/1/3#newcode413 Line 413: private static void installInitialLoadSequenceField(JProgram program, On 2009/07/07 00:38:28, bobv wrote: > Add a comment with an example of the AST this should create. Done. http://gwt-code-reviews.appspot.com/47806/diff/1/11 File user/src/com/google/gwt/core/client/impl/AsyncFragmentLoader.java (right): http://gwt-code-reviews.appspot.com/47806/diff/1/11#newcode86 Line 86: Integer size); On 2009/07/07 00:38:28, bobv wrote: > Document why the arguments are boxed. Done. It's because they are optional arguments. http://gwt-code-reviews.appspot.com/47806/diff/1/11#newcode105 Line 105: * A trivial queue of int's that should compile much better than a LinkedList<Integer>. On 2009/06/27 00:23:12, scottb wrote: > > ? Done. http://gwt-code-reviews.appspot.com/47806/diff/1/11#newcode109 Line 109: private static class BoundedIntQueue { On 2009/06/27 00:23:12, scottb wrote: > API suggestion: instead of add/remove, maybe offer/poll (goes with peek) or > enqueue/dequeue? I was trying to match the Java API. Does that sound like a good idea? http://java.sun.com/j2se/1.5.0/docs/api/java/util/Queue.html I started to change the names around as you suggest, but in the end it looks like it already matches that API pretty well: - add() is like offer(), but offer() is specced to return true or false depending on whether it succeeded. That's not really supported by this queue. - poll() is like remove(), except it's supposed to return null if it fails. That's not supported, either. - peek() is also supposed to check for an empty collection, but this implementation does not. I'm not sure what else to call it, though. Do you still think they should be changed? I don't care too much, except that the implementation should remain concise no matter what the names are. This class cuts some corners than a general-purpose queue class might not want to even when run in the browser. http://gwt-code-reviews.appspot.com/47806/diff/1/11#newcode202 Line 202: private static class StandardLoadingStrategy implements LoadingStrategy { On 2009/07/07 00:38:28, bobv wrote: > Rename to XhrLoadingStrategy to be more descriptive. Much better. Done. http://gwt-code-reviews.appspot.com/47806/diff/1/11#newcode204 Line 204: String fragmentUrl = gwtStartLoadingFragment(fragment, loadErrorHandler); On 2009/07/07 00:38:28, bobv wrote: > Can this be renamed to something like "maybeGetFragmentUrl"? It's harder than it sounds because it's part of the linker API. The underlying JavaScript function is something like __gwtStartLoadingFragment, so I kept the Java name the same. Do you think the Java method should be changed even without the JavaScript one? Or that both should be changed? I agree that the name could be better; it's what it is for historical reasons. http://gwt-code-reviews.appspot.com/47806/diff/1/11#newcode206 Line 206: if (fragmentUrl != null) { On 2009/07/07 00:38:28, bobv wrote: > Invert logic and return early? Done. http://gwt-code-reviews.appspot.com/47806/diff/1/11#newcode213 Line 213: public void onReadyStateChange(XMLHttpRequest xhr) { On 2009/07/07 00:38:28, bobv wrote: > This parameter masks the final variable. Oops! Hmm. I renamed the parameter to be "ignored". http://gwt-code-reviews.appspot.com/47806/diff/1/11#newcode236 Line 236: private native void gwtInstallCode(String text) /*-{ On 2009/07/07 00:38:28, bobv wrote: > Please add some javadoc. It would be helpful to indicate where any eval errors > would be reported. Done. There is currently no well-thought out way to deal with eval errors. So if you download a Wifi login screen instead of your desired JavaScript, that fragment simply won't load and run until you refresh the whole page. http://gwt-code-reviews.appspot.com/47806/diff/1/11#newcode277 Line 277: evt.fragment = [email protected]::intValue()(); The events are documented here: http://code.google.com/p/google-web-toolkit/wiki/LightweightMetricsDesign There are several runAsync events floating around, not all of which regard a download. For example, "runCallbacks12" is an event that happens after 1 or 2 fragments have been downloaded, so there isn't any one fragment to associate it with. http://gwt-code-reviews.appspot.com/47806/diff/1/11#newcode280 Line 280: evt.size = [email protected]::intValue()(); On 2009/07/07 00:38:28, bobv wrote: > Is the boxing necessary for size? When is a zero-size ever a correct value? There was some discussion about when to use 0, -1, and non-presence. I don't remember all of the rationale. According to the above linked doc, -1 is used when the size isn't known, and non-presence is used for events that don't have a size at all. In fact, most events don't have a size; only the *end* event corresponding to a *download* group has one. http://gwt-code-reviews.appspot.com/47806 --~--~---------~--~----~------------~-------~--~----~ http://groups.google.com/group/Google-Web-Toolkit-Contributors -~----------~----~----~----~------~----~------~--~---
