Amp-Ad upgrade: move remote.html existence check before network predicate execution #8249
Conversation
|
Hi, ampproject bot here! Here are a list of the owners that can approve your files. You may leave an issue comment stating "@ampprojectbot retry!" to force me to re-evaluate this Pull Request's status /to lannka zhouyx
For any issues please file a bug at https://github.com/google/github-owners-bot/issues |
keithwrightbos
changed the title
| @@ -76,8 +76,8 @@ export class AmpAd extends AMP.BaseElement { | |||
|
|
|||
| // TODO(tdrl): Check amp-ad registry to see if they have this already. | |||
| if (!a4aRegistry[type] || | |||
| !a4aRegistry[type](this.win, this.element) || | |||
| this.win.document.querySelector('meta[name=amp-3p-iframe-src]')) { | |||
| this.win.document.querySelector('meta[name=amp-3p-iframe-src]') || | |||
There was a problem hiding this comment.
Nit: I would put this check very first, so that the a4aRegistry[type] goes right beside a4aRegistry[type](this.win, this.element), which is the more common idiom I think.
There was a problem hiding this comment.
I put it this way for minuscule performance reason where presumably the check for type existence in the config object is much less expensive than querying the DOM for remote.html meta existence.
| !a4aRegistry[type](this.win, this.element) || | ||
| this.win.document.querySelector('meta[name=amp-3p-iframe-src]')) { | ||
| this.win.document.querySelector('meta[name=amp-3p-iframe-src]') || | ||
| !a4aRegistry[type](this.win, this.element)) { |
There was a problem hiding this comment.
shall we fix the side effect of this function call? or somehow give it a clearer naming.
For Fast Fetch network support selection within AmpAd element, a predicate is registered within the network config that when executed returns whether Fast Fetch flow should be used. In the case of AdSense & Doubleclick, the function execution includes selection into control or experiment to isolate the impact of Fast vs Delayed Fetch. Currently Fast Fetch does not support remote.html usage due to its nature of being custom JS and as such should be carved out of valid traffic for Fast Fetch selection. This check was moved out of Doubleclick specific predicate to general AmpAd upgrade within #8248 when it was discovered that publishers were also using remote.html for AdSense. However, when it was moved, it was placed AFTER the predicate execution causing selection into the Fast Fetch experiment to not actually result in a Fast Fetch request.
cc @ampproject/a4a