Fast Fetch, non-AMP creatives remove remove load promise requirement for removing loader #9056
Conversation
| this.baseInstance_.emitLifecycleEvent('adSlotUnhidden'); | ||
| } | ||
| return Promise.resolve(); | ||
| } else { |
There was a problem hiding this comment.
No need to have this as an else block, as the if block returns.
| @@ -96,6 +96,8 @@ export class AmpAdXOriginIframeHandler { | |||
| this.element_.creativeId = info.data.id; | |||
| }); | |||
|
|
|||
| // TODO(keithwrightbos) - move to within not A4A else block to ensure this | |||
There was a problem hiding this comment.
this API should be available to both fast fetch and delayed fetch, right? why is the TODO?
There was a problem hiding this comment.
TODO is because getHTML is arguably only for ad tags. For Fast fetch, no ad tag executes so no need to expose
| // bootstrap-loaded to indicate ad request was sent, either way we know | ||
| // that occurred for Fast Fetch. | ||
| this.element_.appendChild(this.iframe); | ||
| return Promise.resolve(); |
There was a problem hiding this comment.
I commented on the thread. This is not ideal. layoutCallback should return the promise most representative of loading time. To update rendering state we should simply call renderStart_. E.g.
this.element_.appendChild(this.iframe); // THIS IS AN EXISTING LINE
if (opt_isA4A) {
this.renderStart_(info);
renderStartResolve();
}
The same can be arranged via micro-tasks if it's more desirable to keep all render-start code together up top.
There was a problem hiding this comment.
Done @dvoytenko PTAL
…ed_remove_load_delay
…ed_remove_load_delay
…ed_remove_load_delay
| // Set iframe initially hidden which will be removed on render-start or | ||
| // load, whichever is earlier. | ||
| setStyle(this.iframe, 'visibility', 'hidden'); | ||
| this.baseInstance_.lifecycleReporter.addPingsForVisibility(this.element_); |
There was a problem hiding this comment.
Should we do this for the other case as well?
…for removing loader (ampproject#9056) * Remove load promise constraint for loader removal if Fast Fetch, non-AMP creative * refactor * review comments * fix test failures * fix test failure * fix test failure * fix test failure
Fast Fetch, non-AMP creatives require load before loading message is removed. This does not meet parity with delayed fetch that uses either bootstrap-loaded or render-start messages both of which serve to indicate ad tag has executed (and for render-start gotten an ad response). Given Fast Fetch does not write the xdomain frame until both of those have occurred, no need to wait.
cc/ @ampproject/a4a