Display the event type from DOM events in chrome profiles#4604
Conversation
canova
left a comment
canova
left a comment
There was a problem hiding this comment.
Thanks! Looks good to me. I think we can give a better name to type2 but let me know what you think!
| display: ['marker-chart', 'marker-table', 'timeline-overview'], | ||
| data: [ | ||
| { | ||
| key: 'type2', |
There was a problem hiding this comment.
how about making this eventType instead?
There was a problem hiding this comment.
I thought about that, but this would be right only for DispatchEvent markers. What if other markers also have type in their payload, but that's not related to events?
There was a problem hiding this comment.
Hm, this schema's name is EventDispatch though. Do we expect other markers to have the same name as well?
There was a problem hiding this comment.
Oh, I see what you mean now. We need to change the code below that's generic. Hm, I guess it's okay to keep it this way.
There was a problem hiding this comment.
I can add a comment to be more explicit about this though.
There was a problem hiding this comment.
Yeah, a comment could be nice. It wasn't clear to me at first. Thanks!
| argData.type2 = argData.type; | ||
| } | ||
| if (argData && 'category' in argData) { | ||
| argData.category2 = argData.category; |
There was a problem hiding this comment.
I guess we don't use the category2? Do we still need it? I may be missing something though.
There was a problem hiding this comment.
I was thinking that I don't want to lose any data.
You could argue that we can lose type2 and category2 😅
Maybe we can use something less typical such as __type or $type or __original__type?
There was a problem hiding this comment.
I see. I don't have a lot of good alternatives tbh. I think it's fine to keep type2 and catergory2 too then.
There was a problem hiding this comment.
Thanks!
It's super easy to change later if we need it too, as it's used only here with this indirection.
There was a problem hiding this comment.
I've been thinking that our internal "type" should probably have been more collision-proof. But I guess this is a bit late for that now.
2 participants
With this change, it's now possible to see the dom event types in the marker chart and table. They also show up in the timeline.
Moreover now it should be easier to display data for other events.
The more difficult thing left is to map the stack trace that's part of some of these events to the stack table, so that we could display the cause in the tooltip... A task for a follow-up.
Here is a profile imported from this deploy preview: https://share.firefox.dev/3npso3p (notice that the dom event names show up in the marker chart and table)
And here is the same profile displayed with this deploy preview (notice they also show up in the timeline now)
Design notes: I used this
type2property so that we still havetypeto pick the schema. Ideally we'd make the name pick up the schema too but I was afraid of regressions with gecko profiles, so I'd like to have some more available time to work on the profiler codebase to be able to do that. Tell me what you think!