Move all creative rendering for A4A to layoutcallback #6249

keithwrightbos · 2016-11-18T21:51:23Z

Previously AMP Creatives were rendered as part of promise chain initiated via onLayoutMeasure. Move to use layoutCallback which will allow for dynamically changing priority if AMP creative (to be done in later PR) as well as enforce data-loading-strategy = prefer-viewability-over-views data attribute.

…ack; tests TBD

lannka

Good refactoring. Much easier to read now.

lannka · 2016-11-22T18:51:30Z

extensions/amp-a4a/0.1/amp-a4a.js

+      const iframe = /** @type {!HTMLIFrameElement} */(
+        createElementWithAttributes(
+          /** @type {!Document} */(this.element.ownerDocument), 'iframe', {
+            'frameborder': '0', 'allowfullscreen': '',


nit: remove the "'" around the keys

lannka · 2016-11-22T18:52:54Z

extensions/amp-a4a/0.1/amp-a4a.js

+    this.emitLifecycleEvent('renderFriendlyStart', creativeMetaData);
+    try {
+      // Create and setup friendly iframe.
+      dev().assert(!!this.element.ownerDocument);


is this check necessary? or do you want to add a message?

lannka · 2016-11-22T19:30:42Z

extensions/amp-a4a/0.1/amp-a4a.js

+      this.applyFillContent(iframe);
+      const fontsArray = [];
+      if (creativeMetaData.customStylesheets) {
+        creativeMetaData.customStylesheets.forEach(s => {


according to CreativeMetaDataDef, CreativeMedaData.customStylesheets is Array<string>. Where is the href attribute from?

lannka · 2016-11-22T21:29:20Z

extensions/amp-a4a/0.1/amp-a4a.js

+          this.onAmpCreativeRender();
+          return true;
+        });
+    } catch (e) {


this will not catch any error thrown inside the promise then blocks.

Yuu'll want to use the().catch().

lannka · 2016-11-22T21:30:31Z

extensions/amp-a4a/0.1/amp-a4a.js

-    return super.renderOutsideViewport();
+    const elementCheck = getAmpAdRenderOutsideViewport(this.element);
+    return typeof elementCheck == 'number' ?
+      elementCheck : super.renderOutsideViewport();


pls indent 2 more spaces for line break.

lannka · 2016-11-22T21:34:32Z

extensions/amp-ad/0.1/concurrent-load.js

- * @param {!Window} win
- */
-export function decrementLoadingAds(timerId, win) {
-  timerFor(win).cancel(timerId);


I suppose you did research and it's safe to let it go? :-)

Yes, previously we incremented at start of layoutCallback and decrement once we know if creative is AMP. Now, we only increment after knowing if that creative is not AMP.

lannka · 2016-11-22T21:43:38Z

extensions/amp-a4a/0.1/amp-a4a.js

-              ' any ad');
-        }
+    // Promise chain will have determined if creative is valid AMP.
+    return this.adPromise_.then(promiseResult => {


rename promiseResult => creativeMetaData ?

lannka · 2016-11-22T21:45:55Z

extensions/amp-a4a/0.1/amp-a4a.js

+      if (promiseResult) {
+        dev().assert(promiseResult.minifiedCreative);
+        // Must be an AMP creative.
+        const metaData = /** @type {CreativeMetaDataDef} */(promiseResult);


did you try annotate this.adPromise_ instead?

tdrl

D'oh! I had started this review, but not sent it to you yet. Sorry! Sending what I have now and will finish reading and send more.

tdrl · 2016-11-22T18:12:37Z

extensions/amp-a4a/0.1/amp-a4a.js

@@ -115,6 +106,7 @@ export const LIFECYCLE_STAGES = {
  renderCrossDomainEnd: '9',
  preAdThrottle: '10',
  renderSafeFrameStart: '11',
+  throttled3p: '12',


This is different than preAdThrottle, I assume?

tdrl · 2016-11-22T18:18:02Z

extensions/amp-a4a/0.1/amp-a4a.js

-    /** @private {?Promise<!boolean>} */
+    /** @const @private{string} tag to be used for dev/user logging */
+    this.logTag_ =
+      ('AMP-A4A-' + this.element.getAttribute('type')).toUpperCase();


So this will be 'AMP-A4A-NULL' in the nasty case that this is constructed before its attributes are populated? That's probably okay for our purposes, I guess.

tdrl · 2016-11-22T18:31:36Z

extensions/amp-a4a/0.1/amp-a4a.js

-    return super.renderOutsideViewport();
+    const elementCheck = getAmpAdRenderOutsideViewport(this.element);
+    return typeof elementCheck == 'number' ?
+      elementCheck : super.renderOutsideViewport();


I don't think this fall-through logic is quite the same as before, is it? If elementCheck == false, then you'll return super.renderOutsideViewport(), while before you would have returned false. I don't think it makes a practical difference at the moment (I'm sure that elementCheck can ever be false), but worth verifying that this will be ok.

tdrl · 2016-11-22T18:32:20Z

extensions/amp-a4a/0.1/amp-a4a.js

    }
    // Otherwise the ad is good to go.
-    return super.renderOutsideViewport();
+    const elementCheck = getAmpAdRenderOutsideViewport(this.element);


elementCheck seems vague. Maybe renderOutsideViewportCheck?

tdrl · 2016-11-22T18:46:44Z

extensions/amp-a4a/0.1/amp-a4a.js

+          // variable.
+          this.isVerifiedAmpCreative_ = true;
+          return utf8Decode(creative).then(
+            creative => this.getAmpAdMetadata_(creative));


I think you can chain this to the main promise chain, so that we don't have then-inside-then:

.then(creative => { this.isVerifiedAmpCreative_ = true; return utf8Decode(creative); }) .then(creativeDecoded => { return this.getAmpAdMetadata_(creativeDecoded); });

Done though I have to check that creative and creativeDecoded exist as part of the return chain

tdrl · 2016-11-28T17:46:08Z

extensions/amp-a4a/0.1/amp-a4a.js

+          // viewport but cannot wait on promise.  Sadly, need a state a
+          // variable.
+          this.isVerifiedAmpCreative_ = true;
+          return utf8Decode(creative).then(


Heads-up that this is about to get changed out from under you. They're migrating utf8Decode to synchronous. Maybe we can get this change in first. fingers crossed

tdrl · 2016-11-28T17:52:59Z

extensions/amp-a4a/0.1/amp-a4a.js

          // on precisely the same creative that was validated
          // via #validateAdResponse_.  See GitHub issue
          // https://github.com/ampproject/amphtml/issues/4187

          // TODO(levitzky) If creative comes back null, we should consider re-
          // fetching the signing server public keys and try the verification
          // step again.
-          return creative && this.maybeRenderAmpAd_(creative);
-        })
-        .catch(error => this.promiseErrorHandler_(error));


After going over promise catch semantics w/ Taymon the other day, I think I was wrong to recommend killing this catch. I think we need it for the cases where layoutCallback is deferred by the AMP runtime. In that case, the error will fall off the end here and not be preserved. I think it will be picked up by the outer AMP catch, but we'd rather explicitly catch it here and format via promiseErrorHandler.

tdrl · 2016-11-28T17:57:09Z

extensions/amp-a4a/0.1/amp-a4a.js

@@ -504,37 +509,26 @@ export class AmpA4A extends AMP.BaseElement {
  /** @override */


It won't let me attach comments to stuff that was folded (WTF GitHub?), but now I question what happens at line 493 (in your version). We're throwing from a Promise.catch there and I think that's converted into another rejection, which, again, will fall off the end of the promise chain if layoutCallback isn't pending itself.

not sure where this lines up but hopefully fixed when I explicitly added catch to promise chain initialization in onLayoutMeasure

tdrl · 2016-11-28T17:58:19Z

extensions/amp-a4a/0.1/amp-a4a.js

+    // Promise chain will have determined if creative is valid AMP.
+    return this.adPromise_.then(promiseResult => {
+      if (promiseResult) {
+        dev().assert(promiseResult.minifiedCreative);


If we don't eliminate the .catch at the end of the onLayoutMeasure promise chain, then I think that promiseResult can be an Error here, so you'll need a case for that, rather than just an assert.

tdrl · 2016-11-28T18:11:47Z

extensions/amp-a4a/0.1/amp-a4a.js

@@ -879,7 +878,10 @@ export class AmpA4A extends AMP.BaseElement {
        creative.slice(metadataStart + METADATA_STRING.length, metadataEnd));
      const ampRuntimeUtf16CharOffsets =
        metaDataObj['ampRuntimeUtf16CharOffsets'];
-      if (!isValidOffsetArray(ampRuntimeUtf16CharOffsets)) {
+      if (!isArray(ampRuntimeUtf16CharOffsets) ||
+          ampRuntimeUtf16CharOffsets.length != 2 &&


Why did you inline this and get rid of the function? Just b/c it's only used here these days? I still think it makes the code more readable, but if you'd rather kill it, no problem. But I think you want to convert all of the && to || here.

tdrl · 2016-11-28T18:16:02Z

extensions/amp-a4a/0.1/test/test-amp-a4a.js

+      return executeLayoutCallbackTest(true);
+    });
+    it('#layoutCallback not valid AMP', () => {
+      return executeLayoutCallbackTest(true);


tdrl · 2016-11-28T18:22:10Z

extensions/amp-a4a/0.1/test/test-amp-a4a.js

        a4a.onLayoutMeasure();
        expect(a4a.adPromise_).to.be.instanceof(Promise);
-        return a4a.adPromise_.then(() => {
+        return a4a.adPromise_.then(promiseResult => {


Perhaps not in this test, but somewhere, we should have a test for the different throw/catch cases. I'm not sure we (I, anyway) fully understand those interactions.

tdrl · 2016-11-28T18:23:38Z

extensions/amp-a4a/0.1/test/test-amp-a4a.js

+      incrementLoadingAds(fixture.win);
+      expect(a4a.renderOutsideViewport()).to.be.false;
+    });
+    it('should return true if throttled, but AMP creative', () => {


true => 3?

True means take default which is 3.

But you test for 3.

tdrl · 2016-11-28T18:37:14Z

extensions/amp-ad/0.1/amp-ad-3p-impl.js

-    return super.renderOutsideViewport();
+    const elementCheck = getAmpAdRenderOutsideViewport(this.element);
+    return typeof elementCheck == 'number' ?
+      elementCheck : super.renderOutsideViewport();


I thought I had commented on this before, but I don't see the comment now, so apologies if this is redundant.

In the case that getAmpAdRenderOutsideViewport returns a non-numeric value, you can lose that value here. Previously, if allowRender was false, it would be returned. Now if elementCheck is false, you'll return super.renderOutsideViewport(), which is 3.

Hmm... right now getAmpAdRenderOutsideViewport will never return false (either true or 1.25) so this is not currently an issue but I do agree that how this is setup is somewhat fragile. I reworked so that getAmpAdRenderOutsideViewport now returns ?number where null indicates use default. Let me know what you think.

tdrl · 2016-11-28T18:38:24Z

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

@@ -28,7 +28,7 @@ import {timerFor} from '../../../src/timer';
 import {setStyle} from '../../../src/style';
 import {AdDisplayState} from './amp-ad-ui';

-const TIMEOUT_VALUE = 10000;
+const TIMEOUT_VALUE = 1000000;


16 min? That's a long timeout.

Same question :-)

…r_layout

keithwrightbos · 2016-11-28T22:02:53Z

@lannka PTAL

…r_layout

lannka

LGTM with one comment

lannka · 2016-11-29T20:37:14Z

extensions/amp-a4a/0.1/amp-a4a.js

    }
    // Otherwise the ad is good to go.
-    return super.renderOutsideViewport();
+    const elementCheck = getAmpAdRenderOutsideViewport(this.element);
+    return typeof elementCheck == 'number' ?


to avoid this weird typeof check, shall we just make the function always return a number?

I don't have reference to the default value of 3 and didn't want to hardcode it just in case its is later changed...

keithwrightbos · 2016-11-29T21:03:05Z

@tdrl PTAL

taymonbeal · 2016-11-29T22:33:47Z

LGTM

tdrl · 2016-11-29T22:41:30Z

I'm mostly good, but there are still a couple of open questions / issues:

Moving utf8decode.then outside the .then block that it's in: Move all creative rendering for A4A to layoutcallback #6249 (comment)
Changed logic of renderOutsideViewport: Move all creative rendering for A4A to layoutcallback #6249 (comment) and Move all creative rendering for A4A to layoutcallback #6249 (comment) (did end up making the same comment twice b/c GitHub hid it from me. Ugh. Sorry about that.)

Other than those, LGTM.

…r_layout

keithwrightbos · 2016-11-30T15:05:01Z

@tdrl PTAL

tdrl · 2016-11-30T15:10:10Z

LGTM. Thanks! @lannka I'm happy, and it looks like everyone else is, so please merge when you get a chance.

lannka · 2016-11-30T17:11:56Z

Will merge once Travis is happy too

keithwrightbos · 2016-11-30T21:27:30Z

@lannka travis is happy, please merge

* Initial modifications to move all creative rendering into layoutCallback; tests TBD * Fix merge issues * Test fixes * Fix tests * revert a4a examples * PR review * PR feedback * PR review * fix presubmit errors * fix test failure

keithwrightbos added 5 commits November 18, 2016 09:27

Initial modifications to move all creative rendering into layoutCallb…

055ce0c

…ack; tests TBD

Fix merge issues

cda294f

Test fixes

b3950d1

Fix tests

27cc98e

revert a4a examples

43dc54c

tdrl added NEEDS REVIEW WG: monetization labels Nov 22, 2016

tdrl assigned lannka and tdrl Nov 22, 2016

lannka requested changes Nov 22, 2016

View reviewed changes

tdrl reviewed Nov 28, 2016

View reviewed changes

keithwrightbos added 2 commits November 28, 2016 14:13

Merge branch 'master' of github.com:ampproject/amphtml into a4a_rende…

594d58b

…r_layout

PR review

4df14a6

keithwrightbos mentioned this pull request Nov 29, 2016

Disallow Ad Slots from A4A if using prefer-viewability-over-views #6175

Closed

Merge branch 'master' of github.com:ampproject/amphtml into a4a_rende…

415c040

…r_layout

lannka approved these changes Nov 29, 2016

View reviewed changes

lannka added LGTM and removed NEEDS REVIEW labels Nov 29, 2016

PR feedback

712c8ea

keithwrightbos added 3 commits November 30, 2016 09:27

Merge branch 'master' of github.com:ampproject/amphtml into a4a_rende…

4799f0c

…r_layout

Merge branch 'master' of github.com:ampproject/amphtml into a4a_rende…

a008fc5

…r_layout

PR review

5d6b311

keithwrightbos added 2 commits November 30, 2016 13:17

fix presubmit errors

592825f

fix test failure

6e0e6a6

lannka merged commit 627cdff into ampproject:master Nov 30, 2016

taymonbeal deleted the a4a_render_layout branch November 30, 2016 21:52

		@@ -504,37 +509,26 @@ export class AmpA4A extends AMP.BaseElement {
		/** @override */

Move all creative rendering for A4A to layoutcallback #6249

Move all creative rendering for A4A to layoutcallback #6249

Conversation

keithwrightbos commented Nov 18, 2016

lannka left a 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

tdrl left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

keithwrightbos commented Nov 28, 2016

lannka left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

keithwrightbos commented Nov 29, 2016

taymonbeal commented Nov 29, 2016

tdrl commented Nov 29, 2016

keithwrightbos commented Nov 30, 2016

tdrl commented Nov 30, 2016

lannka commented Nov 30, 2016

keithwrightbos commented Nov 30, 2016