Brian Slesinsky has posted comments on this change.
Change subject: Changes StackTraceDeobfuscator to abstract and provide
various factory methods.
......................................................................
Patch Set 1:
(8 comments)
....................................................
File user/src/com/google/gwt/core/server/impl/StackTraceDeobfuscator.java
Line 54: * {@link ClassLoader}.
"that loads symbol map and source map files as resources under the given
resource path. Uses StackTraceObfuscator's ClassLoader."
Line 59: return new StackTraceDeobfuscator() {
Stack traces will be easier to read if you convert these anonymous classes
into nested classes, so it's clearer which one is in use.
Line 72: * Creates a deobfuscator that loads symbol map files from the
given file path.
"loads symbol and source map files from the given directory". Maybe the
argument should be a File?
Line 83: * Creates a deobfuscator that loads symbol map files from the
given url path.
"loads symbol and source map files from a URL beneath the given parent."
This seems rather dicy. We will end up doing two HTTP requests for each
file (one for the symbol map and another for the source map). If the file
doesn't exist, there's only one request, but still someone could use this
API as a way to do a denial-of-service attack on whichever server we're
loading the files from. Maybe leave out this constructor for now?
Line 85: public static StackTraceDeobfuscator fromFileSystem(final URL
urlPath) {
"fromUrl(final URL parentDir)"
Line 98: private static class SymbolCache {
According to GWT's usual stye, nested classes go above static methods.
However, I think it would be better to put the inner classes at the end.
Line 158: * Note that, this has no effect on permutations that the
symbols have already been loaded for.
"This will only have an effect on symbol maps that haven't been loaded yet."
Line 338: protected abstract InputStream openInputStream(String fileName)
throws IOException;
"This method will be called with a filename (not including a slash) for a
symbol map or a source map file and the implementation should open the file
in the appropriate directory, or throw an IOException if the file wasn't
found. The returned InputStream doesn't need to be buffered."
However, maybe we want to do a better job of logging unexpected errors? It
seems like reporting a simple log message (not a full stack trace) for the
first few missing files would make it easier to fix configuration errors,
instead of dropping all IOExceptions silently like we do now. And if it's
not just a missing file then I think it deserves a full stack trace.
So perhaps we should have a specific exception for a normal file not found
error, so we can catch it separately?
--
To view, visit https://gwt-review.googlesource.com/2270
To unsubscribe, visit https://gwt-review.googlesource.com/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I562e052caef8da7f3434319cf11b8984bc347fe5
Gerrit-PatchSet: 1
Gerrit-Project: gwt
Gerrit-Branch: master
Gerrit-Owner: Goktug Gokdogan <gok...@google.com>
Gerrit-Reviewer: Brian Slesinsky <skybr...@google.com>
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 google-web-toolkit-contributors+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/groups/opt_out.
