http://gwt-code-reviews.appspot.com/880802/diff/12001/13003
File
samples/logexample/src/com/google/gwt/sample/logexample/client/LogExample.java
(right):

http://gwt-code-reviews.appspot.com/880802/diff/12001/13003#newcode55
samples/logexample/src/com/google/gwt/sample/logexample/client/LogExample.java:55:
});
Reverted for now.  Ray added this to RequestFactory, so I need to fix it
there as well.  And I should probably add it to the codelab at the same
time.  Will send one CL to do this in all 3 places, but don't want to
hold up this patch because of it.

On 2010/09/20 20:18:30, fredsa wrote:
The remainer of the code should be executed in a (documented)
DeferredCommand
lest the UEH will not have taken effect. See
http://code.google.com/p/gwt-dnd/wiki/GettingStarted and search for
onModuleLoad2 as an example

Should also note to the reader that any (static or non-static)
initialization
code in this class could throw an exception before the UEH is
installed, which
means the UEH will not be called in that case

http://gwt-code-reviews.appspot.com/880802/diff/12001/13009
File
user/src/com/google/gwt/logging/server/RemoteLoggingServiceImpl.java
(right):

http://gwt-code-reviews.appspot.com/880802/diff/12001/13009#newcode29
user/src/com/google/gwt/logging/server/RemoteLoggingServiceImpl.java:29:
new StackTraceDeobfuscator("");
On 2010/09/20 20:18:30, fredsa wrote:
Document why we're passing in the empty string

I was just passing a directory that woudn't work to start out with, but
there's no really good reason to do that - initialized as null now for
clarity.

http://gwt-code-reviews.appspot.com/880802/diff/12001/13010
File
user/src/com/google/gwt/logging/server/RemoteLoggingServiceUtil.java
(right):

http://gwt-code-reviews.appspot.com/880802/diff/12001/13010#newcode37
user/src/com/google/gwt/logging/server/RemoteLoggingServiceUtil.java:37:
if (lr == null) {
On 2010/09/20 20:18:30, fredsa wrote:
Why would this be null? Maybe document a possible reason?

Done.

http://gwt-code-reviews.appspot.com/880802/diff/12001/13010#newcode52
user/src/com/google/gwt/logging/server/RemoteLoggingServiceUtil.java:52:
return e.toString();
On 2010/09/20 20:18:30, fredsa wrote:
I'm not a fan of returning just the exception message and giving the
recipient
no way to access the stack trace. Can we do something more useful for
the
caller?

Actually I dont' think this exception is thrown anymore - removed the
whole thing.

http://gwt-code-reviews.appspot.com/880802/diff/12001/13011
File user/src/com/google/gwt/logging/server/StackTraceDeobfuscator.java
(right):

http://gwt-code-reviews.appspot.com/880802/diff/12001/13011#newcode68
user/src/com/google/gwt/logging/server/StackTraceDeobfuscator.java:68:
public void setSymbolMapsDirectory(String dir) {
There really isn't a default directory - setting this to "" was just
leftover from some debugging - removed.

On 2010/09/20 20:18:30, fredsa wrote:
Because symbolMapsDirectory = "", callers can be expected to pass in
the empty
string in order to get the default symbol maps directory. If that's an
expected
"feature" of the API, it should be documented so that even if the
default symbol
maps directory changes, anyone updating this class will know to make
the
corresponding changes here.

http://gwt-code-reviews.appspot.com/880802/diff/12001/13011#newcode69
user/src/com/google/gwt/logging/server/StackTraceDeobfuscator.java:69:
if (!dir.equals(symbolMapsDirectory)) {
This isn't a test to see if we're using the default directory (there's
no such thing), but actually a test to see if the directory has changed.
We read the symbolMaps variable in lazily, so if the directory changes,
we want to reset the symbolMaps variable to null so it gets re-read.
Added some comments to explain this.

On 2010/09/20 20:18:30, fredsa wrote:
[See above comment] In fact, if that is to be a documented feature,
then I'd
expect an explicit test here against the empty string ("") and not
symbolMapsDirectory, which just so happens to be the empty string.

Alternate suggestion: Make passing in null be or use the no-arg ctor
imply the
default directory.

http://gwt-code-reviews.appspot.com/880802/diff/12001/13015
File
user/src/com/google/gwt/requestfactory/client/RequestFactoryLogHandler.java
(right):

http://gwt-code-reviews.appspot.com/880802/diff/12001/13015#newcode57
user/src/com/google/gwt/requestfactory/client/RequestFactoryLogHandler.java:57:
* Level being used app-wide. This handler also takes string swhich it
will
On 2010/09/20 20:18:30, fredsa wrote:
string swhich -> strings, which

Done.

http://gwt-code-reviews.appspot.com/880802/diff/12001/13015#newcode65
user/src/com/google/gwt/requestfactory/client/RequestFactoryLogHandler.java:65:
super(ignoredLoggerSubstrings);
On 2010/09/20 20:18:30, fredsa wrote:
Are substrings robust enough?
Can you include explicit examples of the substrings that one would
pass in and
some common loggers which would be tested?

Changed to full name matching (rather than substrings).  Users of this
class would be defining their own loggers, so it's hard to give an
example of a "common logger" other than than saying "the name of the
logger(s) that will be used to log acknowledgements of activity going
accross the wire".  The only logger defined by the framework is the root
logger...

http://gwt-code-reviews.appspot.com/880802/diff/12001/13016
File user/src/com/google/gwt/requestfactory/server/Logging.java (right):

http://gwt-code-reviews.appspot.com/880802/diff/12001/13016#newcode38
user/src/com/google/gwt/requestfactory/server/Logging.java:38:
RpcRequestBuilder.STRONG_NAME_HEADER);
On 2010/09/20 20:18:30, fredsa wrote:
Should probably assert that the header is not null.

Also, is there a corresponding test to assert that the header is
present?

The case when strongName is null is actually handled by the
StackTraceDeobfuscator class. Added a comment to this effect

http://gwt-code-reviews.appspot.com/880802/diff/12001/13017
File user/src/com/google/gwt/requestfactory/shared/LoggingRequest.java
(right):

http://gwt-code-reviews.appspot.com/880802/diff/12001/13017#newcode30
user/src/com/google/gwt/requestfactory/shared/LoggingRequest.java:30:
RequestObject<String> logMessage(String serializedLogRecordString);
On 2010/09/20 20:18:30, fredsa wrote:
Can you document what return values should be considered success vs.
failure?
What actual string values might one get back?

Done.

http://gwt-code-reviews.appspot.com/880802/show

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

Reply via email to