[gwt-contrib] Re: Adds support for List collections. Request methods are now permitted to return (issue893801)
The patch did not apply cleanly. I went through and tried to provide
some feedback anyway.
http://gwt-code-reviews.appspot.com/893801/diff/16001/17001
File
user/src/com/google/gwt/requestfactory/client/impl/AbstractJsonProxyListRequest.java
(right):
http://gwt-code-reviews.appspot.com/893801/diff/16001/17001#newcode22
user/src/com/google/gwt/requestfactory/client/impl/AbstractJsonProxyListRequest.java:22:
import com.google.gwt.requestfactory.shared.SyncResult;
SyncResult has been removed?
http://gwt-code-reviews.appspot.com/893801/diff/16001/17001#newcode54
user/src/com/google/gwt/requestfactory/client/impl/AbstractJsonProxyListRequest.java:54:
public void handleResult(Object jsoResult, SetSyncResult syncResults)
{
1.5-ism: @Override
http://gwt-code-reviews.appspot.com/893801/diff/16001/17002
File
user/src/com/google/gwt/requestfactory/client/impl/AbstractJsonValueListRequest.java
(right):
http://gwt-code-reviews.appspot.com/893801/diff/16001/17002#newcode22
user/src/com/google/gwt/requestfactory/client/impl/AbstractJsonValueListRequest.java:22:
import com.google.gwt.requestfactory.shared.SyncResult;
SyncResult was removed?
http://gwt-code-reviews.appspot.com/893801/diff/16001/17003
File
user/src/com/google/gwt/requestfactory/client/impl/AbstractRequest.java
(right):
http://gwt-code-reviews.appspot.com/893801/diff/16001/17003#newcode197
user/src/com/google/gwt/requestfactory/client/impl/AbstractRequest.java:197:
th...@com.google.gwt.requestfactory.client.impl.abstractrequest::pushToValueStore(Ljava/lang/String;Lcom/google/gwt/core/client/JavaScriptObject;)(schemaAndId[2],
jso);
Was the change to '2' intentional here?
http://gwt-code-reviews.appspot.com/893801/diff/16001/17006
File user/src/com/google/gwt/requestfactory/client/impl/JsoList.java
(right):
http://gwt-code-reviews.appspot.com/893801/diff/16001/17006#newcode217
user/src/com/google/gwt/requestfactory/client/impl/JsoList.java:217:
return (T)
rf.getValueStore().getRecordBySchemaAndId(rf.getSchema(key[2]),
getRecordBySchemaAndId is not applicable for the arguments. Could it be
because the patch did not apply cleanly?
http://gwt-code-reviews.appspot.com/893801/diff/16001/17013
File
user/src/com/google/gwt/requestfactory/shared/impl/CollectionProperty.java
(right):
http://gwt-code-reviews.appspot.com/893801/diff/16001/17013#newcode29
user/src/com/google/gwt/requestfactory/shared/impl/CollectionProperty.java:29:
public class CollectionPropertyC extends Collection, E extends
PropertyC {
Maybe add a comment that this handles Lists and Sets or is that a given?
http://gwt-code-reviews.appspot.com/893801/diff/16001/17014
File user/src/com/google/gwt/requestfactory/shared/impl/TypeLibrary.java
(right):
http://gwt-code-reviews.appspot.com/893801/diff/16001/17014#newcode60
user/src/com/google/gwt/requestfactory/shared/impl/TypeLibrary.java:60:
public static boolean isProxyType(Class? type) {
Should this check for EntityProxy as a super type?
http://gwt-code-reviews.appspot.com/893801/show
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Moved Property and EnumProperty from the (issue881801)
Reviewers: rjrjr, Description: Moved Property and EnumProperty from the com.google.gwt.requestfactory.client.impl package to com.google.gwt.requestfactory.shared.impl package. Please review this at http://gwt-code-reviews.appspot.com/881801/show Affected files: M user/src/com/google/gwt/app/rebind/EditorSupportGenerator.java M user/src/com/google/gwt/requestfactory/client/impl/AbstractRequest.java M user/src/com/google/gwt/requestfactory/client/impl/DeltaValueStoreJsonImpl.java D user/src/com/google/gwt/requestfactory/client/impl/EnumProperty.java D user/src/com/google/gwt/requestfactory/client/impl/Property.java M user/src/com/google/gwt/requestfactory/client/impl/ProxyImpl.java M user/src/com/google/gwt/requestfactory/client/impl/ProxyJsoImpl.java M user/src/com/google/gwt/requestfactory/client/impl/ProxySchema.java M user/src/com/google/gwt/requestfactory/rebind/RequestFactoryGenerator.java M user/src/com/google/gwt/requestfactory/server/JsonRequestProcessor.java A user/src/com/google/gwt/requestfactory/shared/impl/EnumProperty.java A user/src/com/google/gwt/requestfactory/shared/impl/Property.java M user/test/com/google/gwt/editor/rebind/model/EditorModelTest.java M user/test/com/google/gwt/requestfactory/client/impl/SimpleBazProxyImpl.java M user/test/com/google/gwt/requestfactory/client/impl/SimpleFooProxyProperties.java -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Fixed a bug in PropertyColumn introduced by r8780. PropertyColumn needs access to the underlyin... (issue889801)
Reviewers: rjrjr,
Description:
Fixed a bug in PropertyColumn introduced by r8780. PropertyColumn needs
access to the underlying class literals in order to get the value to
render.
Please review this at http://gwt-code-reviews.appspot.com/889801/show
Affected files:
M
samples/expenses/src/main/java/com/google/gwt/sample/expenses/client/ui/employee/EmployeeListView.java
M
samples/expenses/src/main/java/com/google/gwt/sample/expenses/client/ui/report/ReportListView.java
M user/src/com/google/gwt/app/place/PropertyColumn.java
Index:
samples/expenses/src/main/java/com/google/gwt/sample/expenses/client/ui/employee/EmployeeListView.java
===
---
samples/expenses/src/main/java/com/google/gwt/sample/expenses/client/ui/employee/EmployeeListView.java
(revision 8793)
+++
samples/expenses/src/main/java/com/google/gwt/sample/expenses/client/ui/employee/EmployeeListView.java
(working copy)
@@ -56,7 +56,7 @@
columns.add(PropertyColumn.EmployeeProxy
getStringPropertyColumn(password, Password));
columns.add(new PropertyColumnEmployeeProxy, EmployeeProxy(
-supervisor, Supervisor, EmployeeRenderer.instance()));
+supervisor, Supervisor, EmployeeProxy.class,
EmployeeRenderer.instance()));
return columns;
}
Index:
samples/expenses/src/main/java/com/google/gwt/sample/expenses/client/ui/report/ReportListView.java
===
---
samples/expenses/src/main/java/com/google/gwt/sample/expenses/client/ui/report/ReportListView.java
(revision 8793)
+++
samples/expenses/src/main/java/com/google/gwt/sample/expenses/client/ui/report/ReportListView.java
(working copy)
@@ -56,16 +56,16 @@
ListPropertyColumnReportProxy, ? columns = new
ArrayListPropertyColumnReportProxy, ?();
-columns.add(new PropertyColumnReportProxy, Date(created, Created,
+columns.add(new PropertyColumnReportProxy,
Date(created, Created, Date.class,
new DateTimeFormatRenderer(DateTimeFormat.getShortDateFormat(;
columns.add(PropertyColumn.ReportProxy
getStringPropertyColumn(purpose, Purpose));
columns.add(new PropertyColumnReportProxy, EmployeeProxy(
-reporter, Reporter, EmployeeRenderer.instance()));
+reporter, Reporter, EmployeeProxy.class,
EmployeeRenderer.instance()));
columns.add(new PropertyColumnReportProxy, EmployeeProxy(
-approvedSupervisor, Approved Supervisor Key,
EmployeeRenderer.instance()));
+approvedSupervisor, Approved Supervisor Key,
EmployeeProxy.class, EmployeeRenderer.instance()));
return columns;
}
Index: user/src/com/google/gwt/app/place/PropertyColumn.java
===
--- user/src/com/google/gwt/app/place/PropertyColumn.java (revision 8793)
+++ user/src/com/google/gwt/app/place/PropertyColumn.java (working copy)
@@ -38,24 +38,27 @@
public static R extends EntityProxy PropertyColumnR, String
getStringPropertyColumn(
String property, String displayName) {
return new PropertyColumnR, String(property, displayName,
-PassthroughRenderer.instance());
+String.class, PassthroughRenderer.instance());
}
+ private final ClassT clazz;
private String displayName;
private final RendererT renderer;
private final String property;
private final String[] paths;
- public PropertyColumn(String property, String displayName,
ProxyRendererT renderer) {
+ public PropertyColumn(String property, String displayName, ClassT
clazz, ProxyRendererT renderer) {
this.displayName = displayName;
this.property = property;
+this.clazz = clazz;
this.renderer = renderer;
this.paths = pathinate(property, renderer);
}
- public PropertyColumn(String property, String displayName, RendererT
renderer) {
+ public PropertyColumn(String property, String displayName, ClassT
clazz, RendererT renderer) {
this.displayName = displayName;
this.property = property;
+this.clazz = clazz;
this.renderer = renderer;
this.paths = new String[] {property};
}
@@ -71,7 +74,7 @@
@Override
public String getValue(R object) {
ProxyImpl proxyImpl = (ProxyImpl) object;
-return renderer.render(proxyImpl.Tget(property, String.class));
+return renderer.render(proxyImpl.Tget(property, clazz));
}
private String[] pathinate(String property, ProxyRendererT renderer) {
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Fixes a couple of build breaks in the samples/expenses app. (issue872801)
Reviewers: rjrjr, Description: Fixes a couple of build breaks in the samples/expenses app. Review by: rj...@google.com Please review this at http://gwt-code-reviews.appspot.com/872801/show Affected files: M samples/expenses/src/main/java/com/google/gwt/sample/expenses/client/ExpenseDetails.java M samples/expenses/src/main/java/com/google/gwt/sample/expenses/client/ExpenseTree.java M samples/expenses/src/main/java/com/google/gwt/sample/expenses/client/MobileExpenseEntry.java M samples/expenses/src/main/java/com/google/gwt/sample/expenses/client/MobileReportEntry.java -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Fixes https://jira.springsource.org/browse/ROO-1213 - Hide Property from public API. (issue842803)
http://gwt-code-reviews.appspot.com/842803/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Fixes https://jira.springsource.org/browse/ROO-1213 - Hide Property from public API. (issue842803)
http://gwt-code-reviews.appspot.com/842803/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Fixes https://jira.springsource.org/browse/ROO-1213 - Hide Property from public API. (issue842803)
Reviewers: amitmanjhi, Description: Fixes https://jira.springsource.org/browse/ROO-1213 - Hide Property from public API. Properties are now computed from the *Proxy classes and emitted into the generated *ProxyImpl classes. Server side code will infer what the properties should be by reflecting over the *Proxy classes looking for bean-like property methods. Review by: amitman...@google.com Please review this at http://gwt-code-reviews.appspot.com/842803/show Affected files: M samples/dynatablerf/src/com/google/gwt/sample/dynatablerf/shared/AddressProxy.java M samples/dynatablerf/src/com/google/gwt/sample/dynatablerf/shared/PersonProxy.java M user/src/com/google/gwt/app/place/PropertyColumn.java M user/src/com/google/gwt/app/rebind/EditorSupportGenerator.java M user/src/com/google/gwt/requestfactory/client/impl/AbstractRequest.java M user/src/com/google/gwt/requestfactory/client/impl/DeltaValueStoreJsonImpl.java A user/src/com/google/gwt/requestfactory/client/impl/Property.java M user/src/com/google/gwt/requestfactory/client/impl/ProxyImpl.java M user/src/com/google/gwt/requestfactory/client/impl/ProxyJsoImpl.java M user/src/com/google/gwt/requestfactory/client/impl/ProxySchema.java M user/src/com/google/gwt/requestfactory/rebind/RequestFactoryGenerator.java M user/src/com/google/gwt/requestfactory/server/JsonRequestProcessor.java M user/src/com/google/gwt/requestfactory/shared/EntityProxy.java M user/src/com/google/gwt/requestfactory/shared/EnumProperty.java M user/src/com/google/gwt/requestfactory/shared/Id.java D user/src/com/google/gwt/requestfactory/shared/Property.java M user/src/com/google/gwt/requestfactory/shared/PropertyReference.java M user/src/com/google/gwt/requestfactory/shared/ProxyListRequest.java M user/src/com/google/gwt/requestfactory/shared/ProxyRequest.java M user/src/com/google/gwt/requestfactory/shared/UserInformationProxy.java M user/src/com/google/gwt/requestfactory/shared/Version.java M user/test/com/google/gwt/editor/rebind/model/EditorModelTest.java M user/test/com/google/gwt/requestfactory/client/impl/DeltaValueStoreJsonImplTest.java M user/test/com/google/gwt/requestfactory/client/impl/ProxyJsoImplTest.java M user/test/com/google/gwt/requestfactory/client/impl/SimpleBazProxyImpl.java A user/test/com/google/gwt/requestfactory/client/impl/SimpleFooProxyProperties.java M user/test/com/google/gwt/requestfactory/shared/SimpleBarProxy.java M user/test/com/google/gwt/requestfactory/shared/SimpleFooProxy.java -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Faster edit-distance computation in JsFunctionClusterer (issue669801)
Is there anything else left to do on this change? Alex's internship is
over, but we'd like to get the change in.
On 2010/08/10 08:44:14, zundel wrote:
Hi Andre, I'm waiting on Ray C or Lex to give the LGTM, but I noticed
this last patch you uploaded left out the editdistance library files.
On Mon, Aug 9, 2010 at 4:48 PM, mailto:avassalo...@google.com
wrote:
http://gwt-code-reviews.appspot.com/669801/diff/33001/34006
File
dev/core/src/com/google/gwt/dev/jjs/impl/JsFunctionClusterer.java
(right):
http://gwt-code-reviews.appspot.com/669801/diff/33001/34006#newcode43
dev/core/src/com/google/gwt/dev/jjs/impl/JsFunctionClusterer.java:43:
Pattern.compile(function |[_a-zA-Z$][.$_a-zA-Z0-9]*=function);
On 2010/08/06 02:12:25, cromwellian wrote:
This regex will incorrect match vtable declarations of the form:
_.name = function() { ... }
These prototype declarations cannot be re-ordered, so don't let it
match a
single '_' symbol.
Fixed.
http://gwt-code-reviews.appspot.com/669801/show
--
Eric Z. Ayers
Google Web Toolkit, Atlanta, GA USA
http://gwt-code-reviews.appspot.com/669801/show
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Removed use of a global table (typeIdArray) for testing castability between types. This informa... (issue750801)
On 2010/08/12 14:21:14, bobv wrote: @Miguel, FWIW, since we the time bounds of further optimizing this patch are not yet clear, I'd be in favor of landing it and then doing a second round of optimizations. This patch achieves its stated goal, although there are some code style issues that need to be addressed. Okay. Lets definitely address the code and style issues before committing this patch. Thanks for calling them out Bob. http://gwt-code-reviews.appspot.com/750801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Made the error messages thrown in case of GWTTestCase timeouts more informative, as requested in (issue734801)
http://gwt-code-reviews.appspot.com/734801/diff/1/2 File user/src/com/google/gwt/junit/JUnitShell.java (right): http://gwt-code-reviews.appspot.com/734801/diff/1/2#newcode1019 user/src/com/google/gwt/junit/JUnitShell.java:1019: + Try increasing this timeout using the '-testMethodTimeout number' option\n); Since the unit expected is not clear from the argument's name, I'd use 'seconds' or 'minutes' instead of number. This applies to your message below as well. Is testMethodTimeout specified in seconds or minutes? http://gwt-code-reviews.appspot.com/734801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Undo the collapse-all-permutations in JUnit.gwt.xml for now. (issue457801)
LGTM Let's circle back on this optimization that we have undone once we stabilize. http://gwt-code-reviews.appspot.com/457801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Removing the batch argument from test targets in the user build file. We can specify it using te... (issue328802)
LGTM
http://gwt-code-reviews.appspot.com/328802/diff/1/2
File user/build.xml (right):
http://gwt-code-reviews.appspot.com/328802/diff/1/2#newcode161
user/build.xml:161: test.args=${test.web.remote.args} -out www -prod
-standardsMode -runStyle RemoteWeb:${gwt.hosts.web.remote}
Where are Xtries added again? I guess that this will make our tests run
slower?
http://gwt-code-reviews.appspot.com/328802/show
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Code Review Request: Add better handling of failure to connect to remote UI
LGTM http://gwt-code-reviews.appspot.com/111802 -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Code Review Request: Add Info Message To Indicate When a Module Load is Complete
LGTM http://gwt-code-reviews.appspot.com/112802/diff/1/2 File dev/core/src/com/google/gwt/dev/shell/OophmSessionHandler.java (right): http://gwt-code-reviews.appspot.com/112802/diff/1/2#newcode185 Line 185: moduleHandle.getLogger().log(TreeLogger.INFO, Nit: Should this be Module XXX was loaded? http://gwt-code-reviews.appspot.com/112802 -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Move setStartupURLs earlier, add launch support in Swing UI
http://gwt-code-reviews.appspot.com/111801/diff/1/3
File dev/core/src/com/google/gwt/dev/DevModeBase.java (right):
http://gwt-code-reviews.appspot.com/111801/diff/1/3#newcode1053
Line 1053: for (String prenormalized : options.getStartupURLs()) {
If no startupURLs are specified on the command line, would it be
possible to compute them here? That way GPE won't need to and all UIs
would benefit.
http://gwt-code-reviews.appspot.com/111801
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Move setStartupURLs earlier, add launch support in Swing UI
http://gwt-code-reviews.appspot.com/111801/diff/1/3
File dev/core/src/com/google/gwt/dev/DevModeBase.java (right):
http://gwt-code-reviews.appspot.com/111801/diff/1/3#newcode1053
Line 1053: for (String prenormalized : options.getStartupURLs()) {
In the event that no -startupURLs were provided couldn't GWT compute a
set of startup URLs? The plugin would not need to have any logic for
adding additional -startupURLs on the commandline and all UIs would
benefit. All UIs are going to have to help the user determine the URLs.
-noserver is interesting, and you are right that it would not know how
to compute the URLs, but in that case GPE would not know how to compute
them either unless a user specified the -startupURLs on the command
line. The 90% case is local server so I'd think that GWT could compute
these.
On 2009/11/23 15:07:42, jat wrote:
On 2009/11/23 14:57:09, mmendez wrote:
If no startupURLs are specified on the command line, would it be
possible to
compute them here? That way GPE won't need to and all UIs would
benefit.
Where would it get them from?
For GPE, Rajeev (in a separate thread) said that it would now
synthesize
-startupUrl args if none were supplied by the user rather than trying
to merge
them in later. That seems a better solution to me.
Also, consider the -noserver case where it has no idea where your
server is or
what web pages it has access to.
http://gwt-code-reviews.appspot.com/111801/diff/1/3#newcode1053
Line 1053: for (String prenormalized : options.getStartupURLs()) {
It is the same issue for the plugin, but I think that the SDK would, by
definition, do a better job. As the algorithm improves everyone would
benefit (all UI implementations).
I'm not a big fan of the magic arguments in the plugin generated command
line because they hinder understanding.
As far as -noserver is concerned, the plugin has the same issue. In
that case, it can't generate a proper startup URL unless the user tells
it. The common case is not -noserver, so I'm not sure it is a good idea
to strike this idea based on that one case.
On 2009/11/23 15:07:42, jat wrote:
On 2009/11/23 14:57:09, mmendez wrote:
If no startupURLs are specified on the command line, would it be
possible to
compute them here? That way GPE won't need to and all UIs would
benefit.
Where would it get them from?
For GPE, Rajeev (in a separate thread) said that it would now
synthesize
-startupUrl args if none were supplied by the user rather than trying
to merge
them in later. That seems a better solution to me.
Also, consider the -noserver case where it has no idea where your
server is or
what web pages it has access to.
http://gwt-code-reviews.appspot.com/111801
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Code Review Request: Send startup URLs over the wire to GPE
LGTM
http://gwt-code-reviews.appspot.com/111807/diff/1/5
File dev/core/src/com/google/gwt/dev/shell/remoteui/RemoteUI.java
(right):
http://gwt-code-reviews.appspot.com/111807/diff/1/5#newcode169
Line 169: public void setStartupUrls(MapString, URL urls) {
Nit: why was this a Map again?
http://gwt-code-reviews.appspot.com/111807
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Code Review Request: Tweaks to enable restart server functionality
LGTM http://gwt-code-reviews.appspot.com/109801 -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Code Review Request: Drop Request/Response Logging Levels down to TRACE when using the Remote UI
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() {
I agree that the code should be in DevModeBase and that we should
consider not using a system property.
The approach being considered here is whether we can make some of the
log levels dynamic based on the UI (or other criteria). That way the
Swing UI can use the log levels that it wants and the RemoteUI (i.e. the
Eclipse view) can get a quieter environment.
One possibility is to modify DevModeBase.createUI to get the currently
configured SCL and set some default log level on it if it is the
JettyLauncher. Or something that enables the JettyLauncher to delegate
some of its log level selections to it.
If we do this right, we might be able to extend this approach to
introduce similar dynamic log level determination based on ui into
r7003.
On 2009/11/19 00:48:47, jat wrote:
Why isn't this in DevModeBase, since that is where the property is
set?
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()) {
If we were to set a default log level for status messages on the SCL you
could just initialize logStatus with that value and only apply your
override behavior here.
On 2009/11/19 00:48:47, jat wrote:
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
[gwt-contrib] Re: Finish hosted server = code server changes
LGTM with one nit. Consider removing the @param or @throws tags that have no associated explanation or filling that explanation in. http://gwt-code-reviews.appspot.com/105802 -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Renamed portHosted to codeServerPort
Reviewers: rdayal, jat, Please review this at http://gwt-code-reviews.appspot.com/103810 Affected files: M dev/core/src/com/google/gwt/dev/DevModeBase.java M user/src/com/google/gwt/junit/JUnitShell.java -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Tweak log levels; don't log launch URLs if using remote UI
Reviewers: rdayal, jat, Please review this at http://gwt-code-reviews.appspot.com/104810 Affected files: M dev/core/src/com/google/gwt/dev/DevMode.java M dev/core/src/com/google/gwt/dev/DevModeBase.java M dev/core/src/com/google/gwt/dev/shell/BrowserListener.java M dev/core/src/com/google/gwt/dev/shell/jetty/JettyLauncher.java M dev/core/src/com/google/gwt/dev/shell/remoteui/RemoteUI.java -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Renamed portHosted to codeServerPort
Thanks guys.
http://gwt-code-reviews.appspot.com/103810/diff/1/2
File dev/core/src/com/google/gwt/dev/DevModeBase.java (right):
http://gwt-code-reviews.appspot.com/103810/diff/1/2#newcode263
Line 263: private static final String CODE_SERVER_PORT_TAG =
-codeServerPort;
Actually, the name change has his suggestion. Thanks for pointing it
out though.
On 2009/11/18 21:09:15, jat wrote:
When we first added -portHosted, there was debate between that and
-hostedPort
and we settled on the former for parallelism with -port. However, I
am ok
either way, just wanted to call out that was discussed before.
http://gwt-code-reviews.appspot.com/103810/diff/1/2#newcode272
Line 272: return new String[] {CODE_SERVER_PORT_TAG, 9997};
That is a good point on the 9997. I'll include it in the final commit.
I suspect that portHosted has the same issue.
On 2009/11/18 21:09:57, rdayal wrote:
Could extract a constant for this, since you use it about 3 lines
below.
http://gwt-code-reviews.appspot.com/103810
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Tweak log levels; don't log launch URLs if using remote UI
Thanks guys.
http://gwt-code-reviews.appspot.com/104810/diff/1/2
File dev/core/src/com/google/gwt/dev/DevMode.java (right):
http://gwt-code-reviews.appspot.com/104810/diff/1/2#newcode323
Line 323: Loading modules);
I can understand that, but is logging the best way to deal with this?
On 2009/11/18 21:21:28, jat wrote:
Loading the modules can take significant time, such as with large
apps. If you
don't have this logging visible, there is no information about what is
going on.
http://gwt-code-reviews.appspot.com/104810/diff/1/3
File dev/core/src/com/google/gwt/dev/DevModeBase.java (right):
http://gwt-code-reviews.appspot.com/104810/diff/1/3#newcode736
Line 736: if (startUp() !options.useRemoteUI()) {
When that lands we can revisit this change. We don't want any noise
coming out of the console when using the remote UI in the meantime.
On 2009/11/18 21:21:28, jat wrote:
I do not think it is appropriate to put a check like this here.
I believe what you want to accomplish will be done with the
DevModeUi.setStartupUrls call I am adding per our discussion.
http://gwt-code-reviews.appspot.com/104810/diff/1/4
File dev/core/src/com/google/gwt/dev/shell/BrowserListener.java (right):
http://gwt-code-reviews.appspot.com/104810/diff/1/4#newcode73
Line 73: TreeLogger branch = logger.branch(TreeLogger.TRACE,
Not sure that I fully parsed that. Are you saying that creating the
module logger before all of this will allow us to use a higher log
level?
Are you saying that it will be hard to figure out what is going between
the time that a browser hits the code server and the module is loaded?
And that this log item fills that void?
On 2009/11/18 21:21:28, jat wrote:
I am not sure I agree with this, but I am willing to do it and see how
it works
out.
Previously, it was very useful to have this information separate from
the module
starting up when using unusual linkers that might interfere with the
module
startup process. Without this, you have to change the log level or
break out
something like wireshark to see network traffic. As we push more
things down
into TRACE, that becomes less viable.
This isn't adding much to the output, and as I understand it Eclipse
focuses on
the last thing that had output -- so the creation of the module logger
should
switch to that right?
http://gwt-code-reviews.appspot.com/104810/diff/1/5
File dev/core/src/com/google/gwt/dev/shell/jetty/JettyLauncher.java
(right):
http://gwt-code-reviews.appspot.com/104810/diff/1/5#newcode148
Line 148: logger.log(TreeLogger.TRACE, format(msg, arg0, arg1));
There are other log context methods that can be used. But yes, we want
this thing to be nice and quiet.
On 2009/11/18 21:21:28, jat wrote:
I believe this will remove all web server logging without lowering the
log
level, and I think that is totally wrong. It is very useful to see
the web
requests that have been served by the embedded web server.
I suggest if you want to suppress it for GPE, have
RemoteUi.getWebServerLogger
return a PrintWriterTreeLogger with the log level set to WARN (or the
user's log
level if higher).
http://gwt-code-reviews.appspot.com/104810
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Fixes checkstyle issues introduced by r7001.
Reviewers: rdayal, jat,
Please review this at http://gwt-code-reviews.appspot.com/103813
Affected files:
M dev/core/src/com/google/gwt/dev/DevModeBase.java
Index: dev/core/src/com/google/gwt/dev/DevModeBase.java
diff --git a/dev/core/src/com/google/gwt/dev/DevModeBase.java
b/dev/core/src/com/google/gwt/dev/DevModeBase.java
index
511d5458f848e1493c608c267ba5840971ae98eb..4bb36ecc206b3a9d83999be4cb8bb2ad4a279d95
100644
--- a/dev/core/src/com/google/gwt/dev/DevModeBase.java
+++ b/dev/core/src/com/google/gwt/dev/DevModeBase.java
@@ -429,6 +429,10 @@ abstract class DevModeBase implements DoneCallback {
return remoteUIClientId;
}
+public int getCodeServerPort() {
+ return portHosted;
+}
+
public File getLogDir() {
return logDir;
}
@@ -444,10 +448,6 @@ abstract class DevModeBase implements DoneCallback {
return port;
}
-public int getCodeServerPort() {
- return portHosted;
-}
-
public String getRemoteUIHost() {
return remoteUIHost;
}
@@ -472,6 +472,10 @@ abstract class DevModeBase implements DoneCallback {
this.remoteUIClientId = clientId;
}
+public void setCodeServerPort(int port) {
+ portHosted = port;
+}
+
public void setLogFile(String filename) {
logDir = new File(filename);
}
@@ -484,10 +488,6 @@ abstract class DevModeBase implements DoneCallback {
this.port = port;
}
-public void setCodeServerPort(int port) {
- portHosted = port;
-}
-
public void setRemoteUIHost(String remoteUIHost) {
this.remoteUIHost = remoteUIHost;
}
@@ -502,6 +502,15 @@ abstract class DevModeBase implements DoneCallback {
}
/**
+ * Controls what code server port to use.
+ */
+ protected interface OptionCodeServerPort {
+int getCodeServerPort();
+
+void setCodeServerPort(int codeServerPort);
+ }
+
+ /**
* Controls whether and where to log data to file.
*
*/
@@ -535,12 +544,6 @@ abstract class DevModeBase implements DoneCallback {
void setPort(int port);
}
- protected interface OptionCodeServerPort {
-int getCodeServerPort();
-
-void setCodeServerPort(int codeServerPort);
- }
-
/**
* Controls the UI that should be used to display the dev mode server's
data.
*/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Code Review Request: Add start() method to MessageTransport
LGTM Make sure to run the updates against the plugin as a cross-check. http://gwt-code-reviews.appspot.com/102813 -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Close MessageTransport socket on error
Reviewers: , Please review this at http://gwt-code-reviews.appspot.com/102803 Affected files: M dev/core/src/com/google/gwt/dev/shell/remoteui/MessageTransport.java M dev/core/src/com/google/gwt/dev/shell/remoteui/RemoteUI.java M dev/core/test/com/google/gwt/dev/shell/remoteui/MessageTransportTest.java --~--~-~--~~~---~--~~ http://groups.google.com/group/Google-Web-Toolkit-Contributors -~--~~~~--~~--~--~---
[gwt-contrib] Initialize UI early; avoids bus error when using GWT 2.0 tip and App Engine 1.2.6 in a project
Reviewers: jat,
Please review this at http://gwt-code-reviews.appspot.com/102805
Affected files:
M dev/core/src/com/google/gwt/dev/DevModeBase.java
Index: dev/core/src/com/google/gwt/dev/DevModeBase.java
diff --git a/dev/core/src/com/google/gwt/dev/DevModeBase.java
b/dev/core/src/com/google/gwt/dev/DevModeBase.java
index
1e4bf5beac6f0229f8344178dbe54c36f099d217..910f4223ad6cf4fc352a18f397511958b29668f5
100644
--- a/dev/core/src/com/google/gwt/dev/DevModeBase.java
+++ b/dev/core/src/com/google/gwt/dev/DevModeBase.java
@@ -720,10 +720,10 @@ abstract class DevModeBase implements DoneCallback {
*/
public final void run() {
try {
+ // Eager AWT init for OS X to ensure safe coexistence with SWT.
+ BootStrapPlatform.initGui();
+
if (startUp()) {
-// Eager AWT init for OS X to ensure safe coexistence with SWT.
-BootStrapPlatform.initGui();
-
// The web server is running now, so launch browsers for startup
urls.
launchStartupUrls(getTopLogger());
}
--~--~-~--~~~---~--~~
http://groups.google.com/group/Google-Web-Toolkit-Contributors
-~--~~~~--~~--~--~---
[gwt-contrib] Re: Close MessageTransport socket on error
Thanks Rajeev! Also included Scott's assertion message improvements.
Committed as r6859.
http://gwt-code-reviews.appspot.com/102803/diff/1/3
File dev/core/src/com/google/gwt/dev/shell/remoteui/RemoteUI.java
(right):
http://gwt-code-reviews.appspot.com/102803/diff/1/3#newcode129
Line 129: + e.getLocalizedMessage());
Switched to e.toString() instead.
On 2009/11/11 21:51:44, rdayal wrote:
Not part of your patch, but could you put a null guard here? Though it
never
happens now, it may be the case that a termination occurs without any
exception.
http://gwt-code-reviews.appspot.com/102803/diff/1/3#newcode134
Line 134: try {
I'm fine to move it, but is there a reason that the location matters?
It would be good to add an explanation.
On 2009/11/11 21:49:53, rdayal wrote:
Do this before telling the Dev Mode server that its okay to shut down.
http://gwt-code-reviews.appspot.com/102803
--~--~-~--~~~---~--~~
http://groups.google.com/group/Google-Web-Toolkit-Contributors
-~--~~~~--~~--~--~---
[gwt-contrib] Re: Better assertions in MessageTransportTest
LGTM Included in r6859. Thanks Scott. http://gwt-code-reviews.appspot.com/100805 --~--~-~--~~~---~--~~ http://groups.google.com/group/Google-Web-Toolkit-Contributors -~--~~~~--~~--~--~---
[gwt-contrib] Re: Code Review Request: Fix DevMode Server Hang on Unclean Shutdown of Remote UI
LGTM http://gwt-code-reviews.appspot.com/100801 --~--~-~--~~~---~--~~ http://groups.google.com/group/Google-Web-Toolkit-Contributors -~--~~~~--~~--~--~---
[gwt-contrib] Re: Code Review Request: Make Server and Main TreeLoggers log to Console when using RemoteUI
LGTM - Just double check what state the system is in if you throw the IllegalStateExceptions. http://gwt-code-reviews.appspot.com/98801/diff/1/3 File dev/core/src/com/google/gwt/dev/shell/remoteui/RemoteUI.java (right): http://gwt-code-reviews.appspot.com/98801/diff/1/3#newcode119 Line 119: throw new IllegalStateException( Sure, that you want to throw IllegalStateException? What happens? http://gwt-code-reviews.appspot.com/98801/diff/1/3#newcode125 Line 125: throw new IllegalStateException( Same comment as line 119. http://gwt-code-reviews.appspot.com/98801 --~--~-~--~~~---~--~~ http://groups.google.com/group/Google-Web-Toolkit-Contributors -~--~~~~--~~--~--~---
[gwt-contrib] Re: Code Review Request: Addition of clientId and initialization message for the RemoteUI
LGTM - please run ant checkstyle before committing. Also, cherrypick into the 2.0 branch. On 2009/10/30 16:23:34, rdayal wrote: http://gwt-code-reviews.appspot.com/89809 --~--~-~--~~~---~--~~ http://groups.google.com/group/Google-Web-Toolkit-Contributors -~--~~~~--~~--~--~---
[gwt-contrib] Re: mmen...@google.com
LGTM We used to be able to build typeoracles dynamically. If that is still possible, you should add a unit test that generates twice and compares the delta. http://gwt-code-reviews.appspot.com/86806 --~--~-~--~~~---~--~~ http://groups.google.com/group/Google-Web-Toolkit-Contributors -~--~~~~--~~--~--~---
[gwt-contrib] Re: Code Review Request: Using the rebased protobuf library
LGTM http://gwt-code-reviews.appspot.com/83809 --~--~-~--~~~---~--~~ http://groups.google.com/group/Google-Web-Toolkit-Contributors -~--~~~~--~~--~--~---
[gwt-contrib] Re: Code Review Request: Addition of the Message Transport API
I found a couple of failures on my OSX box last night. Let's look at them together. http://gwt-code-reviews.appspot.com/81805/diff/2003/2007 File dev/core/test/com/google/gwt/dev/shell/remoteui/MessageTransportTest.java (right): http://gwt-code-reviews.appspot.com/81805/diff/2003/2007#newcode110 Line 110: MessageTransport messageTransport = new MessageTransport( Nit: messageTransport - Unused local. http://gwt-code-reviews.appspot.com/81805/diff/2003/2007#newcode151 Line 151: public void testExecuteAsyncRequestWithClosedSendStream() throws IOException, This fails sporadically for me on OSX JRE 1.5.0, but not OSX JRE 1.6.0. http://gwt-code-reviews.appspot.com/81805/diff/2003/2007#newcode322 Line 322: public void testExecuteRequestAsyncWithClosedReceiveStreamBeforeResponse() This fails sporadically for me on OSX JRE 1.5.0, but not OSX JRE 1.6.0. http://gwt-code-reviews.appspot.com/81805 --~--~-~--~~~---~--~~ http://groups.google.com/group/Google-Web-Toolkit-Contributors -~--~~~~--~~--~--~---
[gwt-contrib] Re: Code Review Request: Addition of the Message Transport API
LGTM http://gwt-code-reviews.appspot.com/81805 --~--~-~--~~~---~--~~ http://groups.google.com/group/Google-Web-Toolkit-Contributors -~--~~~~--~~--~--~---
[gwt-contrib] Re: Code Review Request: Addition of the DevModeService Server
LGTM http://gwt-code-reviews.appspot.com/80805 --~--~-~--~~~---~--~~ http://groups.google.com/group/Google-Web-Toolkit-Contributors -~--~~~~--~~--~--~---
[gwt-contrib] Re: Code Review Request: Implementation of the RemoteUI [Updated]
LGTM
There are a couple of nits, but this does work end-to-end against the
custom plugin build.
http://gwt-code-reviews.appspot.com/83802/diff/1/2
File dev/core/src/com/google/gwt/dev/HostedModeBase.java (right):
http://gwt-code-reviews.appspot.com/83802/diff/1/2#newcode382
Line 382: options.setRemoteUIArgs(remoteUIArgs);
This seemed more congruent with the pattern that exists in this code.
On 2009/10/20 21:13:09, jat wrote:
Is there value in creating a RemoteUIArgs class here? Also, why not
just create
the RemoteUI instance here and assign it to ui?
http://gwt-code-reviews.appspot.com/83802/diff/1/6
File
dev/core/src/com/google/gwt/dev/shell/remoteui/ViewerServiceClient.java
(right):
http://gwt-code-reviews.appspot.com/83802/diff/1/6#newcode41
Line 41: public class ViewerServiceClient {
Nit: We should revisit the naming pattern here after MS2.
http://gwt-code-reviews.appspot.com/83802
--~--~-~--~~~---~--~~
http://groups.google.com/group/Google-Web-Toolkit-Contributors
-~--~~~~--~~--~--~---
[gwt-contrib] Re: Code Review Request: Removal of the do-not-commit-to-trunk libs
LGTM http://gwt-code-reviews.appspot.com/83804 --~--~-~--~~~---~--~~ http://groups.google.com/group/Google-Web-Toolkit-Contributors -~--~~~~--~~--~--~---
[gwt-contrib] Re: Code Review Request: Addition of the non-jarjared protobuf lib to tools
LGTM - Please make sure to add an issue to the public issue tracker so that we don't forget to jarjar this. I've already verified that the library works correctly after being rebased. http://gwt-code-reviews.appspot.com/83803 --~--~-~--~~~---~--~~ http://groups.google.com/group/Google-Web-Toolkit-Contributors -~--~~~~--~~--~--~---
[gwt-contrib] Re: Fixes a couple of checkstyle errors introduced by r6432.
Reviewers: rdayal,
Message:
Probably not. There is no hard convention around protoc generated
output that I know of.
On 2009/10/21 05:06:49, fabbott wrote:
http://gwt-code-reviews.appspot.com/83806/diff/1/2
File dev/build.xml (right):
http://gwt-code-reviews.appspot.com/83806/diff/1/2#newcode212
Line 212: filename
name=com/google/gwt/dev/shell/remoteui/RemoteMessageProto.java
negate=yes /
perhaps too late, but should we generalize this to e.g.
**/*Proto.java?
Description:
Committed as r6437.
Please review this at http://gwt-code-reviews.appspot.com/83806
Affected files:
M dev/build.xml
M
dev/core/src/com/google/gwt/dev/shell/remoteui/DevModeServiceRequestProcessor.java
Index: dev/build.xml
diff --git a/dev/build.xml b/dev/build.xml
index
be0bce9ab2a085395853bce2168ebcd3428259ad..584c6279f0acfb829cd64ffcb5d5b45366280d75
100755
--- a/dev/build.xml
+++ b/dev/build.xml
@@ -209,6 +209,7 @@
target name=checkstyle description=Static analysis of source
gwt.checkstyle
fileset dir=core/src
+filename
name=com/google/gwt/dev/shell/remoteui/RemoteMessageProto.java
negate=yes /
filename name=com/google/gwt/dev/asm/**/*.java negate=yes /
filename name=com/google/gwt/dev/js/rhino/**/*.java
negate=yes /
filename name=org/eclipse/**/*.java negate=yes /
Index:
dev/core/src/com/google/gwt/dev/shell/remoteui/DevModeServiceRequestProcessor.java
diff --git
a/dev/core/src/com/google/gwt/dev/shell/remoteui/DevModeServiceRequestProcessor.java
b/dev/core/src/com/google/gwt/dev/shell/remoteui/DevModeServiceRequestProcessor.java
index
d17d4fe00572c9cab95a5e7f7707aa8641071e6c..b2442d1355a1654b2542525ed51f48873f182d78
100644
---
a/dev/core/src/com/google/gwt/dev/shell/remoteui/DevModeServiceRequestProcessor.java
+++
b/dev/core/src/com/google/gwt/dev/shell/remoteui/DevModeServiceRequestProcessor.java
@@ -63,7 +63,6 @@ public class DevModeServiceRequestProcessor implements
RequestProcessor {
throw new IllegalArgumentException(
Unknown DevModeService Request: The DevModeService cannot handle
requests of type
+ request.getDevModeRequest().getRequestType().name());
-
}
private Response processCapabilityExchange(int requestId) {
--~--~-~--~~~---~--~~
http://groups.google.com/group/Google-Web-Toolkit-Contributors
-~--~~~~--~~--~--~---
[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] 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
-~--~~~~--~~--~--~---
[gwt-contrib] Re: Addition of Generated Java File (from .proto) and Protobuf Library
LGTM http://gwt-code-reviews.appspot.com/77830 --~--~-~--~~~---~--~~ http://groups.google.com/group/Google-Web-Toolkit-Contributors -~--~~~~--~~--~--~---
[gwt-contrib] Re: Addition of remotemessage.proto file
LGTM - I agree with Tamplin's comments on the package location. It's
been a while, but com.google.gwt.dev.shell.remoteui would seem like a
reasonable package location to me.
http://gwt-code-reviews.appspot.com/78825/diff/1/2
File dev/oophm/src/com/google/gwt/dev/shell/log/remotemessage.proto
(right):
http://gwt-code-reviews.appspot.com/78825/diff/1/2#newcode1
Line 1: package com.google.gwt.dev.shell.log;
Nit: Add copyright header.
http://gwt-code-reviews.appspot.com/78825/diff/1/2#newcode5
Line 5: message Message {
Nit: Add doc comments for the messages and enums.
http://gwt-code-reviews.appspot.com/78825
--~--~-~--~~~---~--~~
http://groups.google.com/group/Google-Web-Toolkit-Contributors
-~--~~~~--~~--~--~---
[gwt-contrib] UiBinderGenerator is not stable (issue 4067)
Reviewers: Ray Ryan,
Description:
UiBinderGenerator is not stable and results extra compiles during web
app reload. In order to avoid unnecessary compiles and type oracle
refreshes, code generators must be stable.
This patch is a partial fix in that visitation orders have been
stabilized, however the issue of generated, unique DOM element
identifiers still exists and will prevent UiBinder from being completely
stable.
Please see issue
http://code.google.com/p/google-web-toolkit/issues/detail?id=4067 for
more details.
Please review this at http://gwt-code-reviews.appspot.com/67808
Affected files:
user/src/com/google/gwt/uibinder/rebind/AbstractFieldWriter.java
user/src/com/google/gwt/uibinder/rebind/model/OwnerClass.java
Index: user/src/com/google/gwt/uibinder/rebind/AbstractFieldWriter.java
--- user/src/com/google/gwt/uibinder/rebind/AbstractFieldWriter.java
(revision 6102)
+++ user/src/com/google/gwt/uibinder/rebind/AbstractFieldWriter.java
(working copy)
@@ -19,7 +19,7 @@ import com.google.gwt.core.ext.UnableToCompleteException;
import com.google.gwt.core.ext.typeinfo.JClassType;
import com.google.gwt.core.ext.typeinfo.JType;
-import java.util.HashSet;
+import java.util.LinkedHashSet;
import java.util.Set;
/**
@@ -33,7 +33,7 @@ abstract class AbstractFieldWriter implements FieldWriter
{
+ @UiConstructor.;
private final String name;
- private final SetFieldWriter needs = new HashSetFieldWriter();
+ private final SetFieldWriter needs = new LinkedHashSetFieldWriter();
private String initializer;
private boolean written;
private MortalLogger logger;
Index: user/src/com/google/gwt/uibinder/rebind/model/OwnerClass.java
--- user/src/com/google/gwt/uibinder/rebind/model/OwnerClass.java
(revision
6102)
+++ user/src/com/google/gwt/uibinder/rebind/model/OwnerClass.java
(working
copy)
@@ -26,9 +26,12 @@ import com.google.gwt.uibinder.rebind.MortalLogger;
import java.util.ArrayList;
import java.util.Collection;
+import java.util.Collections;
+import java.util.Comparator;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
+import java.util.TreeMap;
/**
* Model class with all attributes of the owner class.
@@ -40,7 +43,7 @@ public class OwnerClass {
* Map from field name to model.
*/
private final MapString, OwnerField uiFields =
- new HashMapString, OwnerField();
+ new TreeMapString, OwnerField();
/**
* Map from field type to model.
@@ -75,6 +78,16 @@ public class OwnerClass {
findUiFields(ownerType);
findUiFactories(ownerType);
findUiHandlers(ownerType);
+
+/*
+ * Ensure that the order of these ui handlers is stable to avoid
unnecessary
+ * compilation passes.
+ */
+Collections.sort(uiHandlers, new ComparatorJMethod() {
+ public int compare(JMethod arg0, JMethod arg1) {
+return arg0.getJsniSignature().compareTo(arg1.getJsniSignature());
+ }
+});
}
/**
--~--~-~--~~~---~--~~
http://groups.google.com/group/Google-Web-Toolkit-Contributors
-~--~~~~--~~--~--~---
