Client Template Refactoring

Refactored a bunch of client-side templates (and some data pipelining) code in response to recent discussions and security review.

More to come..

[Evan Gilbert (code review), 2009-03-09 18:02:44]

http://codereview.appspot.com/24071/diff/1/8
File features/src/main/javascript/features/opensocial-templates/compiler.js
(right):

http://codereview.appspot.com/24071/diff/1/8#newcode107
Line 107: return
src.replace(/"(?:[^"\\]|\\.)*"|'(?:[^'\\]|\\.)*'|\w+|[^"'\w]+/g,
Could you document this regex?

http://codereview.appspot.com/24071/diff/1/8#newcode151
Line 151: regex = os.regExps_.CONTEXT_VAR_REMOVAL;
Any reason for the lazy initialization here? Seems to complicate the code (if
you want to have the reference for the regexps close by, you could declare the
constants above the function).

http://codereview.appspot.com/24071/diff/1/8#newcode216
Line 216: ret = void 0;
Are you using this to return undefined instead of null?

If so, do you have a reference for this? I've seen "void(0)" used with URLs and
haven't seen "void 0". Want to make sure that browsers don't choke on this.

If we're not sure about the void syntax, might be easier to do direct return
statements (i.e. "return;")

http://codereview.appspot.com/24071/diff/1/8#newcode423
Line 423: if (isArray(compiledChild)) {
Where does the isArray() function come from?

http://codereview.appspot.com/24071/diff/1/14
File features/src/main/javascript/features/opensocial-templates/container.js
(right):

http://codereview.appspot.com/24071/diff/1/14#newcode83
Line 83: os.Container.disableAutoProcessing = function() {
Usually cleaner to have setAutoProcessing(true|false).

http://codereview.appspot.com/24071/diff/1/9
File features/src/main/javascript/features/opensocial-templates/template.js
(right):

http://codereview.appspot.com/24071/diff/1/9#newcode126
Line 126: * @return void
Think you can just leave out the @return statement. Was this suggested by a
person or JavaScript tool?

http://codereview.appspot.com/24071/diff/1/11
File features/src/main/javascript/features/opensocial-templates/util.js (right):

http://codereview.appspot.com/24071/diff/1/11#newcode87
Line 87: os.convertToCamelCase = function(str) {
Is this function actually used? I couldn't find out where.

[Evan Gilbert (code review), 2009-03-10 17:52:28]

Committed...

http://codereview.appspot.com/24071/diff/1/8
File features/src/main/javascript/features/opensocial-templates/compiler.js
(right):

http://codereview.appspot.com/24071/diff/1/8#newcode216
Line 216: ret = void 0;
On 2009/03/09 21:10:12, levik wrote:
> On 2009/03/09 18:02:45, Evan Gilbert (code review) wrote:
> > Are you using this to return undefined instead of null?
> > 
> > If so, do you have a reference for this? I've seen "void(0)" used with URLs
> and
> > haven't seen "void 0". Want to make sure that browsers don't choke on this.
> > 
> > If we're not sure about the void syntax, might be easier to do direct return
> > statements (i.e. "return;")
> 
> void is a unary operator that returns "undefined", so void(0) === void 0.
> 
> More info here:
>
https://developer.mozilla.org/en/Core_JavaScript_1.5_Reference/Operators/Spec...
> 

I'm going to check this in with void(0) because I'm a little concerned about
browser support - I haven't seen this construct used much while I've seen
href="javascript:void(0)" frequently. If "void 0" is a better syntax and you're
confident in browser support, let me know and I'll switch.

http://codereview.appspot.com/24071/diff/1/14
File features/src/main/javascript/features/opensocial-templates/container.js
(right):

http://codereview.appspot.com/24071/diff/1/14#newcode83
Line 83: os.Container.disableAutoProcessing = function() {
On 2009/03/09 21:10:12, levik wrote:
> On 2009/03/09 18:02:45, Evan Gilbert (code review) wrote:
> > Usually cleaner to have setAutoProcessing(true|false).
> 
> I specifically wanted this to be a one-way toggle - enabling auto processing
is
> ambiguous. If you disable it, you have to do it manually (by calling
.process()
> ) at a later time.

Checking in for now, but still think the set would be better. Every time I've
had a one-way boolean set I always end up needing the other - for example, if
the gadget sets "disableAutoProcessing = true", but one view wants
auto-processing, there's no way to handle this.

Not sure what the ambiguity is - setAutoProcessing(true) seems pretty clear.