Skip to content

Add support for AT Internet#1908

Closed
BenDz wants to merge 2 commits intoampproject:masterfrom
BenDz:atinternet-intgration
Closed

Add support for AT Internet#1908
BenDz wants to merge 2 commits intoampproject:masterfrom
BenDz:atinternet-intgration

Conversation

@BenDz
Copy link
Copy Markdown
Contributor

@BenDz BenDz commented Feb 10, 2016

No description provided.

@cramforce
Copy link
Copy Markdown
Member

@BenDz BenDz force-pushed the atinternet-intgration branch from 3320487 to 16ef72a Compare February 10, 2016 16:18
@rudygalfi
Copy link
Copy Markdown
Contributor

Did you see my note about adding ${ampdocUrl} and ${canonicalUrl} to your requests? Without this change, the only information you'll have to know the source page of the requests would be a referrer, which could not work in some cases.

For example, consider the URL https://cdn.ampproject.org/c/washingtonpost.com/amphtml/news/wonk/wp/2016/02/08/the-fairest-and-funniest-way-to-split-the-dinner-bill/, which is a Washington Post article served by the AMP Cache. With just a referrer-based tracking, you would attribute the traffic to cdn.ampproject.org. However, in this case, the appropriate attribution is to washingtonpost.com, which you can get via either of these variables: ${ampdocUrl} and ${canonicalUrl}.

@BenDz
Copy link
Copy Markdown
Contributor Author

BenDz commented Feb 10, 2016

Hi @rudygalfi - Yes, but unfortunately, we can't process it for the moment. I need to talk about that with our teams.
Thanks for the feedback anyway!

@cramforce
Copy link
Copy Markdown
Member

@BenDz There is one lint error (See travis). Please fix and this is good to go.

}
},

'atinternet': {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there seem to be two copies of the config. Can you remove one of them?

@BenDz BenDz force-pushed the atinternet-intgration branch from 206482e to 2522627 Compare February 11, 2016 07:31
@googlebot
Copy link
Copy Markdown

We found a Contributor License Agreement for you (the sender of this pull request) and all commit authors, but as best as we can tell these commits were authored by someone else. If that's the case, please add them to this pull request and have them confirm that they're okay with these commits being contributed to Google. If we're mistaken and you did author these commits, just reply here to confirm.

Trailing space removal
@BenDz BenDz force-pushed the atinternet-intgration branch from 2522627 to 4d5a5d0 Compare February 11, 2016 08:51
@amphtml-team
Copy link
Copy Markdown

@BenDz Wondering if you've had a chance to consider the proposed revision
yet? Any update you can offer? Thanks!
On Feb 10, 2016 11:31 PM, "googlebot" notifications@github.com wrote:

We found a Contributor License Agreement for you (the sender of this pull
request) and all commit authors, but as best as we can tell these commits
were authored by someone else. If that's the case, please add them to this
pull request and have them confirm that they're okay with these commits
being contributed to Google. If we're mistaken and you did author these
commits, just reply here to confirm.


Reply to this email directly or view it on GitHub
#1908 (comment).

You received this message because you are subscribed to the Google Groups
"amphtml-eng-github" group.
To unsubscribe from this group and stop receiving emails from it, send an
email to amphtml-eng-github+unsubscribe@google.com.
To post to this group, send email to amphtml-eng-github@google.com.
To view this discussion on the web visit
https://groups.google.com/a/google.com/d/msgid/amphtml-eng-github/ampproject/amphtml/pull/1908/c182747445%40github.com
https://groups.google.com/a/google.com/d/msgid/amphtml-eng-github/ampproject/amphtml/pull/1908/c182747445%40github.com?utm_medium=email&utm_source=footer
.

You received this message because you are subscribed to the Google Groups
"amphtml-eng" group.
To unsubscribe from this group and stop receiving emails from it, send an
email to amphtml-eng+unsubscribe@google.com.
To post to this group, send email to amphtml-eng@google.com.
To view this discussion on the web visit
https://groups.google.com/a/google.com/d/msgid/amphtml-eng/ampproject/amphtml/pull/1908/c182747445%40github.com
https://groups.google.com/a/google.com/d/msgid/amphtml-eng/ampproject/amphtml/pull/1908/c182747445%40github.com?utm_medium=email&utm_source=footer
.

@BenDz
Copy link
Copy Markdown
Contributor Author

BenDz commented Feb 12, 2016

@amphtml-team We just had a meeting, and we won't push ${ampdocUrl} or ${canonicalUrl} for the moment. The current version can be released. Thanks!

@amphtml-team
Copy link
Copy Markdown

@BenDz that sounds ok. Did you check the lint error that @cramforce
mentioned?
On Feb 12, 2016 8:12 AM, "Benjamin Diolez" notifications@github.com wrote:

@amphtml-team https://github.com/amphtml-team We just had a meeting,
and we won't push ${ampdocUrl} or ${canonicalUrl} for the moment. The
current version can be released. Thanks!


Reply to this email directly or view it on GitHub
#1908 (comment).

You received this message because you are subscribed to the Google Groups
"amphtml-eng-github" group.
To unsubscribe from this group and stop receiving emails from it, send an
email to amphtml-eng-github+unsubscribe@google.com.
To post to this group, send email to amphtml-eng-github@google.com.
To view this discussion on the web visit
https://groups.google.com/a/google.com/d/msgid/amphtml-eng-github/ampproject/amphtml/pull/1908/c183390673%40github.com
https://groups.google.com/a/google.com/d/msgid/amphtml-eng-github/ampproject/amphtml/pull/1908/c183390673%40github.com?utm_medium=email&utm_source=footer
.

You received this message because you are subscribed to the Google Groups
"amphtml-eng" group.
To unsubscribe from this group and stop receiving emails from it, send an
email to amphtml-eng+unsubscribe@google.com.
To post to this group, send email to amphtml-eng@google.com.
To view this discussion on the web visit
https://groups.google.com/a/google.com/d/msgid/amphtml-eng/ampproject/amphtml/pull/1908/c183390673%40github.com
https://groups.google.com/a/google.com/d/msgid/amphtml-eng/ampproject/amphtml/pull/1908/c183390673%40github.com?utm_medium=email&utm_source=footer
.

@BenDz
Copy link
Copy Markdown
Contributor Author

BenDz commented Feb 12, 2016

@amphtml-team This should be good in my last commit BenDz@4d5a5d0

@rudygalfi
Copy link
Copy Markdown
Contributor

@BenDz Thanks, see that. Can you rebase? It looks like there are conflicts to resolve.

@rudygalfi
Copy link
Copy Markdown
Contributor

@rudygalfi
Copy link
Copy Markdown
Contributor

@BenDz We merged your commit here: 4bfd2b5, so closing this pull request.

Please disregard my previous comments. We're all set here. Thanks!

@rudygalfi rudygalfi closed this Feb 12, 2016
@BenDz
Copy link
Copy Markdown
Contributor Author

BenDz commented Feb 15, 2016

@rudygalfi Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants