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

Check for proper mimetype in tags-that-block audit. #1432

Merged
merged 4 commits into from
Jan 10, 2017
Merged

Check for proper mimetype in tags-that-block audit. #1432

merged 4 commits into from
Jan 10, 2017

Conversation

ebidel
Copy link
Contributor

@ebidel ebidel commented Jan 7, 2017

R: @patrickhulce all

Resources that fail or are missing a mimetype are now filtered out of the blocking link/scripts gatherer. This also changes the dbw_tester to not wrap these resources in a <template> so their parser initiated and not script initiated.

See #1431 for a full explanation.

@ebidel ebidel changed the title Filter out 404 links/scripts from tags that block audit. Filter out 404 links/scripts from tags that block audit. They don't block. Jan 7, 2017
@patrickhulce
Copy link
Collaborator

Technically wouldn't a script or stylesheet that 404s still block first paint until the failed request came back as 404? Probably not a big deal over h2, but maybe still worth reporting?

Thanks for fixing! Funny that the DBW test scripts were never actually blocking

@ebidel
Copy link
Contributor Author

ebidel commented Jan 9, 2017

Yea, true. Let me remove the 404 check.

@ebidel
Copy link
Contributor Author

ebidel commented Jan 9, 2017

PTAL. Added a comment on the ?delay param.

@ebidel
Copy link
Contributor Author

ebidel commented Jan 9, 2017

Results are correct:

screen shot 2017-01-09 at 10 15 48 am

Copy link
Collaborator

@patrickhulce patrickhulce left a comment

Choose a reason for hiding this comment

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

awesome thanks!

@brendankenny
Copy link
Member

Should we still report on the 404 assets? Waiting for the 404 response could still take a while in a mobile environment.

(plus it's not bad to warn about 404ed assets; OTOH maybe it's more appropriate for a separate 404 audit or something)

@ebidel
Copy link
Contributor Author

ebidel commented Jan 9, 2017

Should we still report on the 404 assets?

We can (probably should for completeness), but what do you think the behavior should be? record._mimeType == undefined in those cases, so we can't proceed with the rest of the new "is parser generated" checks the audit does.

I was also thinking a separate 404/"wasted resources" audit would be useful.

@patrickhulce
Copy link
Collaborator

record._mimeType == undefined in those cases, so we can't proceed with the rest of the new "is parser generated" checks the audit does.

Is the initiator information still there? We could still link it up with the tags later on and shift the logic down there (for parser initiator)

@ebidel
Copy link
Contributor Author

ebidel commented Jan 9, 2017

PTAL. Like pokemon, we're collecting all the 404s now:

screen shot 2017-01-09 at 2 32 29 pm

Nice. The 404s are blocking paint by a non-trivial amount!

@@ -91,19 +91,25 @@ function collectTagsThatBlockFirstPaint() {

function filteredAndIndexedByUrl(networkRecords) {
return networkRecords.reduce((prev, record) => {
// Filter stylesheet, javascript, and html import mimetypes.
const isHtml = record._mimeType.indexOf('html') > -1;
console.log(record._initiator.type, record._mimeType, record._url)
Copy link
Collaborator

Choose a reason for hiding this comment

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

remove?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

haha yep.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@ebidel
Copy link
Contributor Author

ebidel commented Jan 10, 2017

PTAL

@ebidel ebidel changed the title Filter out 404 links/scripts from tags that block audit. They don't block. Check for proper mimetype i tags that block audit. Jan 10, 2017
@ebidel ebidel changed the title Check for proper mimetype i tags that block audit. Check for proper mimetype in tags-that-block audit. Jan 10, 2017
@patrickhulce
Copy link
Collaborator

LGTM

@ebidel ebidel merged commit 929d0a8 into master Jan 10, 2017
@ebidel ebidel deleted the 1431 branch January 10, 2017 05:35
weiwei-lin pushed a commit to weiwei-lin/lighthouse that referenced this pull request Jan 12, 2017
* Filter out links/scripts that 404 from tags that block audit. Part of GoogleChrome#1431

* Remove 404 check and add comment

* Include 404 scripts/links in blocking resources

* remove console.log
andrewrota pushed a commit to andrewrota/lighthouse that referenced this pull request Jan 13, 2017
* Filter out links/scripts that 404 from tags that block audit. Part of GoogleChrome#1431

* Remove 404 check and add comment

* Include 404 scripts/links in blocking resources

* remove console.log
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants