Refactored sentinel to be passed as parameter by bradfrizzell · Pull Request #6825 · ampproject/amphtml

bradfrizzell · 2016-12-28T16:21:43Z

Previously, sentinel was stored as a data attribute on the iframe. There is no reason it can't be passed as a parameter, however. Passing the sentinel would be preferable to adding a redundant data attribute.

This change broke many tests which expected the data-amp3pSentinel attribute, so they all had to be changed.

This commit may break functionality.

Previously, the sentinel had been passed both on the hash of the iframe src, and as a data attribute. The data attribute was how the sentinel was referenced on the runtime side, even though there was no reason it could not simply be passed. Now, the sentinel is only passed to the iframe as a member of an object that is stringified and attached to the iframe name. On the runtime side, the sentinel is now passed and expected as a parameter to multiple functions that used to ultimately rely on it being available as a data attribute of the iframe.

bradfrizzell · 2016-12-28T16:22:39Z

Apologies for how long this is... this change hit a lot of files. I made a new PR and am closing the WIP previous one

lannka · 2017-01-05T00:10:04Z

3p/messaging.js

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


can we split this change into a standalone PR? for sake of easy rollback in case

sure, I assume I should make a new experiment flag for it as well?

ideally. less hassle for our release eng in case of issues.
a lesson we just learned: #6864

lannka · 2017-01-05T00:37:50Z

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

-    return this.xOriginIframeHandler_.init(iframe, /* opt_isA4A */ true);
+    this.rendered_ = true;
+    return this.xOriginIframeHandler_.init(iframe, sentinel,
+                                           /* opt_isA4A */ true);


nit: can we break after .init(

lannka · 2017-01-05T00:38:16Z

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

-    return this.iframeRenderHelper_(iframe);
+    const sentinel = generateSentinel(window);
+    const metadata = getContextMetadata(window, iframe, sentinel);
+    iframe.setAttribute('name', encodeURI(JSON.stringify(metadata)));


you will want encodeURIComponent. but is that necessary? setAttribute should take care of char escape?

I just tested it in js console and setAttribute did not escape anything. will change to encodeURIComponent

lannka · 2017-01-05T00:39:50Z

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

    expect(child).to.be.ok;
    expect(child.src).to.have.string(srcUrl);
-    expect(child.getAttribute('name')).not.to.be.ok;
+    expect(child.getAttribute('name')).to.be.ok;


do you want to verify the content?

lannka · 2017-01-05T00:40:28Z

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

      const iframe = getIframe(this.element.ownerDocument.defaultView,
          this.element, undefined, opt_context);
+      let sentinel;
+      try {


read flag instead of try-catch?

lannka · 2017-01-05T01:20:46Z

src/iframe-helper.js

-    iframe,
-    opt_is3P
-  );
+      parentWin,


are we able to fit in one line if we break after =?

lannka · 2017-01-05T01:21:18Z

src/iframe-helper.js

-export function listenFor(
-    iframe, typeOfMessage, callback, opt_is3P, opt_includingNestedWindows) {
+export function listenFor(iframe, sentinel, typeOfMessage, callback,
+                          opt_is3P, opt_includingNestedWindows) {


indentation.
please try to config your IDE to match our code style.

lannka · 2017-01-05T01:27:51Z

src/iframe-helper.js

  }
  object.type = type;
-  object.sentinel = getSentinel_(iframe, opt_is3P);
+  object.sentinel = sentinel;


shall we use serializeMessage in 3p-iframe.js

added a todo, will do in later pr as we discussed

lannka · 2017-01-05T01:33:33Z

src/iframe-helper.js

 */
-export function postMessageToWindows(iframe, targets, type, object, opt_is3P) {
+export function postMessageToWindows(
+    iframe, sentinel, targets, type, object, opt_is3P) {


seems param iframe is not needed anymore?

while I think you are right, getting rid of that parameter might be better suited to another PR. I suspect getting rid of it in this one will lead to many additional changes

lannka · 2017-01-05T01:37:46Z

src/iframe-helper.js

 */
-export function postMessage(iframe, type, object, targetOrigin, opt_is3P) {
-  postMessageToWindows(iframe,
+export function postMessage(


is this method used elsewhere?

lannka · 2017-01-06T19:12:44Z

src/intersection-observer-polyfill.js

   * @param {boolean=} opt_is3p
   */
-  constructor(baseElement, iframe, opt_is3p) {
+  constructor(baseElement, iframe, sentinel, opt_is3p) {


you might need to change intersection-observer.js as well now. it's unfortunately reverted from trash.

lannka · 2017-01-06T19:17:05Z

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

-    iframe.setAttribute('data-amp-3p-sentinel', 'amp3ptest' + testIndex);
-    iframe.name = 'test_nomaster';
+    sentinel = 'amp3ptest' + testIndex;
+    iframe.setAttribute('data-amp-3p-sentinel', sentinel);


why are we still setting this attribute?

lannka · 2017-01-06T19:21:56Z

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

   * @private
   */
-  iframeRenderHelper_(iframe) {
+  iframeRenderHelper_(iframe, sentinel) {


nit: for better readability can we move this method after renderViaNameAttrOfXOriginIframe_.

jridgewell · 2017-01-09T20:31:15Z

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

    // TODO(keithwrightbos): noContentCallback?
    this.xOriginIframeHandler_ = new AMP.AmpAdXOriginIframeHandler(this);
-    return this.xOriginIframeHandler_.init(iframe, /* opt_isA4A */ true);
+    this.rendered_ = true;


Why was this added?

hm good question. I don't recall ever writing that line, I wonder if that is a holdover from some merge-conflict-resolution at some point. will remove.

jridgewell · 2017-01-09T20:35:14Z

extensions/amp-facebook/0.1/amp-facebook.js

    const iframe = getIframe(this.win, this.element, 'facebook');
    this.applyFillContent(iframe);
+    const sentinel = generateSentinel(window);
+    iframe.name = encodeURI(JSON.stringify({


Can't use such a predictable name, or we'll run into iframe caching.

sorry, I'm confused at what needs to be done to fix this. The name attribute is totally random, because it contains the sentinel (which has a random string as part of it).

Ah, missed the random sentinel part. Never mind.

jridgewell · 2017-01-09T20:35:52Z

extensions/amp-facebook/0.1/amp-facebook.js

    const iframe = getIframe(this.win, this.element, 'facebook');
    this.applyFillContent(iframe);
+    const sentinel = generateSentinel(window);
+    iframe.name = encodeURI(JSON.stringify({


This should be a part of getIframe.

jridgewell · 2017-01-09T20:37:41Z

extensions/amp-iframe/0.1/amp-iframe.js

    setSandbox(this.element, iframe, this.sandbox_);
    iframe.src = this.iframeSrc;
+    const sentinel = generateSentinel(window);
+    iframe.name = encodeURI(JSON.stringify({


jridgewell · 2017-01-09T20:40:05Z

src/attributes.js

+      .getUnconfirmedReferrerUrl();
+
+  attributes._context = {
+    ampcontextVersion: (getMode().localDev ? 'LOCAL' :


Is this ternary necessary? In localdev mode, the $internalRuntimeVersion$ magic string will be a literal '$internalRuntimeVersion$'.

lannka · 2017-02-15T00:13:40Z

@bradfrizzell are you still on this? or it is already covered by the other PRs?

bradfrizzell · 2017-02-15T00:18:08Z

Still on top of this. It is not a top priority, because the refactor is not blocking any functionality. Focusing on top blockers at the moment. I will close this PR for now though, as when I get back to this I will just do it from scratch. The merge conflict with this right now is a huge pain to deal with, tried to push it through quickly friday but it was not happening at all. I broke all the key functionality out of this PR already and it is all merged. All that is left is the refactoring elements.

bradfrizzell added 30 commits December 27, 2016 16:26

Moved context info to name object, associated changes.

b78a10e

This commit may break functionality.

Fixed broken tests

32e46da

Fixed function binding and addressed small comments

0ab2697

Fixed param order for addNameToIframe

67afb27

Fixed count and fixed param order in docstring

02a2923

Added todo

b26e169

Added code pointer

39bcb12

Fixed broken tests and naming of amp3pSentinel

a5a472c

Fixed broken 3p-messaging tests

3dc4543

Fixed lint issue

cabc0d0

Added comment to address use of name attr. for master

1f802b9

Removed caching of window.name

8252478

Removed line number from code pointer

4e501a7

Redo caching of window.name

66ab0e9

Inlined adding name func

591fd81

Added experiment flag and fixed associated tests.

d5288f8

Changed exp flag name; Undid change to amp3pSentinel name

5dadd43

Added cleanup issue to exp flag; Fixed attaching of context

59423bc

Fixed typo

532dddb

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

92fad6f

Made changes to fix presubmit failures

9248f26

Made changes to fix presubmit failures

46c8afc

Export function getPageViewId

14d2bb3

Fixed means of caching pageViewId

e1a3a43

Broke out reused attribute code, now use docInfo

fafe989

Fixed lint issues

1d5b2f1

Broke out additional attributes to common code

9d224a0

Fixed typos and some broken tests

c0d8965

bradfrizzell added 8 commits December 27, 2016 16:26

Fixed broken tests; fixed amp-iframe to pass sentinel

98a7fca

Added passing sentinel to fix broken tests

ea9b252

Fixed broken iframe-helper tests

0e1959c

Fix broken 3p-frame test

4731939

Fixed broken test, should pull sentinel off name

ca8be7c

Name is now used, have to check that it is ok

4abe735

Fixed passing of sentinel for facebook test

f8f1303

Fix lint mistakes

5197b5a

bradfrizzell assigned lannka Dec 28, 2016

bradfrizzell mentioned this pull request Jan 3, 2017

Fix breakages in AmpContext due to use of IframeMessagingClient #6859

Closed

lannka suggested changes Jan 5, 2017

View reviewed changes

lannka reviewed Jan 6, 2017

View reviewed changes

jridgewell reviewed Jan 9, 2017

View reviewed changes

bradfrizzell mentioned this pull request Jan 20, 2017

Created attributes.js #7129

Merged

bradfrizzell added the WIP label Jan 30, 2017

bradfrizzell closed this Feb 15, 2017

Conversation

bradfrizzell commented Dec 28, 2016

Uh oh!

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

Uh oh!

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!

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!

lannka commented Feb 15, 2017

Uh oh!

bradfrizzell commented Feb 15, 2017

Uh oh!

Reviewers

Assignees

bradfrizzell commented Dec 28, 2016 •

edited

Loading