Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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(third-party-summary): add blocking time impact #9486
core(third-party-summary): add blocking time impact #9486
Changes from 6 commits
d70939c
e434fd4
bbd5328
f46e441
9db995b
782ad13
d55a822
676b9b0
9b36046
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there might have been an issue with describing the "main thread" to the tc... @exterkamp? Or was it for this audit and it's already been clarified :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem was with "thread" being used, in regards to the description in
first-interactive
I think this is fine here. Maybe add something about threads. But I think that is unnecessary. This LGTM.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Plurals must be totally contained in the
{}
. #9460 allows replacements in plurals like this. But I don't want to add more strings that violate this policy (I get a lot of emails saying this is against policy).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would also expand on "third-party" with something like
1 third-party script blocked...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I totally forgot about this whole fiasco, honestly I don't think the number of third parties needs to be communicated here if it causes trouble
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe put some version of the comment in the PR description
in here to give motivation to the particular number?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah sg
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this a TODO or worth reexamining at some point?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO not a TODO ever worth fixing since it should be rare that third party code actually blocks FCP and if it does I'm fine pointing the finger at it anyway. Our slight decoupling of the audit from TBT in external descriptions also makes this less of an issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just interesting to see the impact here drop (104ms -> 18ms)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah I'm a bit disappointed to see the very busy third parties disappear, but if it's an incentive to script responsibly than I guess it's still a very positive outcome so I'm :)
My biggest concern is that no one is going to understand what "main-thread blocking time" actually is and they'll assume it's the time spent which would be sad.
FWIW this is also not as drastic in the lantern case, it goes from 419 -> 250. While still a big drop, the number is still noticeable.