Enable AMP support for tag detection.

[adamsilverstein]

Fixes ##206.

• Add amp cdn url to tag detection
• Add isAMPTag check that finds the required amp-ad-0.js tag.
• Call isAMPTag in isGptTag (to avoid adding across codebase for calls to isGptTag (I can change this if preferred).
• Include Fetch ResourceType in isGptAdRequest so AMP ads are matched.

[adamsilverstein]

@warrengm & @jonkeller - Appreciate any feedback here and also a check for the tags I am detecting to make sure they are right. I'll test against a few sites to see if the results make sense.

[adamsilverstein]

This is ready for some additional review/feedback.

[jonkeller]

Copying my initial July 8 review from chat to here, for posterity. No action required.

* I would prefer if isAmpTag were not inside of isGptTag, but rather they remain siblings and if necessary there be a new method isGptOrAmpTag. But, that's just my vote. Warren can veto it if he wants 🙂
* If he does veto, please do update the JSDoc comment of isGptTag.
* Ditto both previous bullet points with isGptAdRequest
* Is amp-ad-network-adsense-impl still a thing? Are there any other amp-ad-network-something-impls? I honestly don't remember if the former is used anymore, nor if the latter ever existed. I can research more in the morning if nobody knows off the top of their head. But see https://github.com/ampproject/amphtml/blob/b3cbf130aa611e787b231b51724da755059ef9cb/extensions/amp-ad-network-adsense-impl/0.1/amp-ad-network-adsense-impl.js#L69 and the comment on line 17 of the same file.

[adamsilverstein]

Updated based on feedback. I'm not sure if I have captured all of the places that need to check for AMP ads as well, so appreciate review there. Do you have a testbed or a way to reproduce the reporting for each test? it would be great to have a way to make sure each test is working correctly in AMP.

[warrengm]

One small comment but otherwise LGTM. Thanks!

[adamsilverstein]

@warrengm & @jonkeller - this is ready for another review!

[adamsilverstein]

note: broke into multiple lines because I was getting a pre-commit linting error about the line being too long.

[adamsilverstein]

I was able to clean this up a bit further, removing isAMPIframe and isGptOrAMPTag. Running reports against AMP sites now works as expected and I am getting valid looking reports that check out when I review the sites I have tested.

[jonkeller]

LGTM but please don't merge until Warren also says so.

[warrengm]

Thanks for adding support for AMP!

[adamsilverstein]

Thanks for the guidance, reviews and approval. Once someone merges this, do you know how long it would be (roughly) before it becomes available at https://developers.google.com/publisher-ads-audits?

[jonkeller]

I was about to type out a long answer to this, but instead I found this awesome visualization: https://amp.dev/documentation/guides-and-tutorials/contribute/vendor-contributions/release-schedule/

…

On Wed, Jul 22, 2020 at 6:12 PM Adam Silverstein ***@***.***> wrote: Thanks for the guidance, reviews and approval. Once someone merges this, do you know how long it would be (roughly) before it becomes available at https://developers.google.com/publisher-ads-audits? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#207 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AASXMB6TIP5M6BKTFTQ3PHDR45P45ANCNFSM4OVA3HBA> .

[adamsilverstein]

Excellent, thanks for clarifying!

[jonkeller]

Actually, my apologies, what I said about the release schedule was wrong. That was for AMP itself, whereas your change would go in according to the release schedule of this plugin, which is more ad-hoc. Sorry for the confusion!

…

On Thu, Jul 23, 2020 at 8:56 AM Adam Silverstein ***@***.***> wrote: Excellent, thanks for clarifying! — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#207 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AASXMB45PKD4EELYGQXSEVLR5AXPPANCNFSM4OVA3HBA> .