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.

Sign up for GitHub

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

Jump to bottom

misc: replace TODOs with github issue links #7049

Merged
merged 3 commits into from
Jan 22, 2019
Merged

misc: replace TODOs with github issue links #7049

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3ae16a5
misc: replace TODOs with github issue links
patrickhulce Jan 17, 2019
d6d5903
feedback
patrickhulce Jan 17, 2019
4270ee0
remove most of them
patrickhulce Jan 21, 2019
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
3 changes: 1 addition & 2 deletions lighthouse-cli/test/smokehouse/byte-config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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',
},
};
3 changes: 1 addition & 2 deletions lighthouse-cli/test/smokehouse/byte-efficiency/expectations.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
7 changes: 3 additions & 4 deletions lighthouse-core/audits/byte-efficiency/render-blocking-resources.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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) {
Expand Down
2 changes: 1 addition & 1 deletion lighthouse-core/audits/content-width.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
2 changes: 1 addition & 1 deletion lighthouse-core/config/config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<string, any>|Array<any>|undefined|null} base
Expand Down
2 changes: 1 addition & 1 deletion lighthouse-core/gather/driver.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1241,7 +1241,7 @@ class Driver {
* @return {Promise<void>}
*/
async beginEmulation(settings) {
// TODO(phulce): remove this flag on next breaking change
// TODO: https://github.com/GoogleChrome/lighthouse/issues/7044
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure if it's worth keeping the TODO for things like this that aren't so targeted at a single point (or that single point is easy to find again since the goal is to remove something altogether)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

removed

if (!settings.disableDeviceEmulation) {
if (settings.emulatedFormFactor === 'mobile') {
await emulation.enableNexus5X(this);
Expand Down
4 changes: 1 addition & 3 deletions lighthouse-core/gather/gatherers/dobetterweb/optimized-images.js
View file
Open in desktop
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
3 changes: 2 additions & 1 deletion lighthouse-core/gather/gatherers/link-elements.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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(`(() => {
Expand Down
5 changes: 2 additions & 3 deletions lighthouse-core/lib/dependency-graph/simulator/network-analyzer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand Down Expand Up @@ -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.
Expand Down
2 changes: 1 addition & 1 deletion lighthouse-core/lib/dependency-graph/simulator/simulator.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
}
Expand Down
3 changes: 1 addition & 2 deletions lighthouse-core/lib/network-recorder.js
View file
Open in desktop
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: 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
View file
Open in desktop
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
2 changes: 1 addition & 1 deletion lighthouse-core/runner.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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');
}
Expand Down
1 change: 0 additions & 1 deletion lighthouse-core/scripts/assert-golden-lhr-unchanged.sh
View file
Open in desktop
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