FYI - This is from me, I need to set a nickname on my codereview account. On Fri, Dec 12, 2008 at 11:27 AM, <[email protected]> wrote:
> Made a number of small edits to the patch before submitting. > > The only open items are: > - We should change constants to ALL_CAPS > - os.data.getDataContext().putDataSet() is quite verbose and we should > possibly propose spec alternatives. > > > http://codereview.appspot.com/10041/diff/1/7 > File features/opensocial-templates/base.js (right): > > http://codereview.appspot.com/10041/diff/1/7#newcode94 > Line 94: variableSubstitution: /^([^$]*)(\$\{[^\}]*\})([\w\W]*)$/ > Should constants be ALL_CAPS > > http://codereview.appspot.com/10041/diff/1/8 > File features/opensocial-templates/compiler.js (right): > > http://codereview.appspot.com/10041/diff/1/8#newcode176 > Line 176: 'Count': VAR_count, > Removing this "," - it will break IE > > http://codereview.appspot.com/10041/diff/1/8#newcode743 > Line 743: console.log(child); > Removing this and the following log statement, I assume these are > debugging artefacts > > http://codereview.appspot.com/10041/diff/1/9 > File features/opensocial-templates/container.js (right): > > http://codereview.appspot.com/10041/diff/1/9#newcode194 > Line 194: var beforeData = node.getAttribute('before'); > Changing to support both for backwards compatibility. We should update > demos and check with current clients of the pre-release templates before > removing. > > http://codereview.appspot.com/10041/diff/1/9#newcode202 > Line 202: var requiredData = node.getAttribute('require'); > ditto > > http://codereview.appspot.com/10041/diff/1/5 > File features/opensocial-templates/data.js (right): > > http://codereview.appspot.com/10041/diff/1/5#newcode69 > Line 69: // TODO(levik): This attribute may not be used by the handler. > Removed name on TODO, here an in places that were around already. > > http://codereview.appspot.com/10041/diff/1/5#newcode124 > Line 124: os.data.RequestDescriptor.prototype.getSendRequestClosure = > function() { > We should add a bind function - these callbacks will become clearer. > > http://codereview.appspot.com/10041/diff/1/5#newcode299 > Line 299: os.data.getDataContext = function() { > Spec issue - this is very verbose. I'm going to propose a spec change to > make this > os.data.put() and > os.data.get() > > http://codereview.appspot.com/10041/diff/1/5#newcode475 > Line 475: > gadgets.util.registerOnLoadHandler(os.data.processDocumentMarkup); > This was breaking standalone unit tests as gadgets was not defined - > wrapped it in if (window['gadgets'] && window['gadgets']['util']) {} > > http://codereview.appspot.com/10041/diff/1/6 > File features/opensocial-templates/template_test.js (right): > > http://codereview.appspot.com/10041/diff/1/6#newcode353 > Line 353: //os.createNamespace("custom", "#"); > Removed this line > > http://codereview.appspot.com/10041/diff/1/6#newcode357 > Line 357: //assertEquals('helloworld', domutil.getVisibleText(output)); > Reenabled this line > > http://codereview.appspot.com/10041 >
