Implement client cache of GET urls with fragments by cvializ · Pull Request #7562 · ampproject/amphtml

cvializ · 2017-02-15T01:03:11Z

Implements #7463

cramforce · 2017-02-15T01:29:41Z

src/service/xhr-impl.js

-        .then(response => response.json());
+
+    const isCacheable = (init.method === 'GET' && propertyPath);
+    if (isCacheable) {


I bet we'll add an option to opt out of cache at some point, but this is fine :)

Acknowledged.

cramforce · 2017-02-15T01:32:36Z

src/service/xhr-impl.js

-    return this.fetch(input, init)
-        .then(response => response.json());
+
+    const isCacheable = (init.method === 'GET' && propertyPath);


I'm wondering: Is it a valid use case that you sometimes want the a sub path and sometimes the whole thing?

I'd say it's valid. I'll remove the && propertyPath from the condition.

cramforce · 2017-02-15T01:33:34Z

src/utils/object.js

+ * @template T
+ */
+export function sortProperties(obj) {
+  if (!obj || typeof obj !== 'object') { return obj; }


nit: expand conditional into 3 lines.

Done. I removed this method.

cramforce · 2017-02-15T01:35:41Z

src/utils/object.js

+}
+
+/**
+ * Creates a new object identical to the first with sorted properties.


So you are relying on JS' implicit insertion order is read order semantics?

I think I'd prefer a simpler cache key generated that directly writes a string from input. For our effective use case that should just work.

You are suggesting to just JSON.stringify the opt_init object directly without sorting the properties, is that correct? If so I'll do that and then remove this method.

I might misunderstand your code, but what I'm suggesting it to just generate direct string from the traversal instead of generating an object and then stringifying that.

Done. I made a simpler serializer that should be sufficient for this use case.

cramforce · 2017-02-15T01:36:07Z

test/functional/utils/test-cache.js

+    cache.put('d', {foo: 'bar'});
+
+    expect(toArray(cache, 'abcd'.split(''))).to.deep.equal(
+      ['abc', 123, ['x', 'y'], {foo: 'bar'}]);


nit: indent +2

cramforce · 2017-02-15T01:36:28Z

test/functional/utils/test-cache.js

+    cache.put('c', ['x', 'y']);
+    cache.put('d', {foo: 'bar'});
+
+    expect(toArray(cache, 'abcd'.split(''))).to.deep.equal(


jsonEqual gives much nicer error messages :)

cramforce · 2017-02-15T01:37:55Z

src/service/xhr-impl.js

-    if (init.method == 'POST' && !isFormData(init.body)) {
+    const parsedInput = parseUrl(input);
+    const propertyPath = parsedInput.hash.slice(1); // remove # prefix
+    const getPropertyAtPath = getPath.bind(null, propertyPath);


Please don't bind until you know you need it.

cramforce · 2017-02-15T01:41:10Z

src/utils/object.js

+export function getPath(path, obj) {
+  const arrayIndexRe = /\[(\d+)\]/g;
+  const keys = path.replace(arrayIndexRe, '.$1').split('.');
+  return keys.reduce((acc, key) => acc[key], obj);


CC @choumx who is doing this kind of stuff for amp-bind.

Since JSON returns full objects we need to guard this with hasOwnProperty to avoid exposing stuff like constructor and toString.

Please also think about how to make the error message good if this fails to find an element.

Done. I split it out into a less terse loop with special error messaging.

cramforce · 2017-02-15T01:41:39Z

src/service/xhr-impl.js

+} from '../utils/object';
 import {isArray, isObject, isFormData} from '../types';
 import {utf8EncodeSync} from '../utils/bytes';
+import Cache from '../utils/cache';


@dvoytenko How do you feel about this being directly integrated into XHR?

Moved into CachedXhr class

cramforce · 2017-02-15T01:41:58Z

Awesome! Thank you!

jridgewell · 2017-02-15T01:33:37Z

src/service/xhr-impl.js

+   * @private
+   */
+  getCacheKey_(url, opt_init) {
+    const urlWithoutHash = url.slice(0, url.indexOf('#'));


Danger! url.indexOf can return -1, which'll make this be something like:

const url = 'https://example.com'; const urlWithoutHash = url.slice(0, url.indexOf('#')); // => https://example.co

We have a removeFragment helper to do this.

jridgewell · 2017-02-15T01:35:30Z

src/service/xhr-impl.js

-        .then(response => response.json());
+
+    const isCacheable = (init.method === 'GET' && propertyPath);
+    if (isCacheable) {


I don't think this is the right class for cacheable queries. How about a composed CachedXHR class?

Makes sense, I'll try that out.

jridgewell · 2017-02-15T01:38:18Z

src/service/xhr-impl.js

+    const isCacheable = (init.method === 'GET' && propertyPath);
+    if (isCacheable) {
+      const cacheKey = this.getCacheKey_(input, opt_init);
+      const cachedPromise = this.fragmentCache_.get(cacheKey);


All the fetchX methods suffer from fragments causing repeated requests. With the composed class suggestion, we can solve all of them.

Hmm I'm not sure I understand. The fragment is a path to a property inside a JSON object. How should fetchDocument or fetchText specify a subset of data like the issue specs?

The fragment is a path to a property inside a JSON object.

We've defined that for fetchJSON only.

But fragments can be added to any request, and they're never sent to the server. So, fecthDoc("#frag1") and fecthDoc("#frag2") are the same request, but won't be cached properly. They should be cached, without doing anything special to the response (no deep JSON property returning, etc).

jridgewell · 2017-02-15T01:39:32Z

src/utils/cache.js

+   * @param value {T}
+   */
+  put(key, value) {
+    if (typeof key !== 'string') {


This is guaranteed by Closure types.

Ok cool. Is there a way to test this guarantee, or should I not test it?

No need to test, gulp check-types does it for us.

jridgewell · 2017-02-15T01:43:50Z

src/utils/cache.js

+    return this.map_[key];
+  }
+
+  get length() {


Magic getters are significantly slower than normal property accesses or methods. This is simple enough that we can update #length after every #put, or we could just make it a method.

I'll just remove the length getter, I don't see a good use-case for it given it only increases to the size and then remains constant.

jridgewell · 2017-02-15T01:44:45Z

src/utils/object.js

+export function getPath(path, obj) {
+  const arrayIndexRe = /\[(\d+)\]/g;
+  const keys = path.replace(arrayIndexRe, '.$1').split('.');
+  return keys.reduce((acc, key) => acc[key], obj);


This will throw if acc is falsey!

jridgewell · 2017-02-15T01:46:26Z

src/utils/object.js

+  const keys = Object.keys(obj).sort();
+  return keys.reduce((acc, key) => {
+    const value = obj[key];
+    if (typeof value === 'object') {


Test that it's not an array either.

jridgewell · 2017-02-28T00:49:17Z

src/utils/object.js

+ *
+ * @param {T} obj a map-like value
+ * @param {string} path a dot-separated list of keys to reference a value
+ * @return {T}


This should be *, it's not guaranteed to be an Object (which T has to be).

Right, even if it was, the shape wouldn't be the same

jridgewell · 2017-02-28T00:50:51Z

src/utils/cache.js

+    this.queue_ = [];
+
+    /** @private @const {Object<string, T>} */
+    this.map_ = {};


map, please.

jridgewell · 2017-02-28T00:51:19Z

src/service/xhr-impl.js

+   * @param {string=} opt_accept The HTTP Accept header value.
+   * @return {!FetchInitDef}
+   */
+  setupInit_(opt_init, opt_accept) {


Is there a reason to make this an instance method?

To use in the first line of CachedXhr#fetchJson. I thought it made sense to share it through subclassing rather than exporting, so that it was clear that it isn't a part of Xhr's public API

With d61c81f#r103352978, it won't be necessary. You'll just call Xhr#fetchJson, and cache the result if cacheable.

jridgewell · 2017-02-28T00:52:53Z

src/service/xhr-impl.js

@@ -191,8 +193,8 @@ export class Xhr {
      init.headers['Content-Type'] = 'application/json;charset=utf-8';


Somehow missed this before. Would you mind deleting this line, and updating the 'application/json' above to be 'application/json;charset=utf-8'?

There's a test checking that this property is present in the request headers, and the second argument to setupInit sets the Accepts header, not Content-Type.
I can change the test to succeed, but I just want to make sure this is what you really intended.

Whoops, I thought the second param was for Content-Type. Thanks for looking into it.

Cool, no problem

jridgewell · 2017-02-28T00:54:02Z

src/service/xhr-impl.js

-    const init = setupInit(opt_init, 'application/json');
-    if (init.method == 'POST' && !isFormData(init.body)) {
+    const init = this.setupInit_(opt_init, 'application/json');
+    const getResponseJson = response => response.json();


Nit: if we're going to pull this into a variable, hoist it out of the function.

jridgewell · 2017-02-28T00:55:03Z

src/service/cached-xhr-impl.js

+ *   ampCors: (boolean|undefined)
+ * }}
+ */
+let FetchInitDef;


This should be exported from xhr-impl.js, then you can reference the type in this file.

jridgewell · 2017-02-28T00:59:00Z

src/service/cached-xhr-impl.js

+   * @return {!Promise<!JSONType>}
+   * @override
+   */
+  fetchJson(input, opt_init) {


There's a lot of duplication with Xhr#fetchJson here. I think we can simplify down based on:

Any call with opt_init is un-cacheable

Removes crazy simpleSerialize

Handles POST case

Actually, that's it. 😉

Simple enough.

jridgewell · 2017-02-28T01:00:01Z

src/service/cached-xhr-impl.js

+      // Since a fragment is present, cache the full response promise, then
+      // return a promise with just the value specified by the fragment.
+      this.fragmentCache_.put(cacheKey, fetchPromise);
+      return fetchPromise.then(getPropertyAtPath);


Is there any reason to put this functionality in CachedXhr? Seems like CachedXhr is generally useful, while deep JSON gets are only useful to <amp-list>?

I belive the intended use-case for the fragment is e-commerce JSON APIs that return a big user-object with nested data that are displayed on many different parts of the page. For example, a user object with a name that is displayed at the top and bottom, a shopping cart list, etc. and each piece of the UI that references a deep property of the user-object would specify which property through the fragment. Does that answer your question?

Nope. I understand the use case, but I don't think the functionality belongs here. You could just as easily call getPath like so:

// amp-list.js cachedXhr.fetchJson(endpoint).then(json => getPath(fragment, json))

That keeps CachedXhr generically useful, while giving <amp-list> the special deep JSON gets.

It's true you could do that, and that would follow the single responsibility principle. But you'd be duplicating that fragment extraction and getPath logic in any component or extension where you'd want to get a sub-object of the overall response, which I think would be 3 or more places, more than just amp-list. IMO it keeps things DRYer if that logic is implicitly triggered in CachedXhr by the presence of a fragment. It also enables any AMP element to access a fragment without explicitly coding it into every fetchJson call site, you'd just swap in the CachedXhr class. If you think the separation will be more beneficial than the deduplication, I'll split it out.

The duplication can be handled with a helper, and code size changes should be negligible. For my rationale, I need this to be useable by amp-call-tracking.js and amp-ad-custom.js, both which do not expect this behavior.

That makes sense. Thank you for explaining your reasoning, I'll do it this way.

cramforce · 2017-02-28T02:16:31Z

src/utils/object.js

+ *
+ * @param {T} obj a map-like value
+ * @param {string} path a dot-separated list of keys to reference a value
+ * @return {T}


Right, even if it was, the shape wouldn't be the same

cramforce · 2017-02-28T02:17:10Z

src/service/cached-xhr-impl.js

+
+
+/**
+ * A service that polyfills Fetch API for use within AMP.


jridgewell · 2017-03-02T01:17:14Z

src/url.js

+  if (index == -1) {
+    return '';
+  }
+  return url.substring(index + 1);


Nit: location.hash returns the #. Why don't we do so as well?

I removed the # here for convenience, but keeping it does follow the POLS

jridgewell · 2017-03-02T01:18:01Z

src/utils/object.js

+ * @return {*}
+ * @template T
+ */
+export function getPath(path, obj) {


Nit: Can we flip the params?

Sure. I had path first for to make currying with bind easy to use to map on an array of objects, but I'm not doing that anywhere.

jridgewell · 2017-03-02T01:20:04Z

src/service/cached-xhr-impl.js

+   * @return {!Promise<!JSONType>}
+   * @override
+   */
+  fetchJson_(input, init) {


Why not just override the public methods?

fetchJSON(input, init) { return this.cached(input) || this.cacheFetch_(input, super.fetchJSON(input, init)); }

The public methods accept an optional opt_init value, while the private methods have init guaranteed to be defined by setupInit in the corresponding public method. This lets the cache separately key the edge case where identical URLs serve different documents for different Accepts header values. I could override the public methods, but then I think I'd need to expose setupInit to the subclass. I'll change it if you prefer it implemented that way.

for different Accepts header values

Don't we know that by virtue of the public method being called? Ie, we can do:

fetchJSON(input, init) { return this.cached(input, 'json') || this.cacheFetch_(input, 'json', super.fetchJSON(input, init)); } fetch(input, init) { return this.cached(input, 'generic') || this.cacheFetch_(input, 'generic', super.fetchJSON(input, init)); } fetchText(input, init) { return this.cached(input, 'text') || this.cacheFetch_(input, 'text', super.fetchJSON(input, init)); }

#getCacheKey can then just concat the URL and the method to generate.

Oh true, thanks I didn't see that

jridgewell · 2017-03-02T21:24:40Z

src/utils/cache.js

+export class Cache {
+  /**
+   * Construct a new Cache with the given size
+   * @param size {number=}


The type definitions in this file are backwards. Should be @param {number=} size

jridgewell · 2017-03-02T21:25:17Z

src/utils/object.js

+ * e.g.
+ * `getPath({a: {b: [{c: 2}]}}, 'a.b[0].c') === 2`
+ *
+ * @param {T} obj a map-like value


Can you help me understand why is this different than hasOwn, which returns a boolean but specifies a @param {T} obj?

I actually don't have a good answer. @erwinmombay?

jridgewell · 2017-03-02T21:25:57Z

test/functional/test-cached-xhr.js

+        xhr.fetchJson('/get?k=v1'),
+        xhr.fetchJson('/get?k=v1'),
+      ]).then(results => {
+        expect(fetchStub.calledOnce).to.be.true;


expect(fetchStub).to.be.calledOnce;

jridgewell · 2017-03-02T21:27:13Z

test/functional/test-cached-xhr.js

+import {installCachedXhrService} from '../../src/service/cached-xhr-impl';
+
+
+describes.realWin('CachedXhr', {}, env => {


Test that POSTs aren't cached.

jridgewell · 2017-03-02T21:27:44Z

test/functional/test-cached-xhr.js

+import {installCachedXhrService} from '../../src/service/cached-xhr-impl';
+
+
+describes.realWin('CachedXhr', {}, env => {


Test that fragments do not affect caching.

jridgewell · 2017-03-04T00:35:56Z

test/functional/test-xhr.js

+      return response.text()
+          .then(result => {
+            const callCloneText = () => clone.text().then(clonedText => {
+              expect(clonedText).to.equal(TEST_TEXT);


This expectation will not be validated, since the test will have exited before this is run.

jridgewell · 2017-03-04T00:37:24Z

test/functional/test-xhr.js

+            const callCloneText = () => clone.text().then(clonedText => {
+              expect(clonedText).to.equal(TEST_TEXT);
+            });
+            expect(callCloneText, 'should not throw').to.not.throw(Error);


This wrapping isn't necessary. If the clone's #text were to throw syncly, it would cause the tests to fail (because the promise block it's inside is waited on by the test).

Thanks, makes sense.

jridgewell · 2017-03-04T00:38:03Z

test/functional/test-xhr.js

+              expect(clonedText).to.equal(TEST_TEXT);
+            });
+            expect(callCloneText, 'should not throw').to.not.throw(Error);
+            expect(result).to.equal(TEST_TEXT);


Return the clone's #text promise here, with its expectation in a then block.

cramforce · 2017-03-10T20:08:24Z

src/runtime.js

  installTimerService(global);
  installVsyncService(global);
  installXhrService(global);
+  installCachedXhrService(global);


Could you run gulp size and see the impact on the core payload?

Certainly, here are the results.

max min gzip

master 1.11 MB 245.71 kB amp.js

PR #7562 1.11 MB 246.56 kB amp.js

You're gzip is a little large. 😜

Haha just copying what I see in my terminal:

I think something might be broken with that script

Run gulp dist

Ah that makes sense, I had just run build. What do you think of adding dist as a gulp task dependency for the size task? Are there any cases where we want to run size on files produced by a task other than dist?

Here are the correct numbers:

max min gzip file

master 1.11 MB 206.7 kB 61.94 kB v0.js / amp.js

PR #7562 1.11 MB 207.05 kB 62.11 kB v0.js / amp.js

Ping @erwinmombay.

cramforce · 2017-03-13T18:05:13Z

Sorry, for opening pandoras box. I think we should reconsider the implementation strategy, but would like to get input from others: Delete the app layer cache from this CL. We have to document how to do the fragment stuff anyway, so we might as well document people should do something like set a 1 minute max-age on the response. We just need to test whether Safari does the right thing in that case. They sometimes do redundant fetching of concurrent requests to same resource.

ampprojectbot · 2017-03-16T23:17:49Z

Hi, ampproject bot here! Here are a list of the owners that can approve your files.

You may leave an issue comment stating "@ampprojectbot retry!" to force me to re-evaluate this Pull Request's status

/to cramforce dvoytenko jridgewell

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

/to dvoytenko jridgewell

src/service/xhr-impl.js
src/url.js
src/utils/object.js

/to alanorozco camelburrito chenshay choumx cvializ ericlindley-g erwinmombay gregable honeybadgerdontcare jridgewell kmh287 lannka mkhatib mrjoro muxin newmuis powdercloud zhouyx

test/functional/test-object.js
test/functional/test-xhr.js

For any issues please file a bug at https://github.com/google/github-owners-bot/issues

cvializ · 2017-03-17T18:06:13Z

PTAL, removed app layer cache as requested by @cramforce and implemented fetch-batching per @jridgewell's suggestion

cramforce

Looks great, but have one question?

cramforce · 2017-03-17T19:37:43Z

extensions/amp-list/0.1/amp-list.js

          }
-          return xhrFor(this.win).fetchJson(src, opts);
+          const fetchPromise = batchedXhrFor(this.win).fetchJson(src, opts);
+          const fragment = getFragment(src).slice(1);


Should this be a feature of BatchedXhr?

Justin and I discussed keeping the fragment-extraction and object property access behavior inside of the new Xhr class, but we decided to keep them separate. Discussion here.

cramforce

LGTM

Important follow up is to support the path based stuff in more places and to document it.

cvializ · 2017-03-17T20:43:55Z

Wahoo! I created an issue to track the documentation effort. I don't have merge permission yet, so feel free to merge any time.

jridgewell · 2017-03-21T19:01:49Z

src/service/batched-xhr-impl.js

+    if (isBatchable) {
+      this.fetchPromises_[key] = fetchPromise.then(response => {
+        delete this.fetchPromises_[key];
+        return response.clone();


Bug: This isn't sufficient to prevent double draining. The problem is this is part of the promise chain.
So, first call to fetch returns the clone. I then add to the chain to read the body, and issue a second request to the same endpoint. The second request will see the batched call, and return a clone of that promise (a clone of a clone) Problem is, that clone will happen after the body read. (Because execution chains on a single promise is defined)

This should return the response without cloning. This total promise chain will be stored as the batch. For the return value, we should return the batching promise with a .then((resp) => resp.clone()) added to its chain.

After thinking about it, I'm not sure your assessment is correct. The clone is created, but not returned to the caller, just to the batching mechanism. Your argument assumes that the first fetch returns the clone, which is not what is happening in this code. So nothing can drain the body of the response, so there cannot be a double drain.

Perfect, missed that.

dvoytenko · 2017-03-27T23:46:36Z

src/utils/object.js

+ * @param {string} path a dot-separated list of keys to reference a value
+ * @return {*}
+ */
+export function getPath(obj, path) {


@cvializ Is this method significantly different from https://github.com/ampproject/amphtml/blob/master/src/json.js#L84 ?

If no, please file a bug to reconcile with P1.

Ah! I did not see that. I'll file the bug

…mpproject#7562)

cvializ requested review from aghassemi, cramforce and jridgewell February 15, 2017 01:03

cramforce requested changes Feb 15, 2017

View reviewed changes

jridgewell requested changes Feb 15, 2017

View reviewed changes

cvializ mentioned this pull request Feb 27, 2017

Weekly Status 2017-02-27 #7813

Closed

cvializ force-pushed the cv-fragment-cache branch 2 times, most recently from f45fda1 to d61c81f Compare February 28, 2017 00:59

jridgewell requested changes Feb 28, 2017

View reviewed changes

cramforce reviewed Feb 28, 2017

View reviewed changes

jridgewell reviewed Mar 2, 2017

View reviewed changes

jridgewell approved these changes Mar 3, 2017

View reviewed changes

cvializ force-pushed the cv-fragment-cache branch from b5d4f06 to c2fca22 Compare March 3, 2017 19:48

jridgewell requested changes Mar 4, 2017

View reviewed changes

jridgewell approved these changes Mar 4, 2017

View reviewed changes

cvializ mentioned this pull request Mar 9, 2017

Weekly Status 2017-03-06 #7995

Closed

cramforce reviewed Mar 10, 2017

View reviewed changes

cvializ mentioned this pull request Mar 10, 2017

Make the gulp size task run after gulp dist. #8066

Closed

Carlos Vializ added 10 commits March 13, 2017 10:22

Implement client cache of GET urls with fragments

a46000a

Extract cache behavior into CachedXhr

d97a2ba

Make CachedXhr more generic. Move fragment logic out to callers.

21a0974

Formatting

2af71e3

Feedback on object.getPath and url.removeFragment

d79fe43

Update amp-list tests to use mocked CachedXhrService.

ba62c37

Simplify the CachedXhr implementation

c09dfd2

Fix jsdoc syntax

ba62b6c

Use describes API and check POST and fragment for CachedXhr tests.

39bc6eb

Implement FetchResponse#clone and override the generic fetch instead.

97e33c9

Carlos Vializ added 3 commits March 13, 2017 10:56

Use describes for Cache tests.

510aac2

Add template type to CachedXhr's backing cache.

737a8d1

Fix badly written Xhr test

d0a25ea

cvializ force-pushed the cv-fragment-cache branch from 73d815c to d0a25ea Compare March 13, 2017 17:57

cramforce mentioned this pull request Mar 13, 2017

amp-list: customize array location in the response #6745

Merged

Remove CachedXhr.

e47c3f1

cvializ force-pushed the cv-fragment-cache branch from d312438 to e47c3f1 Compare March 16, 2017 23:09

Remove app-layer cache

5a8d003

Replace caching with batching

95ab799

cvializ force-pushed the cv-fragment-cache branch from 223e29a to 95ab799 Compare March 17, 2017 18:10

cramforce reviewed Mar 17, 2017

View reviewed changes

cramforce approved these changes Mar 17, 2017

View reviewed changes

cvializ mentioned this pull request Mar 17, 2017

Document fetch batching and fragment-specified object properties #8228

Closed

cramforce merged commit cdf1f1c into ampproject:master Mar 17, 2017

cvializ deleted the cv-fragment-cache branch March 17, 2017 20:45

This was referenced Mar 20, 2017

Weekly Status 2017-03-20 #8253

Closed

Use getService instead of getExistingServiceForWindow for batched-xhr #8288

Merged

jridgewell reviewed Mar 21, 2017

View reviewed changes

cvializ mentioned this pull request Mar 21, 2017

Fix double drain bug #8297

Closed

dvoytenko reviewed Mar 27, 2017

View reviewed changes

cvializ mentioned this pull request Mar 28, 2017

Replace src/utils/object#getPath with src/json#getValueForExpr #8463

Closed

mrjoro pushed a commit to mrjoro/amphtml that referenced this pull request Apr 28, 2017

Implement batching of GET requests and fragment addressing of JSON (a…

344eea3

…mpproject#7562)

		@@ -191,8 +193,8 @@ export class Xhr {
		init.headers['Content-Type'] = 'application/json;charset=utf-8';

		import {installCachedXhrService} from '../../src/service/cached-xhr-impl';


		describes.realWin('CachedXhr', {}, env => {

	max	min	gzip
master	1.11 MB	245.71 kB	amp.js
PR #7562	1.11 MB	246.56 kB	amp.js

	max	min	gzip	file
master	1.11 MB	206.7 kB	61.94 kB	v0.js / amp.js
PR #7562	1.11 MB	207.05 kB	62.11 kB	v0.js / amp.js

Conversation

cvializ commented Feb 15, 2017

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!

cramforce commented Feb 15, 2017

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