V3 background sync #871

philipwalton · 2017-10-10T21:31:27Z

Changes described in #868.

jeffposnick

A lot of the changes are around consuming bodies and needing to clone Requests. See https://developer.mozilla.org/en-US/docs/Web/API/Request/clone

jeffposnick · 2017-10-10T21:56:56Z

infra/testing/sw-env.js

+
+    this.url = url;
+    this.method = options.method || 'GET';
+    this.mode = options.mode || 'same-origin'; // FF defaults to cors


It depends on the context, but for just a random new Request() that you construct from code run in a window client, it looks like Chrome will default to CORS, too:

r = new Request('.'); console.log(r.mode) cors

Maybe just switch the default to 'cors'?

Sure, I'll do that.

jeffposnick · 2017-10-10T22:03:33Z

infra/testing/sw-env.js

+    return new Request(this.url, this);
+  }
+
+  text() {


The consume body algorithm is relevant here.

I'd expect this code to check bodyUsed:

If it's true, then reject with TypeError.

If it's false, then set it to true and resolve with body.

jeffposnick · 2017-10-10T22:04:10Z

infra/testing/sw-env.js

+    }
+  }
+
+  clone() {


As per the spec, and similar to the comment about the text() method, this should take bodyUsed into account.

jeffposnick · 2017-10-10T22:04:47Z

infra/testing/sw-env.js

+    this.mode = options.mode || 'same-origin'; // FF defaults to cors
+    this.headers = new Headers(options.headers);
+
+    this.bodyUsed = !!options.body;


I think you're misinterpreting bodyUsed. See my comments on clone() and text().

jeffposnick · 2017-10-10T22:13:05Z

packages/workbox-background-sync/lib/Queue.mjs

+ * later. All parts of the storing and replaying process are observable via
+ * callbacks.
+ *
+ * @example


I've been taking out inline @examples from the v3 JSDocs. I think the idea was to go with a model of having runnable examples on Glitch/etc. so there's one thing to keep up to date.

jeffposnick · 2017-10-10T22:48:17Z

packages/workbox-background-sync/lib/StorableRequest.mjs

+
+const serializableProperties = [
+  'method',
+  'body',


I don't think body should be in this list. (See various other comments.)

jeffposnick · 2017-10-10T22:48:44Z

packages/workbox-background-sync/lib/StorableRequest.mjs

+ limitations under the License.
+*/
+
+const serializableProperties = [


I'm assuming that this list comes from https://fetch.spec.whatwg.org/#request

Have you confirmed that all those properties actually can be set when calling new Request()?

And something like signal seems like it would be difficult to correctly (de)serialize.

I haven't confirmed, but I only include these in the requestInit dict if they're present.

But, either way, yeah, it probably doesn't make sense to include properties that are only handled by the UA. I'll look into this further.

jeffposnick · 2017-10-10T22:57:42Z

packages/workbox-background-sync/lib/StorableRequest.mjs

+   * new Request object. A timestamp is also set in the event that when this
+   * object was created is relevant after it's stored.
+   *
+   * @param {StorableRequest|Object} param1


Use options instead of param1.

jeffposnick · 2017-10-10T23:01:08Z

packages/workbox-background-sync/lib/StorableRequest.mjs

+   * new Request object. A timestamp is also set in the event that when this
+   * object was created is relevant after it's stored.
+   *
+   * @param {StorableRequest|Object} param1


I don't follow this JSDoc—what's {StorableRequest|Object}?

It means it could be of either type. In this case a StorableRequest object could be created from a plain object or "cloned" from another StorableRequest instance.

However, looking at the code, I think I only create this from plain objects, so I can remove.

jeffposnick · 2017-10-10T23:03:19Z

packages/workbox-background-sync/lib/StorableRequest.mjs

+   *     defaulting to the current time if not specified.
+   */
+  constructor({url, requestInit, timestamp = Date.now()}) {
+    this.url = url;


Matt's been pretty keen on prefixing all instance variable names with _ as that apparently helps with our uglification.

👍 Unless you expect people to use instance.url but even then, in some situations getters and setters can be smaller when using the private variable lots inside the class.

This needs to be unprefixed as it's public and can/must be editable by the user. E.g. workbox-google-analytics will need to modify these values before requests are replayed.

gauntface · 2017-10-11T12:05:07Z

infra/testing/sw-env.js

+const mockFetch = require('./mock-fetch');
+
+Object.assign(global, makeServiceWorkerEnv());
+global.self = global;


Why are you assigning global to global.self? makeServiceWorkerEnv has a self property which you'll now we overriding with everything node has.

This was necessary to handle the mocks we're adding later. I could try doing the global assignment after adding the mocks to see if that works. I remember there being an issue that this solve, but I can't remember all the various things I tried.

gauntface · 2017-10-11T12:05:31Z

infra/testing/sw-env.js

-const swEnv = makeServiceWorkerEnv();
+// Stub missing/broken Headers API methods in `service-worker-mock`.
+// https://fetch.spec.whatwg.org/#headers-class
+class Headers {


Please move all the mocks into a seperate file to clean up this file.

BTW, the end goal was to PR this back into service-worker-mock.

I can move it, but regardless this was always supposed to be a temporary solution.

gauntface · 2017-10-11T12:08:36Z

infra/testing/sw-env.js

+  }
+};
+
+global.__removeAllEventListeners = () => {


nit: rename to __resetAllEventListeners. Would also be good if you move this to a seperate file and rather than expose this method on global, expose a function from the new file can all tests to call it (This is what the indexedDB module does - feels a little cleaner).

Agreed, though similar to my above comment, I was going to feature request this for the original library. We can discuss whether it makes sense to spend time on this now or spend time on a PR.

gauntface · 2017-10-11T12:09:12Z

infra/testing/sw-env.js


+// Needed to look like a real SW env


Just an FYI - when you rebase, these next few lines will be removed (due to fix in service-worker-mock.

Cool, rebased.

gauntface · 2017-10-11T12:10:34Z

packages/workbox-background-sync/lib/Queue.mjs

+
+
+const {indexedDBHelper, WorkboxError} = _private;
+const names = new Set();


This feels like an unusual pattern - I think you are doing it to ensure a common set of names across all instances of Queue. Would be good to add a comment

What do you mean? Using a set rather than an array?

gauntface · 2017-10-11T12:13:12Z

packages/workbox-background-sync/lib/Queue.mjs

+   * @param {Object} [param2]
+   * @param {number} [param2.maxRetentionTime = 7 days] The amount of time (in
+   *     ms) a request may be retried. After this amount of time has passed,
+   *     the request will be deleted and not retried.


Nit .: reword to "deleted from the queue." insteand of "deleted and not retried."

gauntface

Looks good. I think the code could do with breaking up to simplify some of the flows of logic.

Separating out the QueueModel from the Sync logic should make it a bit easier to follow when / how sync events are / should be registered. Looking at this I'm unsure if this supports non-background-sync browsers.

gauntface · 2017-10-11T12:13:49Z

packages/workbox-background-sync/lib/Queue.mjs

+   * @param {number} [param2.maxRetentionTime = 7 days] The amount of time (in
+   *     ms) a request may be retried. After this amount of time has passed,
+   *     the request will be deleted and not retried.
+   * @param {Object} [param2.callbacks] Callbacks to observe the lifecycle of


Rename to plugins. That's the term used in the module names and in other parts of the API.

gauntface · 2017-10-11T12:19:07Z

packages/workbox-background-sync/lib/Queue.mjs

+   *
+   * @private
+   */
+  _addSyncListener() {


We agreed in the previous team meeting that event listeners would be registered in the default export (i.e. the export that "does the right thing"). This is largely to ensure the logic is seperate from "magic".

Is there anyway of achieving that same line here?

See precaching and routing modules for examples

I'm not sure I understand. Can you give more context?

Looking at those examples, they both expose a single instance of a class, whereas background sync exposes the class itself, and it needs a unique listener per instance.

https://github.com/GoogleChrome/workbox/blob/v3/packages/workbox-routing/default-export.mjs#L22 is an example of what I did for the Router class.

I had trouble wrapping my head around it as well, but once you get a feel for how the default exports are likely be used in the context of workbox-sw, it makes more sense.

I still don't think this applies to the background sync case since I don't instantiate anything in the default export.

That should be okay by me, let's see if @gauntface has any concerns. He's given this setup a lot more thought that I have.

I still have concerns but this module is also too big to cover off everything. I'll play around with it and see if it makes to refactor after.

Ok, I'd be happy to look at a suggestion for how it could be refactored, but since I don't understand what you're asking me to do, I can't really do it myself.

gauntface · 2017-10-11T12:20:36Z

packages/workbox-background-sync/lib/Queue.mjs

+   *
+   * @return {Object}
+   */
+  createPlugin() {


What is the use case for this function?

The RequestWrapper is non-existant now and even then, I'm struggling to see how this would be used.

I added this because it was in the original API design. Here's the original code example (modified to use createPlugin()):

const queue = new workbox.backgroundSync.Queue('myQueue'); const requestWrapper = new workbox.runtimeCaching.RequestWrapper({ plugins: [queue.createPlugin()], }); const route = new workbox.routing.RegExpRoute({ regExp: new RegExp('^https://jsonplaceholder.typicode.com'), handler: new workbox.runtimeCaching.NetworkOnly({requestWrapper}), }); const router = new workbox.routing.Router(); router.registerRoute({route});

I'd drop this in favor of exposing a plugin class like the other plugins. While I understand the convenience of this, nothing else follows this pattern.

I don't like the v2 way (where the plugin class just extends the Queue class), but if you want it to be a class instead of an object, it could be like this:

class QueuePlugin { constructor(queue) { this._queue = queue } fetchDidFail({request}) { this._queue.addRequest(request); } }

And you'd use it like this:

new workbox.runtimeCaching.NetworkOnly({ plugins: new QueuePlugin(queue), });

vs. how I have it currently:

new workbox.runtimeCaching.NetworkOnly({ plugins: queue.createPlugin(), });

Alternatively, we could just do away with this feature entirely, since it's really not that hard to do this:

new workbox.runtimeCaching.NetworkOnly({ plugins: { fetchDidFail: ({request}) { queue.addRequest(request); } } });

WDYT?

I'd vote for the first option.

If we go with explicitly needing to know the lifecycle callback names to use this behavior, then the precedent would be to use that approach with all the other plugins as well.

That isn't a great user experience for developers who care about specifying behaviors, but don't care about the underlying lifecycle callbacks. It's also an issue for the other behaviors that require multiple callbacks (I think cache expiration does?), since it's even harder to properly configure that.

SGTM. Done.

gauntface · 2017-10-11T12:21:41Z

packages/workbox-background-sync/lib/Queue.mjs

+   * @param {...*} args The arguments to invoke the callback with.
+   */
+  _runCallback(name, ...args) {
+    if (typeof this._callbacks[name] == 'function') {


triple equals please. We aren't in closure.

Done. But what about the bytes Matt! The BYTES!

gauntface · 2017-10-11T12:23:50Z

packages/workbox-background-sync/lib/Queue.mjs

+    try {
+      await this._waitUntilActive();
+      await registration.sync.register(`${TAG_PREFIX}:${this._name}`);
+    } catch (err) {


If it does fail, I'd still log an error or warning in dev builds.

gauntface · 2017-10-11T13:24:12Z

test/workbox-background-sync/node/lib/test-Queue.mjs

+    it(`should try to replay the queue on SW startup in browsers that ` +
+        `don't support the sync event`, async function() {
+      // Delete the SyncManager interface to mock a non-supporting browser.
+      const originalSyncManager = registration.sync;


you might be able to do sanbox.stub(registration, 'sync').value(undefined) and that you don't need to track original or perform delete

No, I tried that as well as .value() (leaving it empty) and neither worked. Probably a good feature request for sinon.

gauntface · 2017-10-11T13:25:08Z

test/workbox-background-sync/node/lib/test-Queue.mjs

+
+      expect(Queue.prototype.replayRequests.calledOnce).to.be.true;
+
+      registration.sync = originalSyncManager;


If the expect fails and throws, the original will never be set. Might be best to store the original in the describe and re-set in beforeEach and after (if the sinon approach doesn't work)

If the tests fail, do we care if state gets messed up? I could also do something like this:

try { expect(Queue.prototype.replayRequests.calledOnce).to.be.true; } catch (err) { throw err; } finally { registration.sync = originalSyncManager; }

gauntface · 2017-10-11T13:30:21Z

test/workbox-background-sync/node/lib/test-Queue.mjs

+  });
+
+  describe(`createPlugin`, function() {
+    it(`should return an object implementing the fetchDidFail plugin ` +


This method still feels weird to me. May be naming, but I'm struggling to understand what it's for.

gauntface · 2017-10-11T13:33:13Z

test/workbox-background-sync/node/lib/test-Queue.mjs

+      await queue.addRequest(request);
+
+      // Allow time to ensure async registration didn't happen.
+      await sleep(100);


This feels flakey and unnecessary. This doesn't seem to affect the flow at all of this test. If you do need timeouts please use sinon fake timers.

gauntface · 2017-10-11T13:36:22Z

test/workbox-background-sync/node/lib/test-Queue.mjs

+    it(`should ignore (and remove) requests if maxRetentionTime has passed`,
+        async function() {
+      sandbox.spy(self, 'fetch');
+      const clock = sinon.useFakeTimers({


Can you get this from the sandbox? If so you can drop the restore call at the end of this test.

Wasn't in the documentation, but yep, appears to work. Done.

philipwalton

@gauntface, @jeffposnick PTAL.

Changes since your last reviews:

Per Matt's suggestion: added a QueueStore module to break up Queue logic.
Per Matt's suggestion: removed the requestDidFail and requestDidReplay callbacks and changed allRequestsDidReplay to queueDidReply but now it runs anytime the queue is retried (previously it was only on success)
Added support for Blobs in requests
Removed the use of indexedDBHelper since I decided to use an index instead of getAll().
Per Matt's suggestion: moved sw-env mocks into their own files
Per Matt's suggestion: removed the global __removeAllEventListeners in favor of exporting a resetEventListeners function from the event listener mock.

Note: I'll update #868 with changes after we agree on changes and merge.

jeffposnick · 2017-10-14T21:28:16Z

packages/workbox-background-sync/lib/StorableRequest.mjs

+    const requestInit = {headers: {}};
+
+    // Set the body if present.
+    if (request.method != 'GET') {


Could you switch here and elsewhere to !==? (Shouldn't our linting pick that up?)

I believe the linting only warns against double-equals comparison with falsy values (as that's the only time it gets tricky).

I can change here, but I think we should add this to our style guide if we're going to enforce it, as it's not part of Google JS style.

jeffposnick · 2017-10-14T21:30:45Z

packages/workbox-background-sync/lib/Queue.mjs

+   *
+   * @private
+   */
+  _addSyncListener() {


https://github.com/GoogleChrome/workbox/blob/v3/packages/workbox-routing/default-export.mjs#L22 is an example of what I did for the Router class.

I had trouble wrapping my head around it as well, but once you get a feel for how the default exports are likely be used in the context of workbox-sw, it makes more sense.

gauntface · 2017-10-16T21:58:36Z

infra/testing/sw-env.js

-global.fetch = mockFetch;
+// Assign all properties of `self` to `global`;
+Object.assign(global, serviceWorkerMock());
+global.self = global;


I'm still confused at why this self is needed. serviceWorkerMock creates a self and this will overwrite that.

It's not necessarily needed, but if we don't do this then all the code below would need to be like this:

global.Blob = global.self.Blob = Blob; global.Event = global.self.Event = Event; // ...

So I guess it's a matter of which we prefer. I think what I have is fine since it's actually the same way browsers implement it, i.e. self (and window) is a property of the global object that points to the global object. For example: window.window === window, window.self === window, self.self === self, etc.

gauntface · 2017-10-16T22:02:24Z

packages/workbox-background-sync/lib/Queue.mjs

+   *     in IndexedDB specific to this instance. An error will be thrown if
+   *     a duplicate name is detected.
+   * @param {Object} [param2]
+   * @param {Object} [param2.callbacks] Callbacks to observe the lifecycle of


Is it fair to say that these callbacks are the same as plugins? If so can we rename this to plugins rather than callback for consistency throughout workbox.

I don't think they're the same. As far as I'm aware, a plugin in an object that implements lifecycle callbacks, and this doesn't do that.

I've debated with myself as to whether or not these should be events and the Queue class inherit from something like EventEmitter, but I didn't want to do that in this PR to keep things as close to the original as possible.

gauntface · 2017-10-16T22:03:18Z

packages/workbox-background-sync/lib/Queue.mjs

+   * @param {Object} [param2.callbacks] Callbacks to observe the lifecycle of
+   *     queued requests. Use these to respond to or modify the requests
+   *     during the replay process.
+   * @param {function(StorableRequest):undefined}


What is the syntax for :undefined? The parameter is marked as optional given the square brackets.

This means the function returns undefined. The syntax is described here: https://github.com/google/closure-compiler/wiki/Types-in-the-Closure-Type-System

But that brings up a good point that this is closure compiler jsdoc notation, and I'm not sure if what we use to generate docs will support this. I don't think non-CC-jsdoc syntax has a way to specify what params a function takes, what its this binding is, or what it returns (which is too bad, IMO).

gauntface · 2017-10-16T22:05:51Z

packages/workbox-background-sync/lib/Queue.mjs

+   *
+   * @return {Object}
+   */
+  createPlugin() {


I'd drop this in favor of exposing a plugin class like the other plugins. While I understand the convenience of this, nothing else follows this pattern.

gauntface · 2017-10-16T22:08:46Z

packages/workbox-background-sync/lib/Queue.mjs

+   *
+   * @private
+   * @param {string} name The name of the callback on this._callbacks.
+   * @param {...*} args The arguments to invoke the callback with.


Can you confirm this works in JSDocs with gulp docs

gauntface · 2017-10-16T22:10:12Z

packages/workbox-background-sync/lib/Queue.mjs

+   *
+   * @private
+   */
+  _addSyncListener() {


I still have concerns but this module is also too big to cover off everything. I'll play around with it and see if it makes to refactor after.

gauntface · 2017-10-16T22:11:03Z

packages/workbox-background-sync/lib/Queue.mjs

+   *
+   * @private
+   */
+  async _registerSync() {


I'm assuming calling sync with the same details more than once get's de-duped by the browser?

workbox-pr-bot · 2017-10-17T00:29:36Z

PR-Bot Size Plugin

Changed File Sizes

File	Before	After	Change	GZipped
packages/workbox-cache-expiration/build/browser/workbox-cache-expiration.prod.js	1.45 KB	971 B	-33%	530 B	🎉
packages/workbox-core/build/browser/workbox-core.prod.js	3.99 KB	4.00 KB	+0%	1.78 KB
packages/workbox-precaching/build/browser/workbox-precaching.prod.js	3.70 KB	3.08 KB	-17%	1.37 KB	🎉
packages/workbox-routing/build/browser/workbox-routing.prod.js	1.57 KB	1.48 KB	-6%	754 B
packages/workbox-runtime-caching/build/browser/workbox-runtime-caching.prod.js	2.31 KB	1.81 KB	-22%	660 B	🎉
packages/workbox-sw/build/browser/workbox-sw.prod.js	1.82 KB	1.28 KB	-30%	649 B	🎉

New Files

File	Size	GZipped
packages/workbox-background-sync/build/browser/workbox-background-sync.prod.js	3.04 KB	1.37 KB

All File Sizes

View Table

File	Before	After	Change	GZipped
packages/workbox-background-sync/build/browser/workbox-background-sync.prod.js		3.04 KB		1.37 KB
packages/workbox-cache-expiration/build/browser/workbox-cache-expiration.prod.js	1.45 KB	971 B	-33%	530 B	🎉
packages/workbox-core/build/browser/workbox-core.prod.js	3.99 KB	4.00 KB	+0%	1.78 KB
packages/workbox-precaching/build/browser/workbox-precaching.prod.js	3.70 KB	3.08 KB	-17%	1.37 KB	🎉
packages/workbox-routing/build/browser/workbox-routing.prod.js	1.57 KB	1.48 KB	-6%	754 B
packages/workbox-runtime-caching/build/browser/workbox-runtime-caching.prod.js	2.31 KB	1.81 KB	-22%	660 B	🎉
packages/workbox-sw/build/browser/workbox-sw.prod.js	1.82 KB	1.28 KB	-30%	649 B	🎉

Workbox Aggregate Size Plugin

Total Size: 9.31KB
Percentage of Size Used: 64%

Gzipped: 4.33KB

philipwalton requested review from jeffposnick and gauntface October 10, 2017 21:33

jeffposnick suggested changes Oct 10, 2017

View reviewed changes

gauntface reviewed Oct 11, 2017

View reviewed changes

gauntface suggested changes Oct 11, 2017

View reviewed changes

philipwalton force-pushed the v3-background-sync branch from 110e08b to ffd750d Compare October 11, 2017 14:55

philipwalton added 15 commits October 13, 2017 13:46

Add initial package files

2f9272a

Add StorableRequest class

b32ad3b

Add tests for StorableRequest

aa5615b

Add remaining tests

bd24d62

Fix lint errors

1b4fd11

Update API with additions from changes issue

1bb39c1

Update packages due to indexedDBHelper changes

0128a57

Fix lint errors and merge issues

95774f0

Address review feedback

e15a03a

Address review feedback

9d2a265

Add support for Blobs

61e853e

Testing...

34b65f5

Consolodate replay callbacks to queueDidReplay

01b81ac

Add a QueueStore moodule

07a8879

Include latest v3 changes

122eca3

philipwalton force-pushed the v3-background-sync branch from bc69222 to 122eca3 Compare October 13, 2017 17:53

philipwalton added 3 commits October 13, 2017 14:13

Fix test failures

18d81a3

Split out SW mock APIs into their own files

fea88dd

Fix lint errors

76b650e

philipwalton commented Oct 13, 2017

View reviewed changes

philipwalton force-pushed the v3-background-sync branch from a927efa to 76b650e Compare October 13, 2017 19:52

Remove clearRequire.match for windows compat

db5f881

jeffposnick reviewed Oct 14, 2017

View reviewed changes

Address review feedback

eea42ad

jeffposnick approved these changes Oct 15, 2017

View reviewed changes

gauntface reviewed Oct 16, 2017

View reviewed changes

gauntface approved these changes Oct 16, 2017

View reviewed changes

Update jsdoc and add QueuePlugin class

5deb627

philipwalton merged commit fe217e1 into v3 Oct 17, 2017

philipwalton deleted the v3-background-sync branch October 17, 2017 00:49

This was referenced Oct 30, 2017

background-sync's RequestQueue class has non-deterministic behavior #826

Closed

Add support for non-text request bodies in workbox-background-sync #863

Closed

Changes to the workbox-background-sync API in v3 #868

Closed



		const {indexedDBHelper, WorkboxError} = _private;
		const names = new Set();


		expect(Queue.prototype.replayRequests.calledOnce).to.be.true;

		registration.sync = originalSyncManager;

V3 background sync #871

V3 background sync #871

Conversation

philipwalton commented Oct 10, 2017

jeffposnick 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

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

gauntface 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

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

philipwalton 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