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

core(fr): colocate PerformanceObserver installation with wait logic #12365

Merged
merged 3 commits into from
Apr 14, 2021

Conversation

patrickhulce
Copy link
Collaborator

@patrickhulce patrickhulce commented Apr 14, 2021

Summary
Minor refactor to reduce the amount of global setup necessary to use shared navigation logic between FR and legacy. This PR moves the PerformanceObserver installation to happen on-demand when needed instead of relying on gather-runner to install it globally on document creation. Fewer magic global window properties located in different files is a nice bonus too :)

Related Issues/PRs
ref #11313

@patrickhulce patrickhulce requested a review from a team as a code owner April 14, 2021 19:50
@patrickhulce patrickhulce requested review from connorjclark and removed request for a team April 14, 2021 19:50
@google-cla google-cla bot added the cla: yes label Apr 14, 2021
@patrickhulce patrickhulce changed the title core(fr): move PerformanceObserver installation to wait logic core(fr): colocate PerformanceObserver installation with wait logic Apr 14, 2021
* property on window to the end time of the last long task.
*/
function registerPerformanceObserverInPage() {
// Do not re-register if we've already run this script.
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is new

}
});

observer.observe({entryTypes: ['longtask'], buffered: true});
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as is the buffered flag which didn't exist when we originally wrote this when PerfObservers were brand new hotness :)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this seems like a big fix?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not really that big. we previously installed it before even the document is available and we always assume that a longtask just happened so the likelihood of this catching anything new is very small.

now that we're registering slightly later in the page's lifecycle it makes more semantic sense, but again because of our assumption a longtask just happened on installation it doesn't make much of a difference in practice

Co-authored-by: Connor Clark <cjamcl@google.com>
@patrickhulce
Copy link
Collaborator Author

patrickhulce commented Apr 14, 2021

investigating the metrics smoke failures, can't repro any of them locally but they're definitely in the path of these changes, so it seems legit. found it! was the code review suggestion I hadn't pulled yet :)

@patrickhulce patrickhulce merged commit 21abce8 into master Apr 14, 2021
@patrickhulce patrickhulce deleted the fr_perf_observer_refactor branch April 14, 2021 21:00
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.

3 participants