Reviewers: shindig-dev, opensocial.evan,
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,
On 2009/03/09 18:02:45, Evan Gilbert (code review) wrote:
Could you document this regex?
Moved out to not compile RegEx each time, and documented.
http://codereview.appspot.com/24071/diff/1/8#newcode151
Line 151: regex = os.regExps_.CONTEXT_VAR_REMOVAL;
On 2009/03/09 18:02:45, Evan Gilbert (code review) wrote:
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).
Done.
http://codereview.appspot.com/24071/diff/1/8#newcode216
Line 216: ret = void 0;
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/Special_Operators/void_Operator
http://codereview.appspot.com/24071/diff/1/8#newcode423
Line 423: if (isArray(compiledChild)) {
On 2009/03/09 18:02:45, Evan Gilbert (code review) wrote:
Where does the isArray() function come from?
Used to be in JST - moved to base.js and implemented in a way that works
for us.
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 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.
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
On 2009/03/09 18:02:45, Evan Gilbert (code review) wrote:
Think you can just leave out the @return statement. Was this suggested
by a
person or JavaScript tool?
Done.
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) {
On 2009/03/09 18:02:45, Evan Gilbert (code review) wrote:
Is this function actually used? I couldn't find out where.
I believe David used it somewhere to inject support for opensocial field
getters (i.e. ${person.interests} vs ${person.getField("INTERESTS")}.
Once that support goes away, we can remove this as well.
Description:
Refactored a bunch of client-side templates (and some data pipelining)
code in response to recent discussions and security review.
More to come..
Please review this at http://codereview.appspot.com/24071
Affected files:
features/src/main/javascript/features/opensocial-data-context/datacontext.js
features/src/main/javascript/features/opensocial-data/data.js
features/src/main/javascript/features/opensocial-templates/base.js
features/src/main/javascript/features/opensocial-templates/compiler.js
features/src/main/javascript/features/opensocial-templates/container.js
features/src/main/javascript/features/opensocial-templates/domutil.js
features/src/main/javascript/features/opensocial-templates/namespaces.js
features/src/main/javascript/features/opensocial-templates/os.js
features/src/main/javascript/features/opensocial-templates/template.js
features/src/main/javascript/features/opensocial-templates/util.js
features/src/main/javascript/features/xmlutil/xmlutil.js
features/src/test/javascript/features/opensocial-templates/domutil.js
features/src/test/javascript/features/opensocial-templates/index.html
features/src/test/javascript/features/opensocial-templates/template_test.js
features/src/test/javascript/features/opensocial-templates/util_test.js
