|
|
Created:
14 years, 3 months ago by Alan Leung Modified:
14 years, 2 months ago Base URL:
http://closure-compiler.googlecode.com/svn/trunk/ Visibility:
Public. |
Patch Set 1 : Google Analytics extern contrib #
Total comments: 21
Patch Set 2 : Google Analytics extern contrib #MessagesTotal messages: 12
Brian, please review the change as if it is a Google CL. Andrew would be addressing your comments. Thanks!
Sign in to reply to this message.
Thanks for doing this. Just some minor comments... http://codereview.appspot.com/1704042/diff/2001/3001 File contrib/externs/google_analytics_api.js (right): http://codereview.appspot.com/1704042/diff/2001/3001#newcode34 contrib/externs/google_analytics_api.js:34: * @return {undefined} returns _ga_tracker http://codereview.appspot.com/1704042/diff/2001/3001#newcode54 contrib/externs/google_analytics_api.js:54: * @return {undefined} returns _ga_tracker http://codereview.appspot.com/1704042/diff/2001/3001#newcode68 contrib/externs/google_analytics_api.js:68: * @return {undefined} returns number http://codereview.appspot.com/1704042/diff/2001/3001#newcode81 contrib/externs/google_analytics_api.js:81: _ga_tracker.prototype._addIgnoredOrganic = function(newIgnoredOrganicKeyword) Generally, I'd rather see you break on "= " and start the next line with "funciton(...". Not terribly important though. http://codereview.appspot.com/1704042/diff/2001/3001#newcode86 contrib/externs/google_analytics_api.js:86: * @return {undefined} For methods with no return value, I though @return could be omitted. Maybe I'm wrong though. http://codereview.appspot.com/1704042/diff/2001/3001#newcode113 contrib/externs/google_analytics_api.js:113: * `_gat.GA_EComm_.Transactions_`, but that type is never defined. It's a private type, so I don't think we should have exposed this in the documentation. @return {Object} is probably fine for now. http://codereview.appspot.com/1704042/diff/2001/3001#newcode187 contrib/externs/google_analytics_api.js:187: * @return {undefined} returns string http://codereview.appspot.com/1704042/diff/2001/3001#newcode208 contrib/externs/google_analytics_api.js:208: * @return {string} returns undefined if no visitor custom var is found for the given index http://codereview.appspot.com/1704042/diff/2001/3001#newcode218 contrib/externs/google_analytics_api.js:218: // docs wrong Feel free to email me the link of any docs you think are wrong. I'm happy to check them and fix them if they need to be. http://codereview.appspot.com/1704042/diff/2001/3001#newcode384 contrib/externs/google_analytics_api.js:384: */ @param {string} newRate http://codereview.appspot.com/1704042/diff/2001/3001#newcode404 contrib/externs/google_analytics_api.js:404: * @deprecated It might be helpful to put all the deprecated methods at the bottom.
Sign in to reply to this message.
http://codereview.appspot.com/1704042/diff/2001/3001 File contrib/externs/google_analytics_api.js (right): http://codereview.appspot.com/1704042/diff/2001/3001#newcode34 contrib/externs/google_analytics_api.js:34: * @return {undefined} On 2010/06/23 20:19:28, bnkuhn wrote: > returns _ga_tracker Changed, but docs (http://goo.gl/3H39) only indicate creation, not return. http://codereview.appspot.com/1704042/diff/2001/3001#newcode54 contrib/externs/google_analytics_api.js:54: * @return {undefined} On 2010/06/23 20:19:28, bnkuhn wrote: > returns _ga_tracker Changed, but docs (http://goo.gl/JnK6) only indicate creation, not return. http://codereview.appspot.com/1704042/diff/2001/3001#newcode68 contrib/externs/google_analytics_api.js:68: * @return {undefined} On 2010/06/23 20:19:28, bnkuhn wrote: > returns number Changed, but docs (http://goo.gl/JK7s) don't reflect. It seems that this returns the number of exceptions thrown while executing the function(s) passed to it. http://codereview.appspot.com/1704042/diff/2001/3001#newcode81 contrib/externs/google_analytics_api.js:81: _ga_tracker.prototype._addIgnoredOrganic = function(newIgnoredOrganicKeyword) On 2010/06/23 20:19:28, bnkuhn wrote: > Generally, I'd rather see you break on "= " and start the next line with > "funciton(...". Not terribly important though. Done. http://codereview.appspot.com/1704042/diff/2001/3001#newcode86 contrib/externs/google_analytics_api.js:86: * @return {undefined} On 2010/06/23 20:19:28, bnkuhn wrote: > For methods with no return value, I though @return could be omitted. Maybe I'm > wrong though. There doesn't seem to be a consistent style for this. Some externs, like html5.js (http://goo.gl/HU1U), @return undefined. Other externs, like window.js (http://goo.gl/LxGD) don't specify @return. http://codereview.appspot.com/1704042/diff/2001/3001#newcode187 contrib/externs/google_analytics_api.js:187: * @return {undefined} On 2010/06/23 20:19:28, bnkuhn wrote: > returns string Done. http://codereview.appspot.com/1704042/diff/2001/3001#newcode208 contrib/externs/google_analytics_api.js:208: * @return {string} On 2010/06/23 20:19:28, bnkuhn wrote: > returns undefined if no visitor custom var is found for the given index Done. http://codereview.appspot.com/1704042/diff/2001/3001#newcode218 contrib/externs/google_analytics_api.js:218: // docs wrong On 2010/06/23 20:19:28, bnkuhn wrote: > Feel free to email me the link of any docs you think are wrong. I'm happy to > check them and fix them if they need to be. Will do. http://codereview.appspot.com/1704042/diff/2001/3001#newcode384 contrib/externs/google_analytics_api.js:384: */ On 2010/06/23 20:19:28, bnkuhn wrote: > @param {string} newRate Done. http://codereview.appspot.com/1704042/diff/2001/3001#newcode404 contrib/externs/google_analytics_api.js:404: * @deprecated On 2010/06/23 20:19:28, bnkuhn wrote: > It might be helpful to put all the deprecated methods at the bottom. Good point. Done.
Sign in to reply to this message.
It seems that because I don't own this issue, I can't attach my revised patch. upload.py says: Issue creation errors: {'user': ["You (amattie) don't own this issue (1704042)"]} Should I just email the patch to Alan, or am I missing something?
Sign in to reply to this message.
Please attach another patch and I'll update it. On Thu, Jun 24, 2010 at 9:39 PM, <amattie@gmail.com> wrote: > It seems that because I don't own this issue, I can't attach my revised > patch. upload.py says: > > Issue creation errors: {'user': ["You (amattie) don't own this issue > (1704042)"]} > > Should I just email the patch to Alan, or am I missing something? > > > http://codereview.appspot.com/1704042/show >
Sign in to reply to this message.
Patched! Please take another look.
Sign in to reply to this message.
On 2010/06/25 06:49:27, acleung wrote: > Patched! Please take another look. I already see an issue with _getLinkerUrl and _getVisitorCustomVar. Also, r1 of the patch I submitted ("Patch Set 3") was based against the trunk and not against "Patch Set 2," and so it looks like Set 2 was appended to the bottom. When I resubmit the patch, should I submit a patch against Set 3, or was Set 3 just a bad merge?
Sign in to reply to this message.
Oops I think I screwed up somewhere. Let me try to backtrack it to see if we can avoid losing all the comments. On 2010/06/25 14:57:34, amattie wrote: > On 2010/06/25 06:49:27, acleung wrote: > > Patched! Please take another look. > > I already see an issue with _getLinkerUrl and _getVisitorCustomVar. Also, r1 of > the patch I submitted ("Patch Set 3") was based against the trunk and not > against "Patch Set 2," and so it looks like Set 2 was appended to the bottom. > When I resubmit the patch, should I submit a patch against Set 3, or was Set 3 > just a bad merge?
Sign in to reply to this message.
There we go! Fixed! On 2010/06/25 17:54:28, Alan Leung wrote: > Oops I think I screwed up somewhere. Let me try to backtrack it to see if we can > avoid losing all the comments. > > On 2010/06/25 14:57:34, amattie wrote: > > On 2010/06/25 06:49:27, acleung wrote: > > > Patched! Please take another look. > > > > I already see an issue with _getLinkerUrl and _getVisitorCustomVar. Also, r1 > of > > the patch I submitted ("Patch Set 3") was based against the trunk and not > > against "Patch Set 2," and so it looks like Set 2 was appended to the bottom. > > When I resubmit the patch, should I submit a patch against Set 3, or was Set 3 > > just a bad merge?
Sign in to reply to this message.
ping? On 2010/06/25 21:38:16, Alan Leung wrote: > There we go! Fixed! > > On 2010/06/25 17:54:28, Alan Leung wrote: > > Oops I think I screwed up somewhere. Let me try to backtrack it to see if we > can > > avoid losing all the comments. > > > > On 2010/06/25 14:57:34, amattie wrote: > > > On 2010/06/25 06:49:27, acleung wrote: > > > > Patched! Please take another look. > > > > > > I already see an issue with _getLinkerUrl and _getVisitorCustomVar. Also, r1 > > of > > > the patch I submitted ("Patch Set 3") was based against the trunk and not > > > against "Patch Set 2," and so it looks like Set 2 was appended to the > bottom. > > > When I resubmit the patch, should I submit a patch against Set 3, or was Set > 3 > > > just a bad merge?
Sign in to reply to this message.
LGTM
Sign in to reply to this message.
Thanks! On Mon, Jul 12, 2010 at 7:53 AM, <bnkuhn@gmail.com> wrote: > LGTM > > > http://codereview.appspot.com/1704042/show >
Sign in to reply to this message.
|