http://codereview.appspot.com/28158/diff/1/2 File java/common/src/test/java/org/apache/shindig/expressions/OpensocialFunctionsTest.java (right):
http://codereview.appspot.com/28158/diff/1/2#newcode78 Line 78: String test = "He He"; maybe test a string that actually requires some encoding or decoding? http://codereview.appspot.com/28158/diff/1/12 File java/gadgets/src/main/java/org/apache/shindig/gadgets/templates/FlashTagHandler.java (right): http://codereview.appspot.com/28158/diff/1/12#newcode56 Line 56: static final String TAG_NAME = "Flash"; we should name this xFlash until it makes it into the spec http://codereview.appspot.com/28158/diff/1/12#newcode77 Line 77: } catch (RuntimeException re) { any more specific exception type possible? this'll catch internal errors too. if it needs to be this broad, log a warning. http://codereview.appspot.com/28158/diff/1/12#newcode92 Line 92: config.flashvars += "&st=" + Utf8UrlCoder.encode(processor.getTemplateContext(). flashvars might be empty, in which case the "&" is not needed. http://codereview.appspot.com/28158/diff/1/12#newcode106 Line 106: String altContentId = "altContent" + idGenerator.incrementAndGet(); something more specific than "altContent" as a prefix - "os__flashAlt"? http://codereview.appspot.com/28158/diff/1/12#newcode128 Line 128: altHolder.setAttribute("onclick", "javascript:" + altContentId + "()"); onclick doesn't need javascript: http://codereview.appspot.com/28158/diff/1/12#newcode145 Line 145: builder.append(", \""); i'd kill the extra spaces - no need for prettification http://codereview.appspot.com/28158/diff/1/12#newcode151 Line 151: builder.append(", \"").append(FLASH_MIN_VER).append("\", "); use + on this line - one append() call instead of three http://codereview.appspot.com/28158/diff/1/12#newcode159 Line 159: // Should not happend happend -> happen http://codereview.appspot.com/28158/diff/1/12#newcode170 Line 170: return (SwfObjectConfig)beanConverter.convertToObject(new JSONObject(params), space after ) http://codereview.appspot.com/28158/diff/1/12#newcode174 Line 174: Map<String, String> getAllAttrbutesLowerCase(Element tag, TemplateProcessor processor) { Attrbutes -> Attributes http://codereview.appspot.com/28158/diff/1/12#newcode176 Line 176: for (int i = tag.getAttributes().getLength() - 1; i >= 0; i--) { iterating backwards is odd http://codereview.appspot.com/28158/diff/1/12#newcode189 Line 189: Element head = (Element) DomUtil.getFirstNamedChildNode(doc.getDocumentElement(), "head"); TODO: should have this as part of Gadget API, handled by rewriter http://codereview.appspot.com/28158/diff/1/12#newcode334 Line 334: public void setSeamlesstabbing(Boolean seamlesstabbing) { read this as "seamless stabbing". Heh. http://codereview.appspot.com/28158/diff/1/7 File java/gadgets/src/test/java/org/apache/shindig/gadgets/templates/FlashTagHandlerTest.java (right): http://codereview.appspot.com/28158/diff/1/7#newcode84 Line 84: handler = new FlashTagHandler(new BeanJsonConverter(Guice.createInjector()), featureRegistry); if you're going to create an injector - might as well use ParseModule(), and use that to get the doc provider and parser. (You can inject the test itself to make it easier.) http://codereview.appspot.com/28158/diff/1/14 File javascript/samplecontainer/examples/templates/FlashTag.xml (right): http://codereview.appspot.com/28158/diff/1/14#newcode18 Line 18: <script xmlns:os="http://ns.opensocial.org/2008/markup"; type="text/os-template"> can move the ns to the div http://codereview.appspot.com/28158/diff/1/14#newcode19 Line 19: <os:Flash swf="http://www.adobe.com/shockwave/welcome/flash.swf"; width="500px" height="500px" bgcolor="#123456" menu="false" play="onclick" flashvars="${os:xUrlEncode(me.id)}=${os:xUrlEncode(me.name.familyName)}"> there's already a requirement to support automatic URL encoding of pieces of an EL expression (os:HttpRequest @href, for example). We may want to tackle this soon... http://codereview.appspot.com/28158
