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
Disable remote HTML usage for AdSense Delayed Fetch #10180
Disable remote HTML usage for AdSense Delayed Fetch #10180
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor changes, otherwise lgtm
const meta = parentWindow.document | ||
.querySelector('meta[name="amp-3p-iframe-src"]'); | ||
if (!meta) { | ||
return null; | ||
} | ||
if (opt_disallowCustom) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should move this check above the previous one, so we don't do an unnecessary querySelector()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't because I want to warn if remote HTML is specified so either way I need the query selector
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah I see ok
@@ -229,7 +231,8 @@ export class AmpAd3PImpl extends AMP.BaseElement { | |||
// incrementLoadingAds(). | |||
this.emitLifecycleEvent('adRequestStart'); | |||
const iframe = getIframe(this.element.ownerDocument.defaultView, | |||
this.element, undefined, opt_context); | |||
this.element, this.element.getAttribute('type'), opt_context, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this.element.getAttribute('type') gets called three times in this class. Can we store the type as an attribute directly on the class instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
const meta = parentWindow.document | ||
.querySelector('meta[name="amp-3p-iframe-src"]'); | ||
if (!meta) { | ||
return null; | ||
} | ||
if (opt_disallowCustom) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah I see ok
* Disable remote HTML usage for AdSense Delayed Fetch * lint fixes * lint fixes * test fixes, PR feedback * fix test failures * test fixes
Allow for configuring if network supports remote HTML usage via remoteHTMLDisabled option and enable for AdSense. Also ensure that Fast Fetch is allowed for such networks even if remote HTML is specified.