-
Notifications
You must be signed in to change notification settings - Fork 449
Description
I noticed this issue when I was reviewing this PR: #5714 (comment)
It looks like the way we currently add marker payloads are flawed. First we look at the chrome even to see if it has any markers. If it does, great, we do the usual thing of adding that as a marker payload. But if doesn't exist, we still add a dummy payload for that marker weirdly:
profiler/src/profile-logic/import/chrome.ts
Lines 1006 to 1010 in 2ce8ab6
| const newData = { | |
| ...argData, | |
| type: name, | |
| category: event.cat, | |
| }; |
It's only so we can add the category field there. But the weird thing is that we never show this field in the UI, because this payload doesn't have any marker schema attached to it...
So we simply don't show the category information even though we have that in the profile data. We only add "Other" as the category by default to all of them:
profiler/src/profile-logic/import/chrome.ts
Line 997 in 2ce8ab6
| markers.category.push(otherCategoryIndex); |
This shouldn't be the case though, we should always add this category information to the marker category itself if possible. And then only add a marker payload if the payload exists in the chrome data.
This way, we could properly merge start and end marker payloads as well to display them better.
So the things that we need to do:
- Put the category properly to the marker categories.
- Remove the dummy payload if the event doesn't have any data.
- Remove
beginEventDetailall together with all the logic around it.
These changes are not a lot, but it touches other parts unrelated to the PR #5714, that's why I didn't think it was the right thing to ask in this PR. I think it's better to do them as a follow-up.
┆Issue is synchronized with this Jira Task