From 3ae16a55352f7d28a623b77ac76d7a373f703ac8 Mon Sep 17 00:00:00 2001 From: Patrick Hulce Date: Thu, 17 Jan 2019 13:38:07 -0600 Subject: [PATCH 1/3] misc: replace TODOs with github issue links --- lighthouse-cli/test/smokehouse/byte-config.js | 3 +-- .../test/smokehouse/byte-efficiency/expectations.js | 3 +-- .../audits/byte-efficiency/render-blocking-resources.js | 7 +++---- lighthouse-core/audits/content-width.js | 2 +- lighthouse-core/config/config.js | 2 +- lighthouse-core/gather/driver.js | 2 +- .../gather/gatherers/dobetterweb/optimized-images.js | 4 +--- lighthouse-core/gather/gatherers/link-elements.js | 3 ++- .../lib/dependency-graph/simulator/network-analyzer.js | 5 ++--- .../lib/dependency-graph/simulator/simulator.js | 2 +- lighthouse-core/lib/network-recorder.js | 3 +-- lighthouse-core/lib/network-request.js | 1 - lighthouse-core/runner.js | 2 +- lighthouse-core/scripts/assert-golden-lhr-unchanged.sh | 1 - 14 files changed, 16 insertions(+), 24 deletions(-) diff --git a/lighthouse-cli/test/smokehouse/byte-config.js b/lighthouse-cli/test/smokehouse/byte-config.js index 78c58b7fbab2..957b97a40744 100644 --- a/lighthouse-cli/test/smokehouse/byte-config.js +++ b/lighthouse-cli/test/smokehouse/byte-config.js @@ -22,8 +22,7 @@ module.exports = { 'unused-css-rules', 'unused-javascript', ], - - // TODO(phulce): re-write testers to work with faster lantern loading + // TODO: https://github.com/GoogleChrome/lighthouse/issues/7039 throttlingMethod: 'devtools', }, }; diff --git a/lighthouse-cli/test/smokehouse/byte-efficiency/expectations.js b/lighthouse-cli/test/smokehouse/byte-efficiency/expectations.js index 0822e727ca39..17a43fa183a4 100644 --- a/lighthouse-cli/test/smokehouse/byte-efficiency/expectations.js +++ b/lighthouse-cli/test/smokehouse/byte-efficiency/expectations.js @@ -5,8 +5,7 @@ */ 'use strict'; -// TODO(phulce): assert more score values once Lantern can tie more trace events back to images. -// See https://github.com/GoogleChrome/lighthouse/issues/4600. +// TODO: https://github.com/GoogleChrome/lighthouse/issues/7039 /** * Expected Lighthouse audit values for byte efficiency tests diff --git a/lighthouse-core/audits/byte-efficiency/render-blocking-resources.js b/lighthouse-core/audits/byte-efficiency/render-blocking-resources.js index 62d1bde7d2da..3209655940eb 100644 --- a/lighthouse-core/audits/byte-efficiency/render-blocking-resources.js +++ b/lighthouse-core/audits/byte-efficiency/render-blocking-resources.js @@ -103,14 +103,13 @@ class RenderBlockingResources extends Audit { for (const resource of artifacts.TagsBlockingFirstPaint) { // Ignore any resources that finished after observed FCP (they're clearly not render-blocking) if (resource.endTime * 1000 > fcpTsInMs) continue; - // TODO(phulce): beacon these occurences to Sentry to improve FCP graph + // TODO: https://github.com/GoogleChrome/lighthouse/issues/7041 if (!nodesByUrl[resource.tag.url]) continue; const {node, nodeTiming} = nodesByUrl[resource.tag.url]; // Mark this node and all its dependents as deferrable - // TODO(phulce): make this slightly more surgical - // i.e. the referenced font asset won't become inlined just because you inline the CSS + // TODO: https://github.com/GoogleChrome/lighthouse/issues/7040 node.traverse(node => deferredNodeIds.add(node.id)); // "wastedMs" is the download time of the network request, responseReceived - requestSent @@ -190,7 +189,7 @@ class RenderBlockingResources extends Audit { static async computeWastedCSSBytes(artifacts, context) { const wastedBytesByUrl = new Map(); try { - // TODO(phulce): pull this out into computed artifact + // TODO: https://github.com/GoogleChrome/lighthouse/issues/7042 const results = await UnusedCSS.audit(artifacts, context); // @ts-ignore - TODO(bckenny): details types. for (const item of results.details.items) { diff --git a/lighthouse-core/audits/content-width.js b/lighthouse-core/audits/content-width.js index 7de0b62fb11a..2d2007336c26 100644 --- a/lighthouse-core/audits/content-width.js +++ b/lighthouse-core/audits/content-width.js @@ -34,7 +34,7 @@ class ContentWidth extends Audit { const windowWidth = artifacts.ViewportDimensions.outerWidth; const widthsMatch = viewportWidth === windowWidth; - // TODO(phulce): refactor this `isMobile` boolean to be on context + // TODO: https://github.com/GoogleChrome/lighthouse/issues/7043 const isMobileHost = userAgent.includes('Android') || userAgent.includes('Mobile'); const isMobile = context.settings.emulatedFormFactor === 'mobile' || (context.settings.emulatedFormFactor !== 'desktop' && isMobileHost); diff --git a/lighthouse-core/config/config.js b/lighthouse-core/config/config.js index 54cf15322dfb..0209ac190457 100644 --- a/lighthouse-core/config/config.js +++ b/lighthouse-core/config/config.js @@ -184,7 +184,7 @@ function cleanFlagsForSettings(flags = {}) { return settings; } -// TODO(phulce): disentangle this merge function +// TODO: https://github.com/GoogleChrome/lighthouse/issues/7046 /** * More widely typed than exposed merge() function, below. * @param {Object|Array|undefined|null} base diff --git a/lighthouse-core/gather/driver.js b/lighthouse-core/gather/driver.js index a831effb9e82..e1dba6aa3e70 100644 --- a/lighthouse-core/gather/driver.js +++ b/lighthouse-core/gather/driver.js @@ -1241,7 +1241,7 @@ class Driver { * @return {Promise} */ async beginEmulation(settings) { - // TODO(phulce): remove this flag on next breaking change + // TODO: https://github.com/GoogleChrome/lighthouse/issues/7044 if (!settings.disableDeviceEmulation) { if (settings.emulatedFormFactor === 'mobile') { await emulation.enableNexus5X(this); diff --git a/lighthouse-core/gather/gatherers/dobetterweb/optimized-images.js b/lighthouse-core/gather/gatherers/dobetterweb/optimized-images.js index 67484a2fd2e2..6ca6a9a16416 100644 --- a/lighthouse-core/gather/gatherers/dobetterweb/optimized-images.js +++ b/lighthouse-core/gather/gatherers/dobetterweb/optimized-images.js @@ -129,8 +129,6 @@ class OptimizedImages extends Gatherer { * @return {Promise} */ calculateImageStats(driver, networkRecord) { - // TODO(phulce): remove this dance of trying _getEncodedResponse with a fallback when Audits - // domain hits stable in Chrome 62 return Promise.resolve(networkRecord.requestId).then(requestId => { if (this._getEncodedResponseUnsupported) return; return this._getEncodedResponse(driver, requestId, 'jpeg').then(jpegData => { @@ -153,7 +151,7 @@ class OptimizedImages extends Gatherer { }).then(result => { if (result) return result; - // Take the slower fallback path if getEncodedResponse isn't available yet + // Take the slower fallback path if getEncodedResponse didn't work // CORS canvas tainting doesn't support cross-origin images, so skip them early if (!networkRecord.isSameOrigin && !networkRecord.isBase64DataUri) return null; diff --git a/lighthouse-core/gather/gatherers/link-elements.js b/lighthouse-core/gather/gatherers/link-elements.js index 4a74e3abe378..bc6873ed02a6 100644 --- a/lighthouse-core/gather/gatherers/link-elements.js +++ b/lighthouse-core/gather/gatherers/link-elements.js @@ -16,7 +16,8 @@ class LinkElements extends Gatherer { async afterPass(passContext) { const driver = passContext.driver; - // TODO(phulce): merge this with the main resource header values too + // TODO: https://github.com/GoogleChrome/lighthouse/issues/6747 + // We'll use evaluateAsync because the `node.getAttribute` method doesn't actually normalize // the values like access from JavaScript does. return driver.evaluateAsync(`(() => { diff --git a/lighthouse-core/lib/dependency-graph/simulator/network-analyzer.js b/lighthouse-core/lib/dependency-graph/simulator/network-analyzer.js index ef2187df5f9e..713d92b29b23 100644 --- a/lighthouse-core/lib/dependency-graph/simulator/network-analyzer.js +++ b/lighthouse-core/lib/dependency-graph/simulator/network-analyzer.js @@ -289,8 +289,7 @@ class NetworkAnalyzer { ); } - // TODO(phulce): compute the maximum number of parallel requests (N) and ensure we have at - // least N requests that required new connections + // TODO: https://github.com/GoogleChrome/lighthouse/issues/7045 const firstRecord = originRecords.reduce((a, b) => (a.startTime > b.startTime ? b : a)); connectionWasReused.set(firstRecord.requestId, false); } @@ -448,7 +447,7 @@ class NetworkAnalyzer { * @return {LH.Artifacts.NetworkRequest} */ static findMainDocument(records) { - // TODO(phulce): handle more edge cases like client redirects, or plumb through finalUrl + // TODO: https://github.com/GoogleChrome/lighthouse/issues/7045 const documentRequests = records.filter(record => record.resourceType === NetworkRequest.TYPES.Document); // The main document is the earliest document request, using position in networkRecords array to break ties. diff --git a/lighthouse-core/lib/dependency-graph/simulator/simulator.js b/lighthouse-core/lib/dependency-graph/simulator/simulator.js index fe46d21c207e..6b746e7d4442 100644 --- a/lighthouse-core/lib/dependency-graph/simulator/simulator.js +++ b/lighthouse-core/lib/dependency-graph/simulator/simulator.js @@ -106,7 +106,7 @@ class Simulator { this._cachedNodeListByStartTime = []; // NOTE: We don't actually need *all* of these sets, but the clarity that each node progresses // through the system is quite nice. - // TODO(phulce): consider refactoring this so that it's easier to follow + // TODO: https://github.com/GoogleChrome/lighthouse/issues/7048 for (const state of Object.values(NodeState)) { this._nodes[state] = new Set(); } diff --git a/lighthouse-core/lib/network-recorder.js b/lighthouse-core/lib/network-recorder.js index 4de1df7ffe4e..c3da5c480817 100644 --- a/lighthouse-core/lib/network-recorder.js +++ b/lighthouse-core/lib/network-recorder.js @@ -221,7 +221,7 @@ class NetworkRecorder extends EventEmitter { return; } - // TODO(phulce): log these to sentry? + // TODO: https://github.com/GoogleChrome/lighthouse/issues/7041 if (!data.redirectResponse) { return; } @@ -231,7 +231,6 @@ class NetworkRecorder extends EventEmitter { const modifiedData = { ...data, // Copy over the initiator as well to match DevTools behavior - // TODO(phulce): abandon this DT hack and update Lantern graph to handle it initiator: originalRequest.initiator, requestId: `${originalRequest.requestId}:redirect`, }; diff --git a/lighthouse-core/lib/network-request.js b/lighthouse-core/lib/network-request.js index 883a851dfc30..8c5fc69ec033 100644 --- a/lighthouse-core/lib/network-request.js +++ b/lighthouse-core/lib/network-request.js @@ -51,7 +51,6 @@ const RESOURCE_TYPES = { module.exports = class NetworkRequest { constructor() { this.requestId = ''; - // TODO(phulce): remove default DevTools connectionId this.connectionId = '0'; this.connectionReused = false; diff --git a/lighthouse-core/runner.js b/lighthouse-core/runner.js index cb9fb5f02740..745ebb817b7f 100644 --- a/lighthouse-core/runner.js +++ b/lighthouse-core/runner.js @@ -227,7 +227,7 @@ class Runner { const normalizedGatherSettings = Object.assign({}, artifacts.settings, overrides); const normalizedAuditSettings = Object.assign({}, settings, overrides); - // TODO(phulce): allow change of throttling method to `simulate` + // TODO: https://github.com/GoogleChrome/lighthouse/issues/7047 if (!isDeepEqual(normalizedGatherSettings, normalizedAuditSettings)) { throw new Error('Cannot change settings between gathering and auditing'); } diff --git a/lighthouse-core/scripts/assert-golden-lhr-unchanged.sh b/lighthouse-core/scripts/assert-golden-lhr-unchanged.sh index 99bbc750bfbe..7f29084464a3 100644 --- a/lighthouse-core/scripts/assert-golden-lhr-unchanged.sh +++ b/lighthouse-core/scripts/assert-golden-lhr-unchanged.sh @@ -28,7 +28,6 @@ trap teardown EXIT colorText "Generating a fresh LHR..." "$purple" set -x -# TODO(phulce): add a lantern LHR-differ node "$lhroot_path/lighthouse-cli" -A="$lhroot_path/lighthouse-core/test/results/artifacts" --throttling-method=devtools --quiet --output=json --output-path="$freshLHRPath" set +x From d6d5903b553d5e4ea7a80fa7b41ae802acf5f1fe Mon Sep 17 00:00:00 2001 From: Patrick Hulce Date: Thu, 17 Jan 2019 15:29:23 -0600 Subject: [PATCH 2/3] feedback --- lighthouse-core/gather/driver.js | 1 - 1 file changed, 1 deletion(-) diff --git a/lighthouse-core/gather/driver.js b/lighthouse-core/gather/driver.js index e1dba6aa3e70..9fec43fc478b 100644 --- a/lighthouse-core/gather/driver.js +++ b/lighthouse-core/gather/driver.js @@ -1241,7 +1241,6 @@ class Driver { * @return {Promise} */ async beginEmulation(settings) { - // TODO: https://github.com/GoogleChrome/lighthouse/issues/7044 if (!settings.disableDeviceEmulation) { if (settings.emulatedFormFactor === 'mobile') { await emulation.enableNexus5X(this); From 4270ee085b7e93a0a987b99b69e78e1517f67509 Mon Sep 17 00:00:00 2001 From: Patrick Hulce Date: Mon, 21 Jan 2019 08:41:21 -0800 Subject: [PATCH 3/3] remove most of them --- lighthouse-cli/test/smokehouse/byte-config.js | 1 - .../test/smokehouse/byte-efficiency/expectations.js | 2 -- .../audits/byte-efficiency/render-blocking-resources.js | 4 +--- lighthouse-core/audits/content-width.js | 1 - lighthouse-core/config/config.js | 1 - lighthouse-core/gather/gatherers/link-elements.js | 2 -- .../lib/dependency-graph/simulator/network-analyzer.js | 2 -- lighthouse-core/lib/dependency-graph/simulator/simulator.js | 1 - lighthouse-core/lib/network-recorder.js | 2 +- lighthouse-core/runner.js | 1 - 10 files changed, 2 insertions(+), 15 deletions(-) diff --git a/lighthouse-cli/test/smokehouse/byte-config.js b/lighthouse-cli/test/smokehouse/byte-config.js index 957b97a40744..6718ab5263ee 100644 --- a/lighthouse-cli/test/smokehouse/byte-config.js +++ b/lighthouse-cli/test/smokehouse/byte-config.js @@ -22,7 +22,6 @@ module.exports = { 'unused-css-rules', 'unused-javascript', ], - // TODO: https://github.com/GoogleChrome/lighthouse/issues/7039 throttlingMethod: 'devtools', }, }; diff --git a/lighthouse-cli/test/smokehouse/byte-efficiency/expectations.js b/lighthouse-cli/test/smokehouse/byte-efficiency/expectations.js index 17a43fa183a4..d4a379638e7c 100644 --- a/lighthouse-cli/test/smokehouse/byte-efficiency/expectations.js +++ b/lighthouse-cli/test/smokehouse/byte-efficiency/expectations.js @@ -5,8 +5,6 @@ */ 'use strict'; -// TODO: https://github.com/GoogleChrome/lighthouse/issues/7039 - /** * Expected Lighthouse audit values for byte efficiency tests */ diff --git a/lighthouse-core/audits/byte-efficiency/render-blocking-resources.js b/lighthouse-core/audits/byte-efficiency/render-blocking-resources.js index 3209655940eb..5e1e17e65633 100644 --- a/lighthouse-core/audits/byte-efficiency/render-blocking-resources.js +++ b/lighthouse-core/audits/byte-efficiency/render-blocking-resources.js @@ -103,13 +103,12 @@ class RenderBlockingResources extends Audit { for (const resource of artifacts.TagsBlockingFirstPaint) { // Ignore any resources that finished after observed FCP (they're clearly not render-blocking) if (resource.endTime * 1000 > fcpTsInMs) continue; - // TODO: https://github.com/GoogleChrome/lighthouse/issues/7041 + // TODO: beacon to Sentry, https://github.com/GoogleChrome/lighthouse/issues/7041 if (!nodesByUrl[resource.tag.url]) continue; const {node, nodeTiming} = nodesByUrl[resource.tag.url]; // Mark this node and all its dependents as deferrable - // TODO: https://github.com/GoogleChrome/lighthouse/issues/7040 node.traverse(node => deferredNodeIds.add(node.id)); // "wastedMs" is the download time of the network request, responseReceived - requestSent @@ -189,7 +188,6 @@ class RenderBlockingResources extends Audit { static async computeWastedCSSBytes(artifacts, context) { const wastedBytesByUrl = new Map(); try { - // TODO: https://github.com/GoogleChrome/lighthouse/issues/7042 const results = await UnusedCSS.audit(artifacts, context); // @ts-ignore - TODO(bckenny): details types. for (const item of results.details.items) { diff --git a/lighthouse-core/audits/content-width.js b/lighthouse-core/audits/content-width.js index 2d2007336c26..3f4f06a6bb10 100644 --- a/lighthouse-core/audits/content-width.js +++ b/lighthouse-core/audits/content-width.js @@ -34,7 +34,6 @@ class ContentWidth extends Audit { const windowWidth = artifacts.ViewportDimensions.outerWidth; const widthsMatch = viewportWidth === windowWidth; - // TODO: https://github.com/GoogleChrome/lighthouse/issues/7043 const isMobileHost = userAgent.includes('Android') || userAgent.includes('Mobile'); const isMobile = context.settings.emulatedFormFactor === 'mobile' || (context.settings.emulatedFormFactor !== 'desktop' && isMobileHost); diff --git a/lighthouse-core/config/config.js b/lighthouse-core/config/config.js index 0209ac190457..5671e998c499 100644 --- a/lighthouse-core/config/config.js +++ b/lighthouse-core/config/config.js @@ -184,7 +184,6 @@ function cleanFlagsForSettings(flags = {}) { return settings; } -// TODO: https://github.com/GoogleChrome/lighthouse/issues/7046 /** * More widely typed than exposed merge() function, below. * @param {Object|Array|undefined|null} base diff --git a/lighthouse-core/gather/gatherers/link-elements.js b/lighthouse-core/gather/gatherers/link-elements.js index bc6873ed02a6..9283265e00c7 100644 --- a/lighthouse-core/gather/gatherers/link-elements.js +++ b/lighthouse-core/gather/gatherers/link-elements.js @@ -16,8 +16,6 @@ class LinkElements extends Gatherer { async afterPass(passContext) { const driver = passContext.driver; - // TODO: https://github.com/GoogleChrome/lighthouse/issues/6747 - // We'll use evaluateAsync because the `node.getAttribute` method doesn't actually normalize // the values like access from JavaScript does. return driver.evaluateAsync(`(() => { diff --git a/lighthouse-core/lib/dependency-graph/simulator/network-analyzer.js b/lighthouse-core/lib/dependency-graph/simulator/network-analyzer.js index 713d92b29b23..7f2dd6b0d7fe 100644 --- a/lighthouse-core/lib/dependency-graph/simulator/network-analyzer.js +++ b/lighthouse-core/lib/dependency-graph/simulator/network-analyzer.js @@ -289,7 +289,6 @@ class NetworkAnalyzer { ); } - // TODO: https://github.com/GoogleChrome/lighthouse/issues/7045 const firstRecord = originRecords.reduce((a, b) => (a.startTime > b.startTime ? b : a)); connectionWasReused.set(firstRecord.requestId, false); } @@ -447,7 +446,6 @@ class NetworkAnalyzer { * @return {LH.Artifacts.NetworkRequest} */ static findMainDocument(records) { - // TODO: https://github.com/GoogleChrome/lighthouse/issues/7045 const documentRequests = records.filter(record => record.resourceType === NetworkRequest.TYPES.Document); // The main document is the earliest document request, using position in networkRecords array to break ties. diff --git a/lighthouse-core/lib/dependency-graph/simulator/simulator.js b/lighthouse-core/lib/dependency-graph/simulator/simulator.js index 6b746e7d4442..9c66515202e9 100644 --- a/lighthouse-core/lib/dependency-graph/simulator/simulator.js +++ b/lighthouse-core/lib/dependency-graph/simulator/simulator.js @@ -106,7 +106,6 @@ class Simulator { this._cachedNodeListByStartTime = []; // NOTE: We don't actually need *all* of these sets, but the clarity that each node progresses // through the system is quite nice. - // TODO: https://github.com/GoogleChrome/lighthouse/issues/7048 for (const state of Object.values(NodeState)) { this._nodes[state] = new Set(); } diff --git a/lighthouse-core/lib/network-recorder.js b/lighthouse-core/lib/network-recorder.js index c3da5c480817..4264e34eb219 100644 --- a/lighthouse-core/lib/network-recorder.js +++ b/lighthouse-core/lib/network-recorder.js @@ -221,7 +221,7 @@ class NetworkRecorder extends EventEmitter { return; } - // TODO: https://github.com/GoogleChrome/lighthouse/issues/7041 + // TODO: beacon to Sentry, https://github.com/GoogleChrome/lighthouse/issues/7041 if (!data.redirectResponse) { return; } diff --git a/lighthouse-core/runner.js b/lighthouse-core/runner.js index 745ebb817b7f..e08294dc111a 100644 --- a/lighthouse-core/runner.js +++ b/lighthouse-core/runner.js @@ -227,7 +227,6 @@ class Runner { const normalizedGatherSettings = Object.assign({}, artifacts.settings, overrides); const normalizedAuditSettings = Object.assign({}, settings, overrides); - // TODO: https://github.com/GoogleChrome/lighthouse/issues/7047 if (!isDeepEqual(normalizedGatherSettings, normalizedAuditSettings)) { throw new Error('Cannot change settings between gathering and auditing'); }