Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

misc: replace TODOs with github issue links #7049

Merged
merged 3 commits into from
Jan 22, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 0 additions & 2 deletions lighthouse-cli/test/smokehouse/byte-config.js
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,6 @@ module.exports = {
'unused-css-rules',
'unused-javascript',
],

// TODO(phulce): re-write testers to work with faster lantern loading
throttlingMethod: 'devtools',
},
};
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,6 @@
*/
'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.

/**
* Expected Lighthouse audit values for byte efficiency tests
*/
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -103,14 +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(phulce): beacon these occurences to Sentry to improve FCP graph
// 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(phulce): make this slightly more surgical
// i.e. the referenced font asset won't become inlined just because you inline the CSS
node.traverse(node => deferredNodeIds.add(node.id));

// "wastedMs" is the download time of the network request, responseReceived - requestSent
Expand Down Expand Up @@ -190,7 +188,6 @@ class RenderBlockingResources extends Audit {
static async computeWastedCSSBytes(artifacts, context) {
const wastedBytesByUrl = new Map();
try {
// TODO(phulce): pull this out into computed artifact
const results = await UnusedCSS.audit(artifacts, context);
// @ts-ignore - TODO(bckenny): details types.
for (const item of results.details.items) {
Expand Down
1 change: 0 additions & 1 deletion lighthouse-core/audits/content-width.js
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,6 @@ class ContentWidth extends Audit {
const windowWidth = artifacts.ViewportDimensions.outerWidth;
const widthsMatch = viewportWidth === windowWidth;

// TODO(phulce): refactor this `isMobile` boolean to be on context
const isMobileHost = userAgent.includes('Android') || userAgent.includes('Mobile');
const isMobile = context.settings.emulatedFormFactor === 'mobile' ||
(context.settings.emulatedFormFactor !== 'desktop' && isMobileHost);
Expand Down
1 change: 0 additions & 1 deletion lighthouse-core/config/config.js
Original file line number Diff line number Diff line change
Expand Up @@ -184,7 +184,6 @@ function cleanFlagsForSettings(flags = {}) {
return settings;
}

// TODO(phulce): disentangle this merge function
/**
* More widely typed than exposed merge() function, below.
* @param {Object<string, any>|Array<any>|undefined|null} base
Expand Down
1 change: 0 additions & 1 deletion lighthouse-core/gather/driver.js
Original file line number Diff line number Diff line change
Expand Up @@ -1241,7 +1241,6 @@ class Driver {
* @return {Promise<void>}
*/
async beginEmulation(settings) {
// TODO(phulce): remove this flag on next breaking change
if (!settings.disableDeviceEmulation) {
if (settings.emulatedFormFactor === 'mobile') {
await emulation.enableNexus5X(this);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -129,8 +129,6 @@ class OptimizedImages extends Gatherer {
* @return {Promise<?{fromProtocol: boolean, originalSize: number, jpegSize: number, webpSize: number}>}
*/
calculateImageStats(driver, networkRecord) {
// TODO(phulce): remove this dance of trying _getEncodedResponse with a fallback when Audits
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

at this point, I'm not sure it makes sense. It's a solid fallback option for when the protocol fails.

// 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 => {
Expand All @@ -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;

Expand Down
1 change: 0 additions & 1 deletion lighthouse-core/gather/gatherers/link-elements.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@ class LinkElements extends Gatherer {
async afterPass(passContext) {
const driver = passContext.driver;

// TODO(phulce): merge this with the main resource header values too
// We'll use evaluateAsync because the `node.getAttribute` method doesn't actually normalize
// the values like access from JavaScript does.
return driver.evaluateAsync(`(() => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -289,8 +289,6 @@ class NetworkAnalyzer {
);
}

// TODO(phulce): compute the maximum number of parallel requests (N) and ensure we have at
// least N requests that required new connections
const firstRecord = originRecords.reduce((a, b) => (a.startTime > b.startTime ? b : a));
connectionWasReused.set(firstRecord.requestId, false);
}
Expand Down Expand Up @@ -448,7 +446,6 @@ class NetworkAnalyzer {
* @return {LH.Artifacts.NetworkRequest}
*/
static findMainDocument(records) {
// TODO(phulce): handle more edge cases like client redirects, or plumb through finalUrl
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.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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(phulce): consider refactoring this so that it's easier to follow
for (const state of Object.values(NodeState)) {
this._nodes[state] = new Set();
}
Expand Down
3 changes: 1 addition & 2 deletions lighthouse-core/lib/network-recorder.js
Original file line number Diff line number Diff line change
Expand Up @@ -221,7 +221,7 @@ class NetworkRecorder extends EventEmitter {
return;
}

// TODO(phulce): log these to sentry?
// TODO: beacon to Sentry, https://github.com/GoogleChrome/lighthouse/issues/7041
if (!data.redirectResponse) {
return;
}
Expand All @@ -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
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not convinced this is necessary anymore, it's probably easier to just stay in sync with wht devtools does

initiator: originalRequest.initiator,
requestId: `${originalRequest.requestId}:redirect`,
};
Expand Down
1 change: 0 additions & 1 deletion lighthouse-core/lib/network-request.js
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,6 @@ const RESOURCE_TYPES = {
module.exports = class NetworkRequest {
constructor() {
this.requestId = '';
// TODO(phulce): remove default DevTools connectionId
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not convinced this is necessary anymore

this.connectionId = '0';
this.connectionReused = false;

Expand Down
1 change: 0 additions & 1 deletion lighthouse-core/runner.js
Original file line number Diff line number Diff line change
Expand Up @@ -227,7 +227,6 @@ class Runner {
const normalizedGatherSettings = Object.assign({}, artifacts.settings, overrides);
const normalizedAuditSettings = Object.assign({}, settings, overrides);

// TODO(phulce): allow change of throttling method to `simulate`
if (!isDeepEqual(normalizedGatherSettings, normalizedAuditSettings)) {
throw new Error('Cannot change settings between gathering and auditing');
}
Expand Down
1 change: 0 additions & 1 deletion lighthouse-core/scripts/assert-golden-lhr-unchanged.sh
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,6 @@ trap teardown EXIT

colorText "Generating a fresh LHR..." "$purple"
set -x
# TODO(phulce): add a lantern LHR-differ
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not convinced this is necessary anymore

node "$lhroot_path/lighthouse-cli" -A="$lhroot_path/lighthouse-core/test/results/artifacts" --throttling-method=devtools --quiet --output=json --output-path="$freshLHRPath"
set +x

Expand Down