Mostly LGTM

http://gwt-code-reviews.appspot.com/103812/diff/5/1003
File dev/core/src/com/google/gwt/dev/DevMode.java (right):

http://gwt-code-reviews.appspot.com/103812/diff/5/1003#newcode234
Line 234: public static boolean isUsingRemoteUI() {
Why isn't this in DevModeBase, since that is where the property is set?

http://gwt-code-reviews.appspot.com/103812/diff/5/1004
File dev/core/src/com/google/gwt/dev/DevModeBase.java (right):

http://gwt-code-reviews.appspot.com/103812/diff/5/1004#newcode153
Line 153: * Handles the -portHosted command line flag.
Comment should match.

http://gwt-code-reviews.appspot.com/103812/diff/5/1004#newcode155
Line 155: protected static class ArgHandlerCodeServerPort extends
ArgHandlerString {
Wasn't this already done in Miguel's patch and the checkstlye problem
fixed by Bruce?

http://gwt-code-reviews.appspot.com/103812/diff/5/1004#newcode589
Line 589: protected static final String USING_REMOTE_UI_SYSTEM_PROPERTY
= "USING_REMOTE_UI";
Since properties are global, I think this should include GWT in the name
somewhere.

http://gwt-code-reviews.appspot.com/103812/diff/5/1005
File dev/core/src/com/google/gwt/dev/shell/jetty/JettyLauncher.java
(right):

http://gwt-code-reviews.appspot.com/103812/diff/5/1005#newcode85
Line 85: if (DevMode.isUsingRemoteUI()) {
How about setting this above, something like:
TreeLogger.Type normalLogLevel = DevMode.isUsingRemoteUI() ?
TreeLogger.TRACE : TreeLogger.INFO;

and then using that where appropriate?  I think that woud make this
easier to read.

http://gwt-code-reviews.appspot.com/103812

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

Reply via email to