core: remove dependency on WebInspector #6209

patrickhulce · 2018-10-10T16:16:36Z

Summary
A lot going on here but overall this PR removes the DevTools dependency on the font-size gatherer

Upgrade jest to 23.6 so we can use toMatchInlineSnapshot
Add prettier to devDeps so we can use toMatchInlineSnapshot
Rewrite font-size gatherer for async/await + type-checking (too thorny to try to mess with so much in there the way it was)
Replace the CSS font rule identification logic with custom handler, we actually start identifying 1 new CSS rule that was being missed on CNN! (probably just so old devtools code it got out of sync)
Remove the WebInspector dependency in font-size and delete the code!!

wardpeet · 2018-10-10T18:26:27Z

package.json

    "jsdom": "^9.12.0",
    "mocha": "^3.2.0",
    "npm-run-posix-or-windows": "^2.0.2",
    "nyc": "^11.6.0",
    "postinstall-prepare": "^1.0.1",
+    "prettier": "^1.14.3",


😮 ? mistake or are we moving to prettier?

not used for anything other than toMatchInlineSnapshot right now, you know I'd be on board though :)

yeah sorry I read it to late in your comment above. But I think we should just move 😋 I've found a way to let travis fail without changing code on commit.
prettier --list-different '**/*.{js,json}

wardpeet · 2018-10-10T18:30:06Z

lighthouse-core/gather/gatherers/seo/font-size.js

+  nodes.forEach(node => nodeMap.set(node.nodeId, node));
+  nodes.forEach(node => (node.parentNode = nodeMap.get(node.parentId)));
+  // @ts-ignore - once passing through the filter, it's guaranteed to have a parent
+  return nodes.filter(nodeInBody);


@brendankenny I was struggling with this in my own projects to. Is there no way to fix this with jsdoc?

I was struggling with this in my own projects to. Is there no way to fix this with jsdoc?

there actually is as of tsc 3.1! (in jsdoc. In ts I think it was working in 2.5 or 2.6)

The downside is it can be quite awkward for inline functions in filter(), but this case isn't. I'm not sure how it will work in this instance since nodeInBody is recursive, though.

Instead of nodeInBody returning a boolean, you instead make it one of typescripts type guards, telling the compiler that the boolean return value is actually asserting if the input is a certain type or not. The return annotation for nodeInBody becomes @return {node is LH.Artifacts.FontSize.DomNodeWithParent}. This should work since DomNodeWithParent is a valid derived type of DomNodeMaybeWithParent, though, again, not sure what happens with that recursive call.

worked like a charm 👍

@patrickhulce shouldn't we remove the ts-ignore if it worked?

I did. Maybe Github is being too smart for us and showing the original diff for the old convo? 🤷‍♂️

wardpeet · 2018-10-10T18:32:52Z

lighthouse-core/gather/gatherers/seo/font-size.js

    /** @type {Map<string, LH.Crdp.CSS.CSSStyleSheetHeader>} */
    const stylesheets = new Map();
    /** @param {LH.Crdp.CSS.StyleSheetAddedEvent} sheet */
    const onStylesheetAdd = sheet => stylesheets.set(sheet.header.styleSheetId, sheet.header);
    passContext.driver.on('CSS.styleSheetAdded', onStylesheetAdd);

-    const enableDOM = passContext.driver.sendCommand('DOM.enable');
-    const enableCSS = passContext.driver.sendCommand('CSS.enable');
+    await passContext.driver.sendCommand('DOM.enable');


not that it's a big deal but we had a Promise.all in the previous version. Probably not worth the hassle
await Promise.all([passContext.driver.sendCommand('DOM.enable'), passContext.driver.sendCommand('CSS.enable')]);

sure might as well

wardpeet · 2018-10-10T18:34:50Z

lighthouse-core/gather/gatherers/seo/font-size.js

 */
-function getFontSizeInformation(driver, node) {
-  return driver.sendCommand('CSS.getComputedStyleForNode', {nodeId: node.parentId})
+function fetchComputedFontSize(driver, node) {


any reason why we are not asyncing this function?

oh no real reason other than I didn't touch any of the logic, small diff whens don't mean much in this PR though 😆

wardpeet · 2018-10-10T18:39:46Z

lighthouse-core/gather/gatherers/seo/font-size.js

-          totalTextLength,
-        }));
-      });
+    await passContext.driver.sendCommand('DOM.disable');


same goes for this one that it was a Promise.all before

brendankenny

ok, still working through this, but I think it looks pretty great (and generally applicable if we eventually have other audits needing to look at the style cascade for particular nodes). Some surface-level comments for you until I get back into this in the morning

brendankenny · 2018-10-12T00:08:43Z

lighthouse-core/gather/gatherers/seo/font-size.js

+  nodes.forEach(node => nodeMap.set(node.nodeId, node));
+  nodes.forEach(node => (node.parentNode = nodeMap.get(node.parentId)));
+  // @ts-ignore - once passing through the filter, it's guaranteed to have a parent
+  return nodes.filter(nodeInBody);


I was struggling with this in my own projects to. Is there no way to fix this with jsdoc?

there actually is as of tsc 3.1! (in jsdoc. In ts I think it was working in 2.5 or 2.6)

The downside is it can be quite awkward for inline functions in filter(), but this case isn't. I'm not sure how it will work in this instance since nodeInBody is recursive, though.

Instead of nodeInBody returning a boolean, you instead make it one of typescripts type guards, telling the compiler that the boolean return value is actually asserting if the input is a certain type or not. The return annotation for nodeInBody becomes @return {node is LH.Artifacts.FontSize.DomNodeWithParent}. This should work since DomNodeWithParent is a valid derived type of DomNodeMaybeWithParent, though, again, not sure what happens with that recursive call.

brendankenny · 2018-10-12T00:40:51Z

lighthouse-core/gather/gatherers/seo/font-size.js

+const TEXT_NODE_TYPE = 3;
+
+/** @typedef {LH.Artifacts.FontSize['analyzedFailingNodesData'][0]} FailingNodeData */
+/** @typedef {LH.Artifacts.FontSize.DomNodeMaybeWithParent} DomNodeMaybeWithParent*/


since DomNodeMaybeWithParent is just for this file, can we just make it a local typedef?

also, I apologize for the type names in here I'm responsible for :)

microsoft/TypeScript#20077 seems to suggest inheritance plus recursive type doesn't work so well, I might need some help making it local :)

microsoft/TypeScript#20077 seems to suggest inheritance plus recursive type doesn't work so well,

ah yeah, that comment even has a reference from me talking about the limitiation in a different PR. Nevermind :)

brendankenny · 2018-10-12T00:42:25Z

lighthouse-core/gather/gatherers/seo/font-size.js

+
+/** @typedef {LH.Artifacts.FontSize['analyzedFailingNodesData'][0]} FailingNodeData */
+/** @typedef {LH.Artifacts.FontSize.DomNodeMaybeWithParent} DomNodeMaybeWithParent*/
+
 const Driver = require('../../driver.js'); // eslint-disable-line no-unused-vars


change to an import typedef while in here?

brendankenny · 2018-10-12T00:45:22Z

lighthouse-core/gather/gatherers/seo/font-size.js

