http://gwt-code-reviews.appspot.com/80806/diff/1/2 File dev/core/src/com/google/gwt/dev/shell/remoteui/DevModeServiceServer.java (right):
http://gwt-code-reviews.appspot.com/80806/diff/1/2#newcode117 Line 117: // TODO: There might be a better abstraction that one could use here. You could have the MessageTransport class callback through an interface which dispatches the request to the correct service and returns the response message back to the MessageTransport class. This class could be called RequestProcessor. I think that it would clean this up some. http://gwt-code-reviews.appspot.com/80806/diff/1/4 File dev/core/src/com/google/gwt/dev/shell/remoteui/RemoteUI.java (right): http://gwt-code-reviews.appspot.com/80806/diff/1/4#newcode46 Line 46: public static class RemoteUIException extends RuntimeException { Do you want to have another constructor overload to pass cause exceptions? http://gwt-code-reviews.appspot.com/80806/diff/1/4#newcode75 Line 75: int topLoggerHandle = viewerServiceClient.addMainLog(); Nit: Move to line 78, before first use. http://gwt-code-reviews.appspot.com/80806/diff/1/4#newcode89 Line 89: int webServerLoggerHandle = viewerServiceClient.addServerLog(serverName, Nit: Move to line 93, before first use. http://gwt-code-reviews.appspot.com/80806/diff/1/4#newcode128 Line 128: int logHandle = -1; The assignment of -1 to logHandle seems like dead code. Is there another reason for it? http://gwt-code-reviews.appspot.com/80806/diff/1/4#newcode146 Line 146: // Copied from SwingUI.loadModule Add a TODO or refactor into DevModeUI. http://gwt-code-reviews.appspot.com/80806/diff/1/5 File dev/core/src/com/google/gwt/dev/shell/remoteui/ViewerServiceClient.java (right): http://gwt-code-reviews.appspot.com/80806/diff/1/5#newcode35 Line 35: * TODO: If this becomes part of the public API, we'll need to provide a level FWIW, this lives in dev mode, I'm not sure if it would become public API. I'd actually recommend that this class be package private. I would expect that a ViewerServiceServer class could be public and part of an API since that is what other tool vendors could use to talk the OOPHM log protocol. http://gwt-code-reviews.appspot.com/80806/diff/1/5#newcode49 Line 49: public ViewerServiceRequestException(String message) { Do you want to ever save an accompanying cause exception? http://gwt-code-reviews.appspot.com/80806/diff/1/5#newcode88 Line 88: ViewerRequest.AddLogBranch.Builder addlogBranchBuilder = ViewerRequest.AddLogBranch.newBuilder().setParentLogHandle( Nit: break up changed expression. Although compact, it is hard to read and not more optimal. Actually, there are several in this file. I'd break them all up unless there is a good reason to do so. http://gwt-code-reviews.appspot.com/80806/diff/1/5#newcode215 Line 215: public int addServerLog(String serverName, byte[] serverIcon) Do we need any other information to be able to render an icon from a set of bytes? Are they self describing or do we need to say: this is a png, an ico, etc? http://gwt-code-reviews.appspot.com/80806/diff/1/5#newcode309 Line 309: private void checkCapability(List<ViewerRequestType> viewerCapabilityList, Shouldn't this be checkForRequiredCapability? The current pattern is that all of the ones that we check for be required, but you will also need the notion of an optional capability. checkForOptionalCapability would save state instead of throwing. http://gwt-code-reviews.appspot.com/80806/diff/1/5#newcode323 Line 323: Response response; Lines 323-332 seem to be repeated in a few places in this code, consider refactoring. http://gwt-code-reviews.appspot.com/80806/diff/1/6 File dev/core/src/com/google/gwt/dev/shell/remoteui/ViewerServiceTreeLogger.java (right): http://gwt-code-reviews.appspot.com/80806/diff/1/6#newcode25 Line 25: private int logHandle = -1; If -1 has special meaning, it would be checked for by clients of the API, then consider using a constant. http://gwt-code-reviews.appspot.com/80806 --~--~---------~--~----~------------~-------~--~----~ http://groups.google.com/group/Google-Web-Toolkit-Contributors -~----------~----~----~----~------~----~------~--~---
