Refactored of passing frame attribute/context information by bradfrizzell · Pull Request #6498 · ampproject/amphtml

bradfrizzell · 2016-12-06T19:18:05Z

Previously, all of the context information used to build window.context was passed on the hash of the src url. This is being switched to the name element instead of iframes. This is done to make passing the context information down to nested iframes much more simple, specifically in relation to the AmpContext library.

bradfrizzell · 2016-12-06T19:18:58Z

apologies for the duplicate commit messages, not exactly sure what happened there.

@lannka

lannka

In case you are not aware: what's our plan for the master selection code?

lannka · 2016-12-09T17:23:00Z

src/3p-frame.js

  };
  iframe.setAttribute(
-      'data-amp-3p-sentinel', attributes._context.amp3pSentinel);
+      'data-amp-3p-sentinel', attributes._context.sentinel);


should we just generate the sentinel here, if this is the only place uses the sentinel.

no, it needs to be within the _context object because that is where it gets pulled from by the ampcontext library. the data-amp-3p-sentinel attribute gets deprecated by #6506

lannka · 2016-12-09T17:26:30Z

src/3p-frame.js

  const host = parseUrl(baseUrl).hostname;
  // Pass ad attributes to iframe via the fragment.
-  const src = baseUrl + '#' + JSON.stringify(attributes);
-  const name = host + '_' + attributes.type + '_' + count[attributes.type]++;


was the name attribute used anywhere?

we decided to make a breaking change here @keithwrightbos

I was wondering if this is not used anywhere, can we just remove them?

I believe it is used, at least there are tests that check for it

lannka · 2016-12-09T17:27:22Z

test/functional/test-impression.js

 import * as sinon from 'sinon';

