Fix missing LoAF attribution when entries are dispatched before event entries #487
Conversation
|
Oh, also I could not get a test for this working reliably. Inherently flaky and subject to future Chrome scheduler changes anyways. I verified the issue repro no longer fails, but looking for other ideas on this. Time to bust out some unit tests? Override |
Maybe we should do this clean up in |
There was a problem hiding this comment.
Thanks for taking a look at this @brendankenny, and for the detailed repro. It was very helpful for testing this!
I took Barry's suggestion and made a few other minor tweaks to the code, but other than that LGTM.
|
Updated, and also moved the |
philipwalton
changed the title
fixes #485
As described there, I added an extra condition to keep any LoAFs around that are after any existing known interactions, in case those interactions then come in later.
I then realized that would leave the LoAF collection unbounded, since a page can have a bunch of slow animation frames without the user interacting. I added a
MAX_UNPAIRED_LOAFSof 20, so it will only keep the 20 most recent LoAFs after any known user interaction events. The number isn't hugely important, that just allows a one second delay in event-timing reporting even if the page somehow has filled the time with the shortest (50ms) long animation frames possible. LoAF entries are fairly light weight, so that's not so bad.As I was implementing this, however, I realized that even without this change the
pendingLoAFsarray can grow without bound on a page with e.g. animations and a busy main thread, since LoAFs are appended as they come in but clean up is only scheduled by user interactions. Seems like we should fix? @philipwalton @tunethewebMy only idea right now is to split the event timing and LoAF cleanups into two separate functions, and run each as their respective entries come in, since that's the only time each list of entries grows.