Google Analytics extern contrib

[Alan Leung, 2010-06-17 20:00:41]

Brian, please review the change as if it is a Google CL.

Andrew would be addressing your comments.

Thanks!

[bnkuhn, 2010-06-23 20:19:28]

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.

[Andrew Mattie, 2010-06-25 04:08:45]

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.

[Andrew Mattie, 2010-06-25 04:39:59]

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?

[Alan Leung, 2010-06-25 05:22:04]

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
>

[Alan Leung, 2010-06-25 06:49:27]

Patched! Please take another look.

[Andrew Mattie, 2010-06-25 14:57:34]

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?

[Alan Leung, 2010-06-25 17:54:28]

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?

[Alan Leung, 2010-06-25 21:38:16]

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?

[Alan Leung, 2010-07-09 16:59:44]

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?

[bnkuhn, 2010-07-12 14:53:14]

LGTM

[Alan Leung, 2010-07-12 18:16:15]

Thanks!

On Mon, Jul 12, 2010 at 7:53 AM, <bnkuhn@gmail.com> wrote:

> LGTM
>
>
> http://codereview.appspot.com/1704042/show
>