[gwt-contrib] Switch RequestFactory to use the real ConstraintViolation instead of the hacky Violation interface. (issue1422809)
Reviewers: rjrjr, Message: Request requested. http://gwt-code-reviews.appspot.com/1422809/diff/1/user/src/com/google/web/bindery/requestfactory/server/SimpleRequestProcessor.java File user/src/com/google/web/bindery/requestfactory/server/SimpleRequestProcessor.java (right): http://gwt-code-reviews.appspot.com/1422809/diff/1/user/src/com/google/web/bindery/requestfactory/server/SimpleRequestProcessor.java#newcode526 user/src/com/google/web/bindery/requestfactory/server/SimpleRequestProcessor.java:526: // Construct an ID that represents domainObject Interesting change starts here. Description: Switch RequestFactory to use the real ConstraintViolation instead of the hacky Violation interface. Deprecate Violation and Receiver.onViolation(). Patch by: bobv Review by: rjrjr Please review this at http://gwt-code-reviews.appspot.com/1422809/ Affected files: M samples/dynatablerf/src/com/google/gwt/sample/dynatablerf/client/PersonEditorWorkflow.java M user/src/com/google/web/bindery/requestfactory/gwt/client/RequestFactoryEditorDriver.java M user/src/com/google/web/bindery/requestfactory/gwt/client/impl/AbstractRequestFactoryEditorDriver.java M user/src/com/google/web/bindery/requestfactory/gwt/client/testing/MockRequestFactoryEditorDriver.java M user/src/com/google/web/bindery/requestfactory/server/RequestFactoryJarExtractor.java M user/src/com/google/web/bindery/requestfactory/server/SimpleRequestProcessor.java M user/src/com/google/web/bindery/requestfactory/shared/Receiver.java M user/src/com/google/web/bindery/requestfactory/shared/Violation.java M user/src/com/google/web/bindery/requestfactory/shared/impl/AbstractRequest.java M user/src/com/google/web/bindery/requestfactory/shared/impl/AbstractRequestContext.java M user/src/com/google/web/bindery/requestfactory/shared/messages/ViolationMessage.java M user/test/com/google/gwt/editor/rebind/model/EditorModelTest.java M user/test/com/google/web/bindery/requestfactory/gwt/client/RequestFactoryExceptionPropagationTest.java M user/test/com/google/web/bindery/requestfactory/gwt/client/RequestFactoryTest.java M user/test/com/google/web/bindery/requestfactory/gwt/client/ui/EditorTest.java -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Add RequestContext.append() to allow actions across different domain service types to be combine... (issue1423805)
Reviewers: rjrjr, Description: Add RequestContext.append() to allow actions across different domain service types to be combined in a single HTTP request. Patch by: bobv Review by: rjrjr Please review this at http://gwt-code-reviews.appspot.com/1423805/ Affected files: M user/src/com/google/web/bindery/requestfactory/shared/RequestContext.java M user/src/com/google/web/bindery/requestfactory/shared/impl/AbstractRequestContext.java M user/src/com/google/web/bindery/requestfactory/shared/impl/BaseProxyCategory.java M user/src/com/google/web/bindery/requestfactory/shared/impl/Constants.java M user/src/com/google/web/bindery/requestfactory/vm/InProcessRequestContext.java M user/test/com/google/web/bindery/requestfactory/gwt/client/RequestFactoryTest.java M user/test/com/google/web/bindery/requestfactory/gwt/client/RequestFactoryTestBase.java -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Treats last modified time of jar file entries differently for purposes of computing staleness. (issue1422802)
http://gwt-code-reviews.appspot.com/1422802/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Treats last modified time of jar file entries differently for purposes of computing staleness. (issue1422802)
This greatly simplifies the patch. I'm not sure the extra complexity and time spent to recover cached units by calculating their content ID again is worth it, given the infrequency of jar file updates in the wild.. http://gwt-code-reviews.appspot.com/1422802/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Treats last modified time of jar file entries differently for purposes of computing staleness. (issue1422802)
LGTM I think this is in practice a safe compromise. If a file within a jar changes, then all things in the jar will appear to be updated, but I think this is reasonable. Can refine in the future, to try to detect whether the date of an entry is 0 (or somehow invalid), and only revert to the jar file timestamp in that case. But I think it's worth going with this mod for now, since it should solve the issues at hand, and in practice, invalidating one jar is not as onerous as invalidating everything. http://gwt-code-reviews.appspot.com/1422802/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Move single jso logic from BasicWebModeCompiler to JavaToJavaScriptCompiler. (issue1428802)
LGTM http://gwt-code-reviews.appspot.com/1428802/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Adding benchmarks to measure the performance of updating na existing tables. Also refactored Wid... (issue1419801)
Jaime, can you take this? http://gwt-code-reviews.appspot.com/1419801/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Adding benchmarks to measure the performance of updating na existing tables. Also refactored Wid... (issue1419801)
Jaime and I are going to look at the benchmarks when we have more time. http://gwt-code-reviews.appspot.com/1419801/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: MobileWebApp sample. Showcases GWT providing a single app providing specialized views for Deskto... (issue1427803)
I forgot to check during the review, but we usually put the .project, .classpath, .checkstyle, .settings, and war directory in trunk/eclipse/samples/MobileWebApp http://gwt-code-reviews.appspot.com/1427803/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Structural changes to UiBinder to make fields accessible via getters (issue1420804)
http://gwt-code-reviews.appspot.com/1420804/diff/7001/user/src/com/google/gwt/uibinder/rebind/FieldManager.java
File user/src/com/google/gwt/uibinder/rebind/FieldManager.java (right):
http://gwt-code-reviews.appspot.com/1420804/diff/7001/user/src/com/google/gwt/uibinder/rebind/FieldManager.java#newcode251
user/src/com/google/gwt/uibinder/rebind/FieldManager.java:251: if
(fieldWriter == null) {
Are you sure this isn't a user error? It sure looks like one to me, in
which case this must be a call to logger.die and you have to deal with
all the throws UncaughtException ripples.
http://gwt-code-reviews.appspot.com/1420804/diff/7001/user/src/com/google/gwt/uibinder/rebind/UiBinderWriter.java
File user/src/com/google/gwt/uibinder/rebind/UiBinderWriter.java
(right):
http://gwt-code-reviews.appspot.com/1420804/diff/7001/user/src/com/google/gwt/uibinder/rebind/UiBinderWriter.java#newcode316
user/src/com/google/gwt/uibinder/rebind/UiBinderWriter.java:316: if
(useLazyWidgetBuilders) {
fix indentation
http://gwt-code-reviews.appspot.com/1420804/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Switch RequestFactory to use the real ConstraintViolation instead of the hacky Violation interface. (issue1422809)
Nick, could you take a look at this too? In particular see the bottom of http://gwt-code-reviews.appspot.com/1422809/diff/1/user/src/com/google/web/bindery/requestfactory/server/SimpleRequestProcessor.java On Thu, Apr 21, 2011 at 6:57 AM, b...@google.com wrote: Reviewers: rjrjr, Message: Request requested. http://gwt-code-reviews.appspot.com/1422809/diff/1/user/src/com/google/web/bindery/requestfactory/server/SimpleRequestProcessor.java File user/src/com/google/web/bindery/requestfactory/server/SimpleRequestProcessor.java (right): http://gwt-code-reviews.appspot.com/1422809/diff/1/user/src/com/google/web/bindery/requestfactory/server/SimpleRequestProcessor.java#newcode526 user/src/com/google/web/bindery/requestfactory/server/SimpleRequestProcessor.java:526: // Construct an ID that represents domainObject Interesting change starts here. Description: Switch RequestFactory to use the real ConstraintViolation instead of the hacky Violation interface. Deprecate Violation and Receiver.onViolation(). Patch by: bobv Review by: rjrjr Please review this at http://gwt-code-reviews.appspot.com/1422809/ Affected files: M samples/dynatablerf/src/com/google/gwt/sample/dynatablerf/client/PersonEditorWorkflow.java M user/src/com/google/web/bindery/requestfactory/gwt/client/RequestFactoryEditorDriver.java M user/src/com/google/web/bindery/requestfactory/gwt/client/impl/AbstractRequestFactoryEditorDriver.java M user/src/com/google/web/bindery/requestfactory/gwt/client/testing/MockRequestFactoryEditorDriver.java M user/src/com/google/web/bindery/requestfactory/server/RequestFactoryJarExtractor.java M user/src/com/google/web/bindery/requestfactory/server/SimpleRequestProcessor.java M user/src/com/google/web/bindery/requestfactory/shared/Receiver.java M user/src/com/google/web/bindery/requestfactory/shared/Violation.java M user/src/com/google/web/bindery/requestfactory/shared/impl/AbstractRequest.java M user/src/com/google/web/bindery/requestfactory/shared/impl/AbstractRequestContext.java M user/src/com/google/web/bindery/requestfactory/shared/messages/ViolationMessage.java M user/test/com/google/gwt/editor/rebind/model/EditorModelTest.java M user/test/com/google/web/bindery/requestfactory/gwt/client/RequestFactoryExceptionPropagationTest.java M user/test/com/google/web/bindery/requestfactory/gwt/client/RequestFactoryTest.java M user/test/com/google/web/bindery/requestfactory/gwt/client/ui/EditorTest.java -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
Re: [gwt-contrib] Re: Wrap low-priorty log calls with an 'if' test to avoid unnecessary calls (issue1426802)
On Thu, Apr 21, 2011 at 1:50 PM, Daniel Rice (דניאל רייס) r...@google.com wrote: I ran a Showcase compile with log level 'info' 4 times each way, and took the average of the 3 best times for each way: Without 'if' tests: 396 seconds With 'if' tests: 350 seconds wow, this is fantastic. So this feels like a small but significant savings. My thinking about where to put guards is as follows (for comparison, in Android it was policy to always put guards): 1) For WARNING or ERROR messages, don't put a guard since these will usually be emitted anyway 2) For lower priorities, put a guard if there is any code more complex than simply passing a constant or pre-existing string 2a) If the message is not emitted, we perform one comparison against an object instance field, which we would have done anyway inside of logger.log 2b) If the message is emitted, we have done one extra comparison, but the cost will be swamped by the cost of actually emitting the message So, I don't think we can really make things slower in any realistic scenario. On Tue, Apr 19, 2011 at 3:08 PM, j...@google.com wrote: The actual code changes LGTM, but I am skeptical about the performance benefits of guarding all the ones that do nothing more than string concats. If that is really too expensive, then virtually no log call should be unguarded, and we should just require callers to insert their own guards and remove the check from log/branch. Before committing the guards around just string concats, I would like to see some measurements that suggest it is worth the cost of cluttering up the code. http://gwt-code-reviews.appspot.com/1426802/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors -- Eric Z. Ayers Google Web Toolkit, Atlanta, GA USA -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: MobileWebApp sample. Showcases GWT providing a single app providing specialized views for Deskto... (issue1427803)
Yes, I'll make a patch with those files. I held on that until I could test GPE with the fix for the validation jars On Thu, Apr 21, 2011 at 1:42 PM, jlaba...@google.com wrote: I forgot to check during the review, but we usually put the .project, .classpath, .checkstyle, .settings, and war directory in trunk/eclipse/samples/MobileWebApp http://gwt-code-reviews.appspot.com/1427803/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Switch RequestFactory to use the real ConstraintViolation instead of the hacky Violation interface. (issue1422809)
LGTM
http://gwt-code-reviews.appspot.com/1422809/diff/1/user/src/com/google/web/bindery/requestfactory/gwt/client/impl/AbstractRequestFactoryEditorDriver.java
File
user/src/com/google/web/bindery/requestfactory/gwt/client/impl/AbstractRequestFactoryEditorDriver.java
(right):
http://gwt-code-reviews.appspot.com/1422809/diff/1/user/src/com/google/web/bindery/requestfactory/gwt/client/impl/AbstractRequestFactoryEditorDriver.java#newcode52
user/src/com/google/web/bindery/requestfactory/gwt/client/impl/AbstractRequestFactoryEditorDriver.java:52:
@SuppressWarnings(deprecation)
can this be more localized?
http://gwt-code-reviews.appspot.com/1422809/diff/1/user/src/com/google/web/bindery/requestfactory/shared/Receiver.java
File user/src/com/google/web/bindery/requestfactory/shared/Receiver.java
(right):
http://gwt-code-reviews.appspot.com/1422809/diff/1/user/src/com/google/web/bindery/requestfactory/shared/Receiver.java#newcode74
user/src/com/google/web/bindery/requestfactory/shared/Receiver.java:74:
* type.
Nice touch.
http://gwt-code-reviews.appspot.com/1422809/diff/1/user/src/com/google/web/bindery/requestfactory/shared/Receiver.java#newcode76
user/src/com/google/web/bindery/requestfactory/shared/Receiver.java:76:
* @param errors a Set of {@link Violation} instances
a set of what instances?
http://gwt-code-reviews.appspot.com/1422809/diff/1/user/src/com/google/web/bindery/requestfactory/shared/messages/ViolationMessage.java
File
user/src/com/google/web/bindery/requestfactory/shared/messages/ViolationMessage.java
(right):
http://gwt-code-reviews.appspot.com/1422809/diff/1/user/src/com/google/web/bindery/requestfactory/shared/messages/ViolationMessage.java#newcode28
user/src/com/google/web/bindery/requestfactory/shared/messages/ViolationMessage.java:28:
String TEMPATE = T;
tempLate
http://gwt-code-reviews.appspot.com/1422809/diff/1/user/test/com/google/gwt/editor/rebind/model/EditorModelTest.java
File user/test/com/google/gwt/editor/rebind/model/EditorModelTest.java
(right):
http://gwt-code-reviews.appspot.com/1422809/diff/1/user/test/com/google/gwt/editor/rebind/model/EditorModelTest.java#newcode505
user/test/com/google/gwt/editor/rebind/model/EditorModelTest.java:505:
@SuppressWarnings(deprecation)
can you isolate the deprecated part to a separate method?
http://gwt-code-reviews.appspot.com/1422809/diff/1/user/test/com/google/web/bindery/requestfactory/gwt/client/RequestFactoryExceptionPropagationTest.java
File
user/test/com/google/web/bindery/requestfactory/gwt/client/RequestFactoryExceptionPropagationTest.java
(right):
http://gwt-code-reviews.appspot.com/1422809/diff/1/user/test/com/google/web/bindery/requestfactory/gwt/client/RequestFactoryExceptionPropagationTest.java#newcode62
user/test/com/google/web/bindery/requestfactory/gwt/client/RequestFactoryExceptionPropagationTest.java:62:
public void
onViolation(Setcom.google.web.bindery.requestfactory.shared.Violation
errors) {
Should add a counter for the onConstraintViolation as well. Then let it
call through to super to maintain coverage of the compatibility code.
Or, if that's out of scope for this test, just convert to the new
method?
http://gwt-code-reviews.appspot.com/1422809/diff/1/user/test/com/google/web/bindery/requestfactory/gwt/client/RequestFactoryTest.java
File
user/test/com/google/web/bindery/requestfactory/gwt/client/RequestFactoryTest.java
(right):
http://gwt-code-reviews.appspot.com/1422809/diff/1/user/test/com/google/web/bindery/requestfactory/gwt/client/RequestFactoryTest.java#newcode55
user/test/com/google/web/bindery/requestfactory/gwt/client/RequestFactoryTest.java:55:
public class RequestFactoryTest extends RequestFactoryTestBase {
Hard to tease out the real change from the autodiff. Anything
particularly important?
http://gwt-code-reviews.appspot.com/1422809/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Treats last modified time of jar file entries differently for purposes of computing staleness. (issue1422802)
LVGTM http://gwt-code-reviews.appspot.com/1422802/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] [google-web-toolkit] r10050 committed - Move single jso logic from BasicWebModeCompiler to JavaToJavaScriptCom...
Revision: 10050
Author: gwt.mirror...@gmail.com
Date: Thu Apr 21 12:23:11 2011
Log: Move single jso logic from BasicWebModeCompiler to
JavaToJavaScriptCompiler.
http://gwt-code-reviews.appspot.com/1428802
Review by: robertvaw...@google.com
http://code.google.com/p/google-web-toolkit/source/detail?r=10050
Modified:
/trunk/dev/core/src/com/google/gwt/dev/jdt/BasicWebModeCompiler.java
/trunk/dev/core/src/com/google/gwt/dev/jjs/JavaToJavaScriptCompiler.java
===
--- /trunk/dev/core/src/com/google/gwt/dev/jdt/BasicWebModeCompiler.java
Thu Mar 24 15:47:57 2011
+++ /trunk/dev/core/src/com/google/gwt/dev/jdt/BasicWebModeCompiler.java
Thu Apr 21 12:23:11 2011
@@ -17,8 +17,6 @@
import com.google.gwt.core.ext.TreeLogger;
import com.google.gwt.core.ext.UnableToCompleteException;
-import com.google.gwt.core.ext.typeinfo.JClassType;
-import com.google.gwt.core.ext.typeinfo.TypeOracle;
import com.google.gwt.dev.javac.CompilationState;
import com.google.gwt.dev.javac.CompilationUnit;
import com.google.gwt.dev.javac.CompiledClass;
@@ -68,8 +66,6 @@
TreeLogger logger, String[] seedTypeNames,
ICompilationUnit... additionalUnits) throws
UnableToCompleteException {
-TypeOracle oracle = compilationState.getTypeOracle();
-Set? extends JClassType intfTypes =
oracle.getSingleJsoImplInterfaces();
MapString, CompiledClass classMapBySource =
compilationState.getClassFileMapBySource();
/*
@@ -80,8 +76,8 @@
*/
SetCompilationUnit alreadyAdded = new HashSetCompilationUnit();
-ListICompilationUnit icus = new ArrayListICompilationUnit(
-seedTypeNames.length + intfTypes.size() + additionalUnits.length);
+ListICompilationUnit icus =
+new ArrayListICompilationUnit(seedTypeNames.length +
additionalUnits.length);
Collections.addAll(icus, additionalUnits);
@@ -95,25 +91,6 @@
if (alreadyAdded.add(unit)) {
icus.add(new CompilationUnitAdapter(unit));
- } else {
-logger.log(TreeLogger.WARN, Duplicate compilation unit '
-+ unit.getResourceLocation() + ' in seed types);
- }
-}
-
-/*
- * Add all SingleJsoImpl types that we know about. It's likely that the
- * concrete types are never explicitly referenced from the seed types.
- */
-for (JClassType intf : intfTypes) {
- String implName =
oracle.getSingleJsoImpl(intf).getQualifiedSourceName();
- CompilationUnit unit = getUnitForType(logger, classMapBySource,
implName);
-
- if (alreadyAdded.add(unit)) {
-icus.add(new CompilationUnitAdapter(unit));
-logger.log(TreeLogger.SPAM, Forced compilation of unit '
-+ unit.getResourceLocation()
-+ ' becasue it contains a SingleJsoImpl type);
}
}
===
---
/trunk/dev/core/src/com/google/gwt/dev/jjs/JavaToJavaScriptCompiler.java
Wed Apr 20 08:48:02 2011
+++
/trunk/dev/core/src/com/google/gwt/dev/jjs/JavaToJavaScriptCompiler.java
Thu Apr 21 12:23:11 2011
@@ -42,6 +42,7 @@
import com.google.gwt.dev.cfg.ModuleDef;
import com.google.gwt.dev.javac.CompilationProblemReporter;
import com.google.gwt.dev.javac.CompilationProblemReporter.SourceFetcher;
+import com.google.gwt.dev.javac.typemodel.TypeOracle;
import com.google.gwt.dev.jdt.RebindPermutationOracle;
import com.google.gwt.dev.jdt.WebModeCompilerFrontEnd;
import com.google.gwt.dev.jjs.CorrelationFactory.DummyCorrelationFactory;
@@ -512,6 +513,15 @@
allRootTypes.addAll(JProgram.CODEGEN_TYPES_SET);
allRootTypes.addAll(JProgram.INDEX_TYPES_SET);
allRootTypes.add(FragmentLoaderCreator.ASYNC_FRAGMENT_LOADER);
+/*
+ * Add all SingleJsoImpl types that we know about. It's likely that the
+ * concrete types are never explicitly referenced.
+ */
+TypeOracle typeOracle = rpo.getCompilationState().getTypeOracle();
+for (com.google.gwt.core.ext.typeinfo.JClassType singleJsoIntf :
typeOracle
+.getSingleJsoImplInterfaces()) {
+
allRootTypes.add(typeOracle.getSingleJsoImpl(singleJsoIntf).getQualifiedSourceName());
+}
Memory.maybeDumpMemory(CompStateBuilt);
@@ -522,8 +532,7 @@
ListString finalTypeOracleTypes = Lists.create();
if (precompilationMetrics != null) {
- for (com.google.gwt.core.ext.typeinfo.JClassType type :
rpo.getCompilationState()
- .getTypeOracle().getTypes()) {
+ for (com.google.gwt.core.ext.typeinfo.JClassType type :
typeOracle.getTypes()) {
finalTypeOracleTypes =
Lists.add(finalTypeOracleTypes, type.getPackage().getName()
+ . + type.getName());
}
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Structural changes to UiBinder to make fields accessible via getters (issue1420804)
http://gwt-code-reviews.appspot.com/1420804/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Add RequestContext.append() to allow actions across different domain service types to be combine... (issue1423805)
LGTM
Wow. Wow. This is a very big deal, and from such a simple change!
The patch includes its own demonstration of why it matters. Look at
RequestFactoryTestBase. It's an asynchronous test that can't declare
itself to be finished until two separate reset() messages are
acknowledged by the server. Until now that has required two separate
HTTP requests and some ugly daisy chaining code.
What was:
protected void finishTestAndReset() {
final boolean[] reallyDone = {false, false};
req.simpleFooRequest().reset().fire(new ReceiverVoid() {
@Override
public void onSuccess(Void response) {
reallyDone[0] = true;
if (reallyDone[0] reallyDone[1]) {
finishTest();
}
}
});
req.simpleBarRequest().reset().fire(new ReceiverVoid() {
@Override
public void onSuccess(Void response) {
reallyDone[1] = true;
if (reallyDone[0] reallyDone[1]) {
finishTest();
}
}
});
}
Is now:
protected void finishTestAndReset() {
SimpleFooRequest ctx = req.simpleFooRequest();
ctx.reset();
ctx.append(req.simpleBarRequest()).reset();
ctx.fire(new ReceiverVoid() {
@Override
public void onSuccess(Void response) {
finishTest();
}
});
}
http://gwt-code-reviews.appspot.com/1423805/diff/1/user/src/com/google/web/bindery/requestfactory/shared/RequestContext.java
File
user/src/com/google/web/bindery/requestfactory/shared/RequestContext.java
(right):
http://gwt-code-reviews.appspot.com/1423805/diff/1/user/src/com/google/web/bindery/requestfactory/shared/RequestContext.java#newcode31
user/src/com/google/web/bindery/requestfactory/shared/RequestContext.java:31:
T extends RequestContext T append(T other);
Nice, very simple.
http://gwt-code-reviews.appspot.com/1423805/diff/1/user/src/com/google/web/bindery/requestfactory/shared/impl/AbstractRequestContext.java
File
user/src/com/google/web/bindery/requestfactory/shared/impl/AbstractRequestContext.java
(right):
http://gwt-code-reviews.appspot.com/1423805/diff/1/user/src/com/google/web/bindery/requestfactory/shared/impl/AbstractRequestContext.java#newcode103
user/src/com/google/web/bindery/requestfactory/shared/impl/AbstractRequestContext.java:103:
* an invocation argument.
Don't they also land here via create?
http://gwt-code-reviews.appspot.com/1423805/diff/1/user/test/com/google/web/bindery/requestfactory/gwt/client/RequestFactoryTest.java
File
user/test/com/google/web/bindery/requestfactory/gwt/client/RequestFactoryTest.java
(right):
http://gwt-code-reviews.appspot.com/1423805/diff/1/user/test/com/google/web/bindery/requestfactory/gwt/client/RequestFactoryTest.java#newcode239
user/test/com/google/web/bindery/requestfactory/gwt/client/RequestFactoryTest.java:239:
assertSame(foo2, c3.edit(foo2));
should you test a create from c3 as well? I could imagine there being
upstream / downstream issues for non-neighbors.
http://gwt-code-reviews.appspot.com/1423805/diff/1/user/test/com/google/web/bindery/requestfactory/gwt/client/RequestFactoryTest.java#newcode243
user/test/com/google/web/bindery/requestfactory/gwt/client/RequestFactoryTest.java:243:
fail(Should have thrown IllegalStateException);
because c3 has already been appended?
http://gwt-code-reviews.appspot.com/1423805/diff/1/user/test/com/google/web/bindery/requestfactory/gwt/client/RequestFactoryTest.java#newcode262
user/test/com/google/web/bindery/requestfactory/gwt/client/RequestFactoryTest.java:262:
c3.fire(new ReceiverVoid() {
what happens if you fire c1 or c2 instead? Should that be tested?
http://gwt-code-reviews.appspot.com/1423805/diff/1/user/test/com/google/web/bindery/requestfactory/gwt/client/RequestFactoryTestBase.java
File
user/test/com/google/web/bindery/requestfactory/gwt/client/RequestFactoryTestBase.java
(right):
http://gwt-code-reviews.appspot.com/1423805/diff/1/user/test/com/google/web/bindery/requestfactory/gwt/client/RequestFactoryTestBase.java#newcode148
user/test/com/google/web/bindery/requestfactory/gwt/client/RequestFactoryTestBase.java:148:
ctx.fire(new ReceiverVoid() {
Nice. NICE!
:-)
http://gwt-code-reviews.appspot.com/1423805/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Structural changes to UiBinder to make fields accessible via getters (issue1420804)
http://gwt-code-reviews.appspot.com/1420804/diff/7001/user/src/com/google/gwt/uibinder/rebind/FieldManager.java
File user/src/com/google/gwt/uibinder/rebind/FieldManager.java (right):
http://gwt-code-reviews.appspot.com/1420804/diff/7001/user/src/com/google/gwt/uibinder/rebind/FieldManager.java#newcode251
user/src/com/google/gwt/uibinder/rebind/FieldManager.java:251: if
(fieldWriter == null) {
On 2011/04/21 18:10:36, rjrjr wrote:
Are you sure this isn't a user error? It sure looks like one to me, in
which
case this must be a call to logger.die and you have to deal with all
the throws
UncaughtException ripples.
I don't think this is user related but even if I'm wrong I wouldn't be
able to specify the right message to help developers.
But I can for sure call:
writer.die(The required field %s doesn't exist.);
if you think I should.
Or maybe let a NPE spread indicating that there's a bug somewhere in the
parsers.
http://gwt-code-reviews.appspot.com/1420804/diff/7001/user/src/com/google/gwt/uibinder/rebind/UiBinderWriter.java
File user/src/com/google/gwt/uibinder/rebind/UiBinderWriter.java
(right):
http://gwt-code-reviews.appspot.com/1420804/diff/7001/user/src/com/google/gwt/uibinder/rebind/UiBinderWriter.java#newcode316
user/src/com/google/gwt/uibinder/rebind/UiBinderWriter.java:316: if
(useLazyWidgetBuilders) {
On 2011/04/21 18:10:36, rjrjr wrote:
fix indentation
Done.
http://gwt-code-reviews.appspot.com/1420804/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Structural changes to UiBinder to make fields accessible via getters (issue1420804)
LGTM
http://gwt-code-reviews.appspot.com/1420804/diff/7001/user/src/com/google/gwt/uibinder/rebind/FieldManager.java
File user/src/com/google/gwt/uibinder/rebind/FieldManager.java (right):
http://gwt-code-reviews.appspot.com/1420804/diff/7001/user/src/com/google/gwt/uibinder/rebind/FieldManager.java#newcode245
user/src/com/google/gwt/uibinder/rebind/FieldManager.java:245: * Gets a
FieldWrite given its name or throws a RuntimeException if not found.
FieldWriteR
http://gwt-code-reviews.appspot.com/1420804/diff/7001/user/src/com/google/gwt/uibinder/rebind/FieldManager.java#newcode251
user/src/com/google/gwt/uibinder/rebind/FieldManager.java:251: if
(fieldWriter == null) {
If you're sure, the RTE is fine. Thanks.
http://gwt-code-reviews.appspot.com/1420804/diff/4003/user/src/com/google/gwt/uibinder/rebind/UiBinderWriter.java
File user/src/com/google/gwt/uibinder/rebind/UiBinderWriter.java
(right):
http://gwt-code-reviews.appspot.com/1420804/diff/4003/user/src/com/google/gwt/uibinder/rebind/UiBinderWriter.java#newcode318
user/src/com/google/gwt/uibinder/rebind/UiBinderWriter.java:318: * I'm
expliciting over-simplifying this and assuming that the input
intentionally
http://gwt-code-reviews.appspot.com/1420804/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: MobileWebApp sample. Showcases GWT providing a single app providing specialized views for Deskto... (issue1427803)
I just realized that I don't like the name of this sample. It's not just about being a mobile app. It's about being a well architected one, which provides both a mobile and desktop UI. Shall we call it tasks instead? http://gwt-code-reviews.appspot.com/1427803/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] no warnings in mobile sample (issue1427806)
Reviewers: rchandia,
Description:
no warnings in mobile sample
Please review this at http://gwt-code-reviews.appspot.com/1427806/
Affected files:
M
samples/mobilewebapp/src/com/google/gwt/sample/mobilewebapp/client/MobileWebAppShellBase.java
M
samples/mobilewebapp/src/com/google/gwt/sample/mobilewebapp/client/activity/TaskEditActivity.java
Index:
samples/mobilewebapp/src/com/google/gwt/sample/mobilewebapp/client/MobileWebAppShellBase.java
===
---
samples/mobilewebapp/src/com/google/gwt/sample/mobilewebapp/client/MobileWebAppShellBase.java
(revision 10051)
+++
samples/mobilewebapp/src/com/google/gwt/sample/mobilewebapp/client/MobileWebAppShellBase.java
(working copy)
@@ -52,7 +52,7 @@
*
* @param isPortrait true if in portrait orientation, false if landscape
*/
- protected void adjustOrientation(boolean isPortrait) {
+ protected void adjustOrientation(@SuppressWarnings(unused) boolean
isPortrait) {
// No-op by default.
}
Index:
samples/mobilewebapp/src/com/google/gwt/sample/mobilewebapp/client/activity/TaskEditActivity.java
===
---
samples/mobilewebapp/src/com/google/gwt/sample/mobilewebapp/client/activity/TaskEditActivity.java
(revision 10051)
+++
samples/mobilewebapp/src/com/google/gwt/sample/mobilewebapp/client/activity/TaskEditActivity.java
(working copy)
@@ -25,12 +25,13 @@
import com.google.gwt.user.client.ui.AcceptsOneWidget;
import com.google.web.bindery.requestfactory.shared.Receiver;
+import java.util.logging.Logger;
+
/**
* Activity that presents a task to be edited.
*/
public class TaskEditActivity extends AbstractActivity implements
TaskEditView.Presenter {
-
- private static final int ONE_HOUR = 360;
+ private static final Logger log =
Logger.getLogger(TaskEditActivity.class.getName());
private final ClientFactory clientFactory;
@@ -86,7 +87,7 @@
public void saveTask(boolean addToCalendar) {
if (task == null) {
- doCreateTask(addToCalendar);
+ doCreateTask();
} else {
doUpdateTask();
}
@@ -137,17 +138,15 @@
/**
* Create a new task.
- *
- * @param addToCalendar true to add the task to the calendar
- */
- private void doCreateTask(final boolean addToCalendar) {
+ */
+ private void doCreateTask() {
TaskRequest request = clientFactory.getRequestFactory().taskRequest();
final TaskProxy toCreate = request.create(TaskProxy.class);
populateTaskFromView(toCreate);
request.persist().using(toCreate).fire(new ReceiverVoid() {
@Override
public void onSuccess(Void response) {
-onTaskCreated(toCreate, addToCalendar);
+onTaskCreated(toCreate);
}
});
}
@@ -166,7 +165,7 @@
new ReceiverVoid() {
@Override
public void onSuccess(Void response) {
-onTaskDeleted(toDelete);
+onTaskDeleted();
}
});
}
@@ -188,7 +187,7 @@
request.persist().using(toEdit).fire(new ReceiverVoid() {
@Override
public void onSuccess(Void response) {
-onTaskUpdated(toEdit);
+onTaskUpdated();
}
});
}
@@ -200,15 +199,15 @@
*/
private void notify(String message) {
// TODO Add notification pop-up
+log.fine(Tell the user: + message);
}
/**
* Called when a task has been successfully created.
*
* @param task the task that was created
- * @param addToCalendar true to add the task to the calendar
- */
- private void onTaskCreated(TaskProxy task, boolean addToCalendar) {
+ */
+ private void onTaskCreated(TaskProxy task) {
// Notify the user that the task was created.
notify(Created task ' + task.getName() + ');
@@ -218,10 +217,8 @@
/**
* Called when a task has been successfully deleted.
- *
- * @param task the task that was deleted
- */
- private void onTaskDeleted(TaskProxy task) {
+ */
+ private void onTaskDeleted() {
// Notify the user that the task was deleted.
notify(Task Deleted);
@@ -231,10 +228,8 @@
/**
* Called when a task has been successfully updated.
- *
- * @param task the task that was updated
- */
- private void onTaskUpdated(TaskProxy task) {
+ */
+ private void onTaskUpdated() {
// Notify the user that the task was updated.
notify(Task Updated);
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: no warnings in mobile sample (issue1427806)
Review, please. http://gwt-code-reviews.appspot.com/1427806/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] [google-web-toolkit] r10052 committed - Add RequestContext.append() to allow actions across different domain s...
Revision: 10052
Author: gwt.mirror...@gmail.com
Date: Thu Apr 21 15:19:33 2011
Log: Add RequestContext.append() to allow actions across different
domain service types to be combined in a single HTTP request.
Patch by: bobv
Review by: rjrjr
Review at http://gwt-code-reviews.appspot.com/1423805
http://code.google.com/p/google-web-toolkit/source/detail?r=10052
Modified:
/trunk/user/src/com/google/web/bindery/requestfactory/shared/RequestContext.java
/trunk/user/src/com/google/web/bindery/requestfactory/shared/impl/AbstractRequestContext.java
/trunk/user/src/com/google/web/bindery/requestfactory/shared/impl/BaseProxyCategory.java
/trunk/user/src/com/google/web/bindery/requestfactory/shared/impl/Constants.java
/trunk/user/src/com/google/web/bindery/requestfactory/vm/InProcessRequestContext.java
/trunk/user/test/com/google/web/bindery/requestfactory/gwt/client/RequestFactoryTest.java
/trunk/user/test/com/google/web/bindery/requestfactory/gwt/client/RequestFactoryTestBase.java
===
---
/trunk/user/src/com/google/web/bindery/requestfactory/shared/RequestContext.java
Tue Apr 5 10:47:39 2011
+++
/trunk/user/src/com/google/web/bindery/requestfactory/shared/RequestContext.java
Thu Apr 21 15:19:33 2011
@@ -19,6 +19,26 @@
* The base interface for RequestFactory service endpoints.
*/
public interface RequestContext {
+ /**
+ * Joins another RequestContext to this RequestContext.
+ *
+ * pre
+ * SomeContext ctx = myFactory.someContext();
+ * // Perform operations on ctx
+ * OtherContext other = ctx.append(myFactory.otherContext());
+ * // Perform operations on both other and ctx
+ * ctx.fire() // or other.fire() are equivalent
+ * /pre
+ *
+ * @param other a freshly-constructed RequestContext whose state should
be
+ * bound to this RequestContext
+ * @return {@code other}
+ * @throws IllegalStateException if any methods have been called on
+ * {@code other} or if {@code other} was constructed by a
different
+ * RequestFactory instance
+ */
+ T extends RequestContext T append(T other);
+
/**
* Returns a new mutable proxy that this request can carry to the server,
* perhaps to be persisted. If the object is succesfully persisted, a
PERSIST
===
---
/trunk/user/src/com/google/web/bindery/requestfactory/shared/impl/AbstractRequestContext.java
Mon Apr 18 16:25:25 2011
+++
/trunk/user/src/com/google/web/bindery/requestfactory/shared/impl/AbstractRequestContext.java
Thu Apr 21 15:19:33 2011
@@ -16,7 +16,7 @@
package com.google.web.bindery.requestfactory.shared.impl;
import static
com.google.web.bindery.requestfactory.shared.impl.BaseProxyCategory.stableId;
-import static
com.google.web.bindery.requestfactory.shared.impl.Constants.REQUEST_CONTEXT;
+import static
com.google.web.bindery.requestfactory.shared.impl.Constants.REQUEST_CONTEXT_STATE;
import static
com.google.web.bindery.requestfactory.shared.impl.Constants.STABLE_ID;
import com.google.web.bindery.autobean.shared.AutoBean;
@@ -87,6 +87,63 @@
};
abstract DialectImpl create(AbstractRequestContext context);
}
+
+ /**
+ * Encapsulates all state contained by the AbstractRequestContext.
+ */
+ protected static class State {
+public final AbstractRequestContext canonical;
+public final DialectImpl dialect;
+public final ListAbstractRequest? invocations = new
ArrayListAbstractRequest?();
+
+public boolean locked;
+/**
+ * A map of all EntityProxies that the RequestContext has interacted
with.
+ * Objects are placed into this map by being returned from {@link
#create},
+ * passed into {@link #edit}, or used as an invocation argument.
+ */
+public final MapSimpleProxyId?, AutoBean? extends BaseProxy
editedProxies =
+new LinkedHashMapSimpleProxyId?, AutoBean? extends
BaseProxy();
+/**
+ * A map that contains the canonical instance of an entity to return
in the
+ * return graph, since this is built from scratch.
+ */
+public final MapSimpleProxyId?, AutoBean? returnedProxies =
+new HashMapSimpleProxyId?, AutoBean?();
+
+public final AbstractRequestFactory requestFactory;
+
+/**
+ * A map that allows us to handle the case where the server has sent
back an
+ * unpersisted entity. Because we assume that the server is stateless,
the
+ * client will need to swap out the request-local ids with a regular
+ * client-allocated id.
+ */
+public final MapInteger, SimpleProxyId? syntheticIds =
+new HashMapInteger, SimpleProxyId?();
+
+public State(AbstractRequestFactory requestFactory, DialectImpl
dialect,
+AbstractRequestContext canonical) {
+ this.requestFactory = requestFactory;
+ this.dialect = dialect;
+ this.canonical = canonical;
+}
+
+public AbstractRequestContext
[gwt-contrib] Re: Add RequestContext.append() to allow actions across different domain service types to be combine... (issue1423805)
r10052
http://gwt-code-reviews.appspot.com/1423805/diff/1/user/src/com/google/web/bindery/requestfactory/shared/impl/AbstractRequestContext.java
File
user/src/com/google/web/bindery/requestfactory/shared/impl/AbstractRequestContext.java
(right):
http://gwt-code-reviews.appspot.com/1423805/diff/1/user/src/com/google/web/bindery/requestfactory/shared/impl/AbstractRequestContext.java#newcode103
user/src/com/google/web/bindery/requestfactory/shared/impl/AbstractRequestContext.java:103:
* an invocation argument.
On 2011/04/21 19:36:38, rjrjr wrote:
Don't they also land here via create?
Done.
http://gwt-code-reviews.appspot.com/1423805/diff/1/user/test/com/google/web/bindery/requestfactory/gwt/client/RequestFactoryTest.java
File
user/test/com/google/web/bindery/requestfactory/gwt/client/RequestFactoryTest.java
(right):
http://gwt-code-reviews.appspot.com/1423805/diff/1/user/test/com/google/web/bindery/requestfactory/gwt/client/RequestFactoryTest.java#newcode239
user/test/com/google/web/bindery/requestfactory/gwt/client/RequestFactoryTest.java:239:
assertSame(foo2, c3.edit(foo2));
On 2011/04/21 19:36:38, rjrjr wrote:
should you test a create from c3 as well? I could imagine there being
upstream /
downstream issues for non-neighbors.
Done.
http://gwt-code-reviews.appspot.com/1423805/diff/1/user/test/com/google/web/bindery/requestfactory/gwt/client/RequestFactoryTest.java#newcode243
user/test/com/google/web/bindery/requestfactory/gwt/client/RequestFactoryTest.java:243:
fail(Should have thrown IllegalStateException);
On 2011/04/21 19:36:38, rjrjr wrote:
because c3 has already been appended?
Done.
http://gwt-code-reviews.appspot.com/1423805/diff/1/user/test/com/google/web/bindery/requestfactory/gwt/client/RequestFactoryTest.java#newcode262
user/test/com/google/web/bindery/requestfactory/gwt/client/RequestFactoryTest.java:262:
c3.fire(new ReceiverVoid() {
On 2011/04/21 19:36:38, rjrjr wrote:
what happens if you fire c1 or c2 instead? Should that be tested?
Done.
http://gwt-code-reviews.appspot.com/1423805/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Switch RequestFactory to use the real ConstraintViolation instead of the hacky Violation interface. (issue1422809)
r10053
http://gwt-code-reviews.appspot.com/1422809/diff/1/user/src/com/google/web/bindery/requestfactory/gwt/client/impl/AbstractRequestFactoryEditorDriver.java
File
user/src/com/google/web/bindery/requestfactory/gwt/client/impl/AbstractRequestFactoryEditorDriver.java
(right):
http://gwt-code-reviews.appspot.com/1422809/diff/1/user/src/com/google/web/bindery/requestfactory/gwt/client/impl/AbstractRequestFactoryEditorDriver.java#newcode52
user/src/com/google/web/bindery/requestfactory/gwt/client/impl/AbstractRequestFactoryEditorDriver.java:52:
@SuppressWarnings(deprecation)
Every field and method in this class, save getUserDataObject(), shows a
deprecation warning.
http://gwt-code-reviews.appspot.com/1422809/diff/1/user/src/com/google/web/bindery/requestfactory/shared/Receiver.java
File user/src/com/google/web/bindery/requestfactory/shared/Receiver.java
(right):
http://gwt-code-reviews.appspot.com/1422809/diff/1/user/src/com/google/web/bindery/requestfactory/shared/Receiver.java#newcode76
user/src/com/google/web/bindery/requestfactory/shared/Receiver.java:76:
* @param errors a Set of {@link Violation} instances
On 2011/04/21 18:49:32, rjrjr wrote:
a set of what instances?
Done.
http://gwt-code-reviews.appspot.com/1422809/diff/1/user/src/com/google/web/bindery/requestfactory/shared/messages/ViolationMessage.java
File
user/src/com/google/web/bindery/requestfactory/shared/messages/ViolationMessage.java
(right):
http://gwt-code-reviews.appspot.com/1422809/diff/1/user/src/com/google/web/bindery/requestfactory/shared/messages/ViolationMessage.java#newcode28
user/src/com/google/web/bindery/requestfactory/shared/messages/ViolationMessage.java:28:
String TEMPATE = T;
On 2011/04/21 18:49:32, rjrjr wrote:
tempLate
Done.
http://gwt-code-reviews.appspot.com/1422809/diff/1/user/test/com/google/gwt/editor/rebind/model/EditorModelTest.java
File user/test/com/google/gwt/editor/rebind/model/EditorModelTest.java
(right):
http://gwt-code-reviews.appspot.com/1422809/diff/1/user/test/com/google/gwt/editor/rebind/model/EditorModelTest.java#newcode505
user/test/com/google/gwt/editor/rebind/model/EditorModelTest.java:505:
@SuppressWarnings(deprecation)
On 2011/04/21 18:49:32, rjrjr wrote:
can you isolate the deprecated part to a separate method?
Done.
http://gwt-code-reviews.appspot.com/1422809/diff/1/user/test/com/google/web/bindery/requestfactory/gwt/client/RequestFactoryExceptionPropagationTest.java
File
user/test/com/google/web/bindery/requestfactory/gwt/client/RequestFactoryExceptionPropagationTest.java
(right):
http://gwt-code-reviews.appspot.com/1422809/diff/1/user/test/com/google/web/bindery/requestfactory/gwt/client/RequestFactoryExceptionPropagationTest.java#newcode62
user/test/com/google/web/bindery/requestfactory/gwt/client/RequestFactoryExceptionPropagationTest.java:62:
public void
onViolation(Setcom.google.web.bindery.requestfactory.shared.Violation
errors) {
On 2011/04/21 18:49:32, rjrjr wrote:
Should add a counter for the onConstraintViolation as well. Then let
it call
through to super to maintain coverage of the compatibility code.
Or, if that's out of scope for this test, just convert to the new
method?
Done.
http://gwt-code-reviews.appspot.com/1422809/diff/1/user/test/com/google/web/bindery/requestfactory/gwt/client/RequestFactoryTest.java
File
user/test/com/google/web/bindery/requestfactory/gwt/client/RequestFactoryTest.java
(right):
http://gwt-code-reviews.appspot.com/1422809/diff/1/user/test/com/google/web/bindery/requestfactory/gwt/client/RequestFactoryTest.java#newcode55
user/test/com/google/web/bindery/requestfactory/gwt/client/RequestFactoryTest.java:55:
public class RequestFactoryTest extends RequestFactoryTestBase {
Just copied any tests on onViolation() into a matching
onConstraintViolation() that forwards to the default
Receiver.onConstraintViolation().
http://gwt-code-reviews.appspot.com/1422809/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Add RequestContext.append() to allow actions across different domain service types to be combine... (issue1423805)
On 2011/04/21 19:36:37, rjrjr wrote: LGTM Wow. Wow. This is a very big deal, and from such a simple change! Which to me leaves the question: is our usage described at [1] an intended use case, that's here to stay? or simply a fortunate unspecified behavior, and we should change our code to this new append() feature? (BTW, good job Bob, as always!) [1] http://code.google.com/p/google-web-toolkit/issues/detail?id=5807#c3 http://gwt-code-reviews.appspot.com/1423805/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: no warnings in mobile sample (issue1427806)
LGTM http://gwt-code-reviews.appspot.com/1427806/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: no warnings in mobile sample (issue1427806)
r10054 http://gwt-code-reviews.appspot.com/1427806/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Create a utility class for checking assignability of types for use (issue1420808)
Reviewers: rjrjr, Description: Create a utility class for checking assignability of types for use in finding compatible constructors/methods. Review by: rj...@google.com Please review this at http://gwt-code-reviews.appspot.com/1420808/ Affected files: M user/src/com/google/gwt/uibinder/elementparsers/DateLabelParser.java A user/src/com/google/gwt/uibinder/rebind/TypeOracleUtils.java A user/test/com/google/gwt/uibinder/rebind/TypeOracleUtilsTest.java -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Create a utility class for checking assignability of types for use (issue1420808)
Thanks John, this will be really handy.
http://gwt-code-reviews.appspot.com/1420808/diff/1/user/src/com/google/gwt/uibinder/rebind/TypeOracleUtils.java
File user/src/com/google/gwt/uibinder/rebind/TypeOracleUtils.java
(right):
http://gwt-code-reviews.appspot.com/1420808/diff/1/user/src/com/google/gwt/uibinder/rebind/TypeOracleUtils.java#newcode60
user/src/com/google/gwt/uibinder/rebind/TypeOracleUtils.java:60:
JType... argTypes) {
I don't know that I would ever use this unless it was based on
JClassType.getOverridableMethods().
But rather than hardcoding that choice, perhaps you should make the
first argument a JMethod[] to let me choose the set myself? Can put @see
pointing to getOverridableMethods.
I'm biting my tongue about the fact that this method is unused. I'm
generally opposed to speculative coding, but I can imagine cleaning up
some existing code with this.
http://gwt-code-reviews.appspot.com/1420808/diff/1/user/src/com/google/gwt/uibinder/rebind/TypeOracleUtils.java#newcode65
user/src/com/google/gwt/uibinder/rebind/TypeOracleUtils.java:65: if
(method.isStatic() == isStatic methodName.equals(method.getName())
could you put () around the == statement?
http://gwt-code-reviews.appspot.com/1420808/diff/1/user/src/com/google/gwt/uibinder/rebind/TypeOracleUtils.java#newcode78
user/src/com/google/gwt/uibinder/rebind/TypeOracleUtils.java:78: *
@param argType
What's the difference between a paramType and an argType? Seems like the
distinction matters here, but param and arg look like synonyms to me.
Should the args be leftHandType and rightHandType? This applies
throughout the class of course, not just here.
http://gwt-code-reviews.appspot.com/1420808/diff/1/user/src/com/google/gwt/uibinder/rebind/TypeOracleUtils.java#newcode99
user/src/com/google/gwt/uibinder/rebind/TypeOracleUtils.java:99: //
TODO: handle autoboxing?
not needed by uibinder, fwiw.
http://gwt-code-reviews.appspot.com/1420808/diff/1/user/src/com/google/gwt/uibinder/rebind/TypeOracleUtils.java#newcode140
user/src/com/google/gwt/uibinder/rebind/TypeOracleUtils.java:140:
JType[] ctorArgTypes = paramTypes;
Looks like ctorArgTypes is refactoring chaff, should be inlined.
http://gwt-code-reviews.appspot.com/1420808/diff/1/user/src/com/google/gwt/uibinder/rebind/TypeOracleUtils.java#newcode157
user/src/com/google/gwt/uibinder/rebind/TypeOracleUtils.java:157: } else
{
else if
http://gwt-code-reviews.appspot.com/1420808/diff/1/user/test/com/google/gwt/uibinder/rebind/TypeOracleUtilsTest.java
File user/test/com/google/gwt/uibinder/rebind/TypeOracleUtilsTest.java
(right):
http://gwt-code-reviews.appspot.com/1420808/diff/1/user/test/com/google/gwt/uibinder/rebind/TypeOracleUtilsTest.java#newcode29
user/test/com/google/gwt/uibinder/rebind/TypeOracleUtilsTest.java:29:
import static com.google.gwt.uibinder.rebind.TypeOracleUtils.*; might
make this easier to read.
http://gwt-code-reviews.appspot.com/1420808/diff/1/user/test/com/google/gwt/uibinder/rebind/TypeOracleUtilsTest.java#newcode145
user/test/com/google/gwt/uibinder/rebind/TypeOracleUtilsTest.java:145:
public void testHasCompatibleMethod() {
Many lines too long. Did you auto format?
http://gwt-code-reviews.appspot.com/1420808/diff/1/user/test/com/google/gwt/uibinder/rebind/TypeOracleUtilsTest.java#newcode146
user/test/com/google/gwt/uibinder/rebind/TypeOracleUtilsTest.java:146:
assertTrue(didn't find static m1(Child),
TypeOracleUtils.hasCompatibleMethod(foo, true, m1,
s/didn't find/Should have found/
http://gwt-code-reviews.appspot.com/1420808/diff/1/user/test/com/google/gwt/uibinder/rebind/TypeOracleUtilsTest.java#newcode148
user/test/com/google/gwt/uibinder/rebind/TypeOracleUtilsTest.java:148:
assertFalse(found m1(Child), TypeOracleUtils.hasCompatibleMethod(foo,
false, m1, child));
s/found/Should not have found/
http://gwt-code-reviews.appspot.com/1420808/diff/1/user/test/com/google/gwt/uibinder/rebind/TypeOracleUtilsTest.java#newcode215
user/test/com/google/gwt/uibinder/rebind/TypeOracleUtilsTest.java:215:
protected void setUp() throws Exception {
please throw specific exception types
http://gwt-code-reviews.appspot.com/1420808/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Create a utility class for checking assignability of types for use (issue1420808)
http://gwt-code-reviews.appspot.com/1420808/diff/1/user/src/com/google/gwt/uibinder/rebind/TypeOracleUtils.java
File user/src/com/google/gwt/uibinder/rebind/TypeOracleUtils.java
(right):
http://gwt-code-reviews.appspot.com/1420808/diff/1/user/src/com/google/gwt/uibinder/rebind/TypeOracleUtils.java#newcode60
user/src/com/google/gwt/uibinder/rebind/TypeOracleUtils.java:60:
JType... argTypes) {
On 2011/04/22 01:13:17, rjrjr wrote:
I don't know that I would ever use this unless it was based on
JClassType.getOverridableMethods().
But rather than hardcoding that choice, perhaps you should make the
first
argument a JMethod[] to let me choose the set myself? Can put @see
pointing to
getOverridableMethods.
Ok, how about a method taking the method list and one taking the type,
which defaults to either getMethods or getOverridableMethods (whichever
you think is more useful). I would think in this application it is most
interesting in this context, since you want to know if you can call the
method with a set of parameters, and whether it is implemented on that
class or a superclass is just an implementation detail.
I'm biting my tongue about the fact that this method is unused. I'm
generally
opposed to speculative coding, but I can imagine cleaning up some
existing code
with this.
It just seemed an obvious extension of the constructor code, but it is
certainly easy enough to leave it out if you prefer.
http://gwt-code-reviews.appspot.com/1420808/diff/1/user/src/com/google/gwt/uibinder/rebind/TypeOracleUtils.java#newcode65
user/src/com/google/gwt/uibinder/rebind/TypeOracleUtils.java:65: if
(method.isStatic() == isStatic methodName.equals(method.getName())
On 2011/04/22 01:13:17, rjrjr wrote:
could you put () around the == statement?
Done.
http://gwt-code-reviews.appspot.com/1420808/diff/1/user/src/com/google/gwt/uibinder/rebind/TypeOracleUtils.java#newcode78
user/src/com/google/gwt/uibinder/rebind/TypeOracleUtils.java:78: *
@param argType
On 2011/04/22 01:13:17, rjrjr wrote:
What's the difference between a paramType and an argType? Seems like
the
distinction matters here, but param and arg look like synonyms to me.
paramType is the type of the formal parameter declaration, while argType
is the type of the argument to be passed in.
Should the args be leftHandType and rightHandType? This applies
throughout the
class of course, not just here.
That would work, although the terminology seems a bit odd in the context
of a method call.
http://gwt-code-reviews.appspot.com/1420808/diff/1/user/src/com/google/gwt/uibinder/rebind/TypeOracleUtils.java#newcode140
user/src/com/google/gwt/uibinder/rebind/TypeOracleUtils.java:140:
JType[] ctorArgTypes = paramTypes;
On 2011/04/22 01:13:17, rjrjr wrote:
Looks like ctorArgTypes is refactoring chaff, should be inlined.
Yes, I created it just so I could inline it, then never did :). Done.
http://gwt-code-reviews.appspot.com/1420808/diff/1/user/src/com/google/gwt/uibinder/rebind/TypeOracleUtils.java#newcode157
user/src/com/google/gwt/uibinder/rebind/TypeOracleUtils.java:157: } else
{
On 2011/04/22 01:13:17, rjrjr wrote:
else if
Done.
http://gwt-code-reviews.appspot.com/1420808/diff/1/user/test/com/google/gwt/uibinder/rebind/TypeOracleUtilsTest.java
File user/test/com/google/gwt/uibinder/rebind/TypeOracleUtilsTest.java
(right):
http://gwt-code-reviews.appspot.com/1420808/diff/1/user/test/com/google/gwt/uibinder/rebind/TypeOracleUtilsTest.java#newcode29
user/test/com/google/gwt/uibinder/rebind/TypeOracleUtilsTest.java:29:
On 2011/04/22 01:13:17, rjrjr wrote:
import static com.google.gwt.uibinder.rebind.TypeOracleUtils.*; might
make this
easier to read.
Done.
http://gwt-code-reviews.appspot.com/1420808/diff/1/user/test/com/google/gwt/uibinder/rebind/TypeOracleUtilsTest.java#newcode145
user/test/com/google/gwt/uibinder/rebind/TypeOracleUtilsTest.java:145:
public void testHasCompatibleMethod() {
On 2011/04/22 01:13:17, rjrjr wrote:
Many lines too long. Did you auto format?
Yes, although I did make some edits since then. The @link tags can't be
split, and Reitveld is wrapping at 80 instead of 100. I will make sure
it is formatted properly before submitting.
http://gwt-code-reviews.appspot.com/1420808/diff/1/user/test/com/google/gwt/uibinder/rebind/TypeOracleUtilsTest.java#newcode146
user/test/com/google/gwt/uibinder/rebind/TypeOracleUtilsTest.java:146:
assertTrue(didn't find static m1(Child),
TypeOracleUtils.hasCompatibleMethod(foo, true, m1,
On 2011/04/22 01:13:17, rjrjr wrote:
s/didn't find/Should have found/
Done.
http://gwt-code-reviews.appspot.com/1420808/diff/1/user/test/com/google/gwt/uibinder/rebind/TypeOracleUtilsTest.java#newcode148
user/test/com/google/gwt/uibinder/rebind/TypeOracleUtilsTest.java:148:
assertFalse(found m1(Child), TypeOracleUtils.hasCompatibleMethod(foo,
false, m1, child));
On 2011/04/22 01:13:17, rjrjr wrote:
s/found/Should not have found/
Done.
[gwt-contrib] Re: Create a utility class for checking assignability of types for use (issue1420808)
http://gwt-code-reviews.appspot.com/1420808/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Create a utility class for checking assignability of types for use (issue1420808)
On Thu, Apr 21, 2011 at 6:54 PM, j...@google.com wrote:
http://gwt-code-reviews.appspot.com/1420808/diff/1/user/src/com/google/gwt/uibinder/rebind/TypeOracleUtils.java
File user/src/com/google/gwt/uibinder/rebind/TypeOracleUtils.java
(right):
http://gwt-code-reviews.appspot.com/1420808/diff/1/user/src/com/google/gwt/uibinder/rebind/TypeOracleUtils.java#newcode60
user/src/com/google/gwt/uibinder/rebind/TypeOracleUtils.java:60:
JType... argTypes) {
On 2011/04/22 01:13:17, rjrjr wrote:
I don't know that I would ever use this unless it was based on
JClassType.getOverridableMethods().
But rather than hardcoding that choice, perhaps you should make the
first
argument a JMethod[] to let me choose the set myself? Can put @see
pointing to
getOverridableMethods.
Ok, how about a method taking the method list and one taking the type,
which defaults to either getMethods or getOverridableMethods (whichever
you think is more useful). I would think in this application it is most
interesting in this context, since you want to know if you can call the
method with a set of parameters, and whether it is implemented on that
class or a superclass is just an implementation detail.
Sounds good. Definitely getOverridableMethods.
I'm biting my tongue about the fact that this method is unused. I'm
generally
opposed to speculative coding, but I can imagine cleaning up some
existing code
with this.
It just seemed an obvious extension of the constructor code, but it is
certainly easy enough to leave it out if you prefer.
Nah, I wants it.
http://gwt-code-reviews.appspot.com/1420808/diff/1/user/src/com/google/gwt/uibinder/rebind/TypeOracleUtils.java#newcode65
user/src/com/google/gwt/uibinder/rebind/TypeOracleUtils.java:65: if
(method.isStatic() == isStatic methodName.equals(method.getName())
On 2011/04/22 01:13:17, rjrjr wrote:
could you put () around the == statement?
Done.
http://gwt-code-reviews.appspot.com/1420808/diff/1/user/src/com/google/gwt/uibinder/rebind/TypeOracleUtils.java#newcode78
user/src/com/google/gwt/uibinder/rebind/TypeOracleUtils.java:78: *
@param argType
On 2011/04/22 01:13:17, rjrjr wrote:
What's the difference between a paramType and an argType? Seems like
the
distinction matters here, but param and arg look like synonyms to me.
paramType is the type of the formal parameter declaration, while argType
is the type of the argument to be passed in.
Should the args be leftHandType and rightHandType? This applies
throughout the
class of course, not just here.
That would work, although the terminology seems a bit odd in the context
of a method call.
I guess param and arg make sense. I've never decoupled them before.
http://gwt-code-reviews.appspot.com/1420808/diff/1/user/src/com/google/gwt/uibinder/rebind/TypeOracleUtils.java#newcode140
user/src/com/google/gwt/uibinder/rebind/TypeOracleUtils.java:140:
JType[] ctorArgTypes = paramTypes;
On 2011/04/22 01:13:17, rjrjr wrote:
Looks like ctorArgTypes is refactoring chaff, should be inlined.
Yes, I created it just so I could inline it, then never did :). Done.
http://gwt-code-reviews.appspot.com/1420808/diff/1/user/src/com/google/gwt/uibinder/rebind/TypeOracleUtils.java#newcode157
user/src/com/google/gwt/uibinder/rebind/TypeOracleUtils.java:157: } else
{
On 2011/04/22 01:13:17, rjrjr wrote:
else if
Done.
http://gwt-code-reviews.appspot.com/1420808/diff/1/user/test/com/google/gwt/uibinder/rebind/TypeOracleUtilsTest.java
File user/test/com/google/gwt/uibinder/rebind/TypeOracleUtilsTest.java
(right):
http://gwt-code-reviews.appspot.com/1420808/diff/1/user/test/com/google/gwt/uibinder/rebind/TypeOracleUtilsTest.java#newcode29
user/test/com/google/gwt/uibinder/rebind/TypeOracleUtilsTest.java:29:
On 2011/04/22 01:13:17, rjrjr wrote:
import static com.google.gwt.uibinder.rebind.TypeOracleUtils.*; might
make this
easier to read.
Done.
http://gwt-code-reviews.appspot.com/1420808/diff/1/user/test/com/google/gwt/uibinder/rebind/TypeOracleUtilsTest.java#newcode145
user/test/com/google/gwt/uibinder/rebind/TypeOracleUtilsTest.java:145:
public void testHasCompatibleMethod() {
On 2011/04/22 01:13:17, rjrjr wrote:
Many lines too long. Did you auto format?
Yes, although I did make some edits since then. The @link tags can't be
split, and Reitveld is wrapping at 80 instead of 100. I will make sure
it is formatted properly before submitting.
I have Rietveld set to 100. There's a setting for that.
http://gwt-code-reviews.appspot.com/1420808/diff/1/user/test/com/google/gwt/uibinder/rebind/TypeOracleUtilsTest.java#newcode146
user/test/com/google/gwt/uibinder/rebind/TypeOracleUtilsTest.java:146:
assertTrue(didn't find static m1(Child),
TypeOracleUtils.hasCompatibleMethod(foo, true, m1,
On 2011/04/22 01:13:17, rjrjr wrote:
s/didn't find/Should have found/
Done.
[gwt-contrib] Re: Create a utility class for checking assignability of types for use (issue1420808)
http://gwt-code-reviews.appspot.com/1420808/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Create a utility class for checking assignability of types for use (issue1420808)
http://gwt-code-reviews.appspot.com/1420808/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Create a utility class for checking assignability of types for use (issue1420808)
LGTM On Thu, Apr 21, 2011 at 9:24 PM, j...@google.com wrote: http://gwt-code-reviews.appspot.com/1420808/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
