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"; On 2009/03/31 23:52:23, awiner wrote:
maybe test a string that actually requires some encoding or decoding?
He He -> He+He Made this more obvious however. 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"; On 2009/03/31 23:52:23, awiner wrote:
we should name this xFlash until it makes it into the spec
Done. http://codereview.appspot.com/28158/diff/1/12#newcode77 Line 77: } catch (RuntimeException re) { On 2009/03/31 23:52:23, awiner wrote:
any more specific exception type possible? this'll catch internal
errors too.
if it needs to be this broad, log a warning.
Unfortunately not, BeanJsonConverter upconverts everything to runtime exception http://codereview.appspot.com/28158/diff/1/12#newcode80 Line 80: err.setTextContent("Failed to process os:Flash tag: " + re.getMessage()); On 2009/03/31 21:04:58, levik wrote:
Should we sanitize message? Adam mentioned the sanitizer doesn't look
into text
nodes for HTML markup.
Yes, done http://codereview.appspot.com/28158/diff/1/12#newcode92 Line 92: config.flashvars += "&st=" + Utf8UrlCoder.encode(processor.getTemplateContext(). On 2009/03/31 23:52:23, awiner wrote:
flashvars might be empty, in which case the "&" is not needed.
Done. http://codereview.appspot.com/28158/diff/1/12#newcode106 Line 106: String altContentId = "altContent" + idGenerator.incrementAndGet(); On 2009/03/31 23:52:23, awiner wrote:
something more specific than "altContent" as a prefix -
"os__flashAlt"? Done. http://codereview.appspot.com/28158/diff/1/12#newcode128 Line 128: altHolder.setAttribute("onclick", "javascript:" + altContentId + "()"); On 2009/03/31 23:52:23, awiner wrote:
onclick doesn't need javascript:
Done. http://codereview.appspot.com/28158/diff/1/12#newcode145 Line 145: builder.append(", \""); On 2009/03/31 23:52:23, awiner wrote:
i'd kill the extra spaces - no need for prettification
Done. http://codereview.appspot.com/28158/diff/1/12#newcode151 Line 151: builder.append(", \"").append(FLASH_MIN_VER).append("\", "); On 2009/03/31 23:52:23, awiner wrote:
use + on this line - one append() call instead of three
Done. http://codereview.appspot.com/28158/diff/1/12#newcode159 Line 159: // Should not happend On 2009/03/31 23:52:23, awiner wrote:
happend -> happen
Done. http://codereview.appspot.com/28158/diff/1/12#newcode170 Line 170: return (SwfObjectConfig)beanConverter.convertToObject(new JSONObject(params), On 2009/03/31 23:52:23, awiner wrote:
space after )
Done. http://codereview.appspot.com/28158/diff/1/12#newcode174 Line 174: Map<String, String> getAllAttrbutesLowerCase(Element tag, TemplateProcessor processor) { On 2009/03/31 23:52:23, awiner wrote:
Attrbutes -> Attributes
Done. http://codereview.appspot.com/28158/diff/1/12#newcode176 Line 176: for (int i = tag.getAttributes().getLength() - 1; i >= 0; i--) { On 2009/03/31 23:52:23, awiner wrote:
iterating backwards is odd
Done. http://codereview.appspot.com/28158/diff/1/12#newcode180 Line 180: processor.evaluate(attr.getNodeValue(), String.class, attr.getNodeValue())); On 2009/03/31 21:04:58, levik wrote:
I think using AbstractTagHandler.getValueFromTag() here would be
clearer Seeing as Im already iterating all the attributes here already thats probably a net loss here. http://codereview.appspot.com/28158/diff/1/12#newcode189 Line 189: Element head = (Element) DomUtil.getFirstNamedChildNode(doc.getDocumentElement(), "head"); On 2009/03/31 23:52:23, awiner wrote:
TODO: should have this as part of Gadget API, handled by rewriter
Done. http://codereview.appspot.com/28158/diff/1/10 File java/gadgets/src/main/java/org/apache/shindig/gadgets/templates/TemplateContext.java (right): http://codereview.appspot.com/28158/diff/1/10#newcode45 Line 45: public TemplateContext(GadgetContext gadgetContext, Gadget gadget, Map<String, JSONObject> top) { On 2009/03/31 20:41:35, etnu00 wrote:
GadgetContext is redundant now, since Gadget contains the context.
Done. 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); On 2009/03/31 23:52:23, awiner wrote:
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.)
Done. http://codereview.appspot.com/28158
