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

new_audit: report animations not run on compositor #11105

Merged
merged 81 commits into from
Aug 5, 2020

Conversation

adamraine
Copy link
Member

@adamraine adamraine commented Jul 14, 2020

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

@paulirish
Copy link
Member

super exciting stuff. nice job

@patrickhulce
Copy link
Collaborator

I would like to take a look at this after the weekend too, took a quick scan and seems very exciting! awesome stuff! :)

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.

super excited to get into this underrepresented area of perf in Lighthouse, very impactful first start too nice @adamraine ! 🎉

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.

LGTM, great work @adamraine ! thanks for sticking with all the detours and side PRs split out along the way :)

@patrickhulce
Copy link
Collaborator

@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 :)

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.

wait hang on one last thing sorry 😅

the object-change lost our duplicate support

from https://www.theverge.com
image

for (const {name, failureReasonsMask} of animations) {
if (!failureReasonsMask) continue;
for (const failureReason of getActionableFailureReasons(failureReasonsMask)) {
failureReasons.add({failureReason, animation: name});
Copy link
Collaborator

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

Copy link
Member Author

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]>
@connorjclark
Copy link
Collaborator

the object-change lost our duplicate support

Is there a test for this?

@adamraine
Copy link
Member Author

@connorjclark just added one after fixing that bug

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.

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]>
@adamraine adamraine merged commit 5859bdc into master Aug 5, 2020
@adamraine adamraine deleted the non-composited-animations branch August 5, 2020 15:49
radum added a commit to radum/lighthouse that referenced this pull request Aug 13, 2020
* 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)
  ...
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.

5 participants