Brian Slesinsky has posted comments on this change.
Change subject: Refactors c.g.gwt.logging to use SerializableThrowable.
......................................................................
Patch Set 1:
(6 comments)
....................................................
File user/src/com/google/gwt/logging/impl/StackTracePrintStream.java
Line 25: public class StackTracePrintStream extends PrintStream {
Good point about devmode. Looking at how it's used, perhaps the subclass
interface could be made clearer by making this class abstract and the
methods final except for the ones that are intended to be overridden.
Something like this:
/**
* Appends some text (not containing a newline) to the output.
*/
protected abstract String append(String text);
/**
* Appends a newline to the output.
*/
protected abstract void newline();
Then all the other methods can call these abstract methods, so it's clear
that all the data will flow through these two methods.
....................................................
File user/src/com/google/gwt/logging/server/JsonLogRecordServerUtil.java
Line 80: return new Throwable(cause) {
I doubt it matters, but out of paranoia, could you introduce a nested class
so it has a name?
....................................................
File
user/src/com/google/gwt/user/client/rpc/core/java/util/logging/LogRecord_CustomFieldSerializer.java
Line 36: SerializableThrowable thrown = (SerializableThrowable)
reader.readObject();
I think we should keep the instanceof check just in case there is a
protocol version mismatch. It's better to return some data than to throw a
ClassCastException.
....................................................
File
user/src/com/google/gwt/user/server/rpc/core/java/util/logging/LogRecord_ServerCustomFieldSerializer.java
Line 35
It seems odd that this class existed when it doesn't do anything different
than LogRecord_CustomFieldSerializer. Maybe we should find out why it was
here? I looked at the history and it wasn't obvious.
....................................................
File user/test/com/google/gwt/logging/client/StackTracePrintStreamTest.java
Line 31: StringBuilder stackTraceBuilder = new StringBuilder();
nit: rename to "actual"?
Line 34: StringBuilder expectedStackTraceBuilder = new StringBuilder();
nit: rename to "expected"
--
To view, visit https://gwt-review.googlesource.com/2310
To unsubscribe, visit https://gwt-review.googlesource.com/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifeee26a9b36cbe40bd1cf0942da8fae39179e985
Gerrit-PatchSet: 1
Gerrit-Project: gwt
Gerrit-Branch: master
Gerrit-Owner: Goktug Gokdogan <[email protected]>
Gerrit-Reviewer: Brian Slesinsky <[email protected]>
Gerrit-Reviewer: Goktug Gokdogan <[email protected]>
Gerrit-Reviewer: Thomas Broyer <[email protected]>
Gerrit-HasComments: Yes
--
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
---
You received this message because you are subscribed to the Google Groups "Google Web Toolkit Contributors" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
For more options, visit https://groups.google.com/groups/opt_out.
