Re: A Pending Patch from: Rob Jellinghaus that addresses Issue 389

[Bruce Johnson]

Miguel,

Here are a small bit of feedback on the javadoc in a few files. I
figure it might make sense to pipeline this if that's okay with you.

-- Bruce

On 2/8/07, Miguel Méndez <mme...@google.com> wrote:
> A user/src/com/google/gwt/user/server/rpc/RPC.java
Javadoc...
The class itself:
- Would be nice if there was a code example at the top of how to use
the RPC utility methods in a canonical way.

In decodeRequest(String):
- Does it really need to specify that it calls
decodeRequest(String,Class) as part of its implementation?

In decodeRequest(String, Class):
- Typo in decodeRequest(String, Class). Search for "the this method".
- It isn't really clear what "service" is from reading the doc. Must
it be a class? Can it be an interface? It isn't clear that the phrase
"the requested interface and method" is talking about the payload
contents...it sounds like it could be talking about the 'service'
param itself.

In invoke():
- The doc doesn't make it clear why the result is a String rather than
an object. The API looked orthogonal until I saw that. Wouldn't you:
1) call invoke()
2) receive the result object
3) call encodeSuccess on the object from #2
?
There's probably a good reason, but the javdoc doesn't make it clear.

In encodeSuccess() and encodeFailure():
- Why do you have to pass the Method in? It isn't clear from the Javadoc.

> M user/src/com/google/gwt/user/server/rpc/package.html
- "see and override" sounds a little funny
- The RemoteServiceServlet.processCall reference should be a hyperlink
(@link works from inside package.html)
- "or by code invoked by overrides of the processCall method" is hard
to parse; rephrase?
- The RemoteServiceServlet.handleFailure reference should be a hyperlink

[Miguel Méndez]

Hi Bruce,

I have attached an updated patch which attempts to address these issues.  Additional comments are interleaved below.

On 2/21/07, Bruce Johnson <br...@google.com> wrote:

Miguel,

Here are a small bit of feedback on the javadoc in a few files. I
figure it might make sense to pipeline this if that's okay with you.

Sounds good

-- Bruce

On 2/8/07, Miguel Méndez <mme...@google.com> wrote:
> A     user/src/com/google/gwt/user/server/rpc/RPC.java
Javadoc...
The class itself:
- Would be nice if there was a code example at the top of how to use
the RPC utility methods in a canonical way.

Added the canonical example and a more advanced example as well.

In decodeRequest(String):
- Does it really need to specify that it calls
decodeRequest(String,Class) as part of its implementation?

Not since it is a static method -- removed.

In decodeRequest(String, Class):
- Typo in decodeRequest(String, Class). Search for "the this method".
- It isn't really clear what "service" is from reading the doc. Must
it be a class? Can it be an interface? It isn't clear that the phrase
"the requested interface and method" is talking about the payload
contents...it sounds like it could be talking about the 'service'
param itself.

It can be a class or an interface -- the javadoc has been updated.

In invoke():
- The doc doesn't make it clear why the result is a String rather than
an object. The API looked orthogonal until I saw that. Wouldn't you:
1)  call invoke()
2) receive the result object
3) call encodeSuccess on the object from #2
?
There's probably a good reason, but the javdoc doesn't make it clear.

The RPC.invoke is really a convenience method which stitches together the steps that you outlined.  Hence the string it returns is ready to return to the client and it can be a return value from the service method or an exception thrown by the service method.  We could change it to return an object but then we would need to decide how to handle exceptions thrown by the service method.  It is doable but this definition seemed the most convenient.  A developer could always perform the sequence that you described above if they use Method.invoke rather than RPC.invoke.

I updated the javadoc in attempt to clarify the method.

In encodeSuccess() and encodeFailure():
- Why do you have to pass the Method in? It isn't clear from the Javadoc.

The Method parameter is used to perform additional checks.  In encodeSuccess we make sure that the return value is compatible with the Method's declared return type.  In encodeFailure, we check that the Throwable is compatible with the Method's list of checked exceptions.

> M    user/src/com/google/gwt/user/server/rpc/package.html
- "see and override" sounds a little funny
- The RemoteServiceServlet.processCall reference should be a hyperlink
(@link works from inside package.html)
- "or by code invoked by overrides of the processCall method" is hard
to parse; rephrase?
- The RemoteServiceServlet.handleFailure reference should be a hyperlink

