core(dbw response compression): Exclude non binary files from auditing by FadySamirSadek · Pull Request #4144 · GoogleChrome/lighthouse

FadySamirSadek · 2017-12-30T23:05:25Z

I have followed the recommended approach but I am wondering if there is a better way to know the type of the file and if the extensions I added are enough and if not what can I refer to in order to get a full list of extensions.
#4039

wardpeet · 2018-01-01T21:59:59Z

lighthouse-core/gather/gatherers/dobetterweb/response-compression.js

      if (!isTextBasedResource || !record.resourceSize || !record.finished ||
-        isChromeExtensionResource || !record.transferSize || record.statusCode === 304) {
+          isChromeExtensionResource || !record.transferSize || record.statusCode === 304
+          ||isMediaResource) {


nit: add a space between ||isMediaResource => || isMediaResource

wardpeet · 2018-01-01T22:07:57Z

lighthouse-core/gather/gatherers/dobetterweb/response-compression.js


    networkRecords.forEach(record => {
      const isTextBasedResource = record.resourceType() && record.resourceType().isTextType();
+      const url = record.url.toLowerCase();


const isMediaResource = (record.resourceType() === WebInspector.resourceTypes.Media || record.resourceType() === WebInspector.resourceTypes.Image; is probably a better approach.

Although I think we want svgs to be compressed at they are text based and not binary based.

in this case we're already being fooled by what Chrome thinks the resource type is unfortunately, let's try to add a mime type sanity check instead :)

patrickhulce

thanks so much for picking this one up @FadySamirSadek! great thinking for an exhaustive list of binary extensions!

I left a few hints as to what the issue was referring to with a mime type sanity check that should help make this a little more robust.

patrickhulce · 2018-01-02T17:43:10Z

lighthouse-core/gather/gatherers/dobetterweb/response-compression.js

 const gzip = require('zlib').gzip;

 const compressionTypes = ['gzip', 'br', 'deflate'];
+const mediaTypes = ['mp3', 'wav', 'wma', 'webm', 'jpg', 'gif', 'png', 'webp',


how about we changes this to a set of banned mime types instead of extensions

const binaryMimeTypes = ['image', 'audio', 'video'];

patrickhulce · 2018-01-02T17:43:47Z

lighthouse-core/gather/gatherers/dobetterweb/response-compression.js

-   * @param {!NetworkRecords} networkRecords
-   * @return {!Array<{url: string, isBase64DataUri: boolean, mimeType: string, resourceSize: number}>}
-   */
+     * @param {!NetworkRecords} networkRecords


nit: let's revert these changes to keep it as it was :)

patrickhulce · 2018-01-02T17:47:44Z

lighthouse-core/gather/gatherers/dobetterweb/response-compression.js


    networkRecords.forEach(record => {
      const isTextBasedResource = record.resourceType() && record.resourceType().isTextType();
+      const url = record.url.toLowerCase();


in this case we're already being fooled by what Chrome thinks the resource type is unfortunately, let's try to add a mime type sanity check instead :)

patrickhulce · 2018-01-02T17:48:08Z

lighthouse-core/gather/gatherers/dobetterweb/response-compression.js

    networkRecords.forEach(record => {
      const isTextBasedResource = record.resourceType() && record.resourceType().isTextType();
+      const url = record.url.toLowerCase();
+      let isMediaResource;


once we have a set of mime types we can combine them into the check for isTextBasedResource since that's what we're really getting at

const isBinaryResource = record.mimeType && binaryMimeTypes.some(mimeType => record.mimeType.startsWith(mimeType)); const isTextBasedResource = !isBinaryResource && record.resourceType() && ...

patrickhulce · 2018-01-02T17:48:34Z

lighthouse-core/gather/gatherers/dobetterweb/response-compression.js

      const isContentEncoded = record.responseHeaders.find(header =>
-        header.name.toLowerCase() === 'content-encoding' &&
-        compressionTypes.includes(header.value)
+          header.name.toLowerCase() === 'content-encoding' &&


nit: let's revert this change too

patrickhulce · 2018-01-16T22:24:19Z

hey @FadySamirSadek thanks for starting this! do you think you have time to carry it across the finish line? 🐎 🏁

if not no worries, we have another contributor interested in picking this up :)

FadySamirSadek · 2018-01-17T05:17:50Z

@patrickhulce yes will make progress on it today

wardpeet · 2018-01-26T21:26:10Z

@FadySamirSadek any progress? Sorry to keep nagging but it would be awesome if we could land this! 💪

FadySamirSadek · 2018-01-27T15:10:52Z

@wardpeet @patrickhulce Thank you for keeping me on this , sorry for making you wait that long for a simple issue. Please provide me with any feedback or changes you request.

patrickhulce

awesome! mind adding a quick test case to

lighthouse/lighthouse-core/test/gather/gatherers/dobetterweb/response-compression-test.js

Lines 125 to 132 in c225d01

    
           it('returns only text and non encoded responses', () => { 
        
             return responseCompression.afterPass(options, createNetworkRequests(traceData)) 
        
               .then(artifact => { 
        
                 assert.equal(artifact.length, 2); 
        
                 assert.ok(/index\.css$/.test(artifact[0].url)); 
        
                 assert.ok(/index\.json$/.test(artifact[1].url)); 
        
               }); 
        
           });

and then I think we're good here!

patrickhulce · 2018-01-29T16:27:33Z

lighthouse-core/gather/gatherers/dobetterweb/response-compression.js

    networkRecords.forEach(record => {
-      const isTextBasedResource = record.resourceType() && record.resourceType().isTextType();
+      const isBinaryResource = record.mimeType &&
+      binaryMimeTypes.some(mimeType => record.mimeType.startsWith(mimeType));


nit: let's indent this line

patrickhulce · 2018-01-29T16:27:42Z

lighthouse-core/gather/gatherers/dobetterweb/response-compression.js

+      const isBinaryResource = record.mimeType &&
+      binaryMimeTypes.some(mimeType => record.mimeType.startsWith(mimeType));
+      const isTextBasedResource = !isBinaryResource && record.resourceType() &&
+      record.resourceType().isTextType();


nit: this one too :)

paulirish · 2018-02-07T22:20:55Z

feedback addressed. @patrickhulce ptal.

Excluded media from response compression

453789e

devtools-bot added the waiting4reviewer label Dec 30, 2017

Fixed indentation

9521214

wardpeet reviewed Jan 1, 2018

View reviewed changes

GoogleChrome deleted a comment Jan 1, 2018

patrickhulce suggested changes Jan 2, 2018

View reviewed changes

patrickhulce added waiting4committer and removed waiting4reviewer labels Jan 2, 2018

paulirish mentioned this pull request Jan 16, 2018

Text compression test should not run for binary files #4039

Closed

FadySamirSadek added 2 commits January 27, 2018 16:33

core(dbw response compression): Excluding binary requested changes

0fa4a63

core(dbw response compression): Excluding binary fixed indentation

53cf22c

core(dbw response compression): Reverted wrong indentation

bdc6aa9

patrickhulce reviewed Jan 29, 2018

View reviewed changes

devtools-bot added waiting4reviewer and removed waiting4committer labels Jan 29, 2018

feedback.

edae6b0

paulirish added waiting4committer and removed waiting4reviewer labels Feb 7, 2018

GoogleChrome deleted a comment from googlebot Feb 7, 2018

patrickhulce approved these changes Feb 7, 2018

View reviewed changes

paulirish merged commit 3131106 into GoogleChrome:master Feb 7, 2018

FadySamirSadek deleted the exclude-media-response-compression branch February 8, 2018 07:00

	it('returns only text and non encoded responses', () => {
	return responseCompression.afterPass(options, createNetworkRequests(traceData))
	.then(artifact => {
	assert.equal(artifact.length, 2);
	assert.ok(/index\.css$/.test(artifact[0].url));
	assert.ok(/index\.json$/.test(artifact[1].url));
	});
	});

Conversation

FadySamirSadek commented Dec 30, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wardpeet Jan 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

patrickhulce left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

patrickhulce commented Jan 16, 2018

Uh oh!

FadySamirSadek commented Jan 17, 2018

Uh oh!

wardpeet commented Jan 26, 2018

Uh oh!

FadySamirSadek commented Jan 27, 2018

Uh oh!

patrickhulce left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

paulirish commented Feb 7, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

wardpeet Jan 1, 2018 •

edited

Loading