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

core(network-request): consider secondary headers for content encoded check #15708

Merged
merged 2 commits into from
Dec 19, 2023
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
13 changes: 1 addition & 12 deletions core/gather/gatherers/dobetterweb/response-compression.js
Original file line number Diff line number Diff line change
Expand Up @@ -23,12 +23,6 @@ import {fetchResponseBodyFromCache} from '../../driver/network.js';
import {NetworkRecords} from '../../../computed/network-records.js';

const CHROME_EXTENSION_PROTOCOL = 'chrome-extension:';
const compressionHeaders = [
'content-encoding',
'x-original-content-encoding',
'x-content-encoding-over-network',
];
const compressionTypes = ['gzip', 'br', 'deflate'];
const binaryMimeTypes = ['image', 'audio', 'video'];
/** @type {LH.Crdp.Network.ResourceType[]} */
const textResourceTypes = [
Expand Down Expand Up @@ -71,12 +65,7 @@ class ResponseCompression extends BaseGatherer {
return;
}

const isContentEncoded = (record.responseHeaders || []).find(header =>
compressionHeaders.includes(header.name.toLowerCase()) &&
compressionTypes.includes(header.value)
);

if (!isContentEncoded) {
if (!NetworkRequest.isContentEncoded(record)) {
unoptimizedResponses.push({
requestId: record.requestId,
url: record.url,
Expand Down
12 changes: 10 additions & 2 deletions core/lib/network-request.js
Original file line number Diff line number Diff line change
Expand Up @@ -616,8 +616,16 @@
static isContentEncoded(record) {
// FYI: older devtools logs (like our test fixtures) seems to be lower case, while modern logs
// are Cased-Like-This.
const pattern = /^content-encoding$/i;
return record.responseHeaders.some(item => item.name.match(pattern));
const patterns = global.isLightrider ? [
/^x-original-content-encoding$/i,

Check warning on line 620 in core/lib/network-request.js

View check run for this annotation

Codecov / codecov/patch

core/lib/network-request.js#L620

Added line #L620 was not covered by tests
] : [
/^content-encoding$/i,
/^x-content-encoding-over-network$/i,
];
const compressionTypes = ['gzip', 'br', 'deflate'];
return record.responseHeaders.some(header =>
patterns.some(p => header.name.match(p)) && compressionTypes.includes(header.value)
);
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ const KB = 1024;
const resourceType = 'Script';
describe('Page uses optimized responses', () => {
it('fails when given unminified scripts', () => {
const responseHeaders = [{name: 'Content-Encoding'}];
const responseHeaders = [{name: 'Content-Encoding', value: 'gzip'}];
const commonRecord = {resourceType, responseHeaders};
const auditResult = UnminifiedJavascriptAudit.audit_({
URL: {finalDisplayedUrl: 'https://www.example.com'},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,7 @@ describe('Best Practices: unused css rules audit', () => {
transferSize: 100 * 1024 * 0.5, // compression ratio of 0.5
resourceSize: 100 * 1024,
resourceType: 'Document', // this is a document so it'll use the compressionRatio but not the raw size
responseHeaders: [{name: 'Content-Encoding'}],
responseHeaders: [{name: 'Content-Encoding', value: 'gzip'}],
},
{
url: 'file://a.html',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,9 @@ function getScriptId(url) {
* @param {LH.Crdp.Network.ResourceType} resourceType
*/
function generateRecord(url, transferSize, resourceType) {
return {url, transferSize, resourceType, responseHeaders: [{name: 'Content-Encoding'}]};
return {url, transferSize, resourceType, responseHeaders: [
{name: 'Content-Encoding', value: 'gzip'},
]};
}

/**
Expand Down
4 changes: 2 additions & 2 deletions core/test/computed/metrics/time-to-first-byte-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ function mockNetworkRecords() {
transferSize: 300,
url: requestedUrl,
frameId: 'ROOT_FRAME',
responseHeaders: [{name: 'Content-Encoding'}],
responseHeaders: [{name: 'Content-Encoding', value: 'gzip'}],
},
{
requestId: '2:redirect',
Expand All @@ -59,7 +59,7 @@ function mockNetworkRecords() {
transferSize: 16_000,
url: mainDocumentUrl,
frameId: 'ROOT_FRAME',
responseHeaders: [{name: 'Content-Encoding'}],
responseHeaders: [{name: 'Content-Encoding', value: 'gzip'}],
}];
}

Expand Down
38 changes: 26 additions & 12 deletions core/test/lib/network-recorder-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import assert from 'assert/strict';
import {NetworkRecorder} from '../../lib/network-recorder.js';
import {networkRecordsToDevtoolsLog} from '../network-records-to-devtools-log.js';
import {readJson} from '../test-utils.js';
import {NetworkRequest} from '../../lib/network-request.js';

const devtoolsLogItems = readJson('../fixtures/artifacts/perflog/defaultPass.devtoolslog.json', import.meta);
const prefetchedScriptDevtoolsLog = readJson('../fixtures/prefetched-script.devtoolslog.json', import.meta);
Expand Down Expand Up @@ -165,20 +166,33 @@ describe('network recorder', function() {
expect(records).toMatchObject([{url: 'http://example.com', networkRequestTime: 1, networkEndTime: 2}]);
});

it('should use X-TotalFetchedSize in Lightrider for transferSize', () => {
global.isLightrider = true;
const records = NetworkRecorder.recordsFromLogs(lrRequestDevtoolsLog);
global.isLightrider = false;
describe('Lightrider', () => {
let records;
before(() => {
global.isLightrider = true;
records = NetworkRecorder.recordsFromLogs(lrRequestDevtoolsLog);
});

it('should use X-TotalFetchedSize in Lightrider for transferSize', () => {
expect(records.find(r => r.url === 'https://www.paulirish.com/'))
.toMatchObject({
resourceSize: 75221,
transferSize: 22889,
});
expect(records.find(r => r.url === 'https://www.paulirish.com/javascripts/modernizr-2.0.js'))
.toMatchObject({
resourceSize: 9736,
transferSize: 4730,
});
});

expect(records.find(r => r.url === 'https://www.paulirish.com/'))
.toMatchObject({
resourceSize: 75221,
transferSize: 22889,
it('should use respect X-Original-Content-Encoding', () => {
const record = records.find(r => r.url === 'https://www.paulirish.com/javascripts/modernizr-2.0.js');
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

added this test

expect(NetworkRequest.isContentEncoded(record)).toBe(true);
});
expect(records.find(r => r.url === 'https://www.paulirish.com/javascripts/modernizr-2.0.js'))
.toMatchObject({
resourceSize: 9736,
transferSize: 4730,

after(() => {
global.isLightrider = false;
});
});

Expand Down
2 changes: 1 addition & 1 deletion core/test/lib/script-helpers-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ describe('Script helpers', () => {

describe('#estimateCompressedContentSize', () => {
const estimate = estimateCompressedContentSize;
const encoding = [{name: 'Content-Encoding'}];
const encoding = [{name: 'Content-Encoding', value: 'gzip'}];

it('should estimate by resource type compression ratio when no network info available', () => {
assert.equal(estimate(undefined, 1000, 'Stylesheet'), 200);
Expand Down
Loading