Updated the package.html file.

The current patch contents are listed below.

Affected Paths:
A      user/test/com/google/gwt/user/server/rpc/RPCTest.java
M      user/test/com/google/gwt/user/client/rpc/RemoteServiceServletTest.java
M      user/test/com/google/gwt/user/RPCSuite.java
A      user/javadoc/com/google/gwt/examples/rpc
A      user/javadoc/com/google/gwt/examples/rpc/server
A      user/javadoc/com/google/gwt/examples/rpc/server/AdvancedExample.java
A      user/javadoc/com/google/gwt/examples/rpc/server/CanonicalExample.java
M      user/src/com/google/gwt/user/server/rpc/RemoteServiceServlet.java
A      user/src/com/google/gwt/user/server/rpc/UnexpectedException.java
A      user/src/com/google/gwt/user/server/rpc/RPC.java
A      user/src/com/google/gwt/user/server/rpc/RPCRequest.java
M      user/src/com/google/gwt/user/server/rpc/package.html

--
Miguel

[Bruce Johnson]

Fresh comments here and replies inline below. Thanks again so much for your patience on my painfully long turnaround time.

-- Bruce

Overall
- I couldn't build this from the command line due to doc build problems.
- Copyright headers on anything you touch need to be updated to 2007 (and not 2006-2007). Emily can probably help if the checkstyle rules need tweaking.

RemoteServiceServlet.java
Javadoc for the class
- Funky grammar with the "Or, " paragraphs.

Javadoc for onUnexpectedFailure
- looks funky; maybe overly aggressive doc autoformatting?

Code for doPost
- catch(Throwable e): We need more of an explanation of the possible control flow here. It isn't clear whether or not onUnexpectedFailure() can/should throw an exception or not. The semantics a little confusing themselves, but at least onUnexpectedFailure() is well documented. It's just that here at the call site, you don't really know what to expect.

RPC.java
Javadoc for the class
- Bad corss reference to CanonicalExample#doPost() -- should it be processCall?
- Unneeded comma after Spring

More descriptive methods names would be nice:
- encodeFailure => encodeResponseForFailure
- encodeSuccess => encodeResponseForSuccess
- invoke => invokeAndEncodeResponse
- Yes, these names are longer but I had to reference the Javadoc to understand the semantics, and the longer name reduces the need to look up doc

Javadoc for decodeRequest(String)
(see comments for the other overload, most apply)

Javadoc for decodeRequest(String, Class)
- It still isn't totally clear from the description exactly when I'd want to supply a constraint type or not. Does doing so make my code somehow safer? If I don't supply one and the type gets loaded, does it enforce any checks at all? Maybe an example would help.
- "implements the" might be clearer as "is assignable to the" (requires a tweak to the grammar later in the sentence regarding "method")
- @param payload: rephrase? the payload actually includes more than "the request parameters"
- @param classOrInterface: could rename to "type" to get the same effect; also, would be clearer to change "implements the" to "is assignable to the"
- @throws NullPointerException: nice to add <code> brackets around null [you can ctrl+space immediately after typing null to get doc completion to do that]
- @throws SerializationException: grammar issue
- @throws SecurityException: stale reference to the old param name "constraintClass"
- look for other places you could change "null" to "<code>null</code>"
- look for other places with inline code you could add <code> tags around (e.g. "RPC.getClass().getClassLoader)")

Code for decodeRequest(String, Class)
- The check "RemoteService.class.isAssignableFrom(serviceIntf)" requires the target to implement RemoteService; I'm just checking that this is an okay requirement for the use cases we care about (TMIF welcome)
- Error message typo; search for "RemoteService; ;"

Code for getClassFromSerializedName
- FFTI: is Class.forName() fast? could we speed things up by caching String=>Type mappings in an instance HashMap?

Javadoc for encodeSuccess
- @return: reword? strictly speaking, the string encodes the object passed in -- it really doesn't literally "encode the result of calling a service method" per se (although, yes, that's how it would be used in practice)

Code for encodeSuccess
- throw new IllegalArgumentException
  - the error message relies on Class.getName() generating a user-readable type name...might not look right for all types (e.g. arrays of primitives)?
  - the error message relies on serviceMethod.toString() generating a user-readable method declaration...does it? (TMIF welcome)

Javadoc for invoke
- @param serviceMethod: grammar? spurious period
- @param parameters: the name "args" would be more correct
- @return: grammar
- @throws SecurityException
  - needs to be updated if you rename "parameters"
  - the tense seems different in the two clauses
  - "type-valid" isn't a well known phrase, maybe rephrase to clarity?

Code for invoke
- Tone down both error messages for "Security vioation" to match the tone in decodeRequest(); it isn't necessarily a "real" security violation; it's a blocked attempt to do something incorrect that could be caused by a variety of reasons (e.g. client/server mismatch). Matching the phrasing in decodeRequest() would enhance consistency and is less likely to induce unjustified paranoia.

AdvancedExample.java
Sample code in doPost()
- FFTI (feel free to ignore): the names "returnSuccess", "returnFailure", and "returnGenericFailure" imply we're returning from the function. How about "sendResponseForSuccess", etc? (Or some variation of a name that doesn't mix the "return" keyword idea with the idea of writing a response the socket.

On 2/22/07, Miguel Méndez <mme...@google.com> wrote:

Hi Bruce,

In encodeSuccess() and encodeFailure():
- Why do you have to pass the Method in? It isn't clear from the Javadoc.

The Method parameter is used to perform additional checks.  In encodeSuccess we make sure that the return value is compatible with the Method's declared return type.  In encodeFailure, we check that the Throwable is compatible with the Method's list of checked exceptions.

It makes sense. I thought maybe there would be a more elegant way to do this while reducing possible error modes, but I don't really think there is.

[Scott Blum]

On 2/23/07, Bruce Johnson <br...@google.com> wrote:

Overall

- Copyright headers on anything you touch need to be updated to 2007 (and not 2006-2007). Emily can probably help if the checkstyle rules need tweaking.

Not to derail the thread, but this is an issue we need to address on a much larger scale, and programmatically.  Files using the 2007 header will still break the build; we need to fix this and then do a programmatic sweep of the whole source tree to bump the versions on either a) everything or b) everything that's changed since 1/1/07.

Scott

[Bruce Johnson]

We definitely need to tweak our checkstyle settings to allow either 2006 or 2007 (mutually exclusive), but we I don't think we have to gate that on doing a sweep.

[Scott Blum]

Totally agree with you.  I actually meant that the gating would be in the other direction. :)

[Miguel Méndez]

I have a patch to our checkstyle rules to allow either 2006 or 2007 in the file header.

On 2/23/07, Bruce Johnson < br...@google.com> wrote:

We definitely need to tweak our checkstyle settings to allow either 2006 or 2007 (mutually exclusive), but we I don't think we have to gate that on doing a sweep.

On 2/23/07, Scott Blum < sco...@google.com> wrote:

On 2/23/07, Bruce Johnson <br...@google.com> wrote:

Overall
- Copyright headers on anything you touch need to be updated to 2007 (and not 2006-2007). Emily can probably help if the checkstyle rules need tweaking.

Not to derail the thread, but this is an issue we need to address on a much larger scale, and programmatically.  Files using the 2007 header will still break the build; we need to fix this and then do a programmatic sweep of the whole source tree to bump the versions on either a) everything or b) everything that's changed since 1/1/07.

Scott

--
Miguel

[Rob Jellinghaus]

Just wanted to say briefly: thanks, Bruce, for your incredibly
detailed and helpful comments. I'm very glad to see such care going
into the documentation. And reading between the lines, I'm also glad
the code itself looks mostly good to you (which I infer from the fact
that most of your comments are about documentation)!

"Worth the wait" would be my description of this thread :-)

Also, thanks very much Miguel for effectively owning these changes to
the patch. If you would like me to help with fielding Bruce's
comments, please let me know; otherwise my assumption is that you're
OK with continuing to bring this in to a final landing.

Some specific peanut-gallery replies to Bruce's questions:

>- FFTI: is Class.forName() fast? could we speed things up by caching
>String=>Type mappings in an instance HashMap?

I would want to profile the overall subsystem with a few different
stress cases before adding more optimization like that. I'm sure a
map would be faster than just Class.forName, but I'm not sure it's
enough of a hotspot to matter.

>- The check "RemoteService.class.isAssignableFrom(serviceIntf)" requires the
>target to implement RemoteService; I'm just checking that this is an okay
>requirement for the use cases we care about (TMIF welcome)

The service interface has to implement RemoteService in order for GWT
to compile the client-side service class. So enforcing that
requirement on the server seems congruent with that. A deeper
question might be, why have the RemoteService marker interface at all,
on either client side OR server side? But if you have it on one,
seems good to verify it on the other.

(What does TMIF mean? Too Much InFormation? Google is remarkably
unhelpful on this!)

>Code for encodeSuccess

> - the error message relies on Class.getName() generating a user-readable
>type name...might not look right for all types (e.g. arrays of primitives)?

Good point, albeit moderately cumbersome to fix.

> - the error message relies on serviceMethod.toString() generating a
>user-readable method declaration...does it? (TMIF welcome)

Yes, in my experience it does.

Best regards,
Rob

[John Tamplin]

On 2/23/07, Rob Jellinghaus <rjelli...@gmail.com> wrote:

(What does TMIF mean?  Too Much InFormation?  Google is remarkably
unhelpful on this!)

Trust Me It's Fine - meaning he wasn't necessarily asking for a complete description, just making sure you had considered the question.

--
John A. Tamplin
Software Engineer, Google

[Scott Blum]

On 2/23/07, Rob Jellinghaus <rjelli...@gmail.com> wrote:

>- FFTI: is Class.forName() fast? could we speed things up by caching
>String=>Type mappings in an instance HashMap?

I would want to profile the overall subsystem with a few different
stress cases before adding more optimization like that.  I'm sure a
map would be faster than just Class.forName, but I'm not sure it's
enough of a hotspot to matter.

Just want to point out that, in general, we care a LOT about performance when it comes to code intended to be run in the server side.  We generally optimize for code size on the client, but speed on the server, where potentially ever CPU cycle matters.  I would perhaps suggest that we profile these changes to make sure it's no slower than 1.3.3.

(What does TMIF mean?  Too Much InFormation?  Google is remarkably
unhelpful on this!)

"Trust me, it's fine". :)

Scott

[Rob Jellinghaus]

On Feb 23, 10:54 am, "Scott Blum" <sco...@google.com> wrote:
> Just want to point out that, in general, we care a LOT about performance
> when it comes to code intended to be run in the server side. We generally
> optimize for code size on the client, but speed on the server, where
> potentially ever CPU cycle matters. I would perhaps suggest that we profile
> these changes to make sure it's no slower than 1.3.3.

AFAICT the code is essentially isomorphic to the 1.3.3 code -- it's
basically the same code, just rearranged. I'm not at all opposed to
profiling in order to verify that, but on inspection, at least,
there's nothing very different happening. (Amazing how refactoring
can enhance extensibility without altering function...)

I would say TMIF, but it seems presumptuous somehow :-)

Thanks for the TMIF clues.
Cheers,
Rob

[Bruce Johnson]

Rob, Miguel,

I know you guys have worked really hard on this. Looking forward to the final version, which will be a much-needed enhancement.

On 2/23/07, Rob Jellinghaus <rjelli...@gmail.com> wrote:

AFAICT the code is essentially isomorphic to the 1.3.3 code

Agreed. Scott may not have known that it was essentially a refactor.

I would say TMIF, but it seems presumptuous somehow :-)

A solicited TMIF is a good thing, yet an unsolicited TMIF is practically begging for a debate :-)

I'm acronym crazy, apparently. This comment about caching was a "FFTI" (feel free to ignore)...meaning that it was a just a random bit of feedback but wasn't remotely intended to block acceptance.

I do actually think we might want to revisit the caching issue in a subsequent release since Class.forName() gets called in a critical loop. Maybe I'll ask Toby how well that gets JITted.

-- Bruce

[Miguel Méndez]

Comments are interleaved below.  Please note that I attached a second patch, review pending, which allows for 2006 or 2007 for the copyright date.  To review this change please execute in the trunk:

        svn update -r456
        patch -p0 < issue-389-r456.patch2
        svn add user/test/com/google/gwt/user/server/rpc/RPCTest.java
        svn add user/javadoc/com/google/gwt/examples/rpc/server/AdvancedExample.java
        svn add user/javadoc/com/google/gwt/examples/rpc/server/CanonicalExample.java
        svn add user/src/com/google/gwt/user/server/rpc/UnexpectedException.java
        svn add user/src/com/google/gwt/user/server/rpc/RPC.java
        svn add user/src/com/google/gwt/user/server/rpc/RPCRequest.java
        patch -p0 < checkstyle-copyright-header-update-r456.patch
       
