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

core(render-blocking): address followup feedback #5039

Merged
merged 2 commits into from
Apr 27, 2018
Merged

core(render-blocking): address followup feedback #5039

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d191ce1
core(render-blocking): address followup feedback
patrickhulce Apr 25, 2018
cbd92e3
another todo
patrickhulce Apr 27, 2018
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
17 changes: 10 additions & 7 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 @@ -49,7 +49,9 @@ class RenderBlockingResources extends Audit {
'Resources are blocking the first paint of your page. Consider ' +
'delivering critical JS/CSS inline and deferring all non-critical ' +
'JS/styles. [Learn more](https://developers.google.com/web/tools/lighthouse/audits/blocking-resources).',
requiredArtifacts: ['CSSUsage', 'URL', 'TagsBlockingFirstPaint', 'traces'],
// This audit also looks at CSSUsage but has a graceful fallback if it failed, so do not mark
// it as a "requiredArtifact".
requiredArtifacts: ['URL', 'TagsBlockingFirstPaint', 'traces'],
Copy link
Member

Choose a reason for hiding this comment

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

I like this for working around gatherer errors, but won't this be undesirable if doing an --only-categories/audits? The CSSUsage gatherer could be pruned. It won't be a failure but probably not what you want.

Time to introduce a desiredArtifacts field? :D

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is also true. I have a slight preference for eliminating the failure case in exchange for making the onlyAudits: ['render-blocking-resource'] case a little more forgiving.

I'll add a todo to look into updating after an optionalArtifacts :D

Copy link
Member

Choose a reason for hiding this comment

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

Just to be clear, no changes then? (except for a TODO)

if so, sgtm :)

};
}

Expand All @@ -63,7 +65,7 @@ class RenderBlockingResources extends Audit {
const simulatorData = {devtoolsLog, settings: context.settings};
const traceOfTab = await artifacts.requestTraceOfTab(trace);
const simulator = await artifacts.requestLoadSimulator(simulatorData);
const wastedBytesMap = await RenderBlockingResources.computeWastedCSSBytes(artifacts, context);
const wastedCssBytes = await RenderBlockingResources.computeWastedCSSBytes(artifacts, context);

const metricSettings = {throttlingMethod: 'simulate'};
const metricComputationData = {trace, devtoolsLog, simulator, settings: metricSettings};
Expand All @@ -82,7 +84,7 @@ class RenderBlockingResources extends Audit {

const {node, nodeTiming} = nodesByUrl[resource.tag.url];

// Mark this node and all it's dependents as deferrable
// 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));
Expand All @@ -106,7 +108,7 @@ class RenderBlockingResources extends Audit {
simulator,
fcpSimulation.optimisticGraph,
deferredNodeIds,
wastedBytesMap
wastedCssBytes
);

return {results, wastedMs};
Expand All @@ -125,10 +127,10 @@ class RenderBlockingResources extends Audit {
* @param {Simulator} simulator
* @param {Node} fcpGraph
* @param {Set<string>} deferredIds
* @param {Map<string, number>} wastedBytesMap
* @param {Map<string, number>} wastedCssBytesByUrl
* @return {number}
*/
static estimateSavingsWithGraphs(simulator, fcpGraph, deferredIds, wastedBytesMap) {
static estimateSavingsWithGraphs(simulator, fcpGraph, deferredIds, wastedCssBytesByUrl) {
const originalEstimate = simulator.simulate(fcpGraph).timeInMs;

let totalChildNetworkBytes = 0;
Expand All @@ -139,7 +141,7 @@ class RenderBlockingResources extends Audit {
node.record._resourceType === WebInspector.resourceTypes.Stylesheet;
if (canDeferRequest && isStylesheet) {
// We'll inline the used bytes of the stylesheet and assume the rest can be deferred
const wastedBytes = wastedBytesMap.get(node.record.url) || 0;
const wastedBytes = wastedCssBytesByUrl.get(node.record.url) || 0;
totalChildNetworkBytes += node.record._transferSize - wastedBytes;
}

Expand All @@ -162,6 +164,7 @@ 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);
for (const item of results.details.items) {
wastedBytesByUrl.set(item.url, item.wastedBytes);
Expand Down