Changed amp3pSentinel to sentinel with exp flag #6922

bradfrizzell · 2017-01-06T18:30:53Z

Changed the use of amp3pSentinel to instead just sentinel, because it is no longer just a 3p use case. Put an experiment flag to make sure this change is easy to revert. Still need to add cleanup issue.

jridgewell

This definitely requires tests.

jridgewell · 2017-01-06T19:49:52Z

3p/messaging.js

-    if (data.sentinel != win.context.amp3pSentinel) {
+    if (sentinelNameChange && data.sentinel != win.context.sentinel) {
+      return;
+    } else if (data.sentinel != win.context.amp3pSentinel) {


This needs to check !sentinelNameChange. As is, this will always fail when using the sentinel-name-change experiment is enabled.

…it stuff

jridgewell · 2017-01-11T19:56:17Z

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

@@ -23,6 +23,9 @@ import {adopt} from '../../../../src/runtime';
 import {facebook} from '../../../../3p/facebook';
 import {setDefaultBootstrapBaseUrlForTesting} from '../../../../src/3p-frame';
 import {resetServiceForTesting} from '../../../../src/service';
+import {isExperimentOn} from '../../../../src/experiments';
+
+const sentinelNameChange = isExperimentOn(self, 'sentinel-name-change');


Ditto scenarios.

jridgewell · 2017-01-11T19:58:00Z

test/fixtures/served/iframe.html

+        if (event.data.amp3pSentinel) {
+          var sentinel = event.data.amp3pSentinel;
+        } else {
+          var sentinel = event.data.amp3pSentinel;


Do you mean .sentinel?

... yes ... >.<
fixed

jridgewell · 2017-01-11T19:58:38Z

test/fixtures/served/iframe.html

-      var sentinel = event.data.is3p ? event.data.amp3pSentinel : 'amp';
+      if (event.data.is3p) {
+        if (event.data.amp3pSentinel) {
+          var sentinel = event.data.amp3pSentinel;


This var needs to be hoisted above the if (event.data.is3p).

jridgewell · 2017-01-11T19:59:37Z

src/3p-frame.js

@@ -157,8 +159,13 @@ export function getIframe(parentWindow, parentElement, opt_type, opt_context) {
    // Chrome does not reflect the iframe readystate.
    this.readyState = 'complete';
  };
-  iframe.setAttribute(
-      'data-amp-3p-sentinel', attributes._context.amp3pSentinel);
+  if (sentinelNameChange) {


We can copy the same test from above:

iframe.setAttribute('data-amp-3p-sentinel', attributes._context[sentinelNameChange ? 'sentinel' : 'amp3pSentinel'])

jridgewell · 2017-01-11T20:00:03Z

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

@@ -46,6 +46,7 @@ describe('3p-frame', () => {
   */
  const iframeContextInName = isExperimentOn(
      window, '3p-frame-context-in-name');
+  const sentinelNameChange = isExperimentOn(window, 'sentinel-name-change');


Ditto scenarios.

jridgewell · 2017-01-12T18:40:54Z

src/3p-frame.js

@@ -58,6 +57,7 @@ function getFrameAttributes(parentWindow, element, opt_type, opt_context) {
  const width = element.getAttribute('width');
  const height = element.getAttribute('height');
  const type = opt_type || element.getAttribute('type');
+  const sentinelNameChange = isExperimentOn(self, 'sentinel-name-change');


This should be parentWindow, right?

jridgewell · 2017-01-12T18:41:01Z

src/3p-frame.js

@@ -120,6 +120,8 @@ export function getIframe(parentWindow, parentElement, opt_type, opt_context) {
  const attributes =
      getFrameAttributes(parentWindow, parentElement, opt_type, opt_context);
  const iframe = parentWindow.document.createElement('iframe');
+  const sentinelNameChange = isExperimentOn(self, 'sentinel-name-change');


jridgewell · 2017-01-12T18:41:37Z

test/fixtures/served/iframe.html

-      var sentinel = event.data.is3p ? event.data.amp3pSentinel : 'amp';
+      var sentinel;
+      if (event.data.is3p) {
+        sentinel = event.data.amp3pSentinel ? event.data.amp3pSentinel :


Nit: sentinel = event.data.amp3pSentinel || event.data.sentinel.

jridgewell · 2017-01-12T18:41:58Z

test/fixtures/served/iframe.html

@@ -34,14 +40,15 @@
            width: event.data.width,
          }, '*');
    } else if (event.data.type == 'subscribeToEmbedState') {
+      var sentinel;
      if (event.data.is3p) {
        if (event.data.amp3pSentinel) {


Can we make this a conditional expression too?

jridgewell · 2017-01-12T18:43:03Z

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

+        version: '$internalRuntimeVersion$',
+      };
+      if (sentinelName == 'sentinel') {
+        toggleExperiment(window, 'sentinel-name-change', true);


We'll need to toggle off in an afterEach.

lannka · 2017-01-13T05:36:40Z

src/3p-frame.js

@@ -157,8 +161,8 @@ export function getIframe(parentWindow, parentElement, opt_type, opt_context) {
    // Chrome does not reflect the iframe readystate.
    this.readyState = 'complete';
  };
-  iframe.setAttribute(
-      'data-amp-3p-sentinel', attributes._context.amp3pSentinel);
+  iframe.setAttribute('data-amp-3p-sentinel', attributes._context[


a bad merge?

I'm not sure what your concern is, could you elaborate?

I thought your other PR already removed all the data-amp-3p-sentinel attribute on the iframe, no?

The sentinel refactor one does, but I was under the impression this one should go through first. Hence, this one still has it.

oh i see. nvm

jridgewell

LGTM, defering to @lannka.

defering

lannka

LGTM in general.

lannka · 2017-01-17T21:22:43Z

3p/messaging.js

@@ -25,7 +25,8 @@ export function nonSensitiveDataPostMessage(type, opt_object) {
  }
  const object = opt_object || {};
  object.type = type;
-  object.sentinel = window.context.amp3pSentinel;
+  object.sentinel = window.context.sentinel ? window.context.sentinel :
+      window.context.amp3pSentinel;


object.sentinel = window.context.sentinel || window.context.amp3pSentinel;

lannka · 2017-01-17T21:26:23Z

3p/messaging.js

@@ -79,7 +80,10 @@ function startListening(win) {
    // Parse JSON only once per message.
    const data = /** @type {!Object} */ (
        JSON.parse(event.data.substr(4)));
-    if (data.sentinel != win.context.amp3pSentinel) {
+    if (win.context.sentinel && data.sentinel != win.context.sentinel) {


to make things simple, shall we just do

win.context.sentinel = win.context.sentinel || win.context.amp3pSentinel

during initialization stage.

lannka · 2017-01-17T21:30:00Z

src/3p-frame.js

@@ -157,8 +161,8 @@ export function getIframe(parentWindow, parentElement, opt_type, opt_context) {
    // Chrome does not reflect the iframe readystate.
    this.readyState = 'complete';
  };
-  iframe.setAttribute(
-      'data-amp-3p-sentinel', attributes._context.amp3pSentinel);
+  iframe.setAttribute('data-amp-3p-sentinel', attributes._context[


oh i see. nvm

bradfrizzell · 2017-01-17T21:31:36Z

ping @lannka

bradfrizzell · 2017-01-17T21:32:21Z

ha, whoops! sorry about the ping! I sent it a split second after your review came through. what are the odds of that?

lannka · 2017-01-17T23:39:46Z

build-system/tasks/presubmit-checks.js

@@ -335,6 +337,7 @@ var forbiddenTerms = {
      'extensions/amp-analytics/0.1/cid-impl.js',
      'src/cookies.js',
      'src/experiments.js',
+      'dist.3p/current/integration.js',


are changes in this file needed?

I was getting a presubmit failure due to dist.3p/current/integration.js without these.

lannka · 2017-01-18T19:44:26Z

the PR looks good now.
@bradfrizzell If you can confirm that you tested all the examples in ads.amp.max.html and no new error messages show up in console, I will go ahead and merge.

bradfrizzell · 2017-01-18T20:21:54Z

compared it against master, seeing all the same errors / warnings. looks fine to me

* Changed amp3pSentinel to sentinel with exp flag * Fixed flagging all uses of amp3pSentinel to sentinel change * Fixed presubmit issues * Whitelisted dist.3p/current/integration.js for cookie related presubmit stuff * Fixed dependency issue * Properly test both cases of the experiment * Added cleanup issue * Fixed lint issue * Addressed comments to test both cases of experiment * Fixed toggling off exp and using correct win * Removed unneeded complexity * Reverted presubmit-checks

Changed amp3pSentinel to sentinel with exp flag

b3139f7

bradfrizzell assigned lannka Jan 6, 2017

jridgewell requested changes Jan 6, 2017

View reviewed changes

bradfrizzell added 5 commits January 9, 2017 10:51

Fixed flagging all uses of amp3pSentinel to sentinel change

b003f71

Fixed presubmit issues

3c10aa9

Whitelisted dist.3p/current/integration.js for cookie related presubm…

ee2b56d

…it stuff

Fixed dependency issue

58848dd

Properly test both cases of the experiment

9f8f51f

bradfrizzell mentioned this pull request Jan 11, 2017

Cleanup Sentinel Name Refactor #6990

Closed

Added cleanup issue

da39155

bradfrizzell mentioned this pull request Jan 11, 2017

Ramp up sentinel name change in canary config #6991

Merged

bradfrizzell added 2 commits January 11, 2017 14:29

Merge branch 'master' into frizz-ac-switch-to-sentinel

a0e59da

Fixed lint issue

bf60e84

jridgewell reviewed Jan 11, 2017

View reviewed changes

Addressed comments to test both cases of experiment

190e6ff

jridgewell reviewed Jan 12, 2017

View reviewed changes

lannka reviewed Jan 13, 2017

View reviewed changes

Fixed toggling off exp and using correct win

64436ee

jridgewell previously approved these changes Jan 13, 2017

View reviewed changes

lannka reviewed Jan 17, 2017

View reviewed changes

Removed unneeded complexity

d48ff22

lannka reviewed Jan 17, 2017

View reviewed changes

Reverted presubmit-checks

acc16a1

lannka approved these changes Jan 18, 2017

View reviewed changes

lannka merged commit fe7e0ae into ampproject:master Jan 18, 2017

bradfrizzell deleted the frizz-ac-switch-to-sentinel branch January 19, 2017 16:07

Changed amp3pSentinel to sentinel with exp flag #6922

Changed amp3pSentinel to sentinel with exp flag #6922

Conversation

bradfrizzell commented Jan 6, 2017 • edited

jridgewell 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

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

jridgewell Jan 12, 2017 • 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

jridgewell left a 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

bradfrizzell commented Jan 17, 2017

bradfrizzell commented Jan 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lannka commented Jan 18, 2017

bradfrizzell commented Jan 18, 2017

bradfrizzell commented Jan 6, 2017 •

edited

jridgewell Jan 12, 2017 •

edited