Affected Paths:
M      doc/build.xml

A      user/test/com/google/gwt/user/server/rpc/RPCTest.java
M      user/test/com/google/gwt/user/client/rpc/RemoteServiceServletTest.java
M      user/test/com/google/gwt/user/RPCSuite.java
A      user/javadoc/com/google/gwt/examples/rpc
A      user/javadoc/com/google/gwt/examples/rpc/server
A      user/javadoc/com/google/gwt/examples/rpc/server/AdvancedExample.java
A      user/javadoc/com/google/gwt/examples/rpc/server/CanonicalExample.java
M      user/src/com/google/gwt/user/server/rpc/RemoteServiceServlet.java
A      user/src/com/google/gwt/user/server/rpc/UnexpectedException.java
A      user/src/com/google/gwt/user/server/rpc/RPC.java
A      user/src/com/google/gwt/user/server/rpc/RPCRequest.java
M      user/src/com/google/gwt/user/server/rpc/package.html

On 2/23/07, Bruce Johnson < br...@google.com > wrote:

Overall
- I couldn't build this from the command line due to doc build problems.
- Copyright headers on anything you touch need to be updated to 2007 (and not 2006-2007). Emily can probably help if the checkstyle rules need tweaking.

Document problems have been fixed and it builds correctly now.  You will need to apply the checkstyle patch so that either 2006 or 2007 copyright dates will be accepted.  There is a pending code review for the checkstyle changes.

RemoteServiceServlet.java
Javadoc for the class
- Funky grammar with the "Or, " paragraphs.

Rolled back to original comments since the class' functionality has not changed.  The package documentation provides the necessary additional information.

Javadoc for onUnexpectedFailure
- looks funky; maybe overly aggressive doc autoformatting?

Split up the paragraphs.

Code for doPost
- catch(Throwable e): We need more of an explanation of the possible control flow here. It isn't clear whether or not onUnexpectedFailure() can/should throw an exception or not. The semantics a little confusing themselves, but at least onUnexpectedFailure() is well documented. It's just that here at the call site, you don't really know what to expect.

Added a comment to explain what happens.  Renamed the method onUnexpectedFailure method to doUnexpectedFailure, since it is really a template method, and moved the respondWithFailure call into it.

RPC.java
Javadoc for the class
- Bad corss reference to CanonicalExample#doPost() -- should it be processCall?
- Unneeded comma after Spring

Done

More descriptive methods names would be nice:
- encodeFailure => encodeResponseForFailure
- encodeSuccess => encodeResponseForSuccess
- invoke => invokeAndEncodeResponse
- Yes, these names are longer but I had to reference the Javadoc to understand the semantics, and the longer name reduces the need to look up doc

We do need more descriptive names; I used the ones that you suggested.

Javadoc for decodeRequest(String)
(see comments for the other overload, most apply)

Javadoc for decodeRequest(String, Class)
- It still isn't totally clear from the description exactly when I'd want to supply a constraint type or not. Does doing so make my code somehow safer? If I don't supply one and the type gets loaded, does it enforce any checks at all? Maybe an example would help.
- "implements the" might be clearer as "is assignable to the" (requires a tweak to the grammar later in the sentence regarding "method")
- @param payload: rephrase? the payload actually includes more than "the request parameters"
- @param classOrInterface: could rename to "type" to get the same effect; also, would be clearer to change "implements the" to "is assignable to the"
- @throws NullPointerException: nice to add <code> brackets around null [you can ctrl+space immediately after typing null to get doc completion to do that]
- @throws SerializationException: grammar issue
- @throws SecurityException: stale reference to the old param name "constraintClass"
- look for other places you could change "null" to "<code>null</code>"
- look for other places with inline code you could add <code> tags around (e.g. "RPC.getClass().getClassLoader)")

Updated the javadoc accordingly.