-      return nodes.filter(nodeInBody);
-    });
+async function getAllNodesFromBody(driver) {
+  const nodes = /** @type {DomNodeMaybeWithParent[]} */ (await driver.getNodesInDocument());


is this a bad assertion since parentId might not be assigned on the underlying Protocol.DOM.Node? I'm not sure what the alternative would be...make the Map key on number|undefined?

~~yeah didn't even have to do that, just a fallback for parent id~~

nah you were right first time 👍

brendankenny · 2018-10-12T00:46:56Z

lighthouse-core/gather/gatherers/seo/font-size.js

+ * @param {string} selector
+ * @return {number}
+ */
+function computeSelectorSpecificity(selector) {


I don't know how to interpret this emoji :) haha

brendankenny · 2018-10-12T00:56:13Z

lighthouse-core/gather/gatherers/seo/font-size.js

+}
+
+/**
+ *


maybe a jsdoc string to explain what "direct" means in this context?

👍 renamed the function a bit too

brendankenny · 2018-10-12T01:17:36Z

lighthouse-core/gather/gatherers/seo/font-size.js

 }

 /**
 * @param {Driver} driver
 * @param {LH.Artifacts.FontSize.DomNodeWithParent} node text node
- * @returns {Promise<?{fontSize: number, textLength: number, node: LH.Artifacts.FontSize.DomNodeWithParent}>}
+ * @returns {Promise<?FailingNodeData>}


I guess it's unfortunate to call this FailingNodeData since it might not be failing at this point. Would it lose precision elsewhere to rename it (NodeFontData or whatever)?

yeah I think that works fine 👍

brendankenny · 2018-10-12T01:20:01Z

lighthouse-core/gather/gatherers/seo/font-size.js

+      node: /** @type {LH.Artifacts.FontSize.DomNodeWithParent} */ (node.parentNode),
+    };
+  } catch (err) {
+    Sentry.captureException(err);


should we debounce this in keeping with #5994?

thoughts on #6215 first? :)

brendankenny

ok, I think I'm getting font-size.js now :) Made a few more name suggestions to ease understanding of the flow. I'm continuing the review now, but wanted you to get some feedback

brendankenny · 2018-10-16T22:56:32Z

lighthouse-core/gather/gatherers/seo/font-size.js

+const TEXT_NODE_TYPE = 3;
+
+/** @typedef {LH.Artifacts.FontSize['analyzedFailingNodesData'][0]} FailingNodeData */
+/** @typedef {LH.Artifacts.FontSize.DomNodeMaybeWithParent} DomNodeMaybeWithParent*/


microsoft/TypeScript#20077 seems to suggest inheritance plus recursive type doesn't work so well,

ah yeah, that comment even has a reference from me talking about the limitiation in a different PR. Nevermind :)

brendankenny · 2018-10-16T22:57:56Z

lighthouse-core/gather/gatherers/seo/font-size.js

+    numTypes += types.length;
+  }
+
+  return Math.min(9, numIDs) * 100 + Math.min(9, numClasses) * 10 + Math.min(9, numTypes);


does !important figure into things?

I've ignored !important so far, think it's worth handling? I'll at least comment it's absence

think it's worth handling?

no idea :) I have used it in anger more than once, so if you really really mean to override a font-size with it, it would be annoying if lighthouse didn't recognize it.

Is it non-trivial to support?

Is it non-trivial to support?

Kinda, we basically have to do this entire thing twice, once looking for !important and once not. The good news is that LH will still recognize the percentage of text going down. If you !important'd a valid font size, the risk is that you !important an also invalid font-size and we point to a rule that isn't really winning.

ha, I guess I hadn't completely thought through how this gatherer is working.

Of course we already have the computed style, so the css parsing is just to get the rule that caused that style. It's complicated enough that I had forgotten that by this point 😳 I'll add a comment on analyzeFailingNodes

brendankenny · 2018-10-16T23:01:27Z

lighthouse-core/gather/gatherers/seo/font-size.js

+}
+
+/**
+ * Returns effective CSS rule for given CSS property.


Since this is just matching against font-size, maybe mention that instead. "effective" also isn't immediately clear here, instead of something about how it's the css rule currently setting the style for the font-size property

brendankenny · 2018-10-16T23:17:03Z

lighthouse-core/gather/gatherers/seo/font-size.js

-    .catch(err => {
-      require('../../../lib/sentry.js').captureException(err);
-      return null;
+async function fetchComputedFontSize(driver, node) {


maybe rename node to textNode? Wasn't immediately clear why you'd want the computed style for the node's parent without that info :)

brendankenny · 2018-10-16T23:17:46Z

lighthouse-core/gather/gatherers/seo/font-size.js

+      // @ts-ignore - font size property guaranteed to be returned in getComputedStyle
+      fontSize: parseInt(fontSizeProperty.value, 10),
+      textLength: getNodeTextLength(node),
+      node: /** @type {LH.Artifacts.FontSize.DomNodeWithParent} */ (node.parentNode),


cast not needed anymore, I don't think

brendankenny · 2018-10-16T23:27:50Z

lighthouse-core/gather/gatherers/seo/font-size.js

+      .sort((a, b) => b.textLength - a.textLength)
+      .slice(0, MAX_NODES_ANALYZED);
+
+    return {totalTextLength, visitedTextLength, failingTextLength, nodesToAnalyze};


maybe failingNodes to standardize on that terminology instead of nodesToAnalyze

well the nodesToAnalyze is the capped subset of failingNodes so I like that they're named differently, failingNodesSubsetToAnalyze?

well the nodesToAnalyze is the capped subset of failingNodes so I like that they're named differently, failingNodesSubsetToAnalyze?

failingNodesSubset? cappedFailingNodes? importantFailingNodes? failingNodesToSource?

brendankenny · 2018-10-16T23:28:20Z

lighthouse-core/gather/gatherers/seo/font-size.js

+   * @param {Array<NodeFontData>} nodesToAnalyze
+   */
+  static async analyzeFailingNodes(driver, nodesToAnalyze) {
+    const analysisPromises = nodesToAnalyze.map(async failingNode => {


It would be nice if there was an easier way to express the async behavior needed but still use forEach

brendankenny · 2018-10-16T23:30:57Z

lighthouse-core/gather/gatherers/seo/font-size.js

+      0
+    );
+
+    return {analyzedFailingNodesData, analyzedFailingTextLength};


just analyzedFailingNodes, maybe?

well that's just based on the artifact shape name, feel strongly enough to change the artifact type and its usages up the chain?

well that's just based on the artifact shape name, feel strongly enough to change the artifact type and its usages up the chain?

ha, I didn't before, but I think we maybe should. The Data part is definitely redundant, but what do you think about sourcedFailingNodes?

brendankenny

ok, jk, font-size.js is really the bulk of this

brendankenny · 2018-10-16T23:36:33Z

lighthouse-core/lib/web-inspector.js

@@ -1,61 +0,0 @@
-/**


🎉 🎉 💃 🕺

brendankenny · 2018-10-16T23:45:33Z

.prettierrc

@@ -0,0 +1,6 @@
+printWidth: 100


at the very least this should be under lighthouse-core/test/ :P

fair enough, only a matter of time now anyway

brendankenny · 2018-10-16T23:48:01Z

lighthouse-core/test/gather/gatherers/seo/font-size-test.js

+    const driver = {
+      on() {},
+      off() {},
+      sendCommand(command, params) {


nit: switch to async and can drop the Promise.resolve/reject

brendankenny · 2018-10-17T00:00:36Z

lighthouse-core/test/gather/gatherers/seo/font-size-test.js

+    it('should identify inline styles', () => {
+      const result = FontSizeGather.getEffectiveFontRule({inlineStyle});
+      expect(result).toMatchInlineSnapshot(`
+Object {


IMO, it seems like
a) it would be shorter and easier to read as an inline object literal and an expect.toEqual
b) if reading isn't important, then could just be a regular snapshot (we have shorter ones :)
c) if these were going to occasionally update, maybe there's an advantage for snapshots' ease of use, but these shouldn't ever change
rather than bringing in prettier just for this feature

@paulirish were you as stoked for inlinesnapshot all the things as I was or am I alone on this one :)

@hoten @exterkamp any opinions on snapshot testing?

I like snapshot testing. I can't figure out why one would want to do it inline, though... Seems creating the initial snapshot + approving updates to the snapshot become a lot more menial tasks.

@hoten the inline snapshot is still managed automatically (that's why we needed to update jest and add a prettier dependency), the benefit is you just get to see the result right next to the test instead of a separate file

Snapshot testing all the way! ♥️

I think inline snapshots introduce benefits to 1) testing consistency and 2) dev ergonomics

Consistency

The alternative we're weighing this against is expect.toEqual(obj). But obj could be an object literal or it could just constructed some other way.. (expected = JSON.parse(JSON.stringify(thing)); expected.change = 42)

Adopting inline snapshots brings consistency to this pattern and prioritizes a declarative view of the expected object. I think this encourages more reviewable tests.

Ergonomics

First time writing the assertion, just do expect(result).toMatchInlineSnapshot() and the test will be updated with the current result. When making any changes that affect the expected value, updating them is just a matter of typing u.

To be fair, we don't spend a lot of time writing expecting values, but these workflow improvements make authoring tests faster and easier. (Which encourages more tests).

(I looked across our usage of deepStrictEqual and the large majority are very small expected objects... like 1-3 lines tall)

TBH I think toEqual() continues to make sense if the obj literal is like 1-4ish lines tall. From 5-15 lines tall, inline snapshot I think wins for creating/updating advantages. And past 15 lines moving to a non-inline snapshot makes the most sense.

wdyt?

First time writing the assertion, just do expect(result).toMatchInlineSnapshot() and the test will be updated with the current result. When making any changes that affect the expected value, updating them is just a matter of typing u.

this is actually why I think snapshots are great. I agree they might be overkill for smaller objects but should we really care? It's easier to always snapshot test just to make it consistent for contributers? Or at least go for something like always do inlinesnapshots for audit results & gatherer results?

brendankenny · 2018-10-17T00:01:36Z

lighthouse-core/test/gather/gatherers/seo/font-size-test.js

+    });
+
+    it('should cap the craziness', () => {
+      expect(compute('h1.a.b.c.d.e.f.g.h.i.j.k.l.m.n.o.p')).toEqual(91);


brendankenny

ok, some more naming comments and then I swear I'm done with those :)

analyze definitely confuses things (it confused me), as really the (font size) analysis has already been done at that point. It's more just finding the CSS rule responsible. I guess an argument could be made that that's "analysis", but it didn't really occur to me :)

brendankenny · 2018-10-17T20:14:56Z

lighthouse-core/test/gather/gatherers/seo/font-size-test.js

-        getNodesInDocument() {
-          return Promise.resolve(nodes);
-        },
+          result = {


nit: can just return since nothing else is being done to result (IMO marginally more readable since you have to skip down pretty far to see it's just being returned, but, again, just a nit :)

brendankenny · 2018-10-17T20:20:33Z

lighthouse-core/gather/gatherers/seo/font-size.js

+    numTypes += types.length;
+  }
+
+  return Math.min(9, numIDs) * 100 + Math.min(9, numClasses) * 10 + Math.min(9, numTypes);


ha, I guess I hadn't completely thought through how this gatherer is working.

Of course we already have the computed style, so the css parsing is just to get the rule that caused that style. It's complicated enough that I had forgotten that by this point 😳 I'll add a comment on analyzeFailingNodes

brendankenny · 2018-10-17T20:21:51Z

lighthouse-core/gather/gatherers/seo/font-size.js

+}
+
+/**
+ * Finds the most specific directly matched CSS rule from the list.


font-size rule

brendankenny · 2018-10-17T20:24:49Z

lighthouse-core/gather/gatherers/seo/font-size.js

+   * @param {LH.Gatherer.PassContext['driver']} driver
+   * @param {Array<NodeFontData>} nodesToAnalyze
+   */
+  static async analyzeFailingNodes(driver, nodesToAnalyze) {


Maybe we could make that clearer than "analyze", since it's not really doing any analysis. fetchSourceRule describes itself well, so maybe something like fetchFailingNodeSourceRules? sourceFailingNodes? :)

brendankenny · 2018-10-17T20:25:42Z

lighthouse-core/gather/gatherers/seo/font-size.js

+      .sort((a, b) => b.textLength - a.textLength)
+      .slice(0, MAX_NODES_ANALYZED);
+
+    return {totalTextLength, visitedTextLength, failingTextLength, nodesToAnalyze};


well the nodesToAnalyze is the capped subset of failingNodes so I like that they're named differently, failingNodesSubsetToAnalyze?

failingNodesSubset? cappedFailingNodes? importantFailingNodes? failingNodesToSource?

brendankenny · 2018-10-17T20:26:50Z

lighthouse-core/gather/gatherers/seo/font-size.js

+      0
+    );
+
+    return {analyzedFailingNodesData, analyzedFailingTextLength};


well that's just based on the artifact shape name, feel strongly enough to change the artifact type and its usages up the chain?

ha, I didn't before, but I think we maybe should. The Data part is definitely redundant, but what do you think about sourcedFailingNodes?

brendankenny · 2018-10-17T20:36:34Z

lighthouse-core/test/gather/gatherers/seo/font-size-test.js

+  "styleSheetId": undefined,
+  "type": "Regular",
+}
+`);


also

expect(result).toEqual({ cssProperties: [{ name: 'font-size', value: 12, }], parentRule: { origin: 'user-agent', selectors: [{ text: 'body', }], }, styleSheetId: undefined, type: 'Regular', });

still way better here :P

patrickhulce · 2018-10-19T15:24:31Z

OK now that we've tabled the artifact refactoring for another day I think the only outstanding item here is the toMatchInlineSnapshot debacle.

If the effort to use inline snapshots is going to be this high for every change, the savings in maintaining large expectations will be negated by the effort in justifying their usage, so I'd like a holding rule either way :)

I am pro-snapshots. It seems like @wardpeet is as well with @hoten ~~mixed~~ and @brendankenny against?

@paulirish @exterkamp how do you rule?

connorjclark · 2018-10-19T15:42:42Z

You can put me on the "against" list. I'm not convinced that small objects are a good candidate for snapshot testing. HTML, for sure, but the entire jest / snapshot overhead for this current use case can just be a deep object equality assertion.

brendankenny

Let's land this

🔔 🔔 🔔
👯‍♂️ 👯 🕺
🎉 🌮 🎉

patrickhulce added 3 commits October 10, 2018 10:31

core: remove DevTools dependency from font-size gatherer

ac68e4b

remove the web inspector!!!

9e465d0

tests

33cf19e

patrickhulce requested review from brendankenny and paulirish as code owners October 10, 2018 16:16

yarn updates

797271d

wardpeet reviewed Oct 10, 2018

View reviewed changes

GoogleChrome deleted a comment from patrickhulce Oct 10, 2018

feedback

aced555

brendankenny reviewed Oct 12, 2018

View reviewed changes

feedback

702258f

brendankenny reviewed Oct 16, 2018

View reviewed changes

brendankenny reviewed Oct 17, 2018

View reviewed changes

patrickhulce added 2 commits October 16, 2018 19:56

Merge branch 'master' into remove_devtools

377c910

feedback

707c625

brendankenny reviewed Oct 17, 2018

View reviewed changes

more feedback

45c517e

brendankenny mentioned this pull request Oct 23, 2018

core: avoid webinspector global pollution #5946

Closed

brendankenny approved these changes Oct 23, 2018

View reviewed changes

brendankenny merged commit 9882514 into master Oct 23, 2018

brendankenny deleted the remove_devtools branch October 23, 2018 16:35

+              }
+              /**
+               *

core: remove dependency on WebInspector #6209

core: remove dependency on WebInspector #6209

Conversation

patrickhulce commented Oct 10, 2018

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

brendankenny 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

patrickhulce Oct 12, 2018 • edited Loading

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

brendankenny 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

brendankenny 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

wardpeet Oct 19, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brendankenny left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patrickhulce Oct 12, 2018 •

edited

Loading

wardpeet Oct 19, 2018 •

edited

Loading