Separate render-start and loaded phases for Ads #7391

dvoytenko · 2017-02-07T02:09:43Z

This is a significant overhaul for loading. The loading is split into two phases: render-start and load. Render-start is used to update UX for loading indicator and sticky ad functionality.

Some notes:

Signals are now a more precise and asynchronous way to wait for particular element states, such as built and render-start.
amp-sticky-ad:0.1 will be redone in this PR, after review of 1.0.
Cleanup of events will have to be done after amp-sticky-ad changes in prod. See Cleanup amp:built and amp:load events in favor of signals #7389.
render-start will continue to be optional signal for now so we have to use both: render-start and load-end in most of cases.

zhouyx · 2017-02-07T17:30:24Z

extensions/amp-ad/0.1/amp-ad-xorigin-iframe-handler.js

-      // If support render-start, create a race between render-start no-content
-      this.adResponsePromise_ = listenForOncePromise(this.iframe,
+    // Calculate render-start and no-content signal. These signals are mutually
+    // exlcusive. Whichever arrives first wins.


nit: exclusive

zhouyx · 2017-02-07T17:46:55Z

src/custom-element.js

@@ -1520,6 +1542,11 @@ function createBaseCustomElementClass(win) {
     */
    toggleLoading_(state, opt_cleanup) {
      assertNotTemplate(this);
+      if (state &&
+          (this.layoutCount_ > 0 || this.signals_.get('render-start'))) {


this.layoutCount_ > 0 is same as this.isFirstLayoutCompleted_? Personally feel the other one is more clear

No. I had a "gotcha" with this as well. The old code uses layoutCount_ > 0 everywhere. I simply moved it here as a more central place. Basically what happens is that layoutCount_ is like an attempt - it's always incremented: failure or success. While isFirstLayoutCompleted_ only set on success.

Ha! Good catch

zhouyx · 2017-02-07T18:09:20Z

extensions/amp-ad/0.1/amp-ad-xorigin-iframe-handler.js

-      // If NOT support render-start, listen to bootstrap-loaded no-content
-      // respectively
-      this.adResponsePromise_ = listenForOncePromise(this.iframe,
-        'bootstrap-loaded', true);


bootstrap-loaded is not used anywhere then, we can remove it from integration.js as well.

I'm thinking about bringing it back in the similar fashion we had before. It's not a very decisive points since we are strongly moving toward having render-start in all of the important cases.

I don't feel bootstrap-loaded is useful even we bring it back though.

I described in comments below and in the code why it's advantageous to keep it. I'd rather go with it and we can reconsider later.

zhouyx · 2017-02-07T18:25:41Z

extensions/amp-ad/0.1/amp-ad-xorigin-iframe-handler.js

@@ -125,60 +122,78 @@ export class AmpAdXOriginIframeHandler {
      this.sendEmbedInfo_(this.baseInstance_.isInViewport());
    }));

+    // Iframe.onload normally called by the Ad after full load.
+    const iframeLoadPromise = loadPromise(this.iframe).then(() => {


I don't quite get the change here.
From what I understand, the order should be load-start render-start load-end.
But here I think iframeLoadPromise can be resolved before renderStartPromise. Thus it is possible that we resolve layoutCallback too soon here?

Possible. But in reality this almost never happens. The only real situation I saw is when render-start and onload are triggered at the same time. In that case, onload arrives a few milliseconds earlier.

if onload always arrive after, then it makes sense.

I went through about a dozen of Ad networks and x20 ad impressions. I had onload arrive earlier than render-start in less than 1% of cases and never more than 10ms earlier. That being said, this code fully supports onload to arrive earlier - just doesn't optimize around it.

lannka · 2017-02-07T19:02:35Z

extensions/amp-ad/0.1/amp-ad-xorigin-iframe-handler.js

@@ -125,60 +122,78 @@ export class AmpAdXOriginIframeHandler {
      this.sendEmbedInfo_(this.baseInstance_.isInViewport());
    }));

+    // Iframe.onload normally called by the Ad after full load.
+    const iframeLoadPromise = loadPromise(this.iframe).then(() => {
+      // Wait just a little to allow messages to arrive.


what messages are we thinking of here?

render-start and no-content.

i see. I guess we're only interested no-content at this point. can you just update the doc:

// Wait just a little to allow no-content to arrive.

lannka · 2017-02-07T19:10:16Z

extensions/amp-ad/0.1/amp-ad-xorigin-iframe-handler.js

+      iframeLoadPromise,
+      noContentPromise,
+    ]);
+    return adLoadPromise.then(() => {


i thought we discussed that layoutCallback should be resolved purely on iframeLoad?
This way we separate concepts so that:

iframe onload controls layoutCallback, which controls resource scheduling

render-start and no-content controls UI experience

in the case of no-content, iframe will continue loading. If it is for resource scheduling, we probably only want to resolve adLoadPromise when iframeLoad resolve?
Also is it possible to kill iframe loading in the case of no-content?

That's what we do now - we kill the iframe. If iframe.onload hasn't arrive yet, it never will. Thus, I use no-content to resolve the layout promise.

I see we kill iframe in noContent_().

dvoytenko · 2017-02-07T20:15:06Z

@lannka @zhouyx I brought back bootstrap-loaded event in essentially the same form as before. The reason: I didn't want to keep the Ad hidden for the complete load sequence.

zhouyx · 2017-02-07T21:45:52Z

extensions/amp-ad/0.1/test/test-amp-ad-xorigin-iframe-handler.js

-    it('should resolve on timeout', done => {
-      const noContentSpy =
-          sandbox.spy/*OK*/(iframeHandler, 'freeXOriginIframe');
+    it('should trigger render-start on timeout', done => {


the description is misleading. renderStartedSpy is not called.

Fixed. Meant to say "trigger visibility"

zhouyx · 2017-02-07T22:02:01Z

extensions/amp-ad/0.1/amp-ad-xorigin-iframe-handler.js

+      iframeLoadPromise,
+      noContentPromise,
+    ]);
+    return adLoadPromise.then(() => {


I see we kill iframe in noContent_().

zhouyx · 2017-02-07T22:02:39Z

extensions/amp-ad/0.1/amp-ad-xorigin-iframe-handler.js

+      // If ad loading has succeeded, so should either visibilityPromise or
+      // no-content. Return it here to ensure that rendering has been fully
+      // processed when layout promise is complete.
+      return Promise.race([visibilityPromise, noContentPromise]);


I get it is safer to add another check, but is it really necessary?

i think it's redundant as well.

It's necessary to ensure that the visibility actions have been completed when the main promise is resolved. Otherwise we will have a risk condition between layoutCallback and visibility.

At this point, either iframeLoadPromise or noContentPromise is resolved.
In case of noContentPromise is resolved, this Promise.race is immediately resolved.
In case of iframeLoadPromise is resolved, visibilityPromise is resolved as well, because it's race of iframeLoadPromise with others. So this Promise.race is also immediately resolved.

So, this Promise.race will always be immediately resolved, meaning we don't need it.

Not entirely correct. A risk condition is still possible. Both visibility and layout promises are joined on iframeLoadPromise. When this promise is resolved, both promises will be scheduled to be resolved in microtasks. W/o returning visibility promise here, there's no way to guarantee that visibility's then will be executed before layout promise is resolved.

As a proof, we can remove this return and see tests start failing. Or rather they will become flaky. (I just tried, that's indeed the case. I can demonstrate it if desired.)

Ah, you're right, I missed the then block in visibilityPromise.
It's quite brain twisting here. I doubt 3 months later we still understand all the relations.
Maybe we are running into the wrong direction?

So why does layoutCallback care about the UI treatment that visibilityPromise is handling? Again, I thought we want do decouple the concept here. To me, layoutCallback should only care if the iframe is loaded or deleted.

I see!
But I also agree with @lannka that layoutCallback should not care about UI. As long as the visiblity's then is executed, it is fine to be executed a bit afterwards. Then in our test, we probably should use poll to check UI behaviors?

I agree. I redid this part and remove the dependency on visibility.

I see you remove the visiblityPromise w/o changing the test case.

there's no way to guarantee that visibility's then will be executed before layout promise is resolved.

As you described, wouldn't removing the extra check make tests flaky?

I thought it would. But I'm not seeing flakes right now. So I figured it's just my paranoia. Sorry for the confusion.

jridgewell · 2017-02-07T23:09:29Z

extensions/amp-ad/0.1/amp-ad-xorigin-iframe-handler.js

+      listenForOncePromise(this.iframe,
+          ['render-start', 'no-content'], true).then(info => {
+            const data = info.data;
+            if (data.type == 'render-start') {


I'd much rather see this split into two calls to listenForOncePromise. Then we can remove the two extra promises, and just use the listenForOncePromise return values.

Couple of things here:

They are mutually exclusive. The first one wins, but they do have significant consequences to DOM/UX.

The second branch (the one that doesn't support renderStart) is going away eventually.

They are mutually exclusive.

Isn't that's what the Promise.race is for? All I'm asking for is renderStartPromise and noContentPromise be the return value from listenForOncePromise. We'll still have this if statement after the race.

The fallback branch (bootstrap) makes that difficult, b/c it doesn't work as a race. Once we remove that branch, we can do exactly as you say.

lannka · 2017-02-08T17:25:41Z

extensions/amp-ad/0.1/amp-ad-xorigin-iframe-handler.js

@@ -125,60 +122,78 @@ export class AmpAdXOriginIframeHandler {
      this.sendEmbedInfo_(this.baseInstance_.isInViewport());
    }));

+    // Iframe.onload normally called by the Ad after full load.
+    const iframeLoadPromise = loadPromise(this.iframe).then(() => {
+      // Wait just a little to allow messages to arrive.


i see. I guess we're only interested no-content at this point. can you just update the doc:

// Wait just a little to allow no-content to arrive.

lannka · 2017-02-08T17:29:30Z

extensions/amp-ad/0.1/amp-ad-xorigin-iframe-handler.js

+      // If ad loading has succeeded, so should either visibilityPromise or
+      // no-content. Return it here to ensure that rendering has been fully
+      // processed when layout promise is complete.
+      return Promise.race([visibilityPromise, noContentPromise]);


At this point, either iframeLoadPromise or noContentPromise is resolved.
In case of noContentPromise is resolved, this Promise.race is immediately resolved.
In case of iframeLoadPromise is resolved, visibilityPromise is resolved as well, because it's race of iframeLoadPromise with others. So this Promise.race is also immediately resolved.

So, this Promise.race will always be immediately resolved, meaning we don't need it.

lannka · 2017-02-08T17:30:55Z

extensions/amp-ad/0.1/amp-ad-xorigin-iframe-handler.js

-    if (this.baseInstance_.emitLifecycleEvent) {
-      this.baseInstance_.emitLifecycleEvent('renderCrossDomainStart');
+    this.baseInstance_.renderStarted();
+    if (opt_info) {


nit: return early when opt_info is undefined. to save the indentation.

lannka · 2017-02-08T20:55:03Z

extensions/amp-ad/0.1/amp-ad-xorigin-iframe-handler.js

-          }
-        });
+    // The actual ad load is eariliest of iframe.onload event and no-content.
+    const adLoadPromise = Promise.race([


we can inline the variable now.

lannka · 2017-02-08T20:55:44Z

extensions/amp-ad/0.1/amp-ad-xorigin-iframe-handler.js

+      iframeLoadPromise,
+      noContentPromise,
+    ]);
+    return adLoadPromise.catch(reason => {


shall we just do catch on iframeLoadPromise?

Removed completely. Not possible. And already well processed by the framework itself.

lannka · 2017-02-08T20:55:59Z

extensions/amp-ad/0.1/amp-ad-xorigin-iframe-handler.js

    this.updateSize_(data.height, data.width,
-                info.source, info.origin);
-    this.baseInstance_.signals().signal('render-start');
+                opt_info.source, opt_info.origin);


pls fix indentation

dvoytenko · 2017-02-09T00:18:56Z

@lannka @zhouyx PTAL.

zhouyx · 2017-02-09T00:32:17Z

extensions/amp-sticky-ad/1.0/test/test-amp-sticky-ad.js

-        return false;
+    it('should wait for built and load-end signals', () => {
+      impl.ad_.isBuilt = () => false;
+      impl.vsync_.mutate = function(callback) {


one last question. I see we use vsync_.mutatePromise but mock vsync_mutate here. What are the differences?

No real difference. mutatePromise calls mutate.

Got it. good to sync the 0.1 version

* Separate render-start and loaded phases for Ads * lints * fixes * Process bootstrap-start event * fixes * fixes * remove visibility expectation from load promise * lints * review fixes * lints * sticky-ad-0.1

Dima Voytenko added 2 commits February 6, 2017 17:13

Separate render-start and loaded phases for Ads

0cb9f15

lints

556a94d

dvoytenko assigned lannka Feb 7, 2017

dvoytenko requested review from zhouyx and lannka February 7, 2017 02:09

zhouyx reviewed Feb 7, 2017

View reviewed changes

fixes

aa39857

lannka requested changes Feb 7, 2017

View reviewed changes

Process bootstrap-start event

60478d3

zhouyx suggested changes Feb 7, 2017

View reviewed changes

jridgewell reviewed Feb 7, 2017

View reviewed changes

fixes

cf858cf

lannka requested changes Feb 8, 2017

View reviewed changes

Dima Voytenko added 3 commits February 8, 2017 09:44

fixes

c4cbbea

remove visibility expectation from load promise

001dd36

lints

0838156

lannka reviewed Feb 8, 2017

View reviewed changes

review fixes

3a32ecf

lannka approved these changes Feb 9, 2017

View reviewed changes

zhouyx reviewed Feb 9, 2017

View reviewed changes

Dima Voytenko added 2 commits February 8, 2017 17:10

lints

f036f23

sticky-ad-0.1

e229206

zhouyx approved these changes Feb 9, 2017

View reviewed changes

dvoytenko merged commit 8fc6792 into ampproject:master Feb 9, 2017

dvoytenko deleted the fie14-adlayout branch February 9, 2017 21:01

Separate render-start and loaded phases for Ads #7391

Separate render-start and loaded phases for Ads #7391

Conversation

dvoytenko commented Feb 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dvoytenko commented Feb 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dvoytenko commented Feb 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment