Mostly LGTM with some small suggestions. However, I think there is one big thing that needs further discussion -- I think instead of trying to change the code, I suggest we change the Eclipse warnings to not warn about unused parameters in implemented/overridden methods.
http://gwt-code-reviews.appspot.com/954801/diff/1/6 File dev/core/src/com/google/gwt/dev/jjs/impl/OptimizerStats.java (right): http://gwt-code-reviews.appspot.com/954801/diff/1/6#newcode91 dev/core/src/com/google/gwt/dev/jjs/impl/OptimizerStats.java:91: * Return a human readable string representing the values of all statistics. human-readable http://gwt-code-reviews.appspot.com/954801/diff/1/16 File samples/dynatablerf/src/com/google/gwt/sample/dynatablerf/client/PersonEditorWorkflow.java (right): http://gwt-code-reviews.appspot.com/954801/diff/1/16#newcode104 samples/dynatablerf/src/com/google/gwt/sample/dynatablerf/client/PersonEditorWorkflow.java:104: void onCancel(ClickEvent event) { Put the @SuppressWarnings on the parameter rather than the method, here and below. However, I am not sure if we want to do this -- it seems pretty common that parameters on callbacks will be unused, and so I think it is better to have "Ignore in overriding and implementing methods" checked in the Eclipse warnings configuration. http://gwt-code-reviews.appspot.com/954801/diff/1/19 File samples/expenses/src/main/java/com/google/gwt/sample/expenses/client/ExpenseDetails.java (left): http://gwt-code-reviews.appspot.com/954801/diff/1/19#oldcode46 samples/expenses/src/main/java/com/google/gwt/sample/expenses/client/ExpenseDetails.java:46: import com.google.gwt.requestfactory.shared.Request; Why wasn't this caught by ant checkstyle? http://gwt-code-reviews.appspot.com/954801/diff/1/21 File samples/expenses/src/main/java/com/google/gwt/sample/expenses/client/MobileReportEntry.java (right): http://gwt-code-reviews.appspot.com/954801/diff/1/21#newcode61 samples/expenses/src/main/java/com/google/gwt/sample/expenses/client/MobileReportEntry.java:61: private final Listener listener; Why do we need the field at all if it is unused? http://gwt-code-reviews.appspot.com/954801/diff/1/32 File user/src/com/google/gwt/app/place/Prefix.java (right): http://gwt-code-reviews.appspot.com/954801/diff/1/32#newcode27 user/src/com/google/gwt/app/place/Prefix.java:27: * {code com.google.gwt.app.rebind.PlaceHistoryMapperGenerator} looks Why not link? http://gwt-code-reviews.appspot.com/954801/diff/1/36 File user/src/com/google/gwt/editor/client/AutoBean.java (right): http://gwt-code-reviews.appspot.com/954801/diff/1/36#newcode54 user/src/com/google/gwt/editor/client/AutoBean.java:54: * Returns <code>true</code> if {...@link #setFrozen} has been called. Shouldn't it talk about being last called with true? It looks like the freeze=>setFrozen change was to allow thawing a frozen object. http://gwt-code-reviews.appspot.com/954801/diff/1/37 File user/src/com/google/gwt/editor/client/AutoBeanVisitor.java (right): http://gwt-code-reviews.appspot.com/954801/diff/1/37#newcode52 user/src/com/google/gwt/editor/client/AutoBeanVisitor.java:52: * TODO: document. Should probably ask the person who wrote these methods to provide the description. http://gwt-code-reviews.appspot.com/954801/diff/1/44 File user/src/com/google/gwt/event/shared/EventBus.java (right): http://gwt-code-reviews.appspot.com/954801/diff/1/44#newcode27 user/src/com/google/gwt/event/shared/EventBus.java:27: * <p> See {...@code com.google.gwt.event.shared.testing.CountingEventBus}. Why this change? And if you do want to move it into the main doc, why not @link? http://gwt-code-reviews.appspot.com/954801/diff/1/50 File user/src/com/google/gwt/logging/client/HtmlLogFormatter.java (right): http://gwt-code-reviews.appspot.com/954801/diff/1/50#newcode62 user/src/com/google/gwt/logging/client/HtmlLogFormatter.java:62: * @param event Why use this method here and @SuppressWarnings elsewhere? Again, I am not sure we want to make these changes rather than just change Eclipse's warnings settings (I believe what you get by following eclipse/README.txt will not warn on these). http://gwt-code-reviews.appspot.com/954801/diff/1/65 File user/src/com/google/gwt/safehtml/client/SafeHtmlTemplates.java (right): http://gwt-code-reviews.appspot.com/954801/diff/1/65#newcode31 user/src/com/google/gwt/safehtml/client/SafeHtmlTemplates.java:31: * &064;Template("<span class=\"{3}\">{0}: <a href=\"{1}\">{2}</a></span>") Isn't this supposed to be "@"? http://gwt-code-reviews.appspot.com/954801/diff/1/68 File user/src/com/google/gwt/uibinder/client/UiChild.java (right): http://gwt-code-reviews.appspot.com/954801/diff/1/68#newcode43 user/src/com/google/gwt/uibinder/client/UiChild.java:43: * &064;UiChild MyWidget#addCustomChild(Widget w) </code> and And here. http://gwt-code-reviews.appspot.com/954801/diff/1/72 File user/src/com/google/gwt/user/client/ResponseTextHandler.java (right): http://gwt-code-reviews.appspot.com/954801/diff/1/72#newcode18 user/src/com/google/gwt/user/client/ResponseTextHandler.java:18: // TODO - is this class still used anywhere? It doesn't appear to be -- shouldn't we go ahead and remove it? http://gwt-code-reviews.appspot.com/954801/diff/1/74 File user/src/com/google/gwt/user/client/rpc/core/java/util/logging/Level_CustomFieldSerializer.java (right): http://gwt-code-reviews.appspot.com/954801/diff/1/74#newcode29 user/src/com/google/gwt/user/client/rpc/core/java/util/logging/Level_CustomFieldSerializer.java:29: @SuppressWarnings("unused") Same unused parameter issue. http://gwt-code-reviews.appspot.com/954801/diff/1/75 File user/src/com/google/gwt/user/client/rpc/impl/RemoteServiceProxy.java (right): http://gwt-code-reviews.appspot.com/954801/diff/1/75#newcode276 user/src/com/google/gwt/user/client/rpc/impl/RemoteServiceProxy.java:276: @SuppressWarnings("unused") Why not just rewrite this as: if (statsContext.isStatsAvailable()) { statsContext.stats(statsContext.bytesStat(methodName, requestData.length(), "requestSent"); } http://gwt-code-reviews.appspot.com/954801/diff/1/80 File user/src/com/google/gwt/user/client/ui/Label.java (right): http://gwt-code-reviews.appspot.com/954801/diff/1/80#newcode408 user/src/com/google/gwt/user/client/ui/Label.java:408: * described in {...@link #setText(String, com.google.gwt.i18n.client.HasDirection.Direction) Line too long. http://gwt-code-reviews.appspot.com/954801/diff/1/86 File user/src/com/google/gwt/user/client/ui/TabPanel.java (right): http://gwt-code-reviews.appspot.com/954801/diff/1/86#newcode70 user/src/com/google/gwt/user/client/ui/TabPanel.java:70: //Cannot do anything about tab panel implementing TabListener until next release. Space after //, line too long. http://gwt-code-reviews.appspot.com/954801/diff/1/88 File user/src/com/google/gwt/user/client/ui/Widget.java (right): http://gwt-code-reviews.appspot.com/954801/diff/1/88#newcode381 user/src/com/google/gwt/user/client/ui/Widget.java:381: * {...@link com.google.gwt.event.logical.shared.AttachEvent.Handler AttachEvent.Handler}s. Line too long. http://gwt-code-reviews.appspot.com/954801/diff/1/88#newcode390 user/src/com/google/gwt/user/client/ui/Widget.java:390: * {...@link com.google.gwt.event.logical.shared.AttachEvent.Handler AttachEvent.Handler}s. Line too long. http://gwt-code-reviews.appspot.com/954801/diff/1/92 File user/src/com/google/gwt/xhr/client/XMLHttpRequest.java (right): http://gwt-code-reviews.appspot.com/954801/diff/1/92#newcode26 user/src/com/google/gwt/xhr/client/XMLHttpRequest.java:26: * XMLHttpRequest/</a>/ Won't this line break add a space in the displayed URL? Probably better to do it as ...est/"
http://...
which will avoid that. Likewise all the ones below. http://gwt-code-reviews.appspot.com/954801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
