-
Notifications
You must be signed in to change notification settings - Fork 9.4k
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: refactor finding of trace start #6370
Conversation
16bb0ca
to
35b6693
Compare
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.
very excited about the explicitness this introduces on why startedInPage was even a thing!!
LGTM other than the ts
point, happy to approve if no one else thinks ts change is worth it :)
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!
pid, | ||
tid, | ||
frameId, | ||
}; |
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.
Style bikeshedding: what about some early returns? That makes it a little easier to see it's either the first conditional matches and it's done, or it's the next conditional and done, or it throws.
Something like
static findMainFrameIds(events) {
// Prefer the newer TracingStartedInBrowser event first, if it exists
const startedInBrowserEvt = events.find(e => e.name === 'TracingStartedInBrowser');
if (startedInBrowserEvt && startedInBrowserEvt.args.data &&
startedInBrowserEvt.args.data.frames) {
const mainFrame = startedInBrowserEvt.args.data.frames.find(frame => !frame.parent);
const frameId = mainFrame && mainFrame.frame;
const pid = mainFrame && mainFrame.processId;
const threadNameEvt = events.find(e => e.pid === pid && e.ph === 'M' &&
e.cat === '__metadata' && e.name === 'thread_name' && e.args.name === 'CrRendererMain');
const tid = threadNameEvt && threadNameEvt.tid;
if (pid && tid && frameId) {
return {
pid,
tid,
frameId,
};
}
}
// Support legacy browser versions that do not emit TracingStartedInBrowser event.
// The first TracingStartedInPage in the trace is definitely our renderer thread of interest
// Beware: the tracingStartedInPage event can appear slightly after a navigationStart
const startedInPageEvt = events.find(e => e.name === 'TracingStartedInPage');
if (startedInPageEvt && startedInPageEvt.args && startedInPageEvt.args.data) {
const frameId = startedInPageEvt.args.data.page;
if (frameId) {
return {
pid: startedInPageEvt.pid,
tid: startedInPageEvt.tid,
frameId,
};
}
}
throw new LHError(LHError.errors.NO_TRACING_STARTED);
}
typings/artifacts.d.ts
Outdated
@@ -359,8 +359,8 @@ declare global { | |||
processEvents: Array<TraceEvent>; | |||
/** The subset of trace events from the page's main thread, sorted by timestamp. */ | |||
mainThreadEvents: Array<TraceEvent>; | |||
/** The event marking the start of tracing in the target browser. */ | |||
startedInPageEvt: TraceEvent; | |||
/** Various ids for the current trace context. */ |
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.
maybe something like /** IDs for the trace's main frame, process, and thread. */
?
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.
LGTM2!
Split from #6366.
Refactor how we get the pid/tid/frameId of the current target. The tracing processor was returning a specific event, and even synthesizing it. The callers of this function only grabbed the pid/tid/frameId, so instead of returning an event object I return just the pertinent data.