Add support for AsyncProvider in GIN

Add AsyncProvider support in GIN

[Aragos, 2010-03-20 00:49:55]

Thanks for this contribution - looks good! I added a few comments below but they
shouldn't change the content.

Is there any way to test (in an executable test) whether a code fragment has
already been loaded? That way we could ensure that no later changes destroy the
code separation created by your patch.

http://codereview.appspot.com/646042/diff/2001/3008
File
samples/simple/src/com/google/gwt/inject/example/simple/client/SimpleWidget.java
(right):

http://codereview.appspot.com/646042/diff/2001/3008#newcode75
samples/simple/src/com/google/gwt/inject/example/simple/client/SimpleWidget.java:75:

Remove empty line here and add one after the button definition.

http://codereview.appspot.com/646042/diff/2001/3011
File src/com/google/gwt/inject/rebind/BindingsProcessor.java (right):

http://codereview.appspot.com/646042/diff/2001/3011#newcode468
src/com/google/gwt/inject/rebind/BindingsProcessor.java:468:
AsyncProviderBinding binding = asyncProviderBindingProvider.get();
You'll have to do a dance similar to the provider injections above to
accommodate optional injections.

http://codereview.appspot.com/646042/diff/2001/3010
File src/com/google/gwt/inject/rebind/binding/AsyncProviderBinding.java (right):

http://codereview.appspot.com/646042/diff/2001/3010#newcode2
src/com/google/gwt/inject/rebind/binding/AsyncProviderBinding.java:2: *
Copyright 20 Google Inc.
2010

http://codereview.appspot.com/646042/diff/2001/3010#newcode31
src/com/google/gwt/inject/rebind/binding/AsyncProviderBinding.java:31: * Binding
implementation for {@code AsyncProvider<T>} that generates
This documentation might be more useful on the AsyncProvider since that is what
most developers will be looking at.

http://codereview.appspot.com/646042/diff/2001/3010#newcode79
src/com/google/gwt/inject/rebind/binding/AsyncProviderBinding.java:79: String
methodCode =
Please use a StringBuilder here.

http://codereview.appspot.com/646042/diff/2001/3003
File test/com/google/gwt/inject/client/implicit/AsyncProviderTest.java (right):

http://codereview.appspot.com/646042/diff/2001/3003#newcode60
test/com/google/gwt/inject/client/implicit/AsyncProviderTest.java:60:
fail("Should not happend");
have happened? :)

http://codereview.appspot.com/646042/diff/2001/3003#newcode63
test/com/google/gwt/inject/client/implicit/AsyncProviderTest.java:63:
assertTrue(callbackInvoked[0]);
If the above were truly asynchronous, this assert could fail even though the
callback would eventually be successfully called, correct? So if the test always
succeeds, why does it?

[hermesfreitasjunior, 2010-03-20 12:40:30]

Awesome!

http://codereview.appspot.com/646042/diff/2001/3010
File src/com/google/gwt/inject/rebind/binding/AsyncProviderBinding.java (right):

