A4A amp nameframe #6099

tdrl · 2016-11-09T01:18:24Z

NameFrame is an AMP alternative to SafeFrame, for use in A4A code. It does not support the full SafeFrame API, but it does allow a creative to be set in a cross-domain iframe via the name attribute. It also supports postMessaging out a no-content message in the case of rendering failure.

tdrl · 2016-11-09T01:19:05Z

I'm still doing manual tests and some cleanup, but I wanted to get it to you to review while I'm doing those steps.

keithwrightbos · 2016-11-09T01:39:12Z

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

-    /** @private {?string} */
-    this.experimentalNonAmpCreativeRenderMethod_ = null;
+    /** @private {!CROSS_ORIGIN_RENDERING_MODE} */
+    this.experimentalNonAmpCreativeRenderMethod_ =


Given this now includes the default (currently cache, but likely soon to be safe/name frame for ios), let's remove "experimental" from the name

keithwrightbos · 2016-11-09T01:39:41Z

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

-      if (rendered instanceof Error) {
-        throw rendered;
+        this.creativeBody_ = null;  // Free resources.
+        this.experimentalNonAmpCreativeRenderMethod_ =


Any reason to this here as opposed to unlayoutCallback?

Malte requested it. The idea was to be sure to free the resources as soon as we were sure we're done with them. Since unlayoutCallback isn't guaranteed to be called, he wanted it to happen in place where it would be sure to happen.

keithwrightbos · 2016-11-09T01:42:19Z

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

+    this.emitLifecycleEvent('renderSafeFrameStart');
+    return utf8Decode(creativeBody).then(creative => {
+      /** @const {!Element} */
+      const name_data = Object.create(null);


Why not just const name_data = {'creative': creative}?

Also a decent amount of duplication between this and safeframe code... you could have a single render via frame loader function that takes just the creative and then uses this.experimentalNonAmpCreativeRenderMethod_ to determine which (or you can pass the value if you're worried about state).

keithwrightbos · 2016-11-09T18:02:52Z

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

@@ -567,7 +567,7 @@ export class AmpA4A extends AMP.BaseElement {
      this.adPromise_ = null;
      this.adUrl_ = null;
      this.creativeBody_ = null;
-      this.experimentalNonAmpCreativeRenderMethod_ = null;
+      this.nonAmpCreativeRenderMethod_ = null;


Should this be the default (client cache)? We should have a test where we create A4A Element, call unlayoutcallback, and verify state has not changed.

Default: Whoa, good catch. I missed that one. Thanks.

Test: Added.

keithwrightbos · 2016-11-09T18:03:33Z

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

        }
        this.creativeBody_ = null;  // Free resources.
-        this.experimentalNonAmpCreativeRenderMethod_ =
+        this.nonAmpCreativeRenderMethod_ =


I meant why reset nonAmpCreativeRenderMethod_ here given you also reset in unlayoutCallback? I would remove this since we may want to know later in the flow...

Oh, I see. Yeah, I guess that makes sense. Done.

keithwrightbos · 2016-11-09T18:04:50Z

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

    this.emitLifecycleEvent('renderSafeFrameStart');
    return utf8Decode(creativeBody).then(creative => {
+      let srcPath;
+      let nameData;
+      if (method == CROSS_ORIGIN_RENDERING_MODE.SAFEFRAME) {


you can use a switch instead... I would argue its cleaner. You can also move the dev assert into the else (soon to be default) clause as dev().assert(false)... unless that feels ugly to you

Oh, right. The if ... else if is a leftover. Switch makes more sense now.

Re: location of the assert. I think it should be outside. It's dev mode so that we can see it if we manage to get here by mistake, but it will throw out of the code altogether. If we should happen to get here in prod mode, I'd like a gentler return.

lannka

Looks good in general

lannka · 2016-11-09T21:51:49Z

3p/nameframe.max.html

+<head>
+    <meta charset="UTF-8">
+    <script>
+        window.onload = function() {


curious why do we wait for onload?

I wondered that myself! The issue seems to be that if you do a document.write() during load, it only appends to the doc. If you wait until load is completed, it replaces it, which is the desired behavior here.

good to know

lannka · 2016-11-09T21:54:31Z

build-system/server.js

@@ -504,11 +504,15 @@ function replaceUrls(mode, file) {
    file = file.replace('https://cdn.ampproject.org/a4a-v0.max.js', '/dist/amp-inabox.js');
    file = file.replace(/https:\/\/cdn.ampproject.org\/v0\//g, '/dist/v0/');
    file = file.replace('https://cdn1.ampproject.org/viewer/google/v5.js', 'https://cdn.ampproject.org/viewer/google/v5.js');
+    file = file.replace(/https:\/\/3p.ampproject.net\/(?:\d+\/)?(.+)\.html/,


why is this needed?

In order to serve the nameframe during testing / local dev mode. It wasn't needed for frame.html because its name is initialized differently, I think.

Happy to try other approaches, if you have suggestions. This is the best thing I found so far.

I'm wondering which file needed in the test contains this https://3p.ampproject.net/...html ?

Sorry -- should have been more clear. I meant, when manually loading pages for personal testing, not when running automated tests. If I have a file that looks like,

<body> <h1>A putative ad.</h1> <iframe src="https://3p.ampproject.net/12345678/nameframe.html" name='{"creative":"<html><head><meta charset=\"UTF-8\"><title>Who puts a title on an ad creative anyway?</title></head><body>This is an ad.</body></html>","context":{"foo":3.7,"bar":"kumquat"}}'></iframe> </body>

I can load it directly in the browser, when serving locally.

If this is only for local dev purpose, you can just point the src to localhost directly?

Fair. Rewrite rule removed.

lannka · 2016-11-09T21:55:48Z

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

-    /** @private {?string} */
-    this.experimentalNonAmpCreativeRenderMethod_ = null;
+
+    /** @type {!CROSS_ORIGIN_RENDERING_MODE} @visibleForTesting */


does this pass gulp check-types?

Amazingly enough, yes. I was worried about that, but it didn't complain. Should it?

lannka · 2016-11-09T21:59:20Z

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

   * @return {!Promise} awaiting load event for ad frame
   * @private
   */
-  renderViaSafeFrame_(creativeBody) {
+  renderViaNameAttrOfXDomIframe_(creativeBody) {


since you already named it "nameframe", just call it renderViaNameFrame?

Sadly, it doesn't just do NameFrame yet. At the moment, it does both NameFrame and SafeFrame. This name is fugly, but captures both. Happy to change it in the future, when we remove SafeFrame support.

Fine. I guess you meant XOrigin not XDom?

I was thinking "Dom" == "Domain", not "DOM". But I agree that Origin is more precise. Changed.

lannka · 2016-11-09T22:00:17Z

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

 /** @type {string} @visibleForTesting */
 export const RENDERING_TYPE_HEADER = 'X-AmpAdRender';
+
+/** @enum {string} */
+export const CROSS_ORIGIN_RENDERING_MODE = {


nit: how about XORIGIN_MODE

Sure. Done.

lannka · 2016-11-09T22:06:56Z

test/functional/test-3p-nameframe.js

+  let doc;
+  let iframe;
+  beforeEach(() => {
+    return createIframePromise(/* runtimeOff */ true).then(f => {


use describes.realWin, so you don't need createIframePromise here

I actually tried that and fell back after fighting with it for 2 hours. The issue is that it seems to preempt document.createElement('iframe') so that I couldn't get a cross-domain iframe out of it. As a result, I couldn't set up a NameFrame in it. :-P Quite possible I was just doing it wrong, though. Happy to revisit if you have guidance / code pointers.

Not sure I understand your problem. But would the following work?

describes.realWin('nameframe', {}, env => { let fixture; beforeEach(() => { fixture = env; win = fixture.win; doc = fixture.doc; iframe = doc.createElement('iframe'); iframe.src = 'http://localhost:9876/dist.3p/current/nameframe.max.html'; }

No, it doesn't. This is pretty much what I tried before. When I do this, the iframe that is created has srcdoc set to <h1>Fake iframe</h1>, which makes it some form of a friendly iframe and means that it doesn't load the src and doesn't work as a nameframe.

Including an example test of this format in the current push, so you can double-check and tell me if I'm missing something.

ah, it's caused by this.

you'll want to introduce a new field loadExternalResources in the spec (default to false) to disable doNotLoadExternalResourcesInTest.

Fixed. Thanks for the pointer!

lannka · 2016-11-09T22:08:52Z

test/functional/test-3p-nameframe.js

+      win = fixture.win;
+      doc = fixture.doc;
+      iframe = doc.createElement('iframe');
+      iframe.src = getDefaultBootstrapBaseUrl(win, 'nameframe');


you can just hardcode the URL here. no need for this test to depend on 3p-iframe.js, which is covered with a separate unit test.

Though I do worry that hardcoding in the port is fragile to changes to the test rig. The benefit to doing it via getDefaultBootstrapBaseUrl is that it's then that function's job to worry about things like port number.

lannka · 2016-11-09T22:09:52Z

test/functional/test-3p-nameframe.js

+  it('should load remote nameframe and succeed with valid JSON input', done => {
+    iframe.name = JSON.stringify({
+      creative: `<html>
+<body>


to make it prettier, let's have some harmless indentation :-)

:-) Fair enough. Done.

lannka · 2016-11-09T22:10:23Z

test/functional/test-3p-nameframe.js

+import {getDefaultBootstrapBaseUrl} from '../../src/3p-frame';
+import {createIframePromise} from '../../testing/iframe';
+
+function expectNoContent(win, done) {


move helper method to the bottom of the file.

jridgewell · 2016-11-09T22:18:58Z

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

-    /** @private {?string} */
-    this.experimentalNonAmpCreativeRenderMethod_ = null;
+
+    /** @type {!CROSS_ORIGIN_RENDERING_MODE} @visibleForTesting */


Newline for @visibleForTesting

jridgewell · 2016-11-09T22:19:44Z

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

-          this.experimentalNonAmpCreativeRenderMethod_ =
-              fetchResponse.headers.get(RENDERING_TYPE_HEADER);
+          this.nonAmpCreativeRenderMethod_ =
+              fetchResponse.headers.get(RENDERING_TYPE_HEADER) ||


Do we need to assert that this is a valid enum value?

Can't just assert because I don't want to fail the entire method. Even if it gets an invalid value here, it will fall back to something that will work during rendering. But I agree that a dev notification is in order. (Dev because the value of this header is in the control of the ad network, not the publisher.) Added, along with a test for this case.

tdrl · 2016-11-09T22:48:16Z

Thanks for the feedback @lannka . PTAL

tdrl · 2016-11-15T14:19:53Z

@lannka , @jridgewell PTAL. I've left in an example test configured using describes.realWin as an example of how it breaks -- that's currently failing tests, but everything else should be addressed.

lannka · 2016-11-15T16:55:16Z

@tdrl in case you didn't see my previous comment regarding to the test, can you try disable doNotLoadExternalResourcesInTest?

tdrl · 2016-11-17T02:13:49Z

@lannka Ah, yes, I had missed that previous comment. Thanks for bringing it up. And thanks for the code pointer! Works nicely now.

tdrl · 2016-11-17T02:29:47Z

Thanks, @lannka . PTAL

taymonbeal · 2016-11-17T16:09:45Z

3p/nameframe.max.html

@@ -0,0 +1,26 @@
+<!DOCTYPE html>


Style nit: Needs to be lowercase. Same with UTF-8 below.

Also, use two-space indents throughout the file.

taymonbeal · 2016-11-17T16:11:07Z

3p/nameframe.max.html

@@ -0,0 +1,26 @@
+<!DOCTYPE html>
+<html lang="en">


Semantic nit: Except for the developer-facing error message, which by ancient convention doesn't count, this file contains no English-language content and so a lang attribute is not appropriate.

lannka · 2016-11-17T19:43:41Z

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

@@ -230,68 +287,181 @@ describe('amp-a4a', () => {
      }).onFirstCall().returns(Promise.resolve(mockResponse));
      return createAdTestingIframePromise().then(fixture => {
        const doc = fixture.doc;
-        const a4aElement = createA4aElement(doc);
+        a4aElement = createA4aElement(doc);
        a4aElement.setAttribute('width', 200);


does it make sense to move those repeated setAttribute into createA4aElement? Also, use the helper method createElementWithAttributes()

Done. Also got rid of a number of body.appendChild because it's done in createA4AElement.

lannka · 2016-11-17T19:44:53Z

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

+    });
+
+    afterEach(() => {
+      expect(xhrMock).to.be.calledOnce;


it's not recommended to do a check in fixture.

you can just check it in one test case, no need to check the same thing in every test case

If I expect it to hold in all test cases, why not just add the code once, rather than everywhere? Regardless, I can move it. Does failing in one test case break all test cases that share the same afterEach?

lannka · 2016-11-17T19:47:54Z

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

@@ -598,10 +785,15 @@ describe('amp-a4a', () => {
        a4a.buildCallback();
        a4a.preconnectCallback(false);
        const preconnects = doc.querySelectorAll('link[rel=preconnect]');
-        expect(preconnects.length).to.equal(2);
+        expect(preconnects.length).to.equal(3);


expect(array).to.have.length(3)

Done. Replaced here and elsewhere.

When I looked it up on Chai, it appears that that use of length is deprecated and they suggest lengthOf instead: http://chaijs.com/api/bdd/#method_length

lannka · 2016-11-17T19:48:38Z

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

        expect(preconnects[1].getAttribute('href')).to
+            .have.string('http://ads.localhost');
+        // AdSense origin.
+        expect(preconnects[2].getAttribute('href')).to


expect(x).to.have.property('key', 'value');

lannka · 2016-11-17T23:14:12Z

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

+            dev().assertEnumValue(XORIGIN_MODE,
+                                  this.nonAmpCreativeRenderMethod_,
+                                  'cross-origin render mode header');
+          } catch (err) {


maybe just introduce a util function isValidEnumValue() in types.js

…rame URL.

… it to identify the window internally, leading to many string copies and serious garbage allocation if the name field is large and kept around).

tdrl · 2016-11-28T21:35:43Z

@lannka and @taymonbeal PTAL

jridgewell · 2016-11-28T21:40:21Z

src/types.js

+ * @param {T} s
+ * @return {boolean}
+ */
+export function isEnumValue(enumObj, s) {


Please update assertEnumValue to use this helper.

lannka

LGTM. let me know when it's ready for merge.

tdrl · 2016-11-29T01:28:00Z

I'm happy whenever Travis is.

lannka · 2016-11-29T16:50:35Z

@taymonbeal 's happiness is also important :-) unable to merge before collecting all the balls.

tdrl · 2016-11-29T16:52:45Z

Yup. I think I've addressed all his comments (though he may come up with more). Gave him the heads-up to check it out again this morning.

taymonbeal · 2016-11-29T20:00:11Z

LGTM. Sorry for delays in reviewing this, it got lost in the GitHub noise after I started getting notified on every PR in amphtml (I've since turned this off).

tdrl · 2016-11-29T20:04:27Z

No worries. Thanks for your review, Taymon!

Okay, @lannka -- we're all clear. Press the button!

lannka · 2016-11-29T20:46:06Z

Button pressed

* First draft of nameframe. Still needs tests and some gulp config. * Added server rewrite rules for 3p .html files. * Started test rig for nameframe. * Changes to test rig, but still not functional. * Tests for nameframe itself in place. * Tests for nameframe itself in place. * Update gulp server rules to handle (optional) version number in nameframe URL. * A4A promise chain changes in place. * Moar tests. * Lint fixes. * Changes from reviews. * Changes from reviews. * Changes from reviews. * Don't overwrite the rendering method if no header is present. * Added preconnect. * Updates in response to reviews. * Updates in response to reviews. * Rebase fixes. * Fixes to tests. Clear window.name in nameframe (b/c some browsers use it to identify the window internally, leading to many string copies and serious garbage allocation if the name field is large and kept around). * Remove rewrite rule from server.js. * Updates in response to reviews. * Updates in response to reviews. * Rebase against master and resolve conflicts. * Response to reviews. * Response to reviews.

tdrl added NEEDS REVIEW WG: monetization Type: Feature Request labels Nov 9, 2016

tdrl assigned keithwrightbos and lannka Nov 9, 2016

keithwrightbos requested changes Nov 9, 2016

View reviewed changes

keithwrightbos approved these changes Nov 9, 2016

View reviewed changes

lannka requested changes Nov 9, 2016

View reviewed changes

jridgewell reviewed Nov 9, 2016

View reviewed changes

taymonbeal suggested changes Nov 17, 2016

View reviewed changes

lannka reviewed Nov 17, 2016

View reviewed changes

Terran Lane added 12 commits November 28, 2016 15:00

First draft of nameframe. Still needs tests and some gulp config.

25144fb

Added server rewrite rules for 3p .html files.

2aa2063

Started test rig for nameframe.

ef5b366

Changes to test rig, but still not functional.

8775f58

Tests for nameframe itself in place.

df6cb3c

Tests for nameframe itself in place.

9309a26

Update gulp server rules to handle (optional) version number in namef…

fdc3bc9

…rame URL.

A4A promise chain changes in place.

677dad5

Moar tests.

f1fefb0

Lint fixes.

2bf8755

Changes from reviews.

3b8b3a5

Changes from reviews.

ec0494a

Terran Lane added 11 commits November 28, 2016 16:02

Changes from reviews.

e83e309

Don't overwrite the rendering method if no header is present.

98339b3

Added preconnect.

48e2dab

Updates in response to reviews.

fd743f8

Updates in response to reviews.

a52b366

Rebase fixes.

1f59545

Fixes to tests. Clear window.name in nameframe (b/c some browsers use…

1bcfc7d

… it to identify the window internally, leading to many string copies and serious garbage allocation if the name field is large and kept around).

Remove rewrite rule from server.js.

18cee85

Updates in response to reviews.

162163d

Updates in response to reviews.

4106fa6

Rebase against master and resolve conflicts.

1f75169

jridgewell reviewed Nov 28, 2016

View reviewed changes

Response to reviews.

d875abd

lannka approved these changes Nov 29, 2016

View reviewed changes

Response to reviews.

d67383a

taymonbeal approved these changes Nov 29, 2016

View reviewed changes

lannka merged commit b21e457 into ampproject:master Nov 29, 2016

tdrl deleted the a4a-amp-safeframe branch November 29, 2016 21:25

A4A amp nameframe #6099

A4A amp nameframe #6099

Conversation

tdrl commented Nov 9, 2016

tdrl commented Nov 9, 2016

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

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

tdrl Nov 13, 2016 • edited

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

tdrl commented Nov 9, 2016

tdrl commented Nov 15, 2016

lannka commented Nov 15, 2016

tdrl commented Nov 17, 2016

tdrl commented Nov 17, 2016

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 Nov 28, 2016 • edited

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 commented Nov 28, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lannka left a comment

Choose a reason for hiding this comment

tdrl Nov 13, 2016 •

edited

tdrl Nov 28, 2016 •

edited