core(font-size): speed up gatherer #6391

connorjclark · 2018-10-25T17:04:05Z

See comments in discussion and code. LMK if anything is unclear :)

connorjclark · 2018-10-25T17:04:44Z

lighthouse-core/audits/seo/font-size.js

@@ -208,7 +208,6 @@ class FontSize extends Audit {
      analyzedFailingNodesData,
      analyzedFailingTextLength,
      failingTextLength,
-      visitedTextLength,


I removed visitedTextLength because we can now easily visit every node (it's one protocol call).

connorjclark · 2018-10-25T17:05:12Z

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

+
+    // Locate the document under analysis.
+    // TODO: this needs to use frameId
+    const doc = snapshot.documents.find(doc => lookup(doc.documentURL) === passContext.url);


I need help here.

Hm, is it not true that "The DOMNode at index 0 corresponds to the root document"? Should poke devtools folks to update that then :)

AFAIK, the only main frame id we have/use is in the trace (the mainFrameIds refactor you just did), and I don't think we're collecting it anywhere else that'd be readily available to a gatherer.

You could query it with Page.getResourceTree resourceTreeResponse.frameTree.frame.id or use the network records to get at the frame id if we need to

connorjclark · 2018-10-25T17:05:48Z

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

 const nodes = [
-  {nodeId: 1, nodeName: 'HTML'},
-  {nodeId: 2, nodeName: 'HEAD', parentId: 1},
+  {nodeId: 1, backendNodeId: 100, nodeName: 'HTML'},


@patrickhulce I hope I maintained the integrity of this test :)

patrickhulce

this looks really really good!

I'm thinkin we'll want to do a few sanity comparison checks with sites we used in the original PR in addition to the smoke test, maybe we can look at the discussion there to find a couple URLs?

patrickhulce · 2018-10-25T17:11:17Z

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

+
+    // Locate the document under analysis.
+    // TODO: this needs to use frameId
+    const doc = snapshot.documents.find(doc => lookup(doc.documentURL) === passContext.url);


Hm, is it not true that "The DOMNode at index 0 corresponds to the root document"? Should poke devtools folks to update that then :)

AFAIK, the only main frame id we have/use is in the trace (the mainFrameIds refactor you just did), and I don't think we're collecting it anywhere else that'd be readily available to a gatherer.

You could query it with Page.getResourceTree resourceTreeResponse.frameTree.frame.id or use the network records to get at the frame id if we need to

patrickhulce · 2018-10-25T17:18:30Z

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

+    // but nodes will only contain the Body node and its descendants.
+
+    /** @type {NodeFontData[]} */
+    const failingNodes = [];


do you think we could move all this into some functions for the different steps you're adding so afterPass will be pretty short like before? I think it might help human-parsers figure out what's going on :)

it looks like there's roughly the following stages

get the flattened document + snapshot and make sure its all valid

create our mappings to be able to tie the two together

go through all the nodes and identify the failing ones

is that roughly accurate?

Yeah that sounds good. I mostly wanted to wait for PR feedback before chopping up afterPass, just incase the whole thing was moot or something :P

patrickhulce · 2018-10-25T17:20:34Z

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

    nodeValue: failingText,
    nodeType: FontSizeGather.TEXT_NODE_TYPE,
    parentId: 10,
  },
 ];

+const stringsMap = {};
+const strings = [];
+const dedupe = (value) => {


WDYT about something like getOrCreateStringRef/getOrCreateStringIndex to indicate the main reason we're doing this is so that we're dealing with that .strings property

patrickhulce · 2018-10-25T17:21:49Z

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

+        parentIndex: nodes.map(node => nodes.findIndex(pNode => node.parentId === pNode.nodeId)),
+      },
+      layout: {
+        nodeIndex: nodes.map((_, i) => i), // this isn't very accurate


mind elaborating on the comment a bit? :)

is it inaccurate because the nodeIndexes are usually very large? are they usually not sequential? do any of these things affect the fidelity of the tests?

If you take a look at an actual snapshot, you'll see that there isn't a 1:1 mapping between Nodes and style entries. That's because some elements (like text nodes) don't have styles. Here, I'm being lazy and saying that all nodes have computed styles, and that the mapping is just the identity function.

