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

Remove images from critical request chains. #2085

Merged
merged 1 commit into from
May 9, 2017

Conversation

abacon
Copy link
Contributor

@abacon abacon commented Apr 27, 2017

Fixes #1550. Hope this is still useful!

@wardpeet
Copy link
Collaborator

Thanks for the contribution!

You should have a look at the smokehouse test as they are failing. If you need help just give a shout.

@abacon
Copy link
Contributor Author

abacon commented May 8, 2017

Hi @wardpeet! Thanks so much for your patience with this. I've modified the tests so that they pass, but would you mind checking to make sure that I'm not creating any gaps in coverage?

@@ -38,6 +38,10 @@ class CriticalRequestChains extends ComputedArtifact {
if (resourceTypeCategory === WebInspector.resourceTypes.XHR._category) {
return false;
}
// Images are also non-critical.
if (resourceTypeCategory === WebInspector.resourceTypes.Image._category) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if it would be cleaner to do an array of resourceTypes that are non-critical and check includes on it similar to request priority

Copy link
Member

Choose a reason for hiding this comment

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

good idea. let's do that @abacon

Copy link
Member

Choose a reason for hiding this comment

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

@abacon can you do this in a followup PR?

I'll land this now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, no problem! Thanks for merging. 😍💖💪

Copy link
Contributor

Choose a reason for hiding this comment

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

@abacon Sorry for stealing your thunder on the follow-up PR. I covered it in #2191 already. Thanks for the PR 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nah it's all good! Thanks so much @stramel :)

@@ -38,6 +38,10 @@ class CriticalRequestChains extends ComputedArtifact {
if (resourceTypeCategory === WebInspector.resourceTypes.XHR._category) {
return false;
}
// Images are also non-critical.
if (resourceTypeCategory === WebInspector.resourceTypes.Image._category) {
Copy link
Member

Choose a reason for hiding this comment

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

@abacon can you do this in a followup PR?

I'll land this now.

@paulirish paulirish merged commit b5bf067 into GoogleChrome:master May 9, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants