Disconnect performance observer if the page wasn't hidden prior to the update

[monis0395]

I was going through the code and observed that for few of the metrics we Only report if the page wasn't hidden prior to the metric.

• FID
• FCP
• LCP

Now, if the page is ever hidden and the metric is not yet reported, then we would never update the values for any of the above metrics because visibilityWatcher.firstHiddenTime would always be lower because it tracks hidden time.

Thus I was thinking (CMIIW) if a page is hidden, we can disconnect performance observer for FID & LCP. We are already doing it for FCP for a different reason so no change there.

Also is there a reason for always reporting LCP i.e. even if the value remains unchanged ?

[philipwalton]

Now, if the page is ever hidden and the metric is not yet reported, then we would never update the values for any of the above metrics because visibilityWatcher.firstHiddenTime would always be lower because it tracks hidden time.

Thus I was thinking (CMIIW) if a page is hidden, we can disconnect performance observer for FID & LCP. We are already doing it for FCP for a different reason so no change there.

You're correct that we could safely disconnect the observer in cases where the page was hidden prior to the relevant event.

I originally considered this and decided against it due to the fact that it would increase code size and I was trying to keep the library as small as possible. I also talked to the engineering teams about the performance impact of keeping an observer open without disconnecting it, and they said it was likely negligible.

That being said, I'd be open to PRs to add this if it doesn't increase the code size too much.

Also is there a reason for always reporting LCP i.e. even if the value remains unchanged ?

Hmmm, I don't think there's a reason for that, it's probably just an oversight. Note that it doesn't really make a difference because the reporter function will noop if there are no entries, but I agree it would be better to keep them consistent.

[monis0395]

Thanks for the reply!

You're correct that we could safely disconnect the observer in cases where the page was hidden prior to the relevant event.

I originally considered this and decided against it due to the fact that it would increase code size and I was trying to keep the library as small as possible. I also talked to the engineering teams about the performance impact of keeping an observer open without disconnecting it, and they said it was likely negligible.

That being said, I'd be open to PRs to add this if it doesn't increase the code size too much.

Oh, if its likely negligible then we can skip it, because it would add a lot of conditions and increase the code size. I'll still try to make a PR for it and share with you

Also is there a reason for always reporting LCP i.e. even if the value remains unchanged ?

Hmmm, I don't think there's a reason for that, it's probably just an oversight. Note that it doesn't really make a difference because the reporter function will noop if there are no entries, but I agree it would be better to keep them consistent.

Thanks I will make a separate PR for this!

[monis0395]

@philipwalton I have raised PR #196 & #197. I have tried to keep the changes minimal to ensure low lib size.

Can you please review the changes? Thanks!

PS: Was not able to raise it sooner was caught up in something! 🙈