-
Notifications
You must be signed in to change notification settings - Fork 9.5k
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
new_audit: report animations not run on compositor #11105
Conversation
super exciting stuff. nice job |
I would like to take a look at this after the weekend too, took a quick scan and seems very exciting! awesome stuff! :) |
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.
super excited to get into this underrepresented area of perf in Lighthouse, very impactful first start too nice @adamraine ! 🎉
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.
LGTM, great work @adamraine ! thanks for sticking with all the detours and side PRs split out along the way :)
@paulirish I'm gonna consider Adam's choice here for i18n/table columns good enough to land, but if you disagree with the direction we went with the table we can always adjust in a followup :) |
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.
wait hang on one last thing sorry 😅
the object-change lost our duplicate support
for (const {name, failureReasonsMask} of animations) { | ||
if (!failureReasonsMask) continue; | ||
for (const failureReason of getActionableFailureReasons(failureReasonsMask)) { | ||
failureReasons.add({failureReason, animation: name}); |
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.
ah, drat hang on @adamraine this set isn't doing it's job anymore with the object-form
also brings up a problem when there are no animations with an identifiable name, maybe we use Unknown
and use a string as a key here
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.
~Technically~ an animation could have Unknown
as it's display name. I think undefined
can be used as a key for a map so I will use that.
Co-authored-by: Patrick Hulce <[email protected]>
Is there a test for this? |
@connorjclark just added one after fixing that bug |
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.
LGTM thanks @adamraine for quick fix turnaround! you should be able to rebase to get rid of failures now too :)
Co-authored-by: Patrick Hulce <[email protected]>
* upstream/master: (42 commits) docs: add Code of Conduct to project (GoogleChrome#11212) docs(readme): add related project: lighthouse-viewer (GoogleChrome#11250) core(font-size): remove deprecated DOM.getFlattenedDocument (GoogleChrome#11248) misc: fix typo in method name (GoogleChrome#11239) i18n: make double dollar validation less strict (GoogleChrome#10299) misc: rephrase comments to be more inclusive (GoogleChrome#11228) misc: tweak gcp scripts to work in google corp (GoogleChrome#11233) v6.2.0 (GoogleChrome#11232) report: correctly display CLS in budget table (GoogleChrome#11209) report: vertically center thumbnails (GoogleChrome#11220) i18n: import (GoogleChrome#11225) tests: istanbul ignore inpage function (GoogleChrome#11229) deps(snyk): update script to prune <0.0.0 and update snapshot (GoogleChrome#11223) core(stacks): timeout stack detection (GoogleChrome#11172) core(config): unsized-images to default (GoogleChrome#11217) core(image-elements): collect CSS sizing, ShadowRoot, & position (GoogleChrome#11188) core: add FormElements gatherer (GoogleChrome#11062) new_audit: report animations not run on compositor (GoogleChrome#11105) tests: update chromestatus expecatations (GoogleChrome#11221) deps: update dot-prop secondary dependency (GoogleChrome#11198) ...
Summary
Audit which reports animations that could not be composited. Depends on this chromium CL which adds compositor failure reasons to the inspector trace. Currently only reports a node id for testing purposes. The plan is to report animation name instead which may require more chromium changes since it is not in the animation trace event either.
Related Issues/PRs
#2208