Code for decodeRequest(String, Class)
- The check "RemoteService.class.isAssignableFrom(serviceIntf)" requires the target to implement RemoteService; I'm just checking that this is an okay requirement for the use cases we care about (TMIF welcome)

This is the intent. 

- Error message typo; search for "RemoteService; ;"

Fixed

Code for getClassFromSerializedName
- FFTI: is Class.forName() fast? could we speed things up by caching String=>Type mappings in an instance HashMap?

This was covered in subsequent threads.  Summary: this is the way that it has always worked; we can profile and optimize accordingly as a separate step.

Javadoc for encodeSuccess
- @return: reword? strictly speaking, the string encodes the object passed in -- it really doesn't literally "encode the result of calling a service method" per se (although, yes, that's how it would be used in practice)

Updated

Code for encodeSuccess
- throw new IllegalArgumentException
  - the error message relies on Class.getName() generating a user-readable type name...might not look right for all types (e.g. arrays of primitives)?

Updated to use TypeInfo.getSourceRepresentation(Class, String); this will produce a user-readable string.

  - the error message relies on serviceMethod.toString() generating a user-readable method declaration...does it? (TMIF welcome)

Nested types still include '$'.  For the time being, I convert '$' to '.'.

Javadoc for invoke
- @param serviceMethod: grammar? spurious period
- @param parameters: the name "args" would be more correct
- @return: grammar
- @throws SecurityException
  - needs to be updated if you rename "parameters"
  - the tense seems different in the two clauses
  - "type-valid" isn't a well known phrase, maybe rephrase to clarity?

Attempted to clarify javadoc

Code for invoke
- Tone down both error messages for "Security vioation" to match the tone in decodeRequest(); it isn't necessarily a "real" security violation; it's a blocked attempt to do something incorrect that could be caused by a variety of reasons (e.g. client/server mismatch). Matching the phrasing in decodeRequest() would enhance consistency and is less likely to induce unjustified paranoia.

Done

AdvancedExample.java
Sample code in doPost()
- FFTI (feel free to ignore): the names "returnSuccess", "returnFailure", and "returnGenericFailure" imply we're returning from the function. How about "sendResponseForSuccess", etc? (Or some variation of a name that doesn't mix the "return" keyword idea with the idea of writing a response the socket.

Names changed

On 2/22/07, Miguel Méndez <mme...@google.com > wrote:

Hi Bruce,

In encodeSuccess() and encodeFailure():
- Why do you have to pass the Method in? It isn't clear from the Javadoc.

The Method parameter is used to perform additional checks.  In encodeSuccess we make sure that the return value is compatible with the Method's declared return type.  In encodeFailure, we check that the Throwable is compatible with the Method's list of checked exceptions.

It makes sense. I thought maybe there would be a more elegant way to do this while reducing possible error modes, but I don't really think there is.

--
Miguel

[Toby Reyelts]

Well,

I'd say you might want to be conservative about calling Class.forName excessively based on the way the compilers and JVMs have historically handled class literals.

Prior to JDK 1.4, when you used a class literal, compilers would generate a synthetic method to call Class.forName() to obtain the Class, and a synthetic field to cache the value - thus implying that caching the Class object was worthwhile. Starting with JDK 1.5, class literals are synthesized to an LDC instruction, which totally removes the call to Class.forName.

This seems to imply to me that the compiler/JVM implementors were concerned about minimizing Class.forName calls. I don't believe the JIT can optimize Class.forName any more than any other random method - which, in the case of Class.forName, won't be much, IMHO.

Just my 2 cents,
Toby

On 2/23/07, Bruce Johnson <br...@google.com > wrote:

[Bruce Johnson]

Other than the comments below, Looks Good To Me -- no need for another review. Thanks yet again for your patience on my ridiculously slow turnaround time on these code reviews.

=== RemoteServiceServlet.java
- The hyperlink in processCall() to invokeAndEncodeResponse() comes out broken when we build GWT doc; it looks good in the standard javadoc; would you mind entering a bug against our doc tool?

=== RPC.java
- The first sample looks messed up in the GWT generated doc; it looks good in the standard javadoc; would you enter a bug against doctool for this also, please?
- The advanced example has some weird and apparently-unnecessary concatenations of consecutive string literals in messages (see the catch blocks).
- The first sentence in the javadoc for invokeAndEncodeResponse() needs a comma before the phrase beginning with "which could be"; this isn't a nitpick: without the comma it seems like the phrase is referring to the service method that gets called rather than the result that the invocation produces.

-- Bruce

On 2/22/07, Miguel M�ndez <mme...@google.com > wrote:

Hi Bruce,

In encodeSuccess() and encodeFailure():
- Why do you have to pass the Method in? It isn't clear from the Javadoc.

The Method parameter is used to perform additional checks.  In encodeSuccess we make sure that the return value is compatible with the Method's declared return type.  In encodeFailure, we check that the Throwable is compatible with the Method's list of checked exceptions.

It makes sense. I thought maybe there would be a more elegant way to do this while reducing possible error modes, but I don't really think there is.

--
Miguel

[Rob Jellinghaus]

<very_gentle_nudge> Thanks Bruce for all your attention, and of course
thanks VERY much Miguel for having made this whole patch happen.
Please allow me this minor bump so this doesn't slip out of 1.4, now
that it's ready to land after almost two months! </very_gentle_nudge>

I'm actively working on integrating GWT with Seam, based on my g4jsf
patch, which is ready to land as soon as... well, as soon as THIS
patch lands. Now that Exadel is effectively part of JBoss, the whole
Seam -> ajax4jsf -> GWT connection is just that much more interesting
to JBoss, so it's getting some attention from over there. I'll
definitely keep this group posted on developments.

:-)
Cheers!
Rob

[Miguel Méndez]

I apologize that it has taken so long.  It is not going to slip out of 1.4. 

I've actually got the patch ready to commit right now.  However, I always like to run a full regression against these types of changes as a final cross-check before I commit.  Along the way, I've run into a few failed unit test that I decided to track down before committing -- these failed unit tests are not related to RPC but they do prevent me from completing the regression.

Cheers,

[Bruce Johnson]

Yeah, there's definitely no chance it won't make it into 1.4. All that's left is basically "svn commit" :-)

[Miguel Méndez]

W00T!!!! 

I was finally able to get the patch to successfully complete the the full test suite in hosted & web modes.  I apologize that this took a while, but the RPC suite flushed out a few bugs in the hosted mode refactoring which needed to be tracked down and resolved.  John, thanks for all of your help on that front.

On 3/11/07, Bruce Johnson <br...@google.com> wrote:

=== RemoteServiceServlet.java
- The hyperlink in processCall() to invokeAndEncodeResponse() comes out broken when we build GWT doc; it looks good in the standard javadoc; would you mind entering a bug against our doc tool?

Entered issue 813 for this.

=== RPC.java
- The first sample looks messed up in the GWT generated doc; it looks good in the standard javadoc; would you enter a bug against doctool for this also, please?

By messed up do you mean in a scrollable region?  I did a sort and format on the file which wrapped the line and removed the wrapping.  Should our doctool autowrap or did I miss something?
 

- The advanced example has some weird and apparently-unnecessary concatenations of consecutive string literals in messages (see the catch blocks).

 Fixed.

- The first sentence in the javadoc for invokeAndEncodeResponse() needs a comma before the phrase beginning with "which could be"; this isn't a nitpick: without the comma it seems like the phrase is referring to the service method that gets called rather than the result that the invocation produces.

Good catch.

Congrats Rob!

Committed as r677

Cheers,

--
Miguel

[Bruce Johnson]

Rob, Miguel, you guys rock.

[Rob Jellinghaus]

No, YOU guys rock. Thanks so much for this. Now I can land my g4jsf
patches and I can charge ahead on my Seam demo.

Now... dare I ask whether it's time to have a CONTRIBUTORS file, with
me in it? (and all of you, I presume, also?)

See this thread: http://groups.google.com/group/Google-Web-Toolkit-Contributors/browse_thread/thread/d288a5a6a3c5eb1a

My reading of it is that you guys did plan to have one, once you had
outside contributors. Well, now you do ;-)

(Sandymac should be able to resubmit his compiler patch too, now that
Scott did the necessary cleanup... correct?)

Cheers!!!!!
Rob

On Mar 21, 10:35 am, "Bruce Johnson" <b...@google.com> wrote:
> Rob, Miguel, you guys rock.
>

> On 3/21/07, Miguel Méndez <mmen...@google.com> wrote:
> > W00T!!!!