Race between render-start & no-content #4720
Conversation
zhouyx
force-pushed
the
race-render-start-no-content
branch
from
6f9f8eb
to
966d24e
Compare
|
PTAL |
| @@ -120,21 +116,52 @@ export class AmpAdApiHandler { | |||
| } | |||
| }, this.is3p_, this.is3p_)); | |||
|
|
|||
| // Install API that listen to ad response | |||
| if (waitForRenderStart.indexOf(this.baseInstance_.adType) >= 0) { | |||
| this.isSupportRenderStart_ = true; | |||
There was a problem hiding this comment.
seems it can be a local variable?
zhouyx
force-pushed
the
race-render-start-no-content
branch
from
966d24e
to
aae1360
Compare
|
fixed. Also change the behavior of |
| @@ -172,6 +175,7 @@ export class AmpAd3PImpl extends AMP.BaseElement { | |||
| // We always need the bootstrap. | |||
| preloadBootstrap(this.win); | |||
| const type = this.element.getAttribute('type'); | |||
| this.adType = type; | |||
There was a problem hiding this comment.
We could actually move this up into #buildCallback
| unlistenList[i](); | ||
| } | ||
| msgReceived = true; | ||
| resolve({message, data, source, origin}); |
There was a problem hiding this comment.
does data already contains message?
There was a problem hiding this comment.
right! use data.type instead
|
fixed, PTAL |
| @@ -13,5 +13,6 @@ npm-debug.log | |||
| .tm_properties | |||
| .settings | |||
| build-system/runner/TESTS-TestSuites.xml | |||
| .vscode/ | |||
There was a problem hiding this comment.
hmmm, how is .gitignore not ignoring itself
| resolve(); | ||
| }); | ||
| IframeHelper.listenForOncePromise(testIframe, | ||
| ['no-msg', 'send-intersections', 'send-intersections']) |
There was a problem hiding this comment.
hmm, having the duplicated message is not a valid use case. the behavior is in-deterministic and we don't really care, so there's no need to have a test to protect it.
| * @param {boolean=} opt_is3P | ||
| * @return {!Promise} | ||
| * @return {!Promise<Object{data, source, origin}>} |
There was a problem hiding this comment.
I think the right syntax is: {!Promise<!{data, source, origin}>}
|
LGTM |
| // If support render-start, create a race between render-start no-content | ||
| this.adResponsePromise_ = listenForOncePromise(this.iframe_, | ||
| ['render-start', 'no-content'], this.is3p_).then(info => { | ||
| if (info.data.type == 'render-start') { |
There was a problem hiding this comment.
Is data guaranteed to be an object?
There was a problem hiding this comment.
https://github.com/ampproject/amphtml/blob/master/src/iframe-helper.js#L212
Here datamust have a data.type to get the correct listener
also https://github.com/ampproject/amphtml/blob/master/src/iframe-helper.js#L271
here we use data.sentinel directly.
Listen to
bootstrap-loadedif ad server does not supportrender-start. Listen tono-contentas before.Listen to both
render-startandno-contentif ad server supportsrender-start. Remove listener for the other after receiving one message.Tests added.
#4022
This is part of the work that improve ad loading behavior #4022