http://codereview.appspot.com/646042/diff/2001/3010#newcode84
src/com/google/gwt/inject/rebind/binding/AsyncProviderBinding.java:84: "     
com.google.gwt.core.client.GWT.runAsync(" +
Assuming AsynProvider<T> I think you should call
GWT.runAsync(T.class, new RunAsyncCallback(...

Also, building the string you could use 
RunAsyncCallback.class.getName() instead of hardcoding the name.

[fazal.asim, 2010-03-23 22:47:24]

thanks for reviewing guys!

http://codereview.appspot.com/646042/diff/2001/3008
File
samples/simple/src/com/google/gwt/inject/example/simple/client/SimpleWidget.java
(right):

http://codereview.appspot.com/646042/diff/2001/3008#newcode75
samples/simple/src/com/google/gwt/inject/example/simple/client/SimpleWidget.java:75:

On 2010/03/20 00:49:56, Aragos wrote:
> Remove empty line here and add one after the button definition.

Done.

http://codereview.appspot.com/646042/diff/2001/3011
File src/com/google/gwt/inject/rebind/BindingsProcessor.java (right):

http://codereview.appspot.com/646042/diff/2001/3011#newcode468
src/com/google/gwt/inject/rebind/BindingsProcessor.java:468:
AsyncProviderBinding binding = asyncProviderBindingProvider.get();
On 2010/03/20 00:49:56, Aragos wrote:
> You'll have to do a dance similar to the provider injections above to
> accommodate optional injections.

Done.

http://codereview.appspot.com/646042/diff/2001/3010
File src/com/google/gwt/inject/rebind/binding/AsyncProviderBinding.java (right):

http://codereview.appspot.com/646042/diff/2001/3010#newcode31
src/com/google/gwt/inject/rebind/binding/AsyncProviderBinding.java:31: * Binding
implementation for {@code AsyncProvider<T>} that generates
On 2010/03/20 00:49:56, Aragos wrote:
> This documentation might be more useful on the AsyncProvider since that is
what
> most developers will be looking at.

I was thinking that AsyncProvider can be a generic interface and not tied to
runAsync but I guess I can add it there as well.

http://codereview.appspot.com/646042/diff/2001/3010#newcode79
src/com/google/gwt/inject/rebind/binding/AsyncProviderBinding.java:79: String
methodCode =
On 2010/03/20 00:49:56, Aragos wrote:
> Please use a StringBuilder here.

Done.

http://codereview.appspot.com/646042/diff/2001/3010#newcode84
src/com/google/gwt/inject/rebind/binding/AsyncProviderBinding.java:84: "     
com.google.gwt.core.client.GWT.runAsync(" +
On 2010/03/20 12:40:30, hermesfreitasjunior wrote:
> Assuming AsynProvider<T> I think you should call
> GWT.runAsync(T.class, new RunAsyncCallback(...
> 
> Also, building the string you could use 
> RunAsyncCallback.class.getName() instead of hardcoding the name.
> 
> 

I thought about doing that but the problem is that I can only have only a single
AsyncProvider for a type X. This might be an acceptable in most cases, but
breaks when we have multiple permutations where we create X. Maybe a compromise
would be to add this to gin dsl (don't know if its possible) to specify runAsync
label or maybe just a configuration to turn it on or off? Need to play with it
more.

As for using RunAsyncCallback, that would add a hard dependency on GWT 2.0, this
way, as long as you don't use AsyncProvider, you can still use Pre 2.0. I am ok
either way, so I will defer this decision to Peter.

http://codereview.appspot.com/646042/diff/2001/3003
File test/com/google/gwt/inject/client/implicit/AsyncProviderTest.java (right):

http://codereview.appspot.com/646042/diff/2001/3003#newcode60
test/com/google/gwt/inject/client/implicit/AsyncProviderTest.java:60:
fail("Should not happend");
On 2010/03/20 00:49:56, Aragos wrote:
> have happened? :)

Done.

http://codereview.appspot.com/646042/diff/2001/3003#newcode63
test/com/google/gwt/inject/client/implicit/AsyncProviderTest.java:63:
assertTrue(callbackInvoked[0]);
On 2010/03/20 00:49:56, Aragos wrote:
> If the above were truly asynchronous, this assert could fail even though the
> callback would eventually be successfully called, correct? So if the test
always
> succeeds, why does it?

In dev mode, there is no actual code splitting and the callback is called
immediately, that's why it works. see
http://www.google.com/codesearch/p?hl=en#MTQ26449crI/com/google/gwt/core/clie...

[Aragos, 2010-03-24 00:20:31]

http://codereview.appspot.com/646042/diff/2001/3010
File src/com/google/gwt/inject/rebind/binding/AsyncProviderBinding.java (right):

http://codereview.appspot.com/646042/diff/2001/3010#newcode2
src/com/google/gwt/inject/rebind/binding/AsyncProviderBinding.java:2: *
Copyright 20 Google Inc.
On 2010/03/20 00:49:56, Aragos wrote:
> 2010

Ping? :)

http://codereview.appspot.com/646042/diff/2001/3010#newcode84
src/com/google/gwt/inject/rebind/binding/AsyncProviderBinding.java:84: "     
com.google.gwt.core.client.GWT.runAsync(" +
On 2010/03/23 22:47:24, fazal.asim wrote:
> On 2010/03/20 12:40:30, hermesfreitasjunior wrote:
> > Assuming AsynProvider<T> I think you should call
> > GWT.runAsync(T.class, new RunAsyncCallback(...
> > 
> > Also, building the string you could use 
> > RunAsyncCallback.class.getName() instead of hardcoding the name.
> > 
> > 
> 
> I thought about doing that but the problem is that I can only have only a
single
> AsyncProvider for a type X. This might be an acceptable in most cases, but
> breaks when we have multiple permutations where we create X. Maybe a
compromise
> would be to add this to gin dsl (don't know if its possible) to specify
runAsync
> label or maybe just a configuration to turn it on or off? Need to play with it
> more.

We can consider adding this to the DSL in some way but for now let's get this
version out of the door first.

> 
> As for using RunAsyncCallback, that would add a hard dependency on GWT 2.0,
this
> way, as long as you don't use AsyncProvider, you can still use Pre 2.0. I am
ok
> either way, so I will defer this decision to Peter.

Just using the string (as you do here) doesn't add any work for us and doesn't
introduce the hard dependency. Please leave it as is.

http://codereview.appspot.com/646042/diff/2001/3003
File test/com/google/gwt/inject/client/implicit/AsyncProviderTest.java (right):

http://codereview.appspot.com/646042/diff/2001/3003#newcode63
test/com/google/gwt/inject/client/implicit/AsyncProviderTest.java:63:
assertTrue(callbackInvoked[0]);
On 2010/03/23 22:47:24, fazal.asim wrote:
> On 2010/03/20 00:49:56, Aragos wrote:
> > If the above were truly asynchronous, this assert could fail even though the
> > callback would eventually be successfully called, correct? So if the test
> always
> > succeeds, why does it?
> 
> In dev mode, there is no actual code splitting and the callback is called
> immediately, that's why it works. see
>
http://www.google.com/codesearch/p?hl=en#MTQ26449crI/com/google/gwt/core/clie...
> 

Please add a comment in the test explaining this.

http://codereview.appspot.com/646042/diff/12001/13007
File
samples/simple/src/com/google/gwt/inject/example/simple/client/SimpleAsyncWidget.java
(right):

http://codereview.appspot.com/646042/diff/12001/13007#newcode1
samples/simple/src/com/google/gwt/inject/example/simple/client/SimpleAsyncWidget.java:1:
// Copyright 2010 Google Inc. All Rights Reserved.
Please use the full license header as found in the other files in this project.

http://codereview.appspot.com/646042/diff/12001/13012
File src/com/google/gwt/inject/client/AsyncProvider.java (right):

http://codereview.appspot.com/646042/diff/12001/13012#newcode26
src/com/google/gwt/inject/client/AsyncProvider.java:26: * * <pre style=code>
Remove one *.

http://codereview.appspot.com/646042/diff/12001/13003
File test/com/google/gwt/inject/client/implicit/AsyncProviderTest.java (right):

http://codereview.appspot.com/646042/diff/12001/13003#newcode1
test/com/google/gwt/inject/client/implicit/AsyncProviderTest.java:1: //
Copyright 2010 Google Inc. All Rights Reserved.
Please use the full license header as found in the other files in this project.

http://codereview.appspot.com/646042/diff/12001/13002
File test/com/google/gwt/inject/client/implicit/FooAsync.java (right):

http://codereview.appspot.com/646042/diff/12001/13002#newcode1
test/com/google/gwt/inject/client/implicit/FooAsync.java:1: // Copyright 2010
Google Inc. All Rights Reserved.
Please use the full license header as found in the other files in this project.

[fazal.asim, 2010-03-24 00:37:30]

http://codereview.appspot.com/646042/diff/12001/13007
File
samples/simple/src/com/google/gwt/inject/example/simple/client/SimpleAsyncWidget.java
(right):

http://codereview.appspot.com/646042/diff/12001/13007#newcode1
samples/simple/src/com/google/gwt/inject/example/simple/client/SimpleAsyncWidget.java:1:
// Copyright 2010 Google Inc. All Rights Reserved.
On 2010/03/24 00:20:31, Aragos wrote:
> Please use the full license header as found in the other files in this
project.

Done.

http://codereview.appspot.com/646042/diff/12001/13012
File src/com/google/gwt/inject/client/AsyncProvider.java (right):

http://codereview.appspot.com/646042/diff/12001/13012#newcode26
src/com/google/gwt/inject/client/AsyncProvider.java:26: * * <pre style=code>
On 2010/03/24 00:20:31, Aragos wrote:
> Remove one *.

Done.

http://codereview.appspot.com/646042/diff/12001/13003
File test/com/google/gwt/inject/client/implicit/AsyncProviderTest.java (right):

http://codereview.appspot.com/646042/diff/12001/13003#newcode1
test/com/google/gwt/inject/client/implicit/AsyncProviderTest.java:1: //
Copyright 2010 Google Inc. All Rights Reserved.
On 2010/03/24 00:20:31, Aragos wrote:
> Please use the full license header as found in the other files in this
project.

Done.

http://codereview.appspot.com/646042/diff/12001/13002
File test/com/google/gwt/inject/client/implicit/FooAsync.java (right):

http://codereview.appspot.com/646042/diff/12001/13002#newcode1
test/com/google/gwt/inject/client/implicit/FooAsync.java:1: // Copyright 2010
Google Inc. All Rights Reserved.
On 2010/03/24 00:20:31, Aragos wrote:
> Please use the full license header as found in the other files in this
project.

Done.

[Aragos, 2010-03-24 00:52:43]

LGTM. :) I'll patch and submit as soon as I find a quiet moment.

[Aragos, 2010-03-24 01:25:13]

I patched the latest version of this change and removed some Java5
incompatibilities (@Overrides for interface methods). Unfortunately the tests
fail in web mode, potentially because of the synchronicity assumptions. Please
run the complete tests using "ant clean test" and check what needs to be fixed.

[Aragos, 2010-03-30 21:34:47]

LGTM, patched this into the Gin codebase. By the way, rietveld doesn't send out
an email when a new patch set is uploaded which is why I didn't see this until
today. A message can speed up the reviewing process. :)