It is indeed missing out on a small amount of functionality, namely the nodeId => styleIndex mapping in the implementation. it wasn't immediately clear how to generate that part of the fake snapshot so I just punted it. I could look into it a bit more.

Ah ok, yeah probably don't need to mess with it that much. If there's an easy way to at least trigger one of the branches for a single node or something that might be good, but not worth hurtin' yourself over I'd say

let's trust the protocol documentation

https://chromedevtools.github.io/devtools-protocol/tot/DOMSnapshot#method-captureSnapshot

The DOMNode at index 0 corresponds to the root document.

If it's wrong then testing will catch it :)

patrickhulce · 2018-10-25T17:24:00Z

lighthouse-core/audits/seo/font-size.js

-        disclaimer = ` (based on ${percentageOfVisitedText.toFixed()}% sample)`;
-      }
-
+      const disclaimer = '';


can just remove disclaimer now ;)

patrickhulce · 2018-10-25T17:26:53Z

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

+      const fontSize = parseInt(lookup(fontSizeStringId), 10);
+      backendIdsToPartialFontData.set(doc.nodes.backendNodeId[i], {
+        fontSize,
+        // TODO: trimming this for a second time. maybe don't?


is that because it's internally being trimmed?

because the check before (non empty text node) trims it too.

could just make that function call check for a text node, and have the "non empty" part come after. I'll do that. Was mostly trying to keep the previous function as it was.

but .trim doesn't mutate and we're not reassigning nodeValue so it should still be needed right? I don't think you have to refactor anything :)

getNodeTextLength was being called twice. Just rectified that.

ooooooh I see what you were saying now 👍

connorjclark · 2018-10-25T19:30:24Z

I'll go through the other issue and test some URLs.

brendankenny · 2018-10-26T18:03:10Z

one key question: is it actually faster? :)

connorjclark · 2018-10-26T18:07:43Z

Not sure yet. Need to rebase to get the perf tracing. I'll definitely update w/ my findings

connorjclark · 2018-10-26T20:30:10Z

traces.zip

lh:gather:afterPass:FontSize

	https://www.cnn.com/	https://www.flynashville.com/Pages/default.aspx
master	810	1317
pr	472	492

Would be useful to have a PR hook (or just print something in travis ci) that shows us diffs on these marks. Maybe collect all the timings from the smoke tests, aggregate the durations of each mark, and pretty print the time deltas against master. (opened #6409)

patrickhulce

LGTM!

brendankenny

ok, a bunch of style comments, but this is looking great, and a major improvement in both implementation and performance

brendankenny · 2018-10-30T19:00:54Z

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

-function getNodeTextLength(node) {
-  return !node.nodeValue ? 0 : node.nodeValue.trim().length;
+function getNodeTextLength(nodeValue) {
+  return !nodeValue ? 0 : nodeValue.trim().length;


if this is always a string, seems like return nodeValue.trim().length; is sufficient now?

brendankenny · 2018-10-30T19:15:57Z

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

@@ -31,6 +29,7 @@ const TEXT_NODE_TYPE = 3;
 /** @typedef {import('../../driver.js')} Driver */
 /** @typedef {LH.Artifacts.FontSize['analyzedFailingNodesData'][0]} NodeFontData */
 /** @typedef {LH.Artifacts.FontSize.DomNodeMaybeWithParent} DomNodeMaybeWithParent*/
+/** @typedef {Map<number, {fontSize: number, textLength: number}>} BackendIdsToPartialFontData */


since this is a type name, Partial ends up having a specific meaning (for tsc, at least). Here does it actually mean it's just the subset of font data we're interested in? If so, maybe BackendIdsToFontData seems better

brendankenny · 2018-10-30T19:23:36Z

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

+  calculateBackendIdsToPartialFontData(snapshot) {
+    // Makes the strings access code easier to read.
+    /** @param {number} index */
+    const lookup = (index) => snapshot.strings[index];


This doesn't really seem like it's saving much (and lookup is pretty generic, so it's difficult to tell what it's doing in place). Maybe just drop, or use const strings = snapshot.strings;?

brendankenny · 2018-10-30T19:26:02Z

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

+    if (!doc || !doc.nodes.nodeType || !doc.nodes.nodeName || !doc.nodes.backendNodeId
+      || !doc.nodes.nodeValue || !doc.nodes.parentIndex) {
+      throw new Error('Unexpected response from DOMSnapshot.captureSnapshot.');
+    }


it looks like these are meant to satisfy the type checker? Maybe that's worth commenting, e.g. // satisfy the type checker that all expected values exist

brendankenny · 2018-10-30T19:35:15Z

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

+    }
+
+    // Not all nodes have computed styles (ex: TextNodes), so doc.layout.* is smaller than doc.nodes.*
+    // doc.layout.nodeIndex maps the index into doc.nodes.* to an index into doc.layout.styles.


should this be the opposite ("maps an index in doc.layout.styles to an index in doc.nodes.*") since nodeIndexToStyleIndex is set up to be the inverse?

yup, nice catch

brendankenny · 2018-10-30T20:00:15Z

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

+      if (!textLength) continue; // ignore empty TextNodes
+      const parentStyles = doc.layout.styles[styleIndex];
+      const [fontSizeStringId] = parentStyles;
+      const fontSize = parseInt(lookup(fontSizeStringId), 10);


maybe another comment here that we expect strings like '16px', hence why the parseInt is appropriate

also, do we actually want parseFloat? I guess 11.5 will still fail the < 12 check, but we might as well store the most accurate info

brendankenny · 2018-10-30T20:09:36Z

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

+    // fastest way to get the computed styles of every Node. Bonus, it allows for whitelisting properties.
+    // Once a bad TextNode is identified, its parent Node is needed. DOMSnapshot.captureSnapshot doesn't
+    // give the entire Node object, so DOM.getFlattenedDocument is used. The only connection between a snapshot
+    // Node and an actual Protocol Node is backendId, so that is used to join the two data structures.


maybe just
// Use DOMSnapshot.captureSnapshot to get the computed font-size style of every node.
(the rest can be filled in below as it happens)

brendankenny · 2018-10-30T20:10:04Z

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

+    const snapshot = await passContext.driver.sendCommand('DOMSnapshot.captureSnapshot', {
+      computedStyles: ['font-size'],
+    });
+    // backendIdsToPartialFontData will include all Nodes,


not really true? (it just contains non-empty text nodes)

brendankenny · 2018-10-30T20:11:20Z

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

+    // backendIdsToPartialFontData will include all Nodes,
+    const backendIdsToPartialFontData = this.calculateBackendIdsToPartialFontData(snapshot);
+    // but nodes will only contain the Body node and its descendants.
+    const nodes = await getAllNodesFromBody(passContext.driver);


maybe we now want to differentiate nodes from the nodes in the snapshot, somehow? I'm not sure of a great way. crdpNodes or something?

brendankenny · 2018-10-30T20:13:22Z

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

+  }
+
+  /**
+   * @param {BackendIdsToPartialFontData} backendIdsToPartialFontData


A jsdoc description of the method would be good here (e.g. the comment from below about matching font data from the snapshot to node references via backendId)

brendankenny · 2018-10-30T20:19:11Z

@patrickhulce can you find the discussion # with some test sites? I can't find it now

patrickhulce · 2018-10-30T20:42:55Z

can you find the discussion # with some test sites? I can't find it now

#3533

but I'm starting to be afraid my back and forth with konrad with more URLs was in chats instead of that PR, the only ones I found were cnn and flynashville that @hoten already tested 😞

brendankenny

two last style nits and a question, but otherwise LGTM!

brendankenny · 2018-10-31T06:22:35Z

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

+    /** @type {Map<number, number>} */
+    const nodeIndexToStyleIndex = new Map();
+    for (let i = 0; i < doc.layout.nodeIndex.length; i++) {
+      nodeIndexToStyleIndex.set(doc.layout.nodeIndex[i], i);


just to make sure, are these 1:1?

yah? nah? :)

brendankenny · 2018-10-31T06:24:08Z

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

+      const nodeValue = strings[doc.nodes.nodeValue[i]];
+      if (!isTextNode({
+        nodeType,
+        parentNodeName: strings[doc.nodes.nodeName[doc.nodes.parentIndex[i]]],


still would be good to pull this into a variable (or variables), especially to make the conditional obvious at a glance

to be clear, do you mean

const isTextNode = !isTextNode({ nodeType, parentNodeName: strings[doc.nodes.nodeName[doc.nodes.parentIndex[i]]], }) if (!isTextNode) { continue; }

or something else?

I assumed he meant breaking up strings[doc.nodes.nodeName[doc.nodes.parentIndex[i]]] but even if he didn't, I think that would be nice if it isn't too much trouble :)

brendankenny · 2018-10-31T06:28:07Z

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

+      const textLength = getNodeTextLength(nodeValue);
+      if (!textLength) continue; // ignore empty TextNodes
+      const parentStyles = doc.layout.styles[styleIndex];
+      const [fontSizeStringId] = parentStyles;


a comment here would still be good too. The type is just Array<string>, so it and the protocol docs don't give much help on why this is the way it is

connorjclark · 2018-10-31T14:43:14Z

Oops, I think Github elided some of your comments @brendankenny (4 hidden conversations... when you open this PR), and I totally overlooked :(

Should have addressed all of them now.

paulirish

I'm seeing diff results in master vs this branch.. Do we know why that is?

repro:

lighthouse https://www.haaretz.com/ --only-audits=font-size

paulirish · 2018-10-31T21:16:04Z

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

+    // backendIdsToFontData will include all non-empty TextNodes,
+    const backendIdsToFontData = this.calculateBackendIdsToFontData(snapshot);
+    // but nodes will only contain the Body node and its descendants.
+    const crdpNodes = await getAllNodesFromBody(passContext.driver);


getAllNodesFromBody relies on DOM.getFlattenedDocument but I think all the information we need from that is exposed in the snapshot.

Using #6410 it looks like getFlattenedDocument is slower, so it seems nice to drop it.

driver.getNodesInDocument is only used by font-size, so we can remove it from driver and rely on the capturedSnapshot entirely. wdyt?

The FontSize artifact contains entire CRDP Nodes. The snapshot data doesn't have all the data to reconstruct a CRDP Node.

I think we don't need everything that's in CRDP Node.
The audit only uses node.parentNode and node.attributes. It looks like all that data is already in the snapshot.

I think we don't need everything that's in CRDP Node.

we need CSS.getMatchedStylesForNode to get the current styles for a node, which needs the nodeId to query for it

@paulirish , given the above (we need the snapshot + the flattened document protocol calls, to correlate failing nodes with their matched styles), should we still reduce the scope of artifact?

Yeah looks like the snapshot doesn't have nodeId. only backendNodeId. Bummer.

K. We'll keep the current protocol calls. :)

paulirish · 2018-10-31T21:17:31Z

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

+      const textLength = getNodeTextLength(nodeValue);
+      if (!textLength) continue; // ignore empty TextNodes
+      const parentStyles = doc.layout.styles[styleIndex];
+      // each styles is an array of string indices, one for each property given


each styles? doc.layout.styles is an array of.... ?

doc.layout.styles is an array of an array of string indices ...

connorjclark · 2018-10-31T22:12:24Z

I get a different set of results.

Differences could be due to plugins (but we ran it from the CLI, so nah), or the site serving different content based on user agent, or the code being wrong :P

connorjclark · 2018-10-31T23:47:22Z

Tried this URL: https://www.clumsycrafter.com/the-water-blob-tutorial-using-an-iron-to-seal-the-edges/

The old code reports the "Report this ad" as an illegible node:

The new code does not.

For this site, it's obvious that this ad takes awhile to display on the page. I think the difference we are seeing is due to the old code allowing the page more time to do stuff (we do afterPass after a relatively quiet period, but it is rather arbitrary), so the page has time to render things (like the above ad). The code changes here shouldn't affect how early things like snapshot/get flattened document are called, but maybe something else in master (but not this branch) is. I'll merge master into this branch now so I can eliminate that potential difference.

paulirish · 2018-11-01T01:42:22Z

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

+    // backendIdsToFontData will include all non-empty TextNodes,
+    const backendIdsToFontData = this.calculateBackendIdsToFontData(snapshot);
+    // but nodes will only contain the Body node and its descendants.
+    const crdpNodes = await getAllNodesFromBody(passContext.driver);


Yeah looks like the snapshot doesn't have nodeId. only backendNodeId. Bummer.

K. We'll keep the current protocol calls. :)

paulirish · 2018-11-01T01:44:10Z

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

      passContext.driver.sendCommand('DOM.enable'),
      passContext.driver.sendCommand('CSS.enable'),
    ]);

+    // Use DOMSnapshot.captureSnapshot to get the computed font-size style of every node.
+    const snapshot = await passContext.driver.sendCommand('DOMSnapshot.captureSnapshot', {


very small speedup if you run these two in parallel:

lighthouse-core/gather/gatherers/seo/font-size.js | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/lighthouse-core/gather/gatherers/seo/font-size.js b/lighthouse-core/gather/gatherers/seo/font-size.js index e6d0dbe0..3ceac9bd 100644 --- a/lighthouse-core/gather/gatherers/seo/font-size.js +++ b/lighthouse-core/gather/gatherers/seo/font-size.js @@ -350,13 +350,15 @@ class FontSize extends Gatherer { ]); // Use DOMSnapshot.captureSnapshot to get the computed font-size style of every node. - const snapshot = await passContext.driver.sendCommand('DOMSnapshot.captureSnapshot', { + const snapshotPromise = passContext.driver.sendCommand('DOMSnapshot.captureSnapshot', { computedStyles: ['font-size'], }); + // but nodes will only contain the Body node and its descendants. + const allNodesPromise = getAllNodesFromBody(passContext.driver); + const [snapshot, crdpNodes] = await Promise.all([snapshotPromise, allNodesPromise]); + // backendIdsToFontData will include all non-empty TextNodes, const backendIdsToFontData = this.calculateBackendIdsToFontData(snapshot); - // but nodes will only contain the Body node and its descendants. - const crdpNodes = await getAllNodesFromBody(passContext.driver); const { totalTextLength, failingTextLength,

connorjclark · 2018-11-01T22:55:26Z

One of the missing nodes for the site Paul tried (https://www.haaretz.com/) has 8px font.

I looked up '8px' in snapshot.strings, found an index. I look for this index in doc.layout.styles, no results. Boo.

I looked up the offending node (from the DOM.getFlattenedDocument method) by class name:

crdpNodes.filter(n => n.attributes && n.attributes.length === 2 && n.attributes[1] === 'cbola-banner-structure__advertisement-title cbola-fade-out')

and walked up the parentNode hierarchy until I got to a BODY element. Grabbed its backendNodeId, and saw that there is no reference to it in doc.nodes.backendNodeIds. Also, this is just one of many (32) BODY nodes in crpdNodes - but doc.nodes.nodeName only references one BODY element.

So, I think that DOMSnapshot.captureSnapshot splits up the various documents in the page (iframes?) differently that how DOM.getFlattendDocument does. Even though the former says documents[0] is the root and the latter says this returns the root document. Where's the disconnect here?

Could it be we need to look at all of these?

connorjclark · 2018-11-02T00:03:12Z

Building backendIdsToFontData from all the documents in snapshot got me this:

which is only missing this from master:

I think that's a shadow element, but I don't see any shadow-root in the inspector.

connorjclark · 2018-11-02T17:47:19Z

Some of the documents in the snapshot don't contain any style information (snapshot.documents[j].layout.styles has no data). Seeing this for the User Agent Stylesheet node with 11px font size from haaretz.com.

So that's not so great.

…e info

brendankenny

changing my review while we figure out the issues so no one accidentally merges :)

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

Should we

figure out the difference another day and close, or
is it in the immediate future and we should mark chillin

connorjclark · 2018-12-10T20:07:35Z

I'll give it another shot in the near future.

brendankenny · 2019-01-30T01:15:19Z

Going to close for now since it's not on our immediate roadmap (I believe), but we can keep the branch open :)

connorjclark added 3 commits October 25, 2018 01:08

core(font-size): speed up font-size gatherer (#6386)

82f6c36

more comments

17413ec

simplify isNonEmptyTextNode by restructuring backend id mapping

99ce40e

connorjclark requested review from brendankenny, patrickhulce and paulirish as code owners October 25, 2018 17:04

connorjclark commented Oct 25, 2018

View reviewed changes

patrickhulce suggested changes Oct 25, 2018

View reviewed changes

connorjclark added 3 commits October 25, 2018 12:26

remove extra string trimming

80b0510

remove disclaimer

70c9768

pr stuff

b3b5779

connorjclark added 2 commits October 26, 2018 10:48

lint

4a31a3e

first doc in snapshot is the one we wat

e5aaa34

connorjclark added 2 commits October 26, 2018 11:08

Merge branch 'master' into issue-6386-speed-up-font-size-gatherer

129029c

break into functions

3c01962

patrickhulce mentioned this pull request Oct 30, 2018

WIP: DZL Testing PR #6427

Closed

patrickhulce approved these changes Oct 30, 2018

View reviewed changes

brendankenny requested changes Oct 30, 2018

View reviewed changes

pr changes

3555192

brendankenny approved these changes Oct 31, 2018

View reviewed changes

brendankenny assigned paulirish Oct 31, 2018

comments

2e7fa88

connorjclark added 2 commits October 31, 2018 07:37

forEach

ec3da81

pr

d6698dd

paulirish requested changes Oct 31, 2018

View reviewed changes

Merge branch 'master' into issue-6386-speed-up-font-size-gatherer

f15f8f2

paulirish approved these changes Nov 1, 2018

View reviewed changes

patrickhulce mentioned this pull request Nov 1, 2018

☂️ OYB Next Steps #6442

Closed

6 tasks

connorjclark added 2 commits November 1, 2018 13:56

snapshot and all nodes concurrent

09b6e93

snapshot and all nodes concurrent

3da4c8b

look at all documents in snapshot. look at all ancestors to find styl…

098508d

…e info

brendankenny requested changes Nov 19, 2018

View reviewed changes

connorjclark added the chillin ❄ label Dec 10, 2018

brendankenny closed this Jan 30, 2019

connorjclark mentioned this pull request Jan 8, 2020

core(font-size): speed up gatherer #10200

Merged

connorjclark mentioned this pull request Aug 4, 2020

drop usage of deprecated DOM.getFlattenedDocument #11210

Closed

core(font-size): speed up gatherer #6391

core(font-size): speed up gatherer #6391

Conversation

connorjclark commented Oct 25, 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

patrickhulce 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

patrickhulce Oct 25, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

connorjclark commented Oct 25, 2018

brendankenny commented Oct 26, 2018

connorjclark commented Oct 26, 2018

connorjclark commented Oct 26, 2018 • edited Loading

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

brendankenny commented Oct 30, 2018

patrickhulce commented Oct 30, 2018

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

connorjclark Oct 31, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

connorjclark commented Oct 31, 2018 • edited Loading

paulirish 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

connorjclark Oct 31, 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

connorjclark commented Oct 31, 2018 • edited Loading

connorjclark commented Oct 31, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

connorjclark commented Nov 1, 2018 • edited Loading

connorjclark commented Nov 2, 2018 • edited Loading

connorjclark commented Nov 2, 2018

brendankenny left a comment

Choose a reason for hiding this comment

brendankenny commented Dec 10, 2018

connorjclark commented Dec 10, 2018

brendankenny commented Jan 30, 2019

patrickhulce Oct 25, 2018 •

edited

Loading

connorjclark commented Oct 26, 2018 •

edited

Loading

connorjclark Oct 31, 2018 •

edited

Loading

connorjclark commented Oct 31, 2018 •

edited

Loading

connorjclark Oct 31, 2018 •

edited

Loading

connorjclark commented Oct 31, 2018 •

edited

Loading

connorjclark commented Oct 31, 2018 •

edited

Loading

connorjclark commented Nov 1, 2018 •

edited

Loading

connorjclark commented Nov 2, 2018 •

edited

Loading