Initial implementation of os:Flash tag

New feature "swfobject" added
Flash tag is a basic mapping of a call to swfobject.
Alternate content can be shown until its clicked (See play=onclick)
Flashvars can have data-pipelined values expanded and the security token is automatically bound
Tag works even if sanitization is enabled but scriptaccess and liveconnect are disabled and network access is restricted to internal
Add os:xFormEncode and os:xFormDecode to supoprt flashvar expansion

[etnu00, 2009-03-31 20:41:34]

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) {
GadgetContext is redundant now, since Gadget contains the context.

[levik, 2009-03-31 21:04:57]

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#newcode80
Line 80: err.setTextContent("Failed to process os:Flash tag: " +
re.getMessage());
Should we sanitize message? Adam mentioned the sanitizer doesn't look into text
nodes for HTML markup.

http://codereview.appspot.com/28158/diff/1/12#newcode180
Line 180: processor.evaluate(attr.getNodeValue(), String.class,
attr.getNodeValue()));
I think using AbstractTagHandler.getValueFromTag() here would be clearer

[awiner, 2009-03-31 23:52:23]

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...

[louiscryan, 2009-04-01 01:44:01]

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.

[louiscryan, 2009-04-01 01:44:41]

Updated patch

[awiner, 2009-04-01 15:07:01]

http://codereview.appspot.com/28158/diff/1011/2016
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/templates/FlashTagHandler.java
(right):

http://codereview.appspot.com/28158/diff/1011/2016#newcode184
Line 184: attr.getNodeValue()));
no need to pass attr.getNodeValue() as a default (in fact harmful).  Non-EL
values will get returned as the original value.