Re: add a test for AsyncFragmentLoader
bo...@google.com
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,
Add a comment with an example of the AST this should create.
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);
Document why the arguments are boxed.
http://gwt-code-reviews.appspot.com/47806/diff/1/11#newcode202
Line 202: private static class StandardLoadingStrategy implements
LoadingStrategy {
Rename to XhrLoadingStrategy to be more descriptive.
http://gwt-code-reviews.appspot.com/47806/diff/1/11#newcode204
Line 204: String fragmentUrl = gwtStartLoadingFragment(fragment,
loadErrorHandler);
Can this be renamed to something like "maybeGetFragmentUrl"?
http://gwt-code-reviews.appspot.com/47806/diff/1/11#newcode206
Line 206: if (fragmentUrl != null) {
Invert logic and return early?
http://gwt-code-reviews.appspot.com/47806/diff/1/11#newcode213
Line 213: public void onReadyStateChange(XMLHttpRequest xhr) {
This parameter masks the final variable.
http://gwt-code-reviews.appspot.com/47806/diff/1/11#newcode236
Line 236: private native void gwtInstallCode(String text) /*-{
Please add some javadoc. It would be helpful to indicate where any eval
errors would be reported.
http://gwt-code-reviews.appspot.com/47806/diff/1/11#newcode277
Line 277: evt.fragment = fragment.@java.lang.Integer::intValue()();
When would there not be a fragment number?
http://gwt-code-reviews.appspot.com/47806/diff/1/11#newcode280
Line 280: evt.size = size.@java.lang.Integer::intValue()();
Is the boxing necessary for size? When is a zero-size ever a correct
value?
sp...@google.com
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 = fragment.@java.lang.Integer::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 = size.@java.lang.Integer::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.