Implemented AmpContext and IframeMessagingClient #6310

bradfrizzell · 2016-11-22T21:40:15Z

No description provided.

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

3p/x-domain-child-message-handler.js

+/**
+ * @abstract
+ */
+export class XDomainChildMessageHandler {


How about IframeMessagingClient?

works for me

lannka · 2016-11-22T21:53:18Z

3p/x-domain-child-message-handler.js

+            try {
+              // We should probably report exceptions within callback
+              this.callbackFor_[payload.type](payload);
+            } catch (err) {


shall we just let it fail? since we've no idea if the failure is recoverable.

Why would letting the error propagate be a good thing? So if the callback fails for whatever reason, that the ad can see that it failed?

lannka · 2016-11-22T21:54:44Z

3p/x-domain-child-message-handler.js

+   *  Only valid for the trivial case when we will always be messaging our parent
+   *  Should be overwritten for subclasses
+   */
+  getAmpWindow(){


getHostWindow(), since in amp-inabox use case, it's beyond AMP

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

3p/ampcontext-lib.js

+ */
+try {
+  console.log("Attempting to make AmpContext");
+  const windowContextCreated = new Event('windowContextCreated');


maybe prefix the event name with amp to avoid conflict?

lannka · 2016-11-22T21:58:51Z

gulpfile.js

@@ -173,6 +173,28 @@ function compile(watch, shouldMinify, opt_preventRemoveAndMakeDir,
    includePolyfills: true,
  });

+  compileJs('./3p/', 'ampcontext.js',


why do we need 2 binaries ?

Well the reason that it is split into 2 files, ampcontext and ampcontext-lib, is because firing the 'windowContextCreated' event from within the AmpContext constructor sends it before the instance is actually ready. Whether we actually need 2 binaries is a good question. Are the binaries only made for files that ad networks should get access to off our cdn? In that case I guess we don't need one for ampcontext.js
@keithwrightbos

lannka · 2016-11-22T22:04:46Z

3p/ampcontext.js

+ *  @returns {function} that when called stops triggering the callback
+ *    every time we receive a page visibility message.
+ */
+AmpContext.prototype.observePageVisibility = function(callback) {


why not moving those methods into the class?

lannka · 2016-11-22T22:06:01Z

3p/ampcontext.js

+  Enum for the different postmessage types for the window.context
+  postmess api.
+*/
+export const MessageType_ = {


do we plan to use it outside this file?

yea I think I was, I can double check where. I know there was a reason I put the export on

lannka · 2016-11-22T22:06:51Z

3p/ampcontext.js

+   *  the class instance.
+   *  @private
+   */
+  setupMetadata_() {


pls move private method to the bottom of the class

lannka · 2016-11-22T22:09:49Z

3p/ampcontext.js

+      this.startTime = context.startTime;
+      this.referrer = context.referrer;
+    } catch (err) {
+      user().error('Could not parse metadata.');


this will be a dev() error

lannka · 2016-11-22T22:10:25Z

3p/ampcontext.js

+
+  /** @override */
+  getSentinel(){
+    this.setupMetadata_();


shall we move this to constructor?

done, actually don't need to call it all in the constructor, setupmetadata takes care of it. just need to overwrite the parent class getSentinel. fixed it.

jridgewell · 2016-11-28T20:36:11Z

3p/ampcontext.js

+      this.ancestors = [];
+      for (let win = this.win_; win && win != win.parent; win = win.parent) {
+        // Add window keeping the top-most one at the front.
+        this.ancestors.unshift(win.parent);


Nit: pushing then reversing the final array will be faster.

jridgewell · 2016-11-28T20:37:44Z

3p/ampcontext.js

+      return this.ancestors[this.depth];
+    } else {
+      dev().error('Incorrect sentinel format.');
+      throw new Error('Incorrect sentinel format.');


This can be done as a dev().assert:

const sentinelMatch = this.sentinel.match(/((\d+)-\d+)/); dev().assert(sentinelMatch, 'Incorrect sentinel format.'); // use sentinelMatch

jridgewell · 2016-11-28T20:38:38Z

3p/ampcontext.js

+        // Add window keeping the top-most one at the front.
+        this.ancestors.unshift(win.parent);
+      }
+      return this.ancestors[this.depth];


So depth is top-down?

jridgewell · 2016-11-28T20:39:49Z

3p/ampcontext.js

+    const sentinelMatch = this.sentinel.match(/((\d+)-\d+)/);
+    if (sentinelMatch) {
+      this.depth = Number(sentinelMatch[2]);
+      this.ancestors = [];


Do we need ancestors as a property?

jridgewell · 2016-11-28T20:42:50Z

3p/iframe-messaging-client.js

+     *  that message
+     *  @private {object}
+     */
+    this.callbackFor_ = {};


We have a map helper in types.js.

jridgewell · 2016-11-28T20:47:54Z

3p/iframe-messaging-client.js

+    listen(this.win_, 'message', message => {
+      // Does it look a message from AMP?
+      if (message.source == this.ampWindow && message.data &&
+          message.data.indexOf('amp-') == 0) {


startsWith

jridgewell · 2016-11-28T20:49:32Z

3p/iframe-messaging-client.js

+              this.callbackFor_[payload.type]) {
+            try {
+              // We should probably report exceptions within callback
+              this.callbackFor_[payload.type](payload);


Nit: this will leak the callbackFor_ object as the this context.

is setting callbackFor_ as private enough? I'm not 100% sure what you are saying by this, sorry. Do you just mean that the way it currently is has callbackFor_ as public?

jridgewell · 2016-11-28T20:51:08Z

3p/iframe-messaging-client.js

+  setupEventListener_() {
+    listen(this.win_, 'message', message => {
+      // Does it look a message from AMP?
+      if (message.source == this.ampWindow && message.data &&


This might be better flattened out:

if (message.source !== this.ampWindow) { return; } if (!startsWith(String(message.data), 'amp-') { return; } if (payload.sentinel !== this.sentinel) { return; }

jridgewell · 2016-11-28T20:52:29Z

3p/ampcontext.js

+  constructor(win) {
+    super(win);
+    this.setupMetadata_();
+    this.ampWindow = this.getHostWindow();


Can this be a private property?

yes that's fine

jridgewell · 2016-11-28T20:55:04Z

3p/ampcontext.js

+  /**
+   *  Takes the current name on the window, and attaches it to
+   *  the name of the iframe.
+   *  @param {Iframe} iframe The iframe we are adding the context to.


HTMLIframeElement?

(for dev: cherry pick on to frizz-ampcontext)

lannka · 2016-11-29T18:37:48Z

3p/ampcontext-lib.js

+ *  functionality.
+ */
+try {
+  console/*OK*/.log('Attempting to make AmpContext');


should use dev()?

lannka · 2016-11-29T18:38:06Z

3p/ampcontext.js

+ */
+import './polyfills';
+import {
+  dev,


nit, can you merge them to one line to save some space?

lannka · 2016-11-29T18:40:26Z

3p/ampcontext.js

+  }
+
+  /** @override */
+  getHostWindow() {


getHostWindow as a public method, can potentially be called multiple times, should return a cache when available.

we can move the computation logic into constructor.
or, if you want to do lazy initialization, which I don't see much value here, you can initialize this.ampWindow here. Then all the other code should not reference this.ampWindow directly, they should reference this method.

done. moved the logic into the constructor. getHostWindow just returns this.ampWindow now

lannka · 2016-11-29T18:41:53Z

3p/ampcontext.js

+        this.ancestors.unshift(win.parent);
+      }
+      return this.ancestors[this.depth];
+    } else {


nitpick: can we reverse the if-else? i.e., fail early in the code and save some indentation.

n/a, got rid of it

lannka · 2016-11-29T18:43:52Z

3p/ampcontext.js

+   */
+  observePageVisibility(callback) {
+    const stopObserveFunc = this.registerCallback_(MessageType_.EMBED_STATE,
+                                                   callback);


we use 4 space indentation for line breaks.

in this case, you can break after '('

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

3p/iframe-messaging-client.js

+import {listen} from '../src/event-helper';
+import {getRandom} from '../src/3p-frame';
+import {user} from '../src/log';
+/**


add a blank line

lannka · 2016-11-29T18:53:03Z

3p/iframe-messaging-client.js

+   * @param {function(object)} callback The callback function to call
+   *   when a message with type messageType is received.
+   */
+  registerCallback_(messageType, callback) {


maybe call it listenFor?

there is already a listenFor function in use, would rather be able to disambiguate

sounds good.

lannka · 2016-11-29T18:56:02Z

gulpfile.js

@@ -173,6 +173,17 @@ function compile(watch, shouldMinify, opt_preventRemoveAndMakeDir,
    includePolyfills: true,
  });

+  compileJs('./3p/', 'ampcontext-lib.js',
+      './dist.3p/' + (shouldMinify ? internalRuntimeVersion : 'current'), {
+    minifiedName: 'ampcontext-lib.js',


how about ampcontext-v0.js

that's fine

lannka · 2016-11-29T18:59:20Z

3p/ampcontext-lib.js

@@ -0,0 +1,32 @@
+/**
+ * Copyright 2015 The AMP HTML Authors. All Rights Reserved.


lannka · 2016-11-29T18:59:26Z

3p/ampcontext.js

@@ -0,0 +1,203 @@
+/**
+ * Copyright 2015 The AMP HTML Authors. All Rights Reserved.


bradfrizzell · 2016-11-30T16:41:58Z

PTAL - I've addressed all the comments

lannka

LGTM with some cosmetic nitpicks.

lannka · 2016-11-30T16:55:00Z

3p/iframe-messaging-client.js

+            this.callbackFor_[payload.type](payload);
+          } catch (err) {
+            user().error('IFRAME-MSG',
+                         `- Error in registered callback ${payload.type}`, err);


you probably want to config your IDE so that it indents 4 spaces on line break, instead of aligning.

gah whoops, fixed

lannka · 2016-11-30T16:55:26Z

3p/iframe-messaging-client.js

+      if (!message.data) {
+        return;
+      }
+      if (!startsWith(String(message.data), 'amp-')) {


how about merging the 3 ifs into one block

That's how I originally had it, I split it apart as per a comment from @jridgewell

Fine, people have different taste :-)

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

3p/iframe-messaging-client.js

+   * @param {function(object)} callback The callback function to call
+   *   when a message with type messageType is received.
+   */
+  registerCallback_(messageType, callback) {


sounds good.

lannka · 2016-11-30T18:09:34Z

Can you fix the Travis, so I can go ahead and merge?

bradfrizzell · 2016-11-30T18:13:59Z

Just pushed changes ~1 min ago, fixes all of the lint/presubmit/etc stuff. Or at least it does for me...

…

On Wed, Nov 30, 2016 at 1:09 PM, Hongfei Ding ***@***.***> wrote: Can you fix the Travis, so I can go ahead and merge? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#6310 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AEFDACBNgBAaLH0fGTtgAC3O_Ouft6p9ks5rDbvmgaJpZM4K5-Oe> .

bradfrizzell · 2016-11-30T18:34:06Z

it passed all travis stuff, good to go On Wed, Nov 30, 2016 at 1:13 PM, Brad Frizzell <bradfrizzell@google.com> wrote:

…

Just pushed changes ~1 min ago, fixes all of the lint/presubmit/etc stuff. Or at least it does for me... On Wed, Nov 30, 2016 at 1:09 PM, Hongfei Ding ***@***.***> wrote: > Can you fix the Travis, so I can go ahead and merge? > > — > You are receiving this because you authored the thread. > Reply to this email directly, view it on GitHub > <#6310 (comment)>, > or mute the thread > <https://github.com/notifications/unsubscribe-auth/AEFDACBNgBAaLH0fGTtgAC3O_Ouft6p9ks5rDbvmgaJpZM4K5-Oe> > . >

jridgewell · 2016-11-30T19:01:23Z

3p/ampcontext.js

+      // Add window keeping the top-most one at the front.
+      ancestors.push(win.parent);
+    }
+    ancestors.reverse();


We can avoid the reverse by counting backwards:

this.ampWindow = ancestors[ancestors.length - 1 - this.depth];

jridgewell · 2016-11-30T19:01:31Z

3p/ampcontext.js

+    /** Calculate the hostWindow / ampWindow_ */
+    const sentinelMatch = this.sentinel.match(/((\d+)-\d+)/);
+    dev().assert(sentinelMatch, 'Incorrect sentinel format');
+    this.depth = Number(sentinelMatch[2]);


Is this property necessary?

nope, not needed as a property anymore. fixed

jridgewell · 2016-11-30T19:03:04Z

3p/ampcontext.js

+
+  /** @override */
+  registerCallback_(messageType, callback) {
+    user().assertEnumValue(MessageType_, messageType);


This allows the send-* types, which weren't allowed before. Is that intentional?

yes, I had previously made a mistake.

jridgewell · 2016-11-30T19:03:21Z

3p/ampcontext.js

+  observePageVisibility(callback) {
+    const stopObserveFunc = this.registerCallback_(
+        MessageType_.EMBED_STATE, callback);
+    this.ampWindow_.postMessage/*REVIEW*/({


jridgewell · 2016-11-30T19:03:30Z

3p/ampcontext.js

+  observeIntersection(callback) {
+    const stopObserveFunc = this.registerCallback_(
+        MessageType_.INTERSECTION, callback);
+    this.ampWindow_.postMessage/*REVIEW*/({


jridgewell · 2016-11-30T19:03:39Z

3p/ampcontext.js

+   *  @param {int} width The new width for the ad we are requesting.
+   */
+  requestResize(height, width) {
+    this.ampWindow_.postMessage/*REVIEW*/({


jridgewell · 2016-11-30T19:08:05Z

3p/iframe-messaging-client.js

+            this.callbackFor_[payload.type]) {
+          try {
+            // We should probably report exceptions within callback
+            this.callbackFor_[payload.type](payload);


Nit: this leaks the callbackFor_ private property. We can assign the function to a variable, then call the variable to avoid.

jridgewell · 2016-11-30T19:11:04Z

3p/ampcontext.js

+  }
+
+  /** @override */
+  getHostWindow() {


We can avoid these awkward overrides by using a generator method in the base class:

class Base { getHostWindow() { if (!this.window_) { this.window_ = this.generateWindow(); } return this.window_; } generateWindow() { return this.win.parent; } } class Special extends Base { generateWindow() { // depth stuff } }

This sounds better. @bradfrizzell what you say?

implementing.

* Added ampcontext.js, ampcontext-lib and updated gulpfile. * Changed how ampcontext-lib creates window.context (for dev: cherry pick on to frizz-ampcontext) * Split out PostMessenger as a parent class * Changed name of class * Made changes as per Hongfei. Had to expose getRandom(). * super() must be called before we can use this * Fixed broken funtion documentation * Fixed bug in lib and changed error thrown * Made changes to fix presubmit failures * Made fixes as per reviewers * Fixed lint/style and presubmit errors.

lannka requested changes Nov 22, 2016

View reviewed changes

jridgewell reviewed Nov 28, 2016

View reviewed changes

bradfrizzell and others added 9 commits November 29, 2016 09:10

Added ampcontext.js, ampcontext-lib and updated gulpfile.

ed372e1

Changed how ampcontext-lib creates window.context

c9efcaf

(for dev: cherry pick on to frizz-ampcontext)

Split out PostMessenger as a parent class

51f94d3

Changed name of class

5f6b69d

Made changes as per Hongfei. Had to expose getRandom().

361e2f4

super() must be called before we can use this

099a596

Fixed broken funtion documentation

b84acfb

Fixed bug in lib and changed error thrown

c67f665

Made changes to fix presubmit failures

ea1e2ce

lannka reviewed Nov 29, 2016

View reviewed changes

Made fixes as per reviewers

2c5ecb7

lannka approved these changes Nov 30, 2016

View reviewed changes

lannka self-assigned this Nov 30, 2016

lannka added the WG: monetization label Nov 30, 2016

lannka changed the title ~~Implemented AmpContext and XDomainChildMessageHandler~~ Implemented AmpContext and IframeMessagingClient Nov 30, 2016

Fixed lint/style and presubmit errors.

b34a507

lannka merged commit 555b554 into ampproject:master Nov 30, 2016

jridgewell reviewed Nov 30, 2016

View reviewed changes

bradfrizzell mentioned this pull request Dec 1, 2016

Cleaned up AmpContext and IframeMessagingClient. #6441

Merged

		@@ -0,0 +1,32 @@
		/**
		* Copyright 2015 The AMP HTML Authors. All Rights Reserved.

		@@ -0,0 +1,203 @@
		/**
		* Copyright 2015 The AMP HTML Authors. All Rights Reserved.

Implemented AmpContext and IframeMessagingClient #6310

Implemented AmpContext and IframeMessagingClient #6310

Conversation

bradfrizzell commented Nov 22, 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

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

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

bradfrizzell commented Nov 30, 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

lannka commented Nov 30, 2016

bradfrizzell commented Nov 30, 2016 via email

bradfrizzell commented Nov 30, 2016 via email

Choose a reason for hiding this comment