Origin trials by kmh287 · Pull Request #8944 · ampproject/amphtml

kmh287 · 2017-04-25T19:49:20Z

Partial for #8331.

Allows publishers to enable certain experiments, provided that a page includes a token that describes the experiments that should be enabled. The token is formatted as follows:

A base64 encoding of:

1 byte version number, starting at 0
4 byte length of experiments config
The experiments config, as a JSON
The signature of the experiments JSON

The experiments config will contain the experiments to enable, their expirations, and the origin to enable the experiments for. (To prevent copying of one token from one site to another).

/to @choumx @jridgewell

kmh287

@choumx

kmh287 · 2017-05-01T19:45:21Z

src/experiments.js


+function getExperimentsKey_() {
+  let key;
+  const url = '' //TODO(kmh287): fill this in


Not sure how to set up a key to download. Is the extra request worth the time it would take to perform another request?

I don't think we want to do that since it'll prevent offline functionality. We can patch prod if we really need to kill a feature, though I don't think that will be needed in practice.

kmh287 · 2017-05-01T19:45:36Z

src/experiments.js

+      method: 'GET',
+      ampCors: false,
+      credentials: 'omit',
+    }).then(jwkObj => {


How can this be tested? I'm not sure how to generate a JSON web key nor how to use it to sign some fake data.

Give this site a shot.

kmh287 · 2017-05-01T19:46:37Z

src/experiments.js

+ * @param {string} experimentId
+ * @return {!Promise<boolean>}
+ */
+export function isExperimentOnForOriginTrial(win, experimentId) {


Should this function just run once when the module is loaded?

Should be up to the caller. May need to be run multiple times in the case where there are multiple AMP documents (e.g. shadow AMPs) but we should cache the result.

kmh287 · 2017-05-01T19:47:49Z

src/experiments.js

+        throw new Error('Config does not match current origin');;
+      }
+      const experiments = config['experiments'];
+      const experiment = experiments[experimentId];


When we generate these tokens, we're going to need to either keep track of all experiments for each origin on our end, or request that all experiments be requested at once. Alternatively we can allow multiple meta tags with tokens, but that sounds like it could get slow very quickly.

Ack. Publishers that want to be whitelisted for additional experiments will receive a new token that covers old and new experiments.

dreamofabear

Good start! I'll kick off an email thread with the A4A team who owns this code to ask for guidance on usage of this API.

dreamofabear · 2017-05-02T00:12:14Z

src/experiments.js


+function getExperimentsKey_() {
+  let key;
+  const url = '' //TODO(kmh287): fill this in


I don't think we want to do that since it'll prevent offline functionality. We can patch prod if we really need to kill a feature, though I don't think that will be needed in practice.

dreamofabear · 2017-05-02T00:16:02Z

src/experiments.js

+      method: 'GET',
+      ampCors: false,
+      credentials: 'omit',
+    }).then(jwkObj => {


Give this site a shot.

dreamofabear · 2017-05-02T00:18:53Z

src/experiments.js

+ * @param {string} experimentId
+ * @return {!Promise<boolean>}
+ */
+export function isExperimentOnForOriginTrial(win, experimentId) {


Should be up to the caller. May need to be run multiple times in the case where there are multiple AMP documents (e.g. shadow AMPs) but we should cache the result.

dreamofabear · 2017-05-02T00:20:16Z

src/experiments.js

+        throw new Error('Config does not match current origin');;
+      }
+      const experiments = config['experiments'];
+      const experiment = experiments[experimentId];


Ack. Publishers that want to be whitelisted for additional experiments will receive a new token that covers old and new experiments.

dreamofabear · 2017-05-02T00:21:02Z

src/experiments.js

+        return false;
+      }
+      // TODO(kmh287): Transient experiment?
+      toggleExperiment(win, experimentId, /* opt_on */ true);


We shouldn't do this since origin trials have expiry dates. We want the feature to be disabled when the expiry date passes (unless the user has explicitly opted-in).

Doesn't the check on the previous few lines handle that?

Not really since the publisher could just remove the token.

Oh, because the experiment toggle is saved in a cookie by default. Then why not just toggle it as a transient experiment?

googlebot · 2017-05-03T21:28:42Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that they're okay with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

kmh287

Will, I've updated the PR with some tests on tokens I generated. I've got a script I threw together that generates the tokens, but there were a few complications

I had to account for both when generating the tokens.

googlebot · 2017-05-03T21:36:06Z

CLAs look good, thanks!

dreamofabear

Nice! Is this still [WIP]? If not we can remove and ask for review from one of the A4A guys.

dreamofabear · 2017-05-08T15:12:33Z

src/experiments.js

+   */
+  const bytes = stringToBytes(atob(token));
+  const version = bytes[0];
+  if (version === 0) {


Minor: Early out here to reduce brace nesting below.

done, though we'll need to revert if another version is added

dreamofabear · 2017-05-08T15:17:57Z

src/experiments.js

+   /**
+    * Version 0:
+    * length = 4 bytes representing number of bytes in config
+    * config = string containing the experiment ID, origin URL, etc.


Readability suggestion: Use separate vars for each magic number, e.g.

let current = 1; const bytesForConfigSize = 4; // Parse config size. const bytesForConfig = bytesToInt(bytes.subarray(current, bytesForConfigSize)); current += bytesForConfigSize;

dreamofabear · 2017-05-08T15:19:41Z

src/experiments.js


+/**
+ * Whether the specified experiment is on or off for origin trials. You should
+ * check if the experiment is already enabled before calling this function.


Describe when/how the promise is rejected.

done, let me know if you think it's too much detail

dreamofabear · 2017-05-08T15:21:15Z

src/experiments.js

+        throw new Error('Failed to verify config signature');
+      }
+      const configStr = utf8DecodeSync(configBytes);
+      const config = JSON.parse(configStr);


This can throw errors on invalid JSON. Is the default error message good enough? If not, we have tryParseJson.

This should only fail if we generated a bad token, or if the token has been modified. Do you think we should catch the error, add more context, and then reject?

dreamofabear · 2017-05-08T15:22:41Z

src/experiments.js

+      const url = ampdocServiceFor(win).getAmpDoc().getUrl();
+      const sourceOrigin = getSourceOrigin(url);
+      if (approvedOrigin !== sourceOrigin) {
+        throw new Error('Config does not match current origin');;


dreamofabear · 2017-05-08T15:33:00Z

src/utils/bytes.js

+ * Assumes bytes are big endian.
+ * @param {!Uint8Array} bytes
+ * @return {number}
+ */


Please add a unit test for this method.

dreamofabear · 2017-05-08T15:36:21Z

test/functional/test-experiments.js

+        'AAAAAJF7Im9yaWdpbiI6Imh0dHBzOi8vd3d3Lmdvb2dsZS5jb20iLCJleHBlcmltZW50' +
+        'cyI6eyJhbXAtZXhwaXJlcy1sYXRlciI6eyJleHBpcmF0aW9uIjo5NTYxNzYwMjAwMDAw' +
+        'MH0sImFtcC1leHBpcmVkLSI6eyJleHBpcmF0aW9uIjoxMjMyNDI3NjAwMDAwfX19YmFk' +
+        'c2lnbmF0dXJlISEhISEh';


publicJwk and the tokens need to be in beforeEach?

dreamofabear · 2017-05-08T15:37:14Z

test/functional/test-experiments.js

+  beforeEach(() => {
+    win = env.win;
+    ampdoc = env.ampdoc;
+    sandbox = sinon.sandbox.create();


describes.js provides env.sandbox.

dreamofabear · 2017-05-08T15:37:50Z

test/functional/test-experiments.js

+    //   origin: 'https://www.google.com',
+    //   experiments: {
+    //     'amp-expires-later': {expiration: 95617602000000},
+    //     'amp-expired-': {expiration: 1232427600000},


'amp-expired'

done, thanks

dreamofabear · 2017-05-08T15:39:15Z

test/functional/test-experiments.js

+  });
+
+  it('should throw for missing token', () => {
+    if (!crypto.isCryptoAvailable()) { return; }


Extract this to wrap it methods instead? Probably better to not run than appear to pass.

I think this is how the crypto tests are current setup. (not that that's the best solution here) What do you mean by wrap the it methods?

if (crypto.isCryptoAvailable()) { it(...); }

crypto isn't setup until the beforeEach runs. Besides, I don't think this is really an issue as the case where crypto is unavailable is already tested.

dreamofabear · 2017-05-08T15:47:36Z

The crypto service expects a modified signature format.
The crypto service expects the signature to have been generated by signing modified data

Interesting, looks like an optimization. Let's ask A4A -- maybe we can use this too.

kmh287

@choumx Thanks for being so thorough. Accepted most of your comments but responded to a few. When you think it's good to send out to other reviewers, let me know who you recommend and I'll remove the WIP tags.

kmh287 · 2017-05-08T17:28:50Z

src/experiments.js


+/**
+ * Whether the specified experiment is on or off for origin trials. You should
+ * check if the experiment is already enabled before calling this function.


done, let me know if you think it's too much detail

kmh287 · 2017-05-08T17:29:56Z

src/experiments.js

+   */
+  const bytes = stringToBytes(atob(token));
+  const version = bytes[0];
+  if (version === 0) {


done, though we'll need to revert if another version is added

kmh287 · 2017-05-08T17:33:07Z

src/experiments.js

+   /**
+    * Version 0:
+    * length = 4 bytes representing number of bytes in config
+    * config = string containing the experiment ID, origin URL, etc.


kmh287 · 2017-05-08T17:33:33Z

src/experiments.js

+ * @param {!Object} opt_publicJwk Used for testing only.
+ * @return {!Promise<boolean>}
+ */
+export function isExperimentOnForOriginTrial(win, experimentId, opt_publicJwk) {


@choumx Do you think there's a better way to provide the public key for testing besides the optional third param here?

Depends on how the actual public key is stored. OK for now.

I imagine anyone able to modify the public key (and therefore use tokens they signed themselves) could also just do AMP.toggleExperiment, right?

kmh287 · 2017-05-08T17:40:20Z

src/experiments.js

+        throw new Error('Failed to verify config signature');
+      }
+      const configStr = utf8DecodeSync(configBytes);
+      const config = JSON.parse(configStr);


This should only fail if we generated a bad token, or if the token has been modified. Do you think we should catch the error, add more context, and then reject?

kmh287 · 2017-05-08T17:53:03Z

src/utils/bytes.js

+ * Assumes bytes are big endian.
+ * @param {!Uint8Array} bytes
+ * @return {number}
+ */


kmh287 · 2017-05-08T17:53:21Z

test/functional/test-experiments.js

+  beforeEach(() => {
+    win = env.win;
+    ampdoc = env.ampdoc;
+    sandbox = sinon.sandbox.create();


kmh287 · 2017-05-08T17:53:38Z

test/functional/test-experiments.js

+    //   origin: 'https://www.google.com',
+    //   experiments: {
+    //     'amp-expires-later': {expiration: 95617602000000},
+    //     'amp-expired-': {expiration: 1232427600000},


done, thanks

kmh287 · 2017-05-08T17:54:18Z

test/functional/test-experiments.js

+        'AAAAAJF7Im9yaWdpbiI6Imh0dHBzOi8vd3d3Lmdvb2dsZS5jb20iLCJleHBlcmltZW50' +
+        'cyI6eyJhbXAtZXhwaXJlcy1sYXRlciI6eyJleHBpcmF0aW9uIjo5NTYxNzYwMjAwMDAw' +
+        'MH0sImFtcC1leHBpcmVkLSI6eyJleHBpcmF0aW9uIjoxMjMyNDI3NjAwMDAwfX19YmFk' +
+        'c2lnbmF0dXJlISEhISEh';


kmh287 · 2017-05-08T17:55:25Z

test/functional/test-experiments.js

+  });
+
+  it('should throw for missing token', () => {
+    if (!crypto.isCryptoAvailable()) { return; }


I think this is how the crypto tests are current setup. (not that that's the best solution here) What do you mean by wrap the it methods?

dreamofabear

Can you follow-up on that email thread with two questions:

Why the hash in crypto-impl.js and should we use it?
Storage recommendation for private keys?

dreamofabear · 2017-05-09T14:53:51Z

test/functional/test-experiments.js

+  });
+
+  it('should throw for missing token', () => {
+    if (!crypto.isCryptoAvailable()) { return; }


if (crypto.isCryptoAvailable()) { it(...); }

dreamofabear · 2017-05-09T14:58:05Z

src/experiments.js

+  * config = string containing the experiment ID, origin URL, etc.
+  */
+  const bytesForConfigSize = 4;
+  const configLen =


Nit: configSize to be consistent, or change the other var to bytesForConfigLen.

dreamofabear · 2017-05-09T15:02:42Z

src/experiments.js

+    const config = JSON.parse(configStr);
+
+    const approvedOrigin = config['origin'];
+    const url = ampdocServiceFor(win).getAmpDoc().getUrl();


I think this means that a document (e.g. shadow AMP) could enable experiments for the window. We probably should just use win.location as amp-experiments-opt-in does.

dreamofabear · 2017-05-09T15:08:25Z

src/experiments.js

+    const approvedOrigin = config['origin'];
+    const url = ampdocServiceFor(win).getAmpDoc().getUrl();
+    const sourceOrigin = getSourceOrigin(url);
+    if (approvedOrigin !== sourceOrigin) {


Is there an issue with optional trailing slash?

I believe Location.origin always returns without a trailing slash. We'll need to make sure we create and sign the tokens that way.

dreamofabear · 2017-05-09T15:12:06Z

src/experiments.js

+ * @param {!Object} opt_publicJwk Used for testing only.
+ * @return {!Promise<boolean>}
+ */
+export function isExperimentOnForOriginTrial(win, experimentId, opt_publicJwk) {


Depends on how the actual public key is stored. OK for now.

dreamofabear · 2017-05-09T15:13:45Z

test/functional/test-experiments.js

+    if (token) {
+      meta.setAttribute('content', token);
+    }
+    win.document.head.appendChild(meta);


I assume this is automatically torn down after each unit test?

dreamofabear · 2017-05-09T15:35:45Z

src/experiments.js

+    const experiment = experiments[experimentId];
+    if (!experiment) {
+      return false;
+    }


Now I think of it -- for two origin experiments A and B, we probably want to allow publishers to have the flexibility to choose to enable origin trial A && !B on some pages. Chrome also only has one experiment per token.

Then we should probably run this at page load. Otherwise we will need to apply this process to every token on the page. We could do it once lazily, but then we could still have a promise that takes a very long time to resolve.

WDYT?

These should all be pre-evaluated, that way error reporting can log that each experiment is enabled.

dreamofabear · 2017-05-09T15:37:43Z

src/experiments.js

+/**
+ * Determines if the specified experiment is on or off for origin trials.
+ * Callers shouldcheck if the experiment is already enabled before calling this
+ * function.


Since we're just using a transient experiment, we can check this internally. This should avoid parsing a token more than once.

jridgewell · 2017-05-09T20:10:25Z

src/experiments.js

+  * config = string containing the experiment ID, origin URL, etc.
+  */
+  const bytesForConfigSize = 4;
+  const configSize =


All of this byte array stuff is way more complicated than necessary. Why not just use a comma delimited string?

${VERSION = 0},${BASE_64},${SIGNATURE}

@choumx thoughts?

We can decide on the actual delimiter (commas are used to separate Origin-Trial headers), but anything will do. We have the benefit of not requiring super-high-performance can-never-exhaust-memory strings.

That may be a way to go, considering that the version number, base64 encoded experiments object, and signature can't contain commas.

I believe including the length of the config prevents length extension attack.

In that case, we would need to sign the length, too. I still don't see this as a high priority, though.

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

src/experiments.js

+
+    const expiration = experiment['expiration'];
+    const now = Date.now();
+    if (expiration < now) {


We need to ensure expiration is in milliseconds, or we need to normalize now into seconds.

We control the contents of the token, so we can do the former.

jridgewell · 2017-05-09T20:18:19Z

src/experiments.js

+    const experiment = experiments[experimentId];
+    if (!experiment) {
+      return false;
+    }


These should all be pre-evaluated, that way error reporting can log that each experiment is enabled.

kmh287 · 2017-05-10T13:52:39Z

@choumx The hash in crypto-impl is definitely there to bail out of verification early. We don't really have a choice but to include it, unless we want to add similar methods to crypto-impl that don't require it. I'll ask them about recommendations for private key storage.

EDIT: There would be no need to modify cryppto-impl. If we wanted to get around the hash and version number additions to the signature and version number addition to the pre-signed data, we should just use the browser's crypto directly.

dreamofabear · 2017-05-10T15:25:13Z

src/experiments.js

+  * config = string containing the experiment ID, origin URL, etc.
+  */
+  const bytesForConfigSize = 4;
+  const configSize =


I believe including the length of the config prevents length extension attack.

dreamofabear · 2017-05-10T15:27:46Z

src/experiments.js

+    const approvedOrigin = config['origin'];
+    const url = win.location;
+    const sourceOrigin = getSourceOrigin(url);
+    if (approvedOrigin !== sourceOrigin) {


Since approvedOrigin is just from unfiltered, user-submitted form data, I still think we should make this robust enough such that inclusion of optional fragments like www. and trailing slash will still work.

I agree about trailing slash, but some of that should be handled when we validate the forms.

dreamofabear · 2017-05-10T15:28:08Z

src/experiments.js

+    const experiment = experiments[experimentId];
+    if (!experiment) {
+      return false;
+    }


kmh287 · 2017-05-16T17:28:53Z

@choumx @jridgewell PTAL

dreamofabear

Did you speak with @taymonbeal about the upcoming crypto-impl.js API changes and key rotation strategy?

dreamofabear · 2017-05-16T17:40:48Z

src/experiments.js

 let toggles_ = null;

+/** @type {!Promise} */
+const originTrialsPromise = enableExperimentsForOriginTrials(self);


We should avoid competing with startup tasks. The non-startup flavor of chunk may be a good fit here.

How long does it take to verify a single token? That should decide the granularity of the chunk.

Any suggestions on how to get that to work with the async nature of the token parsing? enableExperimentsForOriginTrials needs to be able to return a promise.

I profiled enableExperimentsFromToken and at 10x throttling, Chomr reported about 5ms, though I'm dubious. Does that number sound reasonable to you?

You can create a new Promise, store the resolve function and call it manually when it completes.

At 5ms it might be fine to have the entire thing in a single chunk.

But how can chunk control when the different parts of the task run if the task has a long promise chain?

kmh287 · 2017-05-16T17:56:30Z

I've put something on our calendars for next week. Taymon took a look over this PR and, I'm assuming, didn't see any glaring issues. Reviewing the PR that changes crypto-impl, I can tel I'll need to tweak the token generation process (and, therefore, the test tokens) but the logic here should be unaffected. Of course, I'll test again after that code is merged to be sure.

jridgewell · 2017-05-16T18:54:27Z

src/experiments.js

+  return crypto.importPublicKey('experiments', publicJwk).then(keyInfo => {
+    return crypto.verifySignature(configBytes, signatureBytes, keyInfo);
+  }).then(verified => {
+    if (!verified) {


This needs to be an Error instance, and you can just throw it.

jridgewell · 2017-05-16T18:54:52Z

src/experiments.js

+
+    const approvedOrigin = parseUrl(config['origin']).origin;
+    const sourceOrigin = getSourceOrigin(win.location);
+    if (approvedOrigin !== sourceOrigin) {


jridgewell · 2017-05-16T18:55:21Z

src/experiments.js

+    if (approvedOrigin !== sourceOrigin) {
+      return Promise.reject('Config does not match current origin');
+    }
+


Multiple experiments per token breaks with Chrome's Origin Trials.

can you link or send me info on that?

https://github.com/jpchase/OriginTrials/blob/gh-pages/explainer.md#trial-tokens

Why I think this is a good idea: #8944 (comment)

I don't understand the argument. Any two trials can be turned on independently, accomplishing #8944 (comment).

jridgewell · 2017-05-16T18:57:00Z

src/experiments.js

+  const configBytes = bytes.subarray(current, current + configSize);
+  current += configSize;
+  const signatureBytes = bytes.subarray(current);
+


This may need to be filled in later once we determine the proper way to store the key pair.

dreamofabear · 2017-05-16T22:59:43Z

src/experiments.js

+    user().warn(TAG, 'Crypto is unavailable');
+    return Promise.resolve();
+  }
+  /**


Continuing from: #8944 (comment)

Should be sign(version + length + config, private_key). My mistake on the doc.

kmh287

Changed to one experiment per token and updated the signature verification.

kmh287 · 2017-05-17T15:39:44Z

src/experiments.js

+    user().warn(TAG, 'Crypto is unavailable');
+    return Promise.resolve();
+  }
+  /**


jridgewell · 2017-05-17T17:18:50Z

src/utils/bytes.js

+export function bytesToUInt32(bytes) {
+  if (bytes.length != 4) {
+    throw new Error('Received byte array with length != 4');
+  }


Nit: We can shorten this a bit by forcing 8 bit ints before shifting:

const val = (bytes[0] & 0xFF) << 24 | (bytes[1] & 0xFF) << 16 | (bytes[2] & 0xFF) << 8 | (bytes[3] & 0xFF);

…ivate key were disclosed and we needed to invalidate all tokens

kmh287 · 2017-05-26T21:32:22Z

@choumx

dreamofabear

One change and then let's get this in. Do you have the token generation steps in a shared doc?

dreamofabear · 2017-05-26T21:38:22Z

src/experiments.js

+//TODO(kmh287, #8331) Replaced empty object literal with real experiment public
+// key jwk.
+/** @type {!Promise} */
+const originTrialsPromise = enableExperimentsForOriginTrials(self, {});


Let's comment this out for now pending resolution of the startup latency issue.

kmh287 · 2017-05-26T22:32:57Z

I sent you a js file a few days ago.

kmh287 added the PR use: Work in Progress label Apr 25, 2017

kmh287 commented May 1, 2017

View reviewed changes

dreamofabear reviewed May 2, 2017

View reviewed changes

googlebot added the cla: yes label May 3, 2017

googlebot added cla: no and removed cla: yes labels May 3, 2017

kmh287 commented May 3, 2017

View reviewed changes

kmh287 force-pushed the origin_trials branch from 08b1e81 to 04ef24e Compare May 3, 2017 21:36

googlebot added cla: yes and removed cla: no labels May 3, 2017

dreamofabear reviewed May 8, 2017

View reviewed changes

kmh287 mentioned this pull request May 8, 2017

Weekly Status 2017-05-08 #9198

Closed

kmh287 commented May 8, 2017

View reviewed changes

dreamofabear reviewed May 9, 2017

View reviewed changes

jridgewell reviewed May 9, 2017

View reviewed changes

dreamofabear reviewed May 10, 2017

View reviewed changes

kmh287 force-pushed the origin_trials branch from f229540 to 34b45a3 Compare May 12, 2017 15:27

kmh287 removed the PR use: Work in Progress label May 15, 2017

kmh287 changed the title ~~[WIP] Origin trials~~ Origin trials May 15, 2017

dreamofabear reviewed May 16, 2017

View reviewed changes

jridgewell requested changes May 16, 2017

View reviewed changes

kmh287 force-pushed the origin_trials branch from 34b45a3 to 120bc68 Compare May 16, 2017 20:11

dreamofabear reviewed May 16, 2017

View reviewed changes

kmh287 commented May 17, 2017

View reviewed changes

src/experiments.js

user().warn(TAG, 'Crypto is unavailable');

return Promise.resolve();

}

/**

Copy link
Copy Markdown

Contributor Author

kmh287 May 17, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

jridgewell reviewed May 17, 2017

View reviewed changes

jridgewell approved these changes May 23, 2017

View reviewed changes

kmh287 added 12 commits May 24, 2017 15:08

origin trials

db767fa

fix rebasing issue

2f0244d

pr comments

cfe23f8

cleanup

d3a0aa7

pr comments

e15b70e

pr comments

87995dd

pr comments

c49db36

pr comments

fdcd6cc

pr comments

e94bf29

no need to constantly reimport public key

e161749

lint issues + justin's suggestion

091c284

satisfy compiler

69a4a75

kmh287 force-pushed the origin_trials branch from ac7b64f to 69a4a75 Compare May 24, 2017 19:09

test case where public key is removed and not replaced i.e. if the pr…

604fefc

…ivate key were disclosed and we needed to invalidate all tokens

dreamofabear approved these changes May 26, 2017

View reviewed changes

comment out for now

dc43e44

kmh287 force-pushed the origin_trials branch from 2bf1f51 to dc43e44 Compare May 26, 2017 21:53

kmh287 merged commit 438575b into ampproject:master May 26, 2017

ampprojectbot added the PR use: In Beta / Experimental label Jun 7, 2017

Conversation

kmh287 commented Apr 25, 2017 • edited by dreamofabear Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kmh287 left a comment

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!

dreamofabear left a comment

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!

kmh287 May 2, 2017 • 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!

googlebot commented May 3, 2017

Uh oh!

kmh287 left a comment

Choose a reason for hiding this comment

Uh oh!

googlebot commented May 3, 2017

Uh oh!

dreamofabear left a comment

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!

kmh287 commented Apr 25, 2017 •

edited by dreamofabear

Loading

kmh287 May 2, 2017 •

edited

Loading