[gwt-contrib] Re: Code Review Request: Addition of the Message Transport API
Sorry that it took me a little while to review this code. I had to
refresh my memory on some of the JRE's built-in concurrency features.
That said, I think that I have two overall comments:
1) We should be able to simplify MessageProcessor if we reused some of
the JRE concurrency support.
2) Shouldn't there be a result (if not a Response) for every Request?
Otherwise, couldn't a client hang if a single request on the server
crashes?
If you agree then, I'd propose the following API for MessageTransport:
class MessageTransport {
/*
* Returns the next available request, blocks the caller if one is
not available.available
*/
public Request nextRequest() ...
/*
* Initiates a request send. Notes that this returns a Future which
a caller can use to poll for the response, or block until the response
arrives. Also provides a mechanism for reporting back
* exceptions.
*/
public FutureResponse sendRequest(Request request) ...
}
That API could be implemented using an ExecutorService for sending
(gives you Futures for free) and a thread for reading
requests/response from the input stream. The Callable passed to the
ExecutorService should be able to use a Condition JRE object to wait for
the input thread to signal it when it sees its associated response. If
the input thread sees a request it could simply queue it into a
LinkedBlockingQueue and leave it there.
MessageProcessor could then become more of a MessageDispatcher or
RequestDispatcher. It could pull the next request and dispatch it to the
correct service.
http://gwt-code-reviews.appspot.com/81805/diff/1/2
File
dev/core/src/com/google/gwt/dev/shell/remoteui/MessageProcessor.java
(right):
http://gwt-code-reviews.appspot.com/81805/diff/1/2#newcode52
Line 52: public static interface RequestHandler {
FWIW, you might be able to get rid of this interface if you simply leave
requests in a queue that a called could pull from.
http://gwt-code-reviews.appspot.com/81805/diff/1/2#newcode71
Line 71: private static class ResponseWaiter {
I think that this class could be replaced with the JRE's FutureT
class.
http://gwt-code-reviews.appspot.com/81805/diff/1/2#newcode143
Line 143: public void sendMessage(Message.Request requestMessage) throws
IOException {
As I've been looking at the code, its occurred to me that we must always
have a response. Otherwise a request could hang forever if the request
handler on the other side of the protocol crashes.
http://gwt-code-reviews.appspot.com/81805/diff/1/2#newcode167
Line 167: public Message.Response sendMessageAndWaitForReturn(
I think that you really want to use an ExecutorService here. It will
give you a FutureResponse that a caller can choose to block on or poll
for a result. Also, a FutureResponse gives you a way to pass
exceptions back to the original caller without having to write so much
code.
http://gwt-code-reviews.appspot.com/81805/diff/1/2#newcode200
Line 200: private int allocateNextMessageId() {
I think that you should use AtomicInteger here. That JRE class will
allow to you get rid of this method and the associated locks.
http://gwt-code-reviews.appspot.com/81805/diff/1/2#newcode221
Line 221: private void startMessageReceiverThread() {
It might be cleaner to simply queue requests and let the called pull
them from a LinkedBlockingQueue.
http://gwt-code-reviews.appspot.com/81805/diff/1/3
File
dev/core/src/com/google/gwt/dev/shell/remoteui/MessageTransport.java
(right):
http://gwt-code-reviews.appspot.com/81805/diff/1/3#newcode46
Line 46: private final BufferedOutputStream out;
I don't think that you need to buffer the socket streams since the
Message.parseXXX and Message.writeXXX buffer internally.
http://gwt-code-reviews.appspot.com/81805/diff/1/3#newcode63
Line 63: out = new BufferedOutputStream(socket.getOutputStream());
I don't believe that the buffering is necessary here.
http://gwt-code-reviews.appspot.com/81805/diff/1/3#newcode91
Line 91: private byte[] receive() throws IOException {
I just noticed that, for java, protoc will generate utility methods on
the message classes for reading and writing length delimited messages
into/out of the streams. What's even better is that the length is
written using the streams int32 encoding. You should use those methods
and delete the receive and send methods.
http://gwt-code-reviews.appspot.com/81805
--~--~-~--~~~---~--~~
http://groups.google.com/group/Google-Web-Toolkit-Contributors
-~--~~~~--~~--~--~---
[gwt-contrib] Comment on UsingOOPHM in google-web-toolkit
Comment by tamplinjohn: @gwtdeveloper: I don't know if you can use the plugin to debug XUL or not -- if you can get hosted.html loaded in the context of your XUL app it might work. Let us know if you figure out how to make it work, but that isn't something we have time to experiment with at the moment. I have created a couple of issues for problems being reported here: - Linux issues loading the plugin on particular distributions http://code.google.com/p/google-web-toolkit/issues/detail?id=4141 I have copied over some of them, but not all the older ones since they were before we had official XPIs avaialble. If you are having problems with a combination not listed there, please add a comment to the issue. - Windows Firefox Buffer too small crashes http://code.google.com/p/google-web-toolkit/issues/detail?id=4142 This appears to be a Firefox bug, but perhaps we can work around it anyway see http://support.mozilla.com/en-US/forum/1/462878 For more information: http://code.google.com/p/google-web-toolkit/wiki/UsingOOPHM --~--~-~--~~~---~--~~ http://groups.google.com/group/Google-Web-Toolkit-Contributors -~--~~~~--~~--~--~---
[gwt-contrib] [google-web-toolkit] r6412 committed - Fix SingleJsoImpl hosted mode crash with contravariant return types in...
Revision: 6412
Author: j...@google.com
Date: Sun Oct 18 11:48:45 2009
Log: Fix SingleJsoImpl hosted mode crash with contravariant return types in
virtual
override scenario.
Put simply
class B extends A{}
interface I {
A returnsA();
}
class Jso extends JavaScriptObject {
B returnsA();
}
class JsoSub extends Jso implements I {}
crashes in CCL$MyInstanceMethodOracle.findOriginalDeclaringClass().
Web mode already handles this correctly.
Patch by: bobv
Review by: scottb
http://code.google.com/p/google-web-toolkit/source/detail?r=6412
Modified:
/trunk/dev/core/src/com/google/gwt/dev/shell/CompilingClassLoader.java
/trunk/dev/core/src/com/google/gwt/dev/shell/rewrite/HostedModeClassRewriter.java
/trunk/dev/core/src/com/google/gwt/dev/shell/rewrite/RewriteSingleJsoImplDispatches.java
/trunk/dev/core/src/com/google/gwt/dev/shell/rewrite/WriteJsoImpl.java
/trunk/user/test/com/google/gwt/dev/jjs/test/singlejso/TypeHierarchyTest.java
===
--- /trunk/dev/core/src/com/google/gwt/dev/shell/CompilingClassLoader.java
Fri Sep 25 11:27:03 2009
+++ /trunk/dev/core/src/com/google/gwt/dev/shell/CompilingClassLoader.java
Sun Oct 18 11:48:45 2009
@@ -35,7 +35,9 @@
import com.google.gwt.dev.shell.rewrite.HasAnnotation;
import com.google.gwt.dev.shell.rewrite.HostedModeClassRewriter;
import
com.google.gwt.dev.shell.rewrite.HostedModeClassRewriter.InstanceMethodOracle;
+import
com.google.gwt.dev.shell.rewrite.HostedModeClassRewriter.SingleJsoImplData;
import com.google.gwt.dev.util.JsniRef;
+import com.google.gwt.dev.util.Name;
import com.google.gwt.dev.util.Util;
import com.google.gwt.dev.util.Name.InternalName;
import com.google.gwt.dev.util.Name.SourceOrBinaryName;
@@ -61,9 +63,9 @@
import java.util.List;
import java.util.Map;
import java.util.Set;
-import java.util.SortedMap;
+import java.util.SortedSet;
import java.util.Stack;
-import java.util.TreeMap;
+import java.util.TreeSet;
/**
* An isolated {...@link ClassLoader} for running all user code. All user
files are
@@ -360,21 +362,34 @@
private final MapString, SetJClassType signatureToDeclaringClasses
= new HashMapString, SetJClassType();
public MyInstanceMethodOracle(SetJClassType jsoTypes,
-JClassType javaLangObject) {
- // Populate the map.
+JClassType javaLangObject, SingleJsoImplData jsoData) {
+
+ // Record that the JSO implements its own methods
for (JClassType type : jsoTypes) {
for (JMethod method : type.getMethods()) {
if (!method.isStatic()) {
-String signature = createSignature(method);
-SetJClassType declaringClasses =
signatureToDeclaringClasses.get(signature);
-if (declaringClasses == null) {
- declaringClasses = new HashSetJClassType();
- signatureToDeclaringClasses.put(signature, declaringClasses);
-}
-declaringClasses.add(type);
+assert !method.isAbstract() : Abstract method in JSO type
++ method;
+add(type, method);
}
}
}
+
+ /*
+ * Record the implementing types for methods defined in SingleJsoImpl
+ * interfaces. We have to make this pass because of possible
variance in
+ * the return types between the abstract method declaration in the
+ * interface and the concrete method.
+ */
+ for (String intfName : jsoData.getSingleJsoIntfTypes()) {
+// We only store the name in the data block to keep it lightweight
+JClassType intf =
typeOracle.findType(Name.InternalName.toSourceName(intfName));
+JClassType jso = typeOracle.getSingleJsoImpl(intf);
+for (JMethod method : intf.getMethods()) {
+ add(jso, method);
+}
+ }
+
// Object clobbers everything.
for (JMethod method : javaLangObject.getMethods()) {
if (!method.isStatic()) {
@@ -389,6 +404,7 @@
public String findOriginalDeclaringClass(String desc, String
signature) {
// Lookup the method.
SetJClassType declaringClasses =
signatureToDeclaringClasses.get(signature);
+ assert declaringClasses != null : No classes for + signature;
if (declaringClasses.size() == 1) {
// Shortcut: if there's only one answer, it must be right.
return createDescriptor(declaringClasses.iterator().next());
@@ -412,6 +428,20 @@
throw new IllegalArgumentException(Could not resolve signature '
+ signature + ' from class ' + desc + ');
}
+
+/**
+ * Record that a given JSO type contains the concrete implementation
of a
+ * (possibly abstract) method.
+ */
+private void add(JClassType type, JMethod method) {
+ String signature = createSignature(method);
+ SetJClassType declaringClasses =
signatureToDeclaringClasses.get(signature);
+ if (declaringClasses ==
[gwt-contrib] [google-web-toolkit] r6414 committed - Fix bad error message from NumberFormat.parse....
Revision: 6414
Author: j...@google.com
Date: Sun Oct 18 11:53:33 2009
Log: Fix bad error message from NumberFormat.parse.
Patch by: jat
Review by: jlabanca
http://code.google.com/p/google-web-toolkit/source/detail?r=6414
Modified:
/trunk/user/src/com/google/gwt/i18n/client/NumberFormat.java
/trunk/user/test/com/google/gwt/i18n/client/NumberFormat_en_Test.java
===
--- /trunk/user/src/com/google/gwt/i18n/client/NumberFormat.javaFri May
29
12:15:37 2009
+++ /trunk/user/src/com/google/gwt/i18n/client/NumberFormat.javaSun Oct
18
11:53:33 2009
@@ -1076,8 +1076,12 @@
}
}
-// parseDouble could throw NumberFormatException, let it do it.
-ret = Double.parseDouble(normalizedText.toString());
+// parseDouble could throw NumberFormatException, rethrow with correct
text.
+try {
+ ret = Double.parseDouble(normalizedText.toString());
+} catch (NumberFormatException e) {
+ throw new NumberFormatException(text);
+}
ret = ret / scale;
return ret;
}
===
--- /trunk/user/test/com/google/gwt/i18n/client/NumberFormat_en_Test.java
Tue Jun 23 20:13:51 2009
+++ /trunk/user/test/com/google/gwt/i18n/client/NumberFormat_en_Test.java
Sun Oct 18 11:53:33 2009
@@ -285,6 +285,15 @@
str = NumberFormat.getFormat(#,##0.00;-# X).format(-1014.336);
assertEquals(-1,014.34 X, str);
}
+
+ public void testParseNotANumber() {
+try {
+ double d = NumberFormat.getDecimalFormat().parse(blue);
+ fail(Expected a NumberFormatException);
+} catch (NumberFormatException e) {
+ assertEquals(blue, e.getMessage());
+}
+ }
public void testPercent() {
String str;
--~--~-~--~~~---~--~~
http://groups.google.com/group/Google-Web-Toolkit-Contributors
-~--~~~~--~~--~--~---
[gwt-contrib] [google-web-toolkit] r6415 committed - Don't print stack trace when we lose the remote OOPHM connection....
Revision: 6415
Author: j...@google.com
Date: Sun Oct 18 11:59:29 2009
Log: Don't print stack trace when we lose the remote OOPHM connection.
Patch by: jat
Review by: jlabanca
http://code.google.com/p/google-web-toolkit/source/detail?r=6415
Modified:
/trunk/dev/core/src/com/google/gwt/dev/shell/BrowserChannelServer.java
===
--- /trunk/dev/core/src/com/google/gwt/dev/shell/BrowserChannelServer.java
Sun Oct 18 11:49:24 2009
+++ /trunk/dev/core/src/com/google/gwt/dev/shell/BrowserChannelServer.java
Sun Oct 18 11:59:29 2009
@@ -171,13 +171,9 @@
returnValue.getValue().toString());
}
} catch (IOException e) {
- // TODO(jat): error handling?
- e.printStackTrace();
- throw new HostedModeException(I/O error communicating with client);
+ throw new RemoteDeathError(e);
} catch (BrowserChannelException e) {
- // TODO(jat): error handling?
- e.printStackTrace();
- throw new HostedModeException(I/O error communicating with client);
+ throw new RemoteDeathError(e);
}
}
@@ -187,9 +183,7 @@
jsniMessage.send();
// we do not wait for a return value
} catch (IOException e) {
- // TODO(jat): error handling?
- e.printStackTrace();
- throw new HostedModeException(I/O error communicating with client);
+ throw new RemoteDeathError(e);
}
}
--~--~-~--~~~---~--~~
http://groups.google.com/group/Google-Web-Toolkit-Contributors
-~--~~~~--~~--~--~---
[gwt-contrib] [google-web-toolkit] r6413 committed - Add URL help on warning message for an old plugin....
Revision: 6413
Author: j...@google.com
Date: Sun Oct 18 11:49:24 2009
Log: Add URL help on warning message for an old plugin.
Patch by: jat
Review by: amitmanjhi
http://code.google.com/p/google-web-toolkit/source/detail?r=6413
Modified:
/trunk/dev/core/src/com/google/gwt/dev/shell/BrowserChannelServer.java
===
--- /trunk/dev/core/src/com/google/gwt/dev/shell/BrowserChannelServer.java
Fri Oct 16 20:54:44 2009
+++ /trunk/dev/core/src/com/google/gwt/dev/shell/BrowserChannelServer.java
Sun Oct 18 11:49:24 2009
@@ -16,12 +16,15 @@
package com.google.gwt.dev.shell;
import com.google.gwt.core.ext.TreeLogger;
+import com.google.gwt.core.ext.TreeLogger.HelpInfo;
import com.google.gwt.dev.shell.JsValue.DispatchObject;
import java.io.IOException;
import java.io.InputStream;
import java.io.OutputStream;
+import java.net.MalformedURLException;
import java.net.Socket;
+import java.net.URL;
import java.util.HashMap;
import java.util.Map;
import java.util.Set;
@@ -233,8 +236,27 @@
moduleName = oldLoadModule.getModuleName();
userAgent = oldLoadModule.getUserAgent();
protocolVersion = 1;
+HelpInfo helpInfo = new HelpInfo() {
+ @Override
+ public String getAnchorText() {
+return UsingOOPHM wiki page;
+ }
+
+ @Override
+ public URL getURL() {
+try {
+ // TODO(jat): better landing page for more info
+ return new URL(
+
http://code.google.com/p/google-web-toolkit/wiki/UsingOOPHM;);
+} catch (MalformedURLException e) {
+ // can't happen
+ return null;
+}
+ }
+};
logger.log(TreeLogger.WARN, Connection from old browser plugin
--
-+ please upgrade to a later version for full functionality);
++ please upgrade to a later version for full functionality,
null,
+helpInfo);
break;
case CHECK_VERSIONS:
String connectError = null;
--~--~-~--~~~---~--~~
http://groups.google.com/group/Google-Web-Toolkit-Contributors
-~--~~~~--~~--~--~---
[gwt-contrib] [google-web-toolkit] r6416 committed - Cleans up SOYC options: ...
Revision: 6416
Author: kpro...@google.com
Date: Sun Oct 18 17:47:18 2009
Log: Cleans up SOYC options:
- soyc detailed now also turns on standard soyc.
- SOYC can now also be invoked with -compileReport.
- the -soyc option has been undocumented.
http://code.google.com/p/google-web-toolkit/source/detail?r=6416
Added:
/trunk/dev/core/src/com/google/gwt/dev/util/arg/ArgHandlerCompileReport.java
Modified:
/trunk/dev/core/src/com/google/gwt/dev/Precompile.java
/trunk/dev/core/src/com/google/gwt/dev/util/arg/ArgHandlerSoyc.java
/trunk/dev/core/src/com/google/gwt/dev/util/arg/ArgHandlerSoycDetailed.java
/trunk/dev/core/src/com/google/gwt/dev/util/arg/OptionSoycDetailed.java
===
--- /dev/null
+++
/trunk/dev/core/src/com/google/gwt/dev/util/arg/ArgHandlerCompileReport.java
Sun Oct 18 17:47:18 2009
@@ -0,0 +1,46 @@
+/*
+ * Copyright 2009 Google Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the License); you may
not
+ * use this file except in compliance with the License. You may obtain a
copy of
+ * the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an AS IS BASIS,
WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+ * License for the specific language governing permissions and limitations
under
+ * the License.
+ */
+package com.google.gwt.dev.util.arg;
+
+import com.google.gwt.util.tools.ArgHandlerFlag;
+
+/**
+ * An ArgHandler that enables Story Of Your Compile data-collection.
+ */
+public class ArgHandlerCompileReport extends ArgHandlerFlag {
+
+ private final OptionSoycEnabled options;
+
+ public ArgHandlerCompileReport(OptionSoycEnabled options) {
+this.options = options;
+ }
+
+ @Override
+ public String getPurpose() {
+return Enable Compile Report (Story of Your Compile);
+ }
+
+ @Override
+ public String getTag() {
+return -compileReport;
+ }
+
+ @Override
+ public boolean setFlag() {
+options.setSoycEnabled(true);
+return true;
+ }
+}
===
--- /trunk/dev/core/src/com/google/gwt/dev/Precompile.java Mon Aug 31
14:02:51 2009
+++ /trunk/dev/core/src/com/google/gwt/dev/Precompile.java Sun Oct 18
17:47:18 2009
@@ -43,6 +43,7 @@
import com.google.gwt.dev.util.Memory;
import com.google.gwt.dev.util.PerfLogger;
import com.google.gwt.dev.util.Util;
+import com.google.gwt.dev.util.arg.ArgHandlerCompileReport;
import com.google.gwt.dev.util.arg.ArgHandlerDisableAggressiveOptimization;
import com.google.gwt.dev.util.arg.ArgHandlerDisableCastChecking;
import com.google.gwt.dev.util.arg.ArgHandlerDisableClassMetadata;
@@ -105,6 +106,7 @@
registerHandler(new ArgHandlerDisableUpdateCheck(options));
registerHandler(new ArgHandlerDumpSignatures(options));
registerHandler(new ArgHandlerMaxPermsPerPrecompile(options));
+ registerHandler(new ArgHandlerCompileReport(options));
registerHandler(new ArgHandlerSoyc(options));
registerHandler(new ArgHandlerSoycDetailed(options));
}
===
--- /trunk/dev/core/src/com/google/gwt/dev/util/arg/ArgHandlerSoyc.java Mon
Dec 1 16:51:17 2008
+++ /trunk/dev/core/src/com/google/gwt/dev/util/arg/ArgHandlerSoyc.java Sun
Oct 18 17:47:18 2009
@@ -37,6 +37,11 @@
public String getTag() {
return -soyc;
}
+
+ @Override
+ public boolean isUndocumented() {
+return true;
+ }
@Override
public boolean setFlag() {
===
---
/trunk/dev/core/src/com/google/gwt/dev/util/arg/ArgHandlerSoycDetailed.java
Wed Sep 30 11:55:14 2009
+++
/trunk/dev/core/src/com/google/gwt/dev/util/arg/ArgHandlerSoycDetailed.java
Sun Oct 18 17:47:18 2009
@@ -45,6 +45,7 @@
@Override
public boolean setFlag() {
options.setSoycExtra(true);
+options.setSoycEnabled(true);
return true;
}
}
===
--- /trunk/dev/core/src/com/google/gwt/dev/util/arg/OptionSoycDetailed.java
Tue Aug 4 09:54:58 2009
+++ /trunk/dev/core/src/com/google/gwt/dev/util/arg/OptionSoycDetailed.java
Sun Oct 18 17:47:18 2009
@@ -30,4 +30,10 @@
* information.
*/
void setSoycExtra(boolean soycExtra);
-}
+
+ /**
+ * Sets whether or not the compiler should record and emit SOYC
information
+ * and build the dashboard.
+ */
+ void setSoycEnabled(boolean enabled);
+}
--~--~-~--~~~---~--~~
http://groups.google.com/group/Google-Web-Toolkit-Contributors
-~--~~~~--~~--~--~---
[gwt-contrib] Re: Code Review Request: Addition of the DevModeService Server
http://gwt-code-reviews.appspot.com/80805/diff/1/2
File
dev/core/src/com/google/gwt/dev/shell/remoteui/DevModeServiceServer.java
(right):
http://gwt-code-reviews.appspot.com/80805/diff/1/2#newcode55
Line 55: public void handleRequest(Request request) {
This queues a request, it doesn't necessarily handle it. Should this
method be enqueueRequest or addRequest?
http://gwt-code-reviews.appspot.com/80805/diff/1/2#newcode83
Line 83: System.err.println(Unknown DevModeRequest type:
Should return unknown request type.
http://gwt-code-reviews.appspot.com/80805/diff/1/2#newcode99
Line 99: capabilityExchangeBuilder =
capabilityExchangeBuilder.setCapability(i,
Don't you want to encode these as the enum values instead or their
backing numbers?
http://gwt-code-reviews.appspot.com/80805/diff/1/3
File
dev/core/src/com/google/gwt/dev/shell/remoteui/MessageProcessor.java
(right):
http://gwt-code-reviews.appspot.com/80805/diff/1/3#newcode211
Line 211: public void sendResponseMessage(Message.Response
responseMessage)
FWIW, you would not need this method if at all if processing a request
simply returned a Result object.
http://gwt-code-reviews.appspot.com/80805/diff/1/6
File dev/core/src/com/google/gwt/dev/shell/remoteui/remotemessage.proto
(right):
http://gwt-code-reviews.appspot.com/80805/diff/1/6#newcode187
Line 187: message CapabilityExchange {
You will also want to think about whether there is any other additional
information that you want to send along for each supported response
type.
http://gwt-code-reviews.appspot.com/80805/diff/1/6#newcode188
Line 188: repeated uint32 capability = 1;
You should be able to declare this as a repeates
ViewerRequest.RequestType. That seems to work for me in some other test
code that I have.
http://gwt-code-reviews.appspot.com/80805/diff/1/6#newcode219
Line 219: message CapabilityExchange {
That should work. See comment on line 188.
http://gwt-code-reviews.appspot.com/80805
--~--~-~--~~~---~--~~
http://groups.google.com/group/Google-Web-Toolkit-Contributors
-~--~~~~--~~--~--~---
[gwt-contrib] Re: Code Review Request: Implementation of RemoteUI
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(ListViewerRequestType
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
-~--~~~~--~~--~--~---
