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.

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

Launch AdSense Canonical Impl Support #23620

Merged

Conversation

keithwrightbos
Copy link
Contributor

Launch usage of amp-ad-network-adsense-impl for Adsense canonical (non-AMP cache) traffic currently relying on delayed fetch.

@lannka lannka self-requested a review July 31, 2019 15:57
Copy link
Contributor

@lannka lannka left a comment

Choose a reason for hiding this comment

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

Good clean up. One question.

isExperimentOn(this.win_, 'inabox-position-api')
isExperimentOn(this.win_, 'inabox-position-api') ||
(/^adsense$/i.test(this.element_.getAttribute('type')) &&
!isGoogleAdsA4AValidEnvironment(this.win_))
Copy link
Contributor

Choose a reason for hiding this comment

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

do you know if AdSense using this position API? It's a bit weird to me that this opt-in based experiment is fully enabled to all AdSense A4A ineligible traffic?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was added to ensure that AMP creatives returned would have access to this information. Since there is no way to know (or trust) that the creative is or is not AMP, we decided to just give access to all AdSense canonical creatives.

Copy link
Contributor

Choose a reason for hiding this comment

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

thx for explaining. would you pls add it as a comment to the code?

isExperimentOn(this.win_, 'inabox-position-api')
isExperimentOn(this.win_, 'inabox-position-api') ||
(/^adsense$/i.test(this.element_.getAttribute('type')) &&
!isGoogleAdsA4AValidEnvironment(this.win_))
Copy link
Contributor

Choose a reason for hiding this comment

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

thx for explaining. would you pls add it as a comment to the code?

@keithwrightbos keithwrightbos merged commit 119aaa2 into ampproject:master Aug 1, 2019
@keithwrightbos keithwrightbos deleted the adsense_canonical_launch branch August 1, 2019 13:33
thekorn pushed a commit to edelight/amphtml that referenced this pull request Sep 11, 2019
* Launch AdSense Canonical Impl Support

* remove adsense delay fetch test

* improve comment related to position API observer

* fix test failure

* fix lint failure

* fix test failure due to AdSense upgrade

* fix test failure due to AdSense upgrade
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants