WIP - sentinel refactor by bradfrizzell · Pull Request #6506 · ampproject/amphtml

bradfrizzell · 2016-12-06T21:58:56Z

Refactored the passing of the sentinel value. No need to have a data attribute on the html element. Should just pass the sentinel as required.

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.

…google/amphtml into frizz-postmessage-api-sentinel-refactor

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.

…google/amphtml into frizz-postmessage-api-sentinel-refactor

…e-api-sentinel-refactor

bradfrizzell · 2016-12-06T21:59:13Z

@lannka

lannka · 2016-12-06T22:08:36Z

src/attributes.js

+  attributes._context = {
+    referrer: self.document.referrer,
+    canonicalUrl,
+    pageViewId: getPageViewId(parentWindow),


We should get values from documentInfoForDoc(element)

documentInfoForDoc only works if the element passed in is already attached to the DOM. The element here (the iframe) is not yet connected to the dom. Thus, documentInfoForDoc will throw. We can't write the element to the dom until its name attribute is set, and we can't set the name until we get the pageViewId. That is why I broke the pageViewId out from documentInfoForDoc. Maybe there is a better fix

you can get it from documentInfoForDoc(parentWindow)

ah that makes sense. thanks

lannka · 2016-12-07T18:31:22Z

src/attributes.js

+
+  let canonicalUrl;
+  try{
+    canonicalUrl = self.document.querySelector(


don't use self, use parentWindow instead.
If the naming parentWindow is confusing, i suggest to switch to win

can actually use the documentInfoForDoc here as well

lannka · 2016-12-07T18:33:18Z

I saw you duplicated some logic from 3p-iframe.js to attributes.js, do you plan to consolidate?

bradfrizzell · 2016-12-08T16:57:41Z

Yes, working on consolidating that now.

jridgewell · 2016-12-08T18:46:16Z

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

      this.lifecycleReporter.sendPing('adRequestStart');
      const iframe = getIframe(this.element.ownerDocument.defaultView,
-          this.element, undefined, opt_context);
+                               this.element, undefined, opt_context);


spacing is incorrect.

jridgewell · 2016-12-08T18:47:42Z

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

-          this.updateSize_(data.height, data.width, source, origin);
-        }, true, true));
+    this.unlisteners_.push(listenFor(this.iframe, this.sentinel, 'embed-size',
+                                     (data, source, origin) => {


jridgewell · 2016-12-08T18:55:04Z

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

      const iframe = getIframe(this.element.ownerDocument.defaultView,
-          this.element, undefined, opt_context);
+                               this.element, undefined, opt_context);
+      const sentinel = JSON.parse(iframe.name).attributes._context.sentinel;


This is not a JSON encoded value, it's https://github.com/ampproject/amphtml/pull/6506/files#diff-34e84f9cf5e8134d86bf1fc893ab293aR123

sorry, this PR really should go out after #6498, that's why this was here

bradfrizzell added 11 commits November 23, 2016 14:41

Made changes to fix presubmit failures

2016d2c

Made changes to fix presubmit failures

bb5f40f

Merge branch 'frizz-postmessage-api-sentinel-refactor' of github.com:…

d8a2a3d

…google/amphtml into frizz-postmessage-api-sentinel-refactor

Made changes to fix presubmit failures

183a6f4

Merge branch 'frizz-postmessage-api-sentinel-refactor' of github.com:…

4a2d8ef

…google/amphtml into frizz-postmessage-api-sentinel-refactor

Export function getPageViewId

37f0edf

Fixed means of caching pageViewId

dc0490b

Merge branch 'frizz-postmessage-api-pageViewId' into frizz-postmessag…

845aa36

…e-api-sentinel-refactor

lannka reviewed Dec 6, 2016

View reviewed changes

lannka self-assigned this Dec 6, 2016

lannka reviewed Dec 7, 2016

View reviewed changes

lannka mentioned this pull request Dec 7, 2016

Export function getPageViewId #6502

Closed

jridgewell requested changes Dec 8, 2016

View reviewed changes

Broke out reused attribute code, now use docInfo

648e531

bradfrizzell mentioned this pull request Dec 9, 2016

Refactored of passing frame attribute/context information #6498

Merged

Fixed lint issues

4ef3841

bradfrizzell added Closed: Duplicate WIP labels Jan 30, 2017

bradfrizzell closed this Jan 30, 2017

Conversation

bradfrizzell commented Dec 6, 2016

Uh oh!

bradfrizzell commented Dec 6, 2016

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 Dec 7, 2016

Uh oh!

bradfrizzell commented Dec 8, 2016

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

bradfrizzell Dec 9, 2016 •

edited

Loading