Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Enable AMP support for tag detection. #207

Merged
merged 13 commits into from
Jul 23, 2020
Merged

Enable AMP support for tag detection. #207

merged 13 commits into from
Jul 23, 2020

Conversation

adamsilverstein
Copy link
Contributor

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.

@googlebot googlebot added the cla: yes CLA has been signed label Jul 8, 2020
@adamsilverstein
Copy link
Contributor Author

@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.

@warrengm warrengm requested a review from jonkeller July 9, 2020 13:46
@adamsilverstein adamsilverstein marked this pull request as ready for review July 14, 2020 20:53
@adamsilverstein
Copy link
Contributor Author

This is ready for some additional review/feedback.

@jonkeller
Copy link
Collaborator

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
Copy link
Contributor Author

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
warrengm requested changes Jul 20, 2020
View reviewed changes
Copy link
Contributor

@warrengm warrengm left a comment

Choose a reason for hiding this comment

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

One small comment but otherwise LGTM. Thanks!

@adamsilverstein adamsilverstein changed the title WIP: Enable AMP support for tag detection. Enable AMP support for tag detection. Jul 20, 2020
@adamsilverstein
Copy link
Contributor Author

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

adamsilverstein
adamsilverstein commented Jul 22, 2020
View reviewed changes
lighthouse-plugin-publisher-ads/utils/resource-classification.js
@@ -203,7 +272,9 @@ function isAdScript(url) {
* @return {boolean}
*/
function isAdRequest(request) {
return isAdSenseAdRequest(request) || isGptAdRequest(request);
return isAdSenseAdRequest(request) ||
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

@adamsilverstein
Copy link
Contributor Author

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
jonkeller approved these changes Jul 22, 2020
View reviewed changes
Copy link
Collaborator

@jonkeller jonkeller left a comment

Choose a reason for hiding this comment

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

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

warrengm
warrengm approved these changes Jul 22, 2020
View reviewed changes
Copy link
Contributor

@warrengm warrengm left a comment

Choose a reason for hiding this comment

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

Thanks for adding support for AMP!

@adamsilverstein
Copy link
Contributor Author

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
Copy link
Collaborator

jonkeller commented Jul 22, 2020 via email

@adamsilverstein
Copy link
Contributor Author

Excellent, thanks for clarifying!

@jonkeller jonkeller merged commit 3eb88e2 into googleads:master Jul 23, 2020
@jonkeller
Copy link
Collaborator

jonkeller commented Jul 23, 2020 via email

@warrengm warrengm mentioned this pull request Oct 9, 2020
Closed
@warrengm warrengm added the v1.2 label Oct 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jburger424 jburger424 jburger424 left review comments

@jonkeller jonkeller jonkeller approved these changes

@warrengm warrengm warrengm approved these changes

Assignees
No one assigned
Labels
cla: yes CLA has been signed v1.2
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@adamsilverstein @jonkeller @jburger424 @warrengm @googlebot