I like the faster performance - though I strongly recommend checking out:
http://code.google.com/p/caliper/wiki/JavaMicrobenchmarks ... before you take results from a looping test as gospel (Caliper has some useful frameworks for gathering results). (Also, consider upgrading to JUEL 2.2.1, which has fixes and may have performance improvements.) The new dependency from DefaultTemplateProcessor to ShindigTypeConverter is ugly and dangerous - all know-how of how to handle type coercion needs to stay in org.apache.shindig.expressions (by a ValueExpression wrapper, if necessary), not into other code. Don't assume that all expression evaluation happens in DefaultTemplateProcessor. The critical thing is that Opensocial spec compliance is paramount, *not* TCK, so commenting out existing tests is a blocker. A major plus would be implementing this such that the choice of JUEL vs. Jasper is a Guice binding - support *both*, and let downstream vendors choose when/if to switch. That shouldn't be that hard - and if we reach a point where Jasper is clearly the right way to go, we can kill the JUEL support. In particular, it would let us do real-world testing of Jasper. For example, it's possible that Jasper uses less CPU but more memory than JUEL, and for some deployments that may be a critical difference. http://codereview.appspot.com/1376043/diff/1/4 File java/common/src/main/java/org/apache/shindig/expressions/ShindigTypeConverter.java (right): http://codereview.appspot.com/1376043/diff/1/4#newcode54 java/common/src/main/java/org/apache/shindig/expressions/ShindigTypeConverter.java:54: public static <T> T convert(Object obj, Class<T> type) throws ELException { Making these functions static is bad - best practices in Guice land is to avoid statics. Note the @Inject above - you're breaking anyone that's chosen to override the type converter. If you need to call this code from somewhere that didn't need it before, the proper fix is injecting an instance of this class into the target. http://codereview.appspot.com/1376043/diff/1/5 File java/common/src/test/java/org/apache/shindig/expressions/ExpressionsTest.java (right): http://codereview.appspot.com/1376043/diff/1/5#newcode134 java/common/src/test/java/org/apache/shindig/expressions/ExpressionsTest.java:134: /*...@test if you're going to suppress a test in JUnit 4, never comment it out. Add the @Ignore annotation, and include a String explaining why the tests is ignored. That said, these tests exist for a reason - they are required to comply with the Opensocial specification. Switching to a faster EL engine is a good thing, but you can only do so if you can make these tests pass. It's a non-starter if they don't (or you have to suppress them). http://codereview.appspot.com/1376043/diff/1/5#newcode223 java/common/src/test/java/org/apache/shindig/expressions/ExpressionsTest.java:223: ValueExpression expr= null; please avoid tabs in all Shindig code. http://codereview.appspot.com/1376043/diff/1/8 File java/gadgets/src/main/java/org/apache/shindig/gadgets/templates/DefaultTemplateProcessor.java (right): http://codereview.appspot.com/1376043/diff/1/8#newcode124 java/gadgets/src/main/java/org/apache/shindig/gadgets/templates/DefaultTemplateProcessor.java:124: resolver.setCtx(this.elContext); Comments as to what this is for? Why is the ELContext passed to the resolver ever wrong? http://codereview.appspot.com/1376043/diff/1/8#newcode487 java/gadgets/src/main/java/org/apache/shindig/gadgets/templates/DefaultTemplateProcessor.java:487: Object result = expr.getValue(elContext); shindig code pretty uniformly uses { } around all branches http://codereview.appspot.com/1376043/diff/1/8#newcode493 java/gadgets/src/main/java/org/apache/shindig/gadgets/templates/DefaultTemplateProcessor.java:493: return defaultValue; call this function only once http://codereview.appspot.com/1376043/diff/1/8#newcode494 java/gadgets/src/main/java/org/apache/shindig/gadgets/templates/DefaultTemplateProcessor.java:494: } odd indents http://codereview.appspot.com/1376043/diff/1/9 File java/gadgets/src/main/java/org/apache/shindig/gadgets/templates/TemplateELResolver.java (right): http://codereview.appspot.com/1376043/diff/1/9#newcode57 java/gadgets/src/main/java/org/apache/shindig/gadgets/templates/TemplateELResolver.java:57: public void setCtx(ELContext ctx) { Name this method setContext(), or setELContext(); eschew abbreviations. That said: can you add Javadoc explaining the conditions under which you'd need this, or why it's necessary below? It's highly unusual to ignore the ELContext passed to a function (and why only in some ELResolver APIs?). http://codereview.appspot.com/1376043/diff/1/9#newcode58 java/gadgets/src/main/java/org/apache/shindig/gadgets/templates/TemplateELResolver.java:58: this.ctx = ctx; odd indent here. http://codereview.appspot.com/1376043/diff/1/9#newcode98 java/gadgets/src/main/java/org/apache/shindig/gadgets/templates/TemplateELResolver.java:98: context = ctx; Add a comment explaining what's going on here. http://codereview.appspot.com/1376043/show