-describe('impression', () => {
+describe('test-impression', () => {


you can revert this change

lannka · 2016-12-09T17:31:02Z

3p/integration.js

    window.context.reportRenderedEntityIdentifier =
        reportRenderedEntityIdentifier;
    window.context.computeInMasterFrame = computeInMasterFrame;
+    window.context.addContextToIframe = getAddContextToIframe(iframeName);


we normally use bind to create a function:

window.context.addContextToIframe = addNameToIframe.bind(null, iframeName) function addNameToIframe(name, iframe) { iframe.name = name; }

lannka · 2016-12-09T17:32:25Z

3p/integration.js

+  data = JSON.parse(window.name).attributes;
+  window.context = data._context;
+} catch (err) {
+  window.context = {};


report dev error here

lannka · 2016-12-09T17:32:47Z

3p/integration.js

+const iframeName = window.name;
+let data;
+try {
+  data = JSON.parse(window.name).attributes;


lannka · 2016-12-09T17:35:41Z

src/3p-frame.js

  // Pass ad attributes to iframe via the fragment.
-  const src = baseUrl + '#' + JSON.stringify(attributes);
-  const name = host + '_' + attributes.type + '_' + count[attributes.type]++;
+  const src = baseUrl;


this line can be removed?

lannka · 2016-12-09T17:37:59Z

src/3p-frame.js

    domFingerprint: domFingerprint(element),
    startTime,
  };
+  attributes.ampcontextVersion = (getMode().localDev ? 'LOCAL' :


should not be in attributes?

what do you mean? i.e. should it be in the _context?

depend on where do you need this? it's not from element attribute, and you don't want them to be shown as attribute, right?

I'll move this as part of the #6506 as I am already making the name building helper function there

seems this line can be added in your next PR, if you plan to move it eventually

lannka · 2016-12-09T17:43:47Z

3p/integration.js

+const iframeName = window.name;
+let data;
+try {
+  data = JSON.parse(window.name).attributes;


probably it's a good time to revisit the data structure, it's quite confusing now.

We can change it from

{ ... attributes: { _context: {} } }

to
{
attributes: {},
context: {},
}

in case this comment was buried.

I can fix this, but it would be a lot easier for me to do it in #6506 if that's alright with you. I need to make a helper function that constructs the whole name object anyways, and that's the PR where I'm going to put it

jridgewell · 2016-12-09T19:42:18Z

3p/integration.js

        reportRenderedEntityIdentifier;
    window.context.computeInMasterFrame = computeInMasterFrame;
-    window.context.addContextToIframe = getAddContextToIframe(iframeName);
+    window.context.addContextToIframe = addNameToIframe.bind(null, iframeName);


This is bound to the iframe param now, not the name.

jridgewell · 2016-12-09T20:08:30Z

3p/integration.js

 }

+/**
+ *  Adds the serialized ad attributes to an iframe's name attribute.


Need to swap.

jridgewell · 2016-12-09T20:08:45Z

src/3p-frame.js

    canary: !!(parentWindow.AMP_CONFIG && parentWindow.AMP_CONFIG.canary),
    hidden: !viewer.isVisible(),
-    amp3pSentinel: generateSentinel(parentWindow),
+    sentinel: generateSentinel(parentWindow),


Is this a breaking change?

I'll double check, but I'm fairly sure that anything this change may break would:
A. Be fixed with the sentinel refactor pr #6506
B. Would not be used by any publishers/advertisers/creatives (as the sentinel here is for the postMessage api that the AmpContext library uses, which is not being used by anyone yet).

jridgewell · 2016-12-09T20:10:16Z

3p/integration.js

+let data;
+try {
+  data = JSON.parse(iframeName).attributes;
+  window.context = data._context;


This is now at data.attributes.context, per https://github.com/ampproject/amphtml/pull/6498/files#diff-34e84f9cf5e8134d86bf1fc893ab293aR128.

data is the attributes off of the name object, so window.context should get data._context, unless I'm misunderstanding what you mean

Ohhh! I missed the .attributes on the line above. Ignore this comment.

jridgewell · 2016-12-09T20:11:15Z

src/3p-frame.js


-  iframe.src = src;
+  iframe.src = baseUrl;
  iframe.name = name;


Need to decodeURIComponent?

No, it's just the base url

jridgewell · 2016-12-09T20:32:55Z

src/3p-frame.js

+  const name = JSON.stringify({
+    host,
+    type: attributes.type,
+    count: count[attributes.type],


Need to increment this value (this was a hard fought bug).

@jridgewell do you know what is the counter used for? I don't see it documented anywhere.

The counter is to ensure the iframe's window is not cached. #2955

Ah, good to know. @bradfrizzell could you add a link in the code?

jridgewell · 2016-12-09T20:33:50Z

3p/integration.js

+let data;
+try {
+  data = JSON.parse(iframeName).attributes;
+  window.context = data._context;


Ohhh! I missed the .attributes on the line above. Ignore this comment.

bradfrizzell · 2016-12-13T21:28:27Z

ptal

lannka · 2016-12-13T23:16:43Z

Some of my comments was buried. For example: #6498 (comment)
#6498 (comment)

bradfrizzell · 2016-12-14T14:49:49Z

Sorry about that, I addressed them. Changing the whole data structure of the attributes would be easier for me to fix in #6506 if that's alright. If you think it must be done as part of this PR I can do that

lannka

Sounds good to me to do them later. Please add a TODO.

I just had one more question about the counter. Not sure if @jridgewell can answer.

Otherwise this PR looks good.

lannka · 2016-12-14T16:21:22Z

src/3p-frame.js

+  const name = JSON.stringify({
+    host,
+    type: attributes.type,
+    count: count[attributes.type],


@jridgewell do you know what is the counter used for? I don't see it documented anywhere.

lannka · 2016-12-14T17:34:10Z

Ah, there's another previous comment not addressed yet (about the master selection code):
#6498 (review)

bradfrizzell · 2016-12-15T18:32:13Z

ptal @lannka

lannka · 2016-12-15T18:36:39Z

3p/integration.js

+ *  @param {string} name A URIencoded JSON object of context
+ *      that needs to be attached to an iframe.
+ *  @param {Element} iframe
+ */


hmm, seems now it's only used in one place, shall we just inline the function?

lannka · 2016-12-15T18:36:43Z

3p/integration.js

-const data = parseFragment(location.hash);
-window.context = data._context;
-
+const iframeName = window.name;


please explain why we cache this value.

I suppose we don't need to. fixed

i actually meant to add a comment why we cache this value.
Otherwise, you might want to move master selection code below all the code that read window.name.

d'oh. undid and added comment

lannka · 2016-12-15T18:38:37Z

src/3p-frame.js

+  // This name attribute may be overwritten if this frame is chosen to
+  // be the master frame. That is ok, as we will read the name off
+  // for our uses before that would occur.
+  // @see https://github.com/ampproject/amphtml/blob/master/3p/integration.js#L315


nit: remove the line number as it might soon be irrelevant

bradfrizzell · 2016-12-15T19:02:00Z

ptal @lannka

lannka · 2016-12-15T19:30:49Z

3p/integration.js

+ *  @param {string} name A URIencoded JSON object of context
+ *      that needs to be attached to an iframe.
+ *  @param {Element} iframe
+ */


sorry, by inline i meant to have

window.context.addContextToIframe = (iframe, name) => { iframe.name = name; }

I don't think doing it like that will work, though. we need the name to get captured in a closure, which is why I'm using bind above. I can move it all together and use bind up there but then it ends up getting a little less clean than just having them separated (in my opinion)

oh sorry, i meant

window.context.addContextToIframe = iframe => { iframe.name = iframeName; }

lannka · 2016-12-15T19:36:30Z

3p/integration.js

-const data = parseFragment(location.hash);
-window.context = data._context;
-
+const iframeName = window.name;


i actually meant to add a comment why we cache this value.
Otherwise, you might want to move master selection code below all the code that read window.name.

bradfrizzell · 2016-12-15T21:01:04Z

ptal

lannka · 2016-12-16T17:43:49Z

src/3p-frame.js

+  // be the master frame. That is ok, as we will read the name off
+  // for our uses before that would occur.
+  // @see https://github.com/ampproject/amphtml/blob/master/3p/integration.js
+  const name = JSON.stringify({


as discussed, we'll introduce an experiment flag to protect this behavior change (URL fragment => name attribute). So that we will have a easy way to recover if anything happen during releasing.

bradfrizzell · 2016-12-19T19:26:28Z

@lannka I added an experiment flag. Please take a look, let me know if I did this correctly.

lannka · 2016-12-19T23:04:20Z

3p/messaging.js

  const object = opt_object || {};
  object.type = type;
-  object.sentinel = window.context.amp3pSentinel;
+  object.sentinel = window.context.sentinel;


could you make this naming change in a separate PR? In case this renaming break anything, we can easily revert a smaller PR.

lannka · 2016-12-19T23:06:11Z

3p/integration.js

+/**
+ * If true, then in experiment where the passing of context metadata
+ * has been moved from the iframe src hash to the iframe name attribute.
+ */


the flag value is not available inside iframe. (I'm working on it right now #6684 ). To not be blocked by me, I suggest you read from fragment first, and then from name.

lannka · 2016-12-19T23:07:42Z

src/3p-frame.js

+ * If true, then in experiment where the passing of context metadata
+ * has been moved from the iframe src hash to the iframe name attribute.
+ */
+const nameExpOn = isExperimentOn(self, 'move-context-to-name');


suggested naming: '3p-iframe-context-in-name'

ended up using 'iframeContextInName'

lannka · 2016-12-19T23:08:31Z

src/3p-frame.js

    domFingerprint: domFingerprint(element),
    startTime,
  };
+  attributes.ampcontextVersion = (getMode().localDev ? 'LOCAL' :


seems this line can be added in your next PR, if you plan to move it eventually

bradfrizzell · 2016-12-20T15:43:05Z

addressed your comments @lannka

lannka

This looks good.

Please do thorough manual tests on your local server, to make sure no new error messages show up in the browser console. You will want to specifically pay attention to :

/examples/ads.amp.max.html
/examples/facebook.amp.max.html
/examples/twitter.amp.max.html
/examples/reddit.amp.max.html

lannka · 2016-12-20T17:37:52Z

tools/experiments/experiments.js

    Spec: 'https://github.com/ampproject/amphtml/issues/3813',
  },
+  {
+    id: 'move-context-to-name',


Change this move-context-to-name to 3p-frame-context-in-name?

lannka · 2016-12-20T17:38:10Z

tools/experiments/experiments.js

+    id: 'move-context-to-name',
+    name: 'Move passing context metadata from url hash to name attribute',
+    Spec: '',
+  },


please create a cleanupIssue as well

lannka · 2016-12-20T17:42:54Z

3p/integration.js

+// Need to cache iframeName as it will be potentially overwritten by
+// masterSelection, as per below.
+const iframeName = window.name;
+try {


can you move the try-catch into parseFragment? And make it return null for exception.

bradfrizzell · 2016-12-20T18:32:22Z

Checked those pages. they all looked good. I'm going to make sure the few console messages are all the same on the master branch

This commit may break functionality.

bradfrizzell · 2016-12-20T19:54:44Z

gah sorry about that, I just stupidly rebased without thinking

bradfrizzell · 2016-12-20T20:31:53Z

ptal / merge if ready

lannka

Please manual test twice, one with the flag on and one with the flag off, if you haven't done so. To toggle the flag, run the following in the console:

AMP.toggleExperiment('3p-frame-context-in-name');

lannka · 2016-12-21T05:16:44Z

src/3p-frame.js

+ * If true, then in experiment where the passing of context metadata
+ * has been moved from the iframe src hash to the iframe name attribute.
+ */
+const iframeContextInName = isExperimentOn(self, 'move-context-to-name');


the string literal needs to be identical to experiment ID you defined in experiments.js.
So, 'move-context-to-name' => '3p-frame-context-in-name'

ah whoops. fixed.

lannka · 2016-12-21T05:17:27Z

tools/experiments/experiments.js

    Spec: 'https://github.com/ampproject/amphtml/issues/3813',
  },
+  {
+    id: 'iframeContextInName',


to follow the convention: '3p-frame-context-in-name'

bradfrizzell · 2016-12-21T16:33:54Z

Ran the manual tests both ways. Everything looked fine, no new error messages/differences between experiment on/off.

bradfrizzell · 2016-12-21T17:00:17Z

PTAL

jridgewell · 2016-12-21T19:21:00Z

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

    expect(docInfo.pageViewId).to.not.be.empty;
-    const amp3pSentinel = iframe.getAttribute('data-amp-3p-sentinel');
+    let amp3pSentinel;
+    if (iframeContextInName) {


These tests need to be done with both iframeContextInName set to false and set to true. As it is, we'll never test the true case.

…#6498) * Moved context info to name object, associated changes. This commit may break functionality. * Fixed broken tests * Fixed function binding and addressed small comments * Added dev message on context parsing failure * Fixed param order for addNameToIframe * Fixed count and fixed param order in docstring * Added todo * Added code pointer * Fixed broken tests and naming of amp3pSentinel * Fixed broken 3p-messaging tests * Fixed lint issue * Added comment to address use of name attr. for master * Removed caching of window.name * Removed line number from code pointer * Redo caching of window.name * Inlined adding name func * Added experiment flag and fixed associated tests. * Changed exp flag name; Undid change to amp3pSentinel name * Added cleanup issue to exp flag; Fixed attaching of context * Fixed jsdoc return type to eliminate presubmit issue * Fixed typo * Fixed exp id name, add additional check before reading off hash

bradfrizzell assigned lannka and jridgewell Dec 8, 2016

bradfrizzell mentioned this pull request Dec 9, 2016

WIP - sentinel refactor #6506

Closed

lannka suggested changes Dec 9, 2016

View reviewed changes

jridgewell reviewed Dec 9, 2016

View reviewed changes

jridgewell requested changes Dec 9, 2016

View reviewed changes

jridgewell reviewed Dec 9, 2016

View reviewed changes

lannka reviewed Dec 14, 2016

View reviewed changes

jridgewell approved these changes Dec 14, 2016

View reviewed changes

lannka suggested changes Dec 15, 2016

View reviewed changes

lannka reviewed Dec 15, 2016

View reviewed changes

lannka reviewed Dec 16, 2016

View reviewed changes

lannka reviewed Dec 19, 2016

View reviewed changes

lannka approved these changes Dec 20, 2016

View reviewed changes

lannka reviewed Dec 20, 2016

View reviewed changes

bradfrizzell mentioned this pull request Dec 20, 2016

Remove iframeContextInName flag #6760

Closed

bradfrizzell added 2 commits December 20, 2016 14:24

Moved context info to name object, associated changes.

33084d0

This commit may break functionality.

Fixed broken tests

b293bb6

bradfrizzell added 16 commits December 20, 2016 14:24

Fixed count and fixed param order in docstring

49cfac7

Added todo

afd5274

Added code pointer

fc06f04

Fixed broken tests and naming of amp3pSentinel

c9fc3bd

Fixed broken 3p-messaging tests

c0a795a

Fixed lint issue

e7c64a6

Added comment to address use of name attr. for master

25b9417

Removed caching of window.name

2b0ef13

Removed line number from code pointer

9a09023

Redo caching of window.name

de4f8d4

Inlined adding name func

ac0a5f4

Added experiment flag and fixed associated tests.

c9575cb

Changed exp flag name; Undid change to amp3pSentinel name

db06757

Added cleanup issue to exp flag; Fixed attaching of context

f6e2c4c

Fixed jsdoc return type to eliminate presubmit issue

e670b0e

Fixed typo

d0b1d87

lannka reviewed Dec 21, 2016

View reviewed changes

Fixed exp id name, add additional check before reading off hash

2c729eb

lannka merged commit dc44b3b into ampproject:master Dec 21, 2016

jridgewell reviewed Dec 21, 2016

View reviewed changes

bradfrizzell mentioned this pull request Dec 21, 2016

Turned on 3p-frame-context-in-name experiment flag in prod #6774

Merged

bradfrizzell mentioned this pull request Jan 3, 2017

Added flag for name refactor to canary config #6858

Merged

bradfrizzell mentioned this pull request Jan 19, 2017

Removed iframeContextInName flag for experiment #7107

Merged

Conversation

bradfrizzell commented Dec 6, 2016

Uh oh!

bradfrizzell commented Dec 6, 2016

Uh oh!

lannka left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bradfrizzell Dec 9, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bradfrizzell Dec 9, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bradfrizzell Dec 9, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bradfrizzell Dec 9, 2016 •

edited

Loading

bradfrizzell Dec 9, 2016 •

edited

Loading

bradfrizzell Dec 9, 2016 •

edited

Loading

bradfrizzell Dec 13, 2016 •

edited

Loading