[gwt-contrib] Re: remove SeriaizableLogRecord and SerializableThrowable (issue875803)

[scottb]

LGTM

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

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

[gwt-contrib] Re: remove SeriaizableLogRecord and SerializableThrowable (issue875803)

[unnurg]

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

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

[gwt-contrib] Re: remove SeriaizableLogRecord and SerializableThrowable (issue875803)

[unnurg]

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

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

[gwt-contrib] Re: remove SeriaizableLogRecord and SerializableThrowable (issue875803)

[unnurg]

comments addressed - although I'm having a couple of issues with the
includes - I'll come and find you to ask in person


http://gwt-code-reviews.appspot.com/875803/diff/1/3
File user/src/com/google/gwt/core/client/impl/SerializableThrowable.java
(right):

http://gwt-code-reviews.appspot.com/875803/diff/1/3#newcode28
user/src/com/google/gwt/core/client/impl/SerializableThrowable.java:28:
public class SerializableThrowable implements IsSerializable {
On 2010/09/27 16:31:28, scottb wrote:

IsSerializable is deprecated, just use java.io.Serializable, right?


Done.

http://gwt-code-reviews.appspot.com/875803/diff/1/3#newcode30
user/src/com/google/gwt/core/client/impl/SerializableThrowable.java:30:
private String message = "";
On 2010/09/27 16:31:28, scottb wrote:

Why empty string rather than null?


Done.

http://gwt-code-reviews.appspot.com/875803/diff/1/3#newcode37
user/src/com/google/gwt/core/client/impl/SerializableThrowable.java:37:
if (t == null) {
On 2010/09/27 16:31:28, scottb wrote:

See comment in LogRecord serializer on removing this null check.


Done.

http://gwt-code-reviews.appspot.com/875803/diff/1/3#newcode73
user/src/com/google/gwt/core/client/impl/SerializableThrowable.java:73:
if (stackTrace != null) {
On 2010/09/27 16:31:28, scottb wrote:

See comment in LogRecord serializer on removing this null check.


Done.

http://gwt-code-reviews.appspot.com/875803/diff/1/5
File user/src/com/google/gwt/logging/client/RemoteLogHandlerBase.java
(right):

http://gwt-code-reviews.appspot.com/875803/diff/1/5#newcode61
user/src/com/google/gwt/logging/client/RemoteLogHandlerBase.java:61:
!excludedLoggerNames.contains(record.getLoggerName()));
On 2010/09/27 16:31:28, scottb wrote:

If it's really ok to do a linear search here, add a comment to that

effect.

Done.

http://gwt-code-reviews.appspot.com/875803/diff/1/17
File
user/src/com/google/gwt/user/client/rpc/core/java/util/logging/LogRecord_CustomFieldSerializer.java
(right):

http://gwt-code-reviews.appspot.com/875803/diff/1/17#newcode38
user/src/com/google/gwt/user/client/rpc/core/java/util/logging/LogRecord_CustomFieldSerializer.java:38:
throws SerializationException {
On 2010/09/27 16:31:28, scottb wrote:

This method should only read Level and Message; just enough to call

the

LogRecord ctor.  The rest should be moved into deserialize().


Done.

http://gwt-code-reviews.appspot.com/875803/diff/1/17#newcode60
user/src/com/google/gwt/user/client/rpc/core/java/util/logging/LogRecord_CustomFieldSerializer.java:60:
writer.writeObject(new SerializableThrowable(lr.getThrown()));
On 2010/09/27 16:31:28, scottb wrote:

If lr.getThrown() is null, just write a null and don't bother wrapping

it.  That

will let you remove some of the new unnecessary null checks in
SerializableThrowable.


Done.

http://gwt-code-reviews.appspot.com/875803/diff/1/17#newcode69
user/src/com/google/gwt/user/client/rpc/core/java/util/logging/LogRecord_CustomFieldSerializer.java:69:
private static class SerializableThrowable2 implements IsSerializable {
On 2010/09/27 16:31:28, scottb wrote:

Don't forget to remove.


Done.

http://gwt-code-reviews.appspot.com/875803/diff/1/18
File user/super/com/google/gwt/emul/java/util/logging/Level.java
(right):

http://gwt-code-reviews.appspot.com/875803/diff/1/18#newcode56
user/super/com/google/gwt/emul/java/util/logging/Level.java:56: }
On 2010/09/27 16:31:28, scottb wrote:

I understand this works for your purposes, but as a practical matter

anyone

trying to serialize Level independently would that that it wouldn't

work at all.

  Either make this class explicitly not serializable in our JRE (and

mark it

transient in LogRecord), or else write a tiny CFS for this class that

does the

correct resolution.


Done.

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

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

[gwt-contrib] Re: remove SeriaizableLogRecord and SerializableThrowable (issue875803)

[unnurg]

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

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

[gwt-contrib] Re: remove SeriaizableLogRecord and SerializableThrowable (issue875803)

[scottb]

http://gwt-code-reviews.appspot.com/875803/diff/1/2
File samples/hello/war/Hello.html (right):

http://gwt-code-reviews.appspot.com/875803/diff/1/2#newcode1
samples/hello/war/Hello.html:1: 
Don't forget to revert.

http://gwt-code-reviews.appspot.com/875803/diff/1/3
File user/src/com/google/gwt/core/client/impl/SerializableThrowable.java
(right):

http://gwt-code-reviews.appspot.com/875803/diff/1/3#newcode28
user/src/com/google/gwt/core/client/impl/SerializableThrowable.java:28:
public class SerializableThrowable implements IsSerializable {
IsSerializable is deprecated, just use java.io.Serializable, right?

http://gwt-code-reviews.appspot.com/875803/diff/1/3#newcode30
user/src/com/google/gwt/core/client/impl/SerializableThrowable.java:30:
private String message = "";
Why empty string rather than null?

http://gwt-code-reviews.appspot.com/875803/diff/1/3#newcode37
user/src/com/google/gwt/core/client/impl/SerializableThrowable.java:37:
if (t == null) {
See comment in LogRecord serializer on removing this null check.

http://gwt-code-reviews.appspot.com/875803/diff/1/3#newcode73
user/src/com/google/gwt/core/client/impl/SerializableThrowable.java:73:
if (stackTrace != null) {
See comment in LogRecord serializer on removing this null check.

http://gwt-code-reviews.appspot.com/875803/diff/1/5
File user/src/com/google/gwt/logging/client/RemoteLogHandlerBase.java
(right):

http://gwt-code-reviews.appspot.com/875803/diff/1/5#newcode61
user/src/com/google/gwt/logging/client/RemoteLogHandlerBase.java:61:
!excludedLoggerNames.contains(record.getLoggerName()));
If it's really ok to do a linear search here, add a comment to that
effect.

http://gwt-code-reviews.appspot.com/875803/diff/1/17
File
user/src/com/google/gwt/user/client/rpc/core/java/util/logging/LogRecord_CustomFieldSerializer.java
(right):

http://gwt-code-reviews.appspot.com/875803/diff/1/17#newcode38
user/src/com/google/gwt/user/client/rpc/core/java/util/logging/LogRecord_CustomFieldSerializer.java:38:
throws SerializationException {
This method should only read Level and Message; just enough to call the
LogRecord ctor.  The rest should be moved into deserialize().

http://gwt-code-reviews.appspot.com/875803/diff/1/17#newcode60
user/src/com/google/gwt/user/client/rpc/core/java/util/logging/LogRecord_CustomFieldSerializer.java:60:
writer.writeObject(new SerializableThrowable(lr.getThrown()));
If lr.getThrown() is null, just write a null and don't bother wrapping
it.  That will let you remove some of the new unnecessary null checks in
SerializableThrowable.

http://gwt-code-reviews.appspot.com/875803/diff/1/17#newcode69
user/src/com/google/gwt/user/client/rpc/core/java/util/logging/LogRecord_CustomFieldSerializer.java:69:
private static class SerializableThrowable2 implements IsSerializable {
Don't forget to remove.

http://gwt-code-reviews.appspot.com/875803/diff/1/18
File user/super/com/google/gwt/emul/java/util/logging/Level.java
(right):

http://gwt-code-reviews.appspot.com/875803/diff/1/18#newcode56
user/super/com/google/gwt/emul/java/util/logging/Level.java:56: }
I understand this works for your purposes, but as a practical matter
anyone trying to serialize Level independently would that that it
wouldn't work at all.  Either make this class explicitly not
serializable in our JRE (and mark it transient in LogRecord), or else
write a tiny CFS for this class that does the correct resolution.

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

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

[gwt-contrib] Re: remove SeriaizableLogRecord and SerializableThrowable (issue875803)

[unnurg